From 9f6a8fa00ed92808c9762df4260179a92ff23c33 Mon Sep 17 00:00:00 2001 From: Nate Schlossberg Date: Wed, 26 Aug 2026 10:42:17 -0700 Subject: [PATCH] Check for property fields first before handling any native fields, in case of a name collision --- server/channels/app/access_control.go | 34 +++++++---- server/channels/app/access_control_test.go | 70 +++++++++++++++++++++- 2 files changed, 93 insertions(+), 11 deletions(-) diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index 5c297b80db1..6e14e1b0a77 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -1778,24 +1778,38 @@ func (a *App) GetAccessControlPolicyAttributes(rctx request.CTX, resourceID stri return map[string][]string{}, nil } - // We will be looking up field names by the store, but first we need to check for any synthetic - // native fields. At the time of writing this comment, all native fields are public, so + // Native fields won't show up in the Property Store, since they are synthetic (generated in code). + // At the time of writing this comment, all native fields are public, so // we could just check for each name and give it a pass without building the array, but this // approach will make sure any potential future native fields can also support different AccessModes. + + // We will check for attributes in the store first because there is a known potential for name collision + // (i.e. if an admin makes an access control property with the same name as a native attribute). + // The collision doesn't happen during storage since there are unqiue prefixes, but the attributes + // returned by GetPolicyRuleAttributes are stripped of those prefixes, leaving it ambiguous. + // While that should be addressed, for now we check for real properties first and then fall back to native fields. nativeFieldsByName := make(map[string]*model.PropertyField) for _, f := range model.NativeUserAttributeFields(cpaGroup.ID) { nativeFieldsByName[f.Name] = f } for fieldName := range attributes { - // Check if this is a (synthetic) native field before reading from the store - field, ok := nativeFieldsByName[fieldName] - if !ok { - // Read directly from the store so this security filter sees the raw - // access_mode, unaffected by property read hooks for the request caller. - var fieldErr error - field, fieldErr = a.Srv().Store().PropertyField().GetFieldByNameForObjectType(rctx, cpaGroup.ID, "", model.PropertyFieldObjectTypeUser, fieldName) - if fieldErr != nil { + // Read directly from the store so this security filter sees the raw + // access_mode, unaffected by property read hooks for the request caller. + field, fieldErr := a.Srv().Store().PropertyField().GetFieldByNameForObjectType(rctx, cpaGroup.ID, "", model.PropertyFieldObjectTypeUser, fieldName) + if fieldErr != nil { + // If the error is due to not being found, we won't skip to the next field just yet + // in case it is a native field + var nfErr *store.ErrNotFound + notFound := errors.As(fieldErr, &nfErr) + if !notFound { + delete(attributes, fieldName) + continue + } + + //If property wasn't found, check if this is a native field. + field = nativeFieldsByName[fieldName] + if field == nil { delete(attributes, fieldName) continue } diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index d34882d53ad..8467a118d3e 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -4790,7 +4790,7 @@ func TestGetAccessControlPolicyAttributes_MaskedFieldsFiltered(t *testing.T) { mockACS := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockACS mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership). - Return(rawAttributes, nil).Once() + Return(maps.Clone(rawAttributes), nil).Once() result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership) require.Nil(t, appErr) @@ -4865,6 +4865,74 @@ func TestGetAccessControlPolicyAttributes_NativeFieldsPassThrough(t *testing.T) mockACS.AssertExpectations(t) } +// TestGetAccessControlPolicyAttributes_MaskedFieldsWithNameCollisionAreFiltered checks that +// a masked field that happens to share the same name as a native filed stil gets filtered. +func TestGetAccessControlPolicyAttributes_MaskedFieldsWithNameCollisionAreFiltered(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + + rctx := request.TestContext(t) + + cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, cErr) + + permNone := model.PermissionLevelNone + + makeField := func(name, accessMode string) { + protected := accessMode == model.PropertyAccessModeSourceOnly || accessMode == model.PropertyAccessModeSharedOnly + f := &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: name, + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Protected: protected, + Attrs: model.StringInterface{model.PropertyAttrsAccessMode: accessMode}, + } + if protected { + f.PermissionField = &permNone + f.Attrs[model.PropertyAttrsProtected] = true + _, err := th.App.Srv().Store().PropertyField().Create(f) + require.NoError(t, err) + } else { + _, appErr := th.App.CreatePropertyField(rctx, f, false, "") + require.Nil(t, appErr) + } + } + + makeField("PublicField", model.PropertyAccessModePublic) + makeField("SourceField", model.PropertyAccessModeSourceOnly) + // This is the field with a native name that we are testing to make sure it is still masked. + makeField(model.NativeAttributePropertyFieldIsBot, model.PropertyAccessModeSharedOnly) + + channelID := model.NewId() + rawAttributes := map[string][]string{ + "PublicField": {"Engineering"}, + "SourceField": {"TopSecret"}, + model.NativeAttributePropertyFieldIsBot: {"false"}, + // This is a real native attribute that is not in the property store -it should pass through. + model.NativeAttributePropertyFieldVerified: {"true"}, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership). + Return(rawAttributes, nil).Once() + + result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership) + require.Nil(t, appErr) + + // Only the public field should survive. + expectedAttributes := map[string][]string{ + "PublicField": {"Engineering"}, + model.NativeAttributePropertyFieldVerified: {"true"}, + } + assert.Equal(t, expectedAttributes, result) + assert.NotContains(t, result, "SourceField") + assert.NotContains(t, result, model.NativeAttributePropertyFieldIsBot) + mockACS.AssertExpectations(t) +} + // TestMergeStoredPolicyExpressions_ActionsLocked verifies that a caller who // cannot see all values in a stored rule cannot change that rule's Actions. // The attack: submit a PUT with the same masked expression but a different