From 64ed8f02dced90137e3da2422b21c4df2cbf96da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20V=C3=A9lez?= Date: Tue, 13 May 2025 09:59:31 +0200 Subject: [PATCH] MM-63912 - enhance validation for channel banner colors (#30981) * MM-63912 - enhance validation for channel banner colors * fix tests * fix unit tests --- server/channels/api4/channel_test.go | 28 +++--- server/i18n/en.json | 4 + server/public/model/channel.go | 9 ++ server/public/model/channel_test.go | 62 ++++++++++++ ...hannel_settings_configuration_tab.test.tsx | 97 +++++++++++++++++++ 5 files changed, 186 insertions(+), 14 deletions(-) diff --git a/server/channels/api4/channel_test.go b/server/channels/api4/channel_test.go index 0e191e863f..17428d12a0 100644 --- a/server/channels/api4/channel_test.go +++ b/server/channels/api4/channel_test.go @@ -172,7 +172,7 @@ func TestCreateChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: model.NewPointer(true), Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } @@ -182,7 +182,7 @@ func TestCreateChannel(t *testing.T) { require.True(t, *createdChannel.BannerInfo.Enabled) require.Equal(t, "banner text", *createdChannel.BannerInfo.Text) - require.Equal(t, "color", *createdChannel.BannerInfo.BackgroundColor) + require.Equal(t, "#dddddd", *createdChannel.BannerInfo.BackgroundColor) }) t.Run("Cannot create channel with banner enabled but not configured", func(t *testing.T) { @@ -767,7 +767,7 @@ func TestPatchChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: model.NewPointer(true), Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } @@ -799,7 +799,7 @@ func TestPatchChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: model.NewPointer(true), Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } @@ -831,7 +831,7 @@ func TestPatchChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: model.NewPointer(true), Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } @@ -841,7 +841,7 @@ func TestPatchChannel(t *testing.T) { require.NotNil(t, patchedChannel.BannerInfo) require.True(t, *patchedChannel.BannerInfo.Enabled) require.Equal(t, "banner text", *patchedChannel.BannerInfo.Text) - require.Equal(t, "color", *patchedChannel.BannerInfo.BackgroundColor) + require.Equal(t, "#dddddd", *patchedChannel.BannerInfo.BackgroundColor) }) t.Run("Should not be able to configure channel banner on a channel as a non-admin channel member", func(t *testing.T) { @@ -856,7 +856,7 @@ func TestPatchChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: model.NewPointer(true), Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } @@ -877,7 +877,7 @@ func TestPatchChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: model.NewPointer(true), Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } @@ -887,7 +887,7 @@ func TestPatchChannel(t *testing.T) { require.NotNil(t, patchedChannel.BannerInfo) require.True(t, *patchedChannel.BannerInfo.Enabled) require.Equal(t, "banner text", *patchedChannel.BannerInfo.Text) - require.Equal(t, "color", *patchedChannel.BannerInfo.BackgroundColor) + require.Equal(t, "#dddddd", *patchedChannel.BannerInfo.BackgroundColor) }) t.Run("Cannot enable channel banner without configuring it", func(t *testing.T) { @@ -923,7 +923,7 @@ func TestPatchChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: nil, Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } @@ -933,7 +933,7 @@ func TestPatchChannel(t *testing.T) { require.NotNil(t, patchedChannel.BannerInfo) require.Nil(t, patchedChannel.BannerInfo.Enabled) require.Equal(t, "banner text", *patchedChannel.BannerInfo.Text) - require.Equal(t, "color", *patchedChannel.BannerInfo.BackgroundColor) + require.Equal(t, "#dddddd", *patchedChannel.BannerInfo.BackgroundColor) patch = &model.ChannelPatch{ BannerInfo: &model.ChannelBannerInfo{ @@ -947,7 +947,7 @@ func TestPatchChannel(t *testing.T) { require.NotNil(t, patchedChannel.BannerInfo) require.True(t, *patchedChannel.BannerInfo.Enabled) require.Equal(t, "banner text", *patchedChannel.BannerInfo.Text) - require.Equal(t, "color", *patchedChannel.BannerInfo.BackgroundColor) + require.Equal(t, "#dddddd", *patchedChannel.BannerInfo.BackgroundColor) }) t.Run("Cannot configure channel banner on a DM channel", func(t *testing.T) { @@ -966,7 +966,7 @@ func TestPatchChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: model.NewPointer(true), Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } @@ -994,7 +994,7 @@ func TestPatchChannel(t *testing.T) { BannerInfo: &model.ChannelBannerInfo{ Enabled: model.NewPointer(true), Text: model.NewPointer("banner text"), - BackgroundColor: model.NewPointer("color"), + BackgroundColor: model.NewPointer("#dddddd"), }, } diff --git a/server/i18n/en.json b/server/i18n/en.json index 910b026d36..f1bbfc667a 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -8660,6 +8660,10 @@ "id": "model.channel.is_valid.banner_info.background_color.empty.app_error", "translation": "Channel banner color cannot be empty when channel banner is enabled" }, + { + "id": "model.channel.is_valid.banner_info.background_color.invalid.app_error", + "translation": "Channel banner color must be a valid hex color (e.g., #FF0000 or #F00)" + }, { "id": "model.channel.is_valid.banner_info.channel_type.app_error", "translation": "Channel banner can only be configured on Public and Private channels" diff --git a/server/public/model/channel.go b/server/public/model/channel.go index 3693e3697d..087f52c4ab 100644 --- a/server/public/model/channel.go +++ b/server/public/model/channel.go @@ -17,6 +17,11 @@ import ( "unicode/utf8" ) +var ( + // Validates both 3-digit (#RGB) and 6-digit (#RRGGBB) hex colors + channelHexColorRegex = regexp.MustCompile(`^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$`) +) + type ChannelType string const ( @@ -312,6 +317,10 @@ func (o *Channel) IsValid() *AppError { if o.BannerInfo.BackgroundColor == nil || len(*o.BannerInfo.BackgroundColor) == 0 { return NewAppError("Channel.IsValid", "model.channel.is_valid.banner_info.background_color.empty.app_error", nil, "", http.StatusBadRequest) } + + if !channelHexColorRegex.MatchString(*o.BannerInfo.BackgroundColor) { + return NewAppError("Channel.IsValid", "model.channel.is_valid.banner_info.background_color.invalid.app_error", nil, "", http.StatusBadRequest) + } } return nil diff --git a/server/public/model/channel_test.go b/server/public/model/channel_test.go index 937241638f..36758bba6d 100644 --- a/server/public/model/channel_test.go +++ b/server/public/model/channel_test.go @@ -87,6 +87,68 @@ func TestChannelIsValid(t *testing.T) { require.NotNil(t, o.IsValid()) } +func TestChannelBannerBackgroundColorValidation(t *testing.T) { + o := Channel{ + Id: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + Name: "valid-name", + Type: ChannelTypeOpen, + Header: "valid-header", + Purpose: "valid-purpose", + BannerInfo: &ChannelBannerInfo{ + Enabled: NewPointer(true), + Text: NewPointer("Banner Text"), + }, + } + + // Test with nil background color + o.BannerInfo.BackgroundColor = nil + require.NotNil(t, o.IsValid()) + require.Equal(t, "model.channel.is_valid.banner_info.background_color.empty.app_error", o.IsValid().Id) + + // Test with empty background color + o.BannerInfo.BackgroundColor = NewPointer("") + require.NotNil(t, o.IsValid()) + require.Equal(t, "model.channel.is_valid.banner_info.background_color.empty.app_error", o.IsValid().Id) + + // Test with invalid background color (no # prefix) + o.BannerInfo.BackgroundColor = NewPointer("FF0000") + require.NotNil(t, o.IsValid()) + require.Equal(t, "model.channel.is_valid.banner_info.background_color.invalid.app_error", o.IsValid().Id) + + // Test with invalid background color (invalid characters) + o.BannerInfo.BackgroundColor = NewPointer("#GGGGGG") + require.NotNil(t, o.IsValid()) + require.Equal(t, "model.channel.is_valid.banner_info.background_color.invalid.app_error", o.IsValid().Id) + + // Test with invalid background color (wrong length) + o.BannerInfo.BackgroundColor = NewPointer("#FF00") + require.NotNil(t, o.IsValid()) + require.Equal(t, "model.channel.is_valid.banner_info.background_color.invalid.app_error", o.IsValid().Id) + + // Test with invalid background color (wrong length) + o.BannerInfo.BackgroundColor = NewPointer("#FF00000") + require.NotNil(t, o.IsValid()) + require.Equal(t, "model.channel.is_valid.banner_info.background_color.invalid.app_error", o.IsValid().Id) + + // Test with valid 6-digit hex color + o.BannerInfo.BackgroundColor = NewPointer("#FF0000") + require.Nil(t, o.IsValid()) + + // Test with valid 6-digit hex color (lowercase) + o.BannerInfo.BackgroundColor = NewPointer("#ff0000") + require.Nil(t, o.IsValid()) + + // Test with valid 3-digit hex color + o.BannerInfo.BackgroundColor = NewPointer("#F00") + require.Nil(t, o.IsValid()) + + // Test with valid 3-digit hex color (lowercase) + o.BannerInfo.BackgroundColor = NewPointer("#f00") + require.Nil(t, o.IsValid()) +} + func TestChannelPreSave(t *testing.T) { o := Channel{Name: "test"} o.PreSave() diff --git a/webapp/channels/src/components/channel_settings_modal/channel_settings_configuration_tab.test.tsx b/webapp/channels/src/components/channel_settings_modal/channel_settings_configuration_tab.test.tsx index ec2b1a4710..80d593f7ce 100644 --- a/webapp/channels/src/components/channel_settings_modal/channel_settings_configuration_tab.test.tsx +++ b/webapp/channels/src/components/channel_settings_modal/channel_settings_configuration_tab.test.tsx @@ -337,4 +337,101 @@ describe('ChannelSettingsConfigurationTab', () => { const errorPanel = errorMessage.closest('.SaveChangesPanel'); expect(errorPanel).toHaveClass('error'); }); + + it('should save valid colors in hex format', async () => { + const {patchChannel} = require('mattermost-redux/actions/channels'); + patchChannel.mockReturnValue({type: 'MOCK_ACTION', data: {}}); + + renderWithContext(); + + // Enable the banner + await act(async () => { + await userEvent.click(screen.getByTestId('channelBannerToggle-button')); + }); + + // Enter banner text + await act(async () => { + const textInput = screen.getByTestId('channel_banner_banner_text_textbox'); + await userEvent.clear(textInput); + await userEvent.type(textInput, 'New banner text'); + }); + + // Enter a valid hex color + await act(async () => { + const colorInput = screen.getByTestId('color-inputColorValue'); + await userEvent.clear(colorInput); + await userEvent.type(colorInput, '#ff0000'); + }); + + // Click the Save button + await act(async () => { + await userEvent.click(screen.getByRole('button', {name: 'Save'})); + }); + + // Verify patchChannel was called with the correct color + expect(patchChannel).toHaveBeenCalledWith('channel1', expect.objectContaining({ + banner_info: expect.objectContaining({ + background_color: expect.stringMatching(/#[0-9a-f]{6}/i), // Match any hex color + }), + })); + }); + + it('only valid colors will make the save changes panel visible', async () => { + const {patchChannel} = require('mattermost-redux/actions/channels'); + patchChannel.mockReturnValue({type: 'MOCK_ACTION', data: {}}); + patchChannel.mockClear(); // Clear any previous calls + + const originalColor = '#DDDDDD'; // Original color + + // Create a channel with an valid color + const channelWithValidColor = { + ...mockChannel, + banner_info: { + enabled: true, + text: 'Test text', + background_color: originalColor, // Valid color + }, + }; + + // Render with the invalid color channel + renderWithContext( + , + ); + + // Enter a invalid hex color + await act(async () => { + const colorInput = screen.getByTestId('color-inputColorValue'); + await userEvent.clear(colorInput); + await userEvent.type(colorInput, 'not-a-color'); + }); + + // Do another action to trigger blur on this input so color is validated + await act(async () => { + const textInput = screen.getByTestId('channel_banner_banner_text_textbox'); + await userEvent.clear(textInput); + await userEvent.type(textInput, 'Test text'); + }); + + // if invalid, the color automatically returns to the original color + expect(screen.getByTestId('color-inputColorValue')).toHaveValue(originalColor); + + // Check that the save changes panel is not visible + expect(screen.queryByRole('button', {name: 'Save'})).not.toBeInTheDocument(); + + // Modify the color to a valid one + await act(async () => { + const colorInput = screen.getByTestId('color-inputColorValue'); + await userEvent.clear(colorInput); + await userEvent.type(colorInput, '#123456'); + }); + + // Add a small delay to ensure all state updates are processed + await new Promise((resolve) => setTimeout(resolve, 0)); + + // Check that the save changes panel is visible + expect(screen.getByRole('button', {name: 'Save'})).toBeInTheDocument(); + }); });