From 3df7bfca88827f48c31b6ae7d20a99b785e81bba Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Wed, 16 Apr 2025 16:04:30 +0200 Subject: [PATCH] Improves validation and sanitization for CPA fields and values (#30694) This change automatically removes options and sync attributes when sanitizing fields that don't support them. As per values, it returns an error when the value for a text type field is longer than the 64 characters limit we're currently applying. The PR fixes a bug on the create CPA field endpoint that was causing the attrs of the CPAField not to be decoded correctly. Co-authored-by: Miguel de la Cruz --- .../api4/custom_profile_attributes_test.go | 87 ++++++++ .../public/model/custom_profile_attributes.go | 31 ++- .../model/custom_profile_attributes_test.go | 193 ++++++++++++++++++ 3 files changed, 309 insertions(+), 2 deletions(-) diff --git a/server/channels/api4/custom_profile_attributes_test.go b/server/channels/api4/custom_profile_attributes_test.go index 11037bc667..aded2c282c 100644 --- a/server/channels/api4/custom_profile_attributes_test.go +++ b/server/channels/api4/custom_profile_attributes_test.go @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "os" + "strings" "testing" "time" @@ -215,6 +216,69 @@ func TestPatchCPAField(t *testing.T) { require.NotEmpty(t, wsField.ID) require.Equal(t, patchedField, &wsField) }) + + t.Run("sanitization should remove options and sync details when necessary", func(t *testing.T) { + // Create a select field with options + optionID1 := model.NewId() + optionID2 := model.NewId() + selectField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeSelect, + Attrs: model.StringInterface{ + "options": []map[string]any{ + {"id": optionID1, "name": "Option 1", "color": "#FF0000"}, + {"id": optionID2, "name": "Option 2", "color": "#00FF00"}, + }, + }, + }) + require.NoError(t, err) + + createdField, _, err := client.CreateCPAField(context.Background(), selectField.ToPropertyField()) + require.NoError(t, err) + require.NotNil(t, createdField) + + // Verify options were created + options, ok := createdField.Attrs["options"] + require.True(t, ok) + require.NotNil(t, options) + + // Patch to change type to text with LDAP attribute + // Options should be automatically removed even though we don't explicitly remove them + ldapAttr := "user_attribute" + textPatch := &model.PropertyFieldPatch{ + Type: model.NewPointer(model.PropertyFieldTypeText), + Attrs: &model.StringInterface{"ldap": ldapAttr}, + } + + patchedTextField, resp, err := client.PatchCPAField(context.Background(), createdField.ID, textPatch) + CheckOKStatus(t, resp) + require.NoError(t, err) + require.Equal(t, model.PropertyFieldTypeText, patchedTextField.Type) + + // Verify options were removed + options = patchedTextField.Attrs["options"] + require.Empty(t, options) + + // Verify LDAP attribute was set + ldap, ok := patchedTextField.Attrs["ldap"] + require.True(t, ok) + require.Equal(t, ldapAttr, ldap) + + // Now patch to change type to date + // LDAP attribute should be automatically removed even though we don't explicitly remove it + datePatch := &model.PropertyFieldPatch{ + Type: model.NewPointer(model.PropertyFieldTypeDate), + } + + patchedDateField, resp, err := client.PatchCPAField(context.Background(), patchedTextField.ID, datePatch) + CheckOKStatus(t, resp) + require.NoError(t, err) + require.Equal(t, model.PropertyFieldTypeDate, patchedDateField.Type) + + // Verify LDAP attribute was removed + ldap = patchedDateField.Attrs["ldap"] + require.Empty(t, ldap) + }) }, "a user with admin permissions should be able to patch the field") } @@ -518,4 +582,27 @@ func TestPatchCPAValues(t *testing.T) { require.NoError(t, json.Unmarshal(patchedValues[createdArrayField.ID], &actualValues)) require.Equal(t, optionsID[2:4], actualValues) }) + + t.Run("an invalid patch should be rejected", func(t *testing.T) { + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + }) + require.NoError(t, err) + + createdField, appErr := th.App.CreateCPAField(field) + require.Nil(t, appErr) + require.NotNil(t, createdField) + + // Create a value that's too long (over 64 characters) + tooLongValue := strings.Repeat("a", model.CPAValueTypeTextMaxLength+1) + values := map[string]json.RawMessage{ + createdField.ID: json.RawMessage(fmt.Sprintf(`"%s"`, tooLongValue)), + } + + _, resp, err := th.Client.PatchCPAValues(context.Background(), values) + CheckBadRequestStatus(t, resp) + require.Error(t, err) + require.Contains(t, err.Error(), "Failed to validate property value") + }) } diff --git a/server/public/model/custom_profile_attributes.go b/server/public/model/custom_profile_attributes.go index 92be0b46d5..1beecf28c0 100644 --- a/server/public/model/custom_profile_attributes.go +++ b/server/public/model/custom_profile_attributes.go @@ -46,11 +46,13 @@ const ( CustomProfileAttributesVisibilityWhenSet = "when_set" CustomProfileAttributesVisibilityAlways = "always" CustomProfileAttributesVisibilityDefault = CustomProfileAttributesVisibilityWhenSet -) -const ( + // CPA options CPAOptionNameMaxLength = 128 CPAOptionColorMaxLength = 128 + + // CPA value constraints + CPAValueTypeTextMaxLength = 64 ) func IsKnownCPAValueType(valueType string) bool { @@ -146,7 +148,28 @@ func (c *CPAField) ToPropertyField() *PropertyField { return &pf } +// SupportsOptions checks the CPAField type and determines if the type +// supports the use of options +func (c *CPAField) SupportsOptions() bool { + return c.Type == PropertyFieldTypeSelect || c.Type == PropertyFieldTypeMultiselect +} + +// SupportsSyncing checks the CPAField type and determines if it +// supports syncing with external sources of truth +func (c *CPAField) SupportsSyncing() bool { + return c.Type == PropertyFieldTypeText +} + func (c *CPAField) SanitizeAndValidate() *AppError { + // first we clean unused attributes depending on the field type + if !c.SupportsOptions() { + c.Attrs.Options = nil + } + if !c.SupportsSyncing() { + c.Attrs.LDAP = "" + c.Attrs.SAML = "" + } + switch c.Type { case PropertyFieldTypeText: if valueType := strings.TrimSpace(c.Attrs.ValueType); valueType != "" { @@ -230,6 +253,10 @@ func SanitizeAndValidatePropertyValue(cpaField *CPAField, rawValue json.RawMessa value = strings.TrimSpace(value) if fieldType == PropertyFieldTypeText { + if len(value) > CPAValueTypeTextMaxLength { + return nil, fmt.Errorf("value too long") + } + if cpaField.Attrs.ValueType == CustomProfileAttributesValueTypeEmail && !IsValidEmail(value) { return nil, fmt.Errorf("invalid email") } diff --git a/server/public/model/custom_profile_attributes_test.go b/server/public/model/custom_profile_attributes_test.go index 6b6e4e1371..a3ed8833af 100644 --- a/server/public/model/custom_profile_attributes_test.go +++ b/server/public/model/custom_profile_attributes_test.go @@ -481,6 +481,192 @@ func TestCPAField_SanitizeAndValidate(t *testing.T) { expectError: true, errorId: "app.custom_profile_attributes.sanitize_and_validate.app_error", }, + + // Test options cleaning for types that don't support options + { + name: "text field with options should clean options", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeText, + }, + Attrs: CPAAttrs{ + Options: []*CustomProfileAttributesSelectOption{ + { + ID: NewId(), + Name: "Option 1", + Color: "#123456", + }, + }, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + Options: nil, // Options should be cleaned + }, + }, + { + name: "date field with options should clean options", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeDate, + }, + Attrs: CPAAttrs{ + Options: []*CustomProfileAttributesSelectOption{ + { + ID: NewId(), + Name: "Option 1", + Color: "#123456", + }, + }, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + Options: nil, // Options should be cleaned + }, + }, + { + name: "user field with options should clean options", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeUser, + }, + Attrs: CPAAttrs{ + Options: []*CustomProfileAttributesSelectOption{ + { + ID: NewId(), + Name: "Option 1", + Color: "#123456", + }, + }, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + Options: nil, // Options should be cleaned + }, + }, + + // Test options preservation for types that support options + { + name: "select field with options should preserve options", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeSelect, + }, + Attrs: CPAAttrs{ + Options: []*CustomProfileAttributesSelectOption{ + { + ID: NewId(), + Name: "Option 1", + Color: "#123456", + }, + }, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + Options: PropertyOptions[*CustomProfileAttributesSelectOption]{ + {Name: "Option 1", Color: "#123456"}, + }, + }, + }, + { + name: "multiselect field with options should preserve options", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeMultiselect, + }, + Attrs: CPAAttrs{ + Options: []*CustomProfileAttributesSelectOption{ + { + ID: NewId(), + Name: "Option 1", + Color: "#123456", + }, + }, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + Options: PropertyOptions[*CustomProfileAttributesSelectOption]{ + {Name: "Option 1", Color: "#123456"}, + }, + }, + }, + + // Test syncing attributes cleaning for types that don't support syncing + { + name: "select field with LDAP and SAML should clean syncing attributes", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeSelect, + }, + Attrs: CPAAttrs{ + LDAP: "ldap_attribute", + SAML: "saml_attribute", + Options: []*CustomProfileAttributesSelectOption{ + { + ID: NewId(), + Name: "Option 1", + Color: "#123456", + }, + }, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + LDAP: "", // Should be cleaned + SAML: "", // Should be cleaned + Options: PropertyOptions[*CustomProfileAttributesSelectOption]{ + {Name: "Option 1", Color: "#123456"}, + }, + }, + }, + { + name: "date field with LDAP and SAML should clean syncing attributes", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeDate, + }, + Attrs: CPAAttrs{ + LDAP: "ldap_attribute", + SAML: "saml_attribute", + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + LDAP: "", // Should be cleaned + SAML: "", // Should be cleaned + }, + }, + + // Test syncing attributes preservation for types that support syncing + { + name: "text field with LDAP and SAML should preserve syncing attributes", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeText, + }, + Attrs: CPAAttrs{ + LDAP: "ldap_attribute", + SAML: "saml_attribute", + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + LDAP: "ldap_attribute", // Should be preserved + SAML: "saml_attribute", // Should be preserved + }, + }, } for _, tt := range tests { @@ -539,6 +725,13 @@ func TestSanitizeAndValidatePropertyValue(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "json: cannot unmarshal number into Go value of type string") }) + + t.Run("value too long", func(t *testing.T) { + longValue := strings.Repeat("a", CPAValueTypeTextMaxLength+1) + _, err := SanitizeAndValidatePropertyValue(&CPAField{PropertyField: PropertyField{Type: PropertyFieldTypeText}}, json.RawMessage(fmt.Sprintf(`"%s"`, longValue))) + require.Error(t, err) + require.Equal(t, "value too long", err.Error()) + }) }) t.Run("date field type", func(t *testing.T) {