From dc7f63b4fc3cf7afcb9d9c03fadb478adcabb691 Mon Sep 17 00:00:00 2001 From: Bill Gardner Date: Wed, 2 Sep 2026 10:32:42 -0400 Subject: [PATCH 1/4] MM-70247: bump msgpack fork to cap byte-string allocation paths (#37980) * MM-70247: bump msgpack fork to cap byte-string allocation paths * MM-70247: bump golang.org/x/image to v0.43.0 to clear Snyk findings * MM-70247: remove stale go.sum entries after merge --- server/go.mod | 2 +- server/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/go.mod b/server/go.mod index dd31340b2848..1fbcb6ba5abc 100644 --- a/server/go.mod +++ b/server/go.mod @@ -232,4 +232,4 @@ require ( ) // See MM-66167, MM-68222 for more details. -replace github.com/vmihailenco/msgpack/v5 => github.com/mattermost/msgpack/v5 v5.0.0-20260408165622-cadfad56a815 +replace github.com/vmihailenco/msgpack/v5 => github.com/mattermost/msgpack/v5 v5.0.0-20260813205620-e158e8d3647e diff --git a/server/go.sum b/server/go.sum index 8d9cbc5b0910..3e44a8c6e1b2 100644 --- a/server/go.sum +++ b/server/go.sum @@ -371,8 +371,8 @@ github.com/mattermost/mattermost/server/public v0.4.3 h1:vxdrD5j3+oVfrSCiSaX/lY6 github.com/mattermost/mattermost/server/public v0.4.3/go.mod h1:2z08gasPXqIIbzl/xf2/2Sfn5ITFLGC6tplhOklyAAQ= github.com/mattermost/morph v1.1.0 h1:Q9vrJbeM3s2jfweGheq12EFIzdNp9a/6IovcbvOQ6Cw= github.com/mattermost/morph v1.1.0/go.mod h1:gD+EaqX2UMyyuzmF4PFh4r33XneQ8Nzi+0E8nXjMa3A= -github.com/mattermost/msgpack/v5 v5.0.0-20260408165622-cadfad56a815 h1:uOi89NvrFmDngqMKjlLDxi+MNzJQLA3TqcU2p8czv34= -github.com/mattermost/msgpack/v5 v5.0.0-20260408165622-cadfad56a815/go.mod h1:GaZTsDaehaPpQVyxrf5mtQlH+pc21PIudVV/E3rRQok= +github.com/mattermost/msgpack/v5 v5.0.0-20260813205620-e158e8d3647e h1:tsfWJPgXPNhPT+kOkBmJJJDSjXSNAsweZrujWt8yfHk= +github.com/mattermost/msgpack/v5 v5.0.0-20260813205620-e158e8d3647e/go.mod h1:GaZTsDaehaPpQVyxrf5mtQlH+pc21PIudVV/E3rRQok= github.com/mattermost/pdf v0.0.0-20260828123129-5b7509a6ca01 h1:bXDcd5MREhXeaY+Gn1qixqN91GEClojJBG9v98cbB6s= github.com/mattermost/pdf v0.0.0-20260828123129-5b7509a6ca01/go.mod h1:pNks5J7leEpCnswIJaGfqiz/MQ0lExHgKl+A53aEXrg= github.com/mattermost/rsc v0.0.0-20160330161541-bbaefb05eaa0 h1:G9tL6JXRBMzjuD1kkBtcnd42kUiT6QDwxfFYu7adM6o= From cfab036615ea572067701083eb64c170a77099e9 Mon Sep 17 00:00:00 2001 From: Nick Misasi Date: Wed, 2 Sep 2026 10:39:02 -0400 Subject: [PATCH 2/4] ABAC: plugin policy self-inclusion and editor/test robustness (#37926) * fix ABAC editor null-safety and ResizeObserver noise in local/dev mode. Null users from plugin RPC and incomplete PropertyField names no longer crash the test results modal or CEL editor; ignore Chromium ResizeObserver loop messages. Co-authored-by: Cursor * Enforce self-inclusion on plugin-owned ABAC policy saves. Non-system-admins must match the expression they save via SavePluginAccessControlPolicy; system admins retain the same carve-out as channel/team policy saves. Co-authored-by: Cursor * Share ABAC write guards across channel and plugin save paths. Extract enforceAccessControlPolicyWriteGuards so plugin saves get value-holding validation when AttributeValueMasking is on (without store merge), keep non-sysadmin self-inclusion always on, and normalize empty plugin query users. Co-authored-by: Cursor * Finish ABAC test extraction after write-guard share. Drop duplicated self-inclusion cases from the ownership suite, remove the weaker users-slice test subsumed by gob coverage, and align the team self-inclusion comment with always-on helpers. Co-authored-by: Cursor * Stub QueryUsersForExpression in channel/team-admin ABAC save tests. Always-on self-inclusion in CreateOrUpdateAccessControlPolicy calls QueryUsersForExpression for non-sysadmins; existing API tests that save policies as channel or team admins panicked on the unstubbed mock. Co-authored-by: mattermost-code * Remove duplicate team self-inclusion check on policy save. CreateOrUpdateAccessControlPolicy already runs always-on self-inclusion via enforceAccessControlPolicyWriteGuards; the extra ValidateTeamAdminSelfInclusion loop double-queried expressions. HTTP still uses the team-specific check in the api4 handler. Co-authored-by: mattermost-code * Type CEL attribute names honestly and cover team self-inclusion through the shared write guard. Plugin proxies can omit attribute names, so the editor type now allows null/undefined instead of a string cast. The new team save test hits checkSelfInclusion with a real expression so QueryUsersForExpression is asserted once. Co-authored-by: Cursor * Satisfy eslint lines-around-comment on CELUserAttribute. Co-authored-by: Cursor --------- Co-authored-by: Cursor Co-authored-by: mattermost-code --- server/channels/api4/access_control_test.go | 16 +- server/channels/app/access_control.go | 138 ++++++++++-------- server/channels/app/access_control_test.go | 5 +- server/channels/app/plugin_access_control.go | 15 ++ .../app/plugin_access_control_gob_test.go | 27 ++++ .../app/plugin_access_control_save_test.go | 112 ++++++++++++++ .../app/plugin_access_control_test.go | 37 ++++- .../channels/app/team_access_control_test.go | 76 +++++++--- .../editors/cel_editor/editor.test.tsx | 11 ++ .../editors/cel_editor/editor.tsx | 12 +- .../modals/policy_test/test_modal.test.tsx | 16 ++ .../modals/policy_test/test_modal.tsx | 5 +- webapp/channels/src/entry.tsx | 4 +- 13 files changed, 384 insertions(+), 90 deletions(-) create mode 100644 server/channels/app/plugin_access_control_save_test.go diff --git a/server/channels/api4/access_control_test.go b/server/channels/api4/access_control_test.go index ec28c4d05baa..8a65b8966212 100644 --- a/server/channels/api4/access_control_test.go +++ b/server/channels/api4/access_control_test.go @@ -16,6 +16,15 @@ import ( "github.com/stretchr/testify/require" ) +// allowSelfInclusion stubs QueryUsersForExpression so checkSelfInclusion +// succeeds for userID. Maybe() so tests whose expressions skip the check +// ("true"/empty) still pass AssertExpectations. +func allowSelfInclusion(mockACS *mocks.AccessControlServiceInterface, userID string) { + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{{Id: userID}}, int64(1), nil). + Maybe() +} + // maskingOffTestConfig disables attribute-value masking for policy-endpoint // tests that do not cover masking. ABAC and other ABAC sub-flags default on. func maskingOffTestConfig(cfg *model.Config) { @@ -146,6 +155,7 @@ func TestCreateAccessControlPolicy(t *testing.T) { th.App.Srv().Channels().AccessControl = mockAccessControlService notFound := model.NewAppError("GetPolicy", "app.access_control.not_found.app_error", nil, "", http.StatusNotFound) mockAccessControlService.On("GetPolicy", mock.AnythingOfType("*request.Context"), privateChannel.Id).Return(nil, notFound) + allowSelfInclusion(mockAccessControlService, channelAdmin.Id) mockAccessControlService.On("SavePolicy", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.AccessControlPolicy")).Return(channelPolicy, nil).Times(1) th.App.UpdateConfig(func(cfg *model.Config) { @@ -270,6 +280,7 @@ func TestCreateAccessControlPolicy(t *testing.T) { // Set up mock expectations notFound := model.NewAppError("GetPolicy", "app.access_control.not_found.app_error", nil, "", http.StatusNotFound) mockAccessControlService.On("GetPolicy", mock.AnythingOfType("*request.Context"), ch.Id).Return(nil, notFound) + allowSelfInclusion(mockAccessControlService, th.BasicUser.Id) mockAccessControlService.On("SavePolicy", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.AccessControlPolicy")).Return(samplePolicy, nil).Times(1) // Set the mock on the app @@ -598,7 +609,9 @@ func TestCreateAccessControlPolicyPreservesSystemManagedFields(t *testing.T) { _, _, err := client.Login(context.Background(), channelAdmin.Email, channelAdmin.Password) require.NoError(t, err) - return privateChannel, client, enableABAC() + mockACS := enableABAC() + allowSelfInclusion(mockACS, channelAdmin.Id) + return privateChannel, client, mockACS } // Logs th.Client in as a team admin and stubs the per-rule self-inclusion check so the request @@ -2027,6 +2040,7 @@ func setupTeamAdminABAC(t *testing.T, th *TestHelper) *mocks.AccessControlServic }) th.AddPermissionToRole(t, model.PermissionManageTeamAccessRules.Id, model.TeamAdminRoleId) + allowSelfInclusion(mockACS, th.TeamAdminUser.Id) return mockACS } diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index 2c5795ed934c..fa63b9069639 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -156,70 +156,18 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model. } } - // Defense in depth: a team admin must remain within their own team policy's - // rules. The api4 handler enforces this for the request path, but guard here - // so any internal caller saving a team policy is held to the same invariant - // regardless of the masking flag. System admins and sessionless internal - // callers may intentionally set rules they don't match, mirroring the - // masking self-inclusion exemption below. - if policy.Type == model.AccessControlPolicyTypeTeam { - if session := rctx.Session(); session != nil && session.UserId != "" && !a.HasPermissionTo(rctx, session.UserId, model.PermissionManageSystem) { - for _, rule := range policy.Rules { - if appErr := a.ValidateTeamAdminSelfInclusion(rctx, session.UserId, rule.Expression); appErr != nil { - return nil, appErr - } - } - } + callerID := "" + if session := rctx.Session(); session != nil { + callerID = session.UserId } - - // ABAC is gated at route registration; only check masking here. Masking is - // attribute-based: edits are allowed with masked values present as long as - // the caller doesn't drop a condition holding values they couldn't see. - if a.Config().FeatureFlags.AttributeValueMasking { - session := rctx.Session() - if session == nil { - return nil, model.NewAppError("CreateOrUpdateAccessControlPolicy", "api.context.session_expired.app_error", nil, "session required for masking validation", http.StatusUnauthorized) - } - callerID := session.UserId - - resolver, appErr := newMaskingResolver(a, rctx, callerID) - if appErr != nil { - return nil, model.NewAppError("CreateOrUpdateAccessControlPolicy", "app.pap.save_policy.resolver_error", nil, "", http.StatusInternalServerError).Wrap(appErr) - } - - // Validate submitted values BEFORE merge: only the values the caller - // actually submitted should be checked against their holdings. Running - // validation after merge would reject the re-injected hidden values - // (e.g. Bravo, Charlie) that the caller legitimately cannot see. - appErr = a.validatePolicyExpressionValues(rctx, policy, resolver) - if appErr != nil { - return nil, appErr - } - - // Merge hidden values back in and block deletion of masked conditions. - mergedHidden, appErr := a.mergeStoredPolicyExpressions(rctx, policy, resolver) - if appErr != nil { - return nil, appErr - } - - // Guard against persisting the sentinel as a real value. - if appErr := rejectMaskedTokens(policy); appErr != nil { - return nil, appErr - } - - // Self-inclusion check applies only to non-admins. System admins may - // legitimately set conditions for attributes they do not personally hold - // (e.g., creating a "Clearance == Top Secret" rule without holding that - // clearance themselves). Masking and write-path value validation still - // apply to system admins above. - if !a.HasPermissionTo(rctx, callerID, model.PermissionManageSystem) { - if appErr := a.checkSelfInclusion(rctx, policy, callerID, mergedHidden); appErr != nil { - return nil, appErr - } - } + // Channel/team UI GET masks values the caller cannot see, so mergeFromStore + // re-injects those hidden literals before persist. + var appErr *model.AppError + policy, appErr = a.enforceAccessControlPolicyWriteGuards(rctx, policy, callerID, true) + if appErr != nil { + return nil, appErr } - var appErr *model.AppError policy, appErr = acs.SavePolicy(rctx, policy) if appErr != nil { return nil, appErr @@ -452,6 +400,74 @@ func saveForbiddenError(rctx request.CTX, where, internalReason string) *model.A return model.NewAppError(where, "app.pap.save_policy.forbidden", nil, "", http.StatusForbidden) } +// enforceAccessControlPolicyWriteGuards runs shared save-path invariants for +// channel/team (CreateOrUpdateAccessControlPolicy) and plugin-owned policies +// (SavePluginAccessControlPolicy). +// +// When AttributeValueMasking is on (applies to all callers, including system +// admins): +// 1. validatePolicyExpressionValues — submitted literals must be held by caller +// 2. mergeStoredPolicyExpressions — only when mergeFromStore is true +// (channel/team UI round-trips masked GET responses; plugin GET is unmasked, +// so plugin saves pass mergeFromStore=false) +// 3. rejectMaskedTokens — never persist the masking sentinel +// +// Self-inclusion always runs for non-sysadmins with a non-empty callerID, even +// when AttributeValueMasking is off. That matches product intent (a non-sysadmin +// cannot save a policy that excludes them) and keeps the plugin path from +// weakening when masking is disabled. Sessionless/internal callers with an empty +// callerID skip self-inclusion. System admins are exempt from self-inclusion but +// remain subject to value-holding validation when masking is on. +// +// Masking with an empty callerID is rejected (session required), matching the +// historical CreateOrUpdateAccessControlPolicy contract. +func (a *App) enforceAccessControlPolicyWriteGuards( + rctx request.CTX, + policy *model.AccessControlPolicy, + callerID string, + mergeFromStore bool, +) (*model.AccessControlPolicy, *model.AppError) { + mergedHidden := false + + if a.Config().FeatureFlags.AttributeValueMasking { + if callerID == "" { + return nil, model.NewAppError("enforceAccessControlPolicyWriteGuards", "api.context.session_expired.app_error", nil, "session required for masking validation", http.StatusUnauthorized) + } + + resolver, appErr := newMaskingResolver(a, rctx, callerID) + if appErr != nil { + return nil, model.NewAppError("enforceAccessControlPolicyWriteGuards", "app.pap.save_policy.resolver_error", nil, "", http.StatusInternalServerError).Wrap(appErr) + } + + // Validate submitted values BEFORE merge: only the values the caller + // actually submitted should be checked against their holdings. Running + // validation after merge would reject the re-injected hidden values + // (e.g. Bravo, Charlie) that the caller legitimately cannot see. + if appErr = a.validatePolicyExpressionValues(rctx, policy, resolver); appErr != nil { + return nil, appErr + } + + if mergeFromStore { + mergedHidden, appErr = a.mergeStoredPolicyExpressions(rctx, policy, resolver) + if appErr != nil { + return nil, appErr + } + } + + if appErr := rejectMaskedTokens(policy); appErr != nil { + return nil, appErr + } + } + + if callerID != "" && !a.HasPermissionTo(rctx, callerID, model.PermissionManageSystem) { + if appErr := a.checkSelfInclusion(rctx, policy, callerID, mergedHidden); appErr != nil { + return nil, appErr + } + } + + return policy, nil +} + // checkSelfInclusion verifies the caller satisfies all policy rules after their edit. // When mergedHidden is true (hidden values were re-injected), a self-exclusion failure // returns the generic forbidden error; otherwise the specific self_exclusion error is used. diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index 96abcaf234d6..82e200196371 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -460,9 +460,8 @@ func TestDeleteAccessControlPolicy(t *testing.T) { // TestCheckSelfInclusion verifies the self-exclusion guard: non-admin callers must // satisfy their own policy after saving, or the save is refused with 403 -// self_exclusion. Sysadmins are exempt at the call site -// (CreateOrUpdateAccessControlPolicy), not inside checkSelfInclusion itself — this -// test exercises the function directly. +// self_exclusion. Sysadmins are exempt in enforceAccessControlPolicyWriteGuards, +// not inside checkSelfInclusion itself — this test exercises the function directly. func TestCheckSelfInclusion(t *testing.T) { t.Run("caller who satisfies the policy passes", func(t *testing.T) { th := Setup(t).InitBasic(t) diff --git a/server/channels/app/plugin_access_control.go b/server/channels/app/plugin_access_control.go index 2d5d046b65ef..1c1f200b0113 100644 --- a/server/channels/app/plugin_access_control.go +++ b/server/channels/app/plugin_access_control.go @@ -160,6 +160,13 @@ func (a *App) EvaluatePluginAccessRequest(rctx request.CTX, pluginID, userID, re // control policy. Version is forced to v0.5 and Active to true (plugin types // have no separate activation lifecycle); policy.ID must be the resource's // stable ID. +// +// Write guards go through enforceAccessControlPolicyWriteGuards with +// mergeFromStore=false: plugin GET returns unmasked policies, so there is no +// masked-value round-trip to repair. Non-system-admin acting users must always +// satisfy the saved expression (self-inclusion), including when +// AttributeValueMasking is off. System admins may author policies they do not +// match, but remain subject to value-holding validation when masking is on. func (a *App) SavePluginAccessControlPolicy(rctx request.CTX, pluginID, actingUserID string, policy *model.AccessControlPolicy) (*model.AccessControlPolicy, *model.AppError) { // Audit every attempt, including precondition failures. auditRec := a.MakeAuditRecord(rctx, model.AuditEventSavePluginAccessControlPolicy, model.AuditStatusFail) @@ -215,6 +222,10 @@ func (a *App) SavePluginAccessControlPolicy(rctx request.CTX, pluginID, actingUs return nil, appErr } + if _, appErr := a.enforceAccessControlPolicyWriteGuards(rctx, policy, actingUserID, false); appErr != nil { + return nil, appErr + } + // Enterprise SavePolicy derives the caller ID from the session, so // synthesize one for the acting user. saveCtx := rctx.WithSession(&model.Session{UserId: actingUserID}) @@ -399,6 +410,10 @@ func (a *App) QueryUsersForPluginAccessControlExpression(rctx request.CTX, plugi if appErr != nil { return nil, appErr } + // Normalize at the producer so gob/JSON never serialize Users as null. + if users == nil { + users = []*model.User{} + } return &model.AccessControlPolicyTestResponse{Users: users, Total: total}, nil } diff --git a/server/channels/app/plugin_access_control_gob_test.go b/server/channels/app/plugin_access_control_gob_test.go index ddc22c299e07..6af1f138634f 100644 --- a/server/channels/app/plugin_access_control_gob_test.go +++ b/server/channels/app/plugin_access_control_gob_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/mattermost/mattermost/server/public/model" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/v8/einterfaces/mocks" @@ -88,6 +89,32 @@ func TestPluginAccessControlGobSafety(t *testing.T) { requirePluginRPCGobSafe(t, &model.AccessControlPolicyTestResponse{Users: []*model.User{th.BasicUser}, Total: 1}) }) + t.Run("query users empty result is non-nil at producer and gob-safe", func(t *testing.T) { + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return(([]*model.User)(nil), int64(0), nil).Once() + + resp, appErr := th.App.QueryUsersForPluginAccessControlExpression( + th.Context, testAgentsPluginID, th.BasicUser.Id, testAgentResourceType, + `user.attributes.dept == "eng"`, "", "", 10) + require.Nil(t, appErr) + // Producer normalizes nil → empty slice so JSON/in-process callers never + // see null users. encoding/gob still decodes empty slices as nil; host + // and plugin UIs keep `users ?? []` as belt-and-braces for that quirk. + require.NotNil(t, resp.Users) + require.Empty(t, resp.Users) + requirePluginRPCGobSafe(t, resp) + + var decoded model.AccessControlPolicyTestResponse + var buf bytes.Buffer + require.NoError(t, gob.NewEncoder(&buf).Encode(resp)) + require.NoError(t, gob.NewDecoder(&buf).Decode(&decoded)) + require.Equal(t, int64(0), decoded.Total) + require.Empty(t, decoded.Users) + mockACS.AssertExpectations(t) + }) + t.Run("evaluation decision carrying a context reason", func(t *testing.T) { decision := model.NewNoPolicyAccessDecision() requirePluginRPCGobSafe(t, &decision) diff --git a/server/channels/app/plugin_access_control_save_test.go b/server/channels/app/plugin_access_control_save_test.go new file mode 100644 index 000000000000..aaac229fa9fe --- /dev/null +++ b/server/channels/app/plugin_access_control_save_test.go @@ -0,0 +1,112 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "net/http" + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/v8/einterfaces/mocks" +) + +func TestSavePluginAccessControlPolicyWriteGuards(t *testing.T) { + notFoundErr := model.NewAppError("GetPolicy", "app.pap.get_policy.app_error", nil, "", http.StatusNotFound) + + t.Run("non-sysadmin self-excluding expression rejected", func(t *testing.T) { + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.AttributeValueMasking = false + }).InitBasic(t) + actingUserID := th.BasicUser.Id + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + p := validPluginPolicy(model.NewId()) + mockACS.On("GetPolicy", mock.Anything, p.ID).Return(nil, notFoundErr).Once() + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{}, int64(0), nil).Once() + + _, appErr := th.App.SavePluginAccessControlPolicy(th.Context, testAgentsPluginID, actingUserID, p) + require.NotNil(t, appErr) + assert.Equal(t, http.StatusForbidden, appErr.StatusCode) + assert.Equal(t, "app.pap.save_policy.self_exclusion", appErr.Id) + mockACS.AssertNotCalled(t, "SavePolicy", mock.Anything, mock.Anything) + mockACS.AssertExpectations(t) + }) + + t.Run("sysadmin may save self-excluding expression", func(t *testing.T) { + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.AttributeValueMasking = false + }).InitBasic(t) + adminID := th.SystemAdminUser.Id + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + p := validPluginPolicy(model.NewId()) + mockACS.On("GetPolicy", mock.Anything, p.ID).Return(nil, notFoundErr).Once() + mockACS.On("SavePolicy", mock.MatchedBy(func(c request.CTX) bool { + return c.Session() != nil && c.Session().UserId == adminID + }), mock.Anything).Return(p, nil).Once() + + _, appErr := th.App.SavePluginAccessControlPolicy(th.Context, testAgentsPluginID, adminID, p) + require.Nil(t, appErr) + mockACS.AssertNotCalled(t, "QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything) + mockACS.AssertExpectations(t) + }) + + t.Run("masking on: non-admin cannot submit unheld literals", func(t *testing.T) { + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.AttributeValueMasking = true + }).InitBasic(t) + actingUserID := th.BasicUser.Id + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + p := validPluginPolicy(model.NewId()) + valueErr := model.NewAppError("ValidateExpressionValuesForCaller", "app.pap.save_policy.forbidden", nil, "", http.StatusForbidden) + mockACS.On("GetPolicy", mock.Anything, p.ID).Return(nil, notFoundErr).Once() + mockACS.On("ValidateExpressionValuesForCaller", mock.Anything, p.Rules[0].Expression, mock.Anything). + Return(valueErr).Once() + + _, appErr := th.App.SavePluginAccessControlPolicy(th.Context, testAgentsPluginID, actingUserID, p) + require.NotNil(t, appErr) + assert.Equal(t, http.StatusForbidden, appErr.StatusCode) + assert.Equal(t, "app.pap.save_policy.forbidden", appErr.Id) + mockACS.AssertNotCalled(t, "SavePolicy", mock.Anything, mock.Anything) + mockACS.AssertNotCalled(t, "MergeExpressionWithMaskedValuesCanonical", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockACS.AssertNotCalled(t, "QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything) + mockACS.AssertExpectations(t) + }) + + t.Run("masking on: plugin path skips store merge", func(t *testing.T) { + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.AttributeValueMasking = true + }).InitBasic(t) + actingUserID := th.BasicUser.Id + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + p := validPluginPolicy(model.NewId()) + mockACS.On("GetPolicy", mock.Anything, p.ID).Return(nil, notFoundErr).Once() + mockACS.On("ValidateExpressionValuesForCaller", mock.Anything, p.Rules[0].Expression, mock.Anything). + Return(nil).Once() + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{{Id: actingUserID}}, int64(1), nil).Once() + mockACS.On("SavePolicy", mock.Anything, mock.Anything).Return(p, nil).Once() + + _, appErr := th.App.SavePluginAccessControlPolicy(th.Context, testAgentsPluginID, actingUserID, p) + require.Nil(t, appErr) + mockACS.AssertNotCalled(t, "MergeExpressionWithMaskedValuesCanonical", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockACS.AssertExpectations(t) + }) +} diff --git a/server/channels/app/plugin_access_control_test.go b/server/channels/app/plugin_access_control_test.go index 75ae5e73c889..6baa61f15e11 100644 --- a/server/channels/app/plugin_access_control_test.go +++ b/server/channels/app/plugin_access_control_test.go @@ -358,7 +358,12 @@ func TestEvaluatePluginAccessRequestStoreError(t *testing.T) { } func TestSavePluginAccessControlPolicy(t *testing.T) { - th := Setup(t).InitBasic(t) + // Write-guard self-inclusion and masking cases live in + // plugin_access_control_save_test.go. Keep this suite on ownership/versioning + // with masking off. + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.AttributeValueMasking = false + }).InitBasic(t) actingUserID := th.BasicUser.Id notFoundErr := model.NewAppError("GetPolicy", "app.pap.get_policy.app_error", nil, "", http.StatusNotFound) @@ -426,6 +431,8 @@ func TestSavePluginAccessControlPolicy(t *testing.T) { expression := p.Rules[0].Expression mockACS.On("GetPolicy", mock.Anything, p.ID).Return(nil, notFoundErr).Once() + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{{Id: actingUserID}}, int64(1), nil).Once() mockACS.On("SavePolicy", mock.MatchedBy(func(c request.CTX) bool { return c.Session() != nil && c.Session().UserId == actingUserID }), mock.MatchedBy(func(saved *model.AccessControlPolicy) bool { @@ -450,6 +457,8 @@ func TestSavePluginAccessControlPolicy(t *testing.T) { p := validPluginPolicy(model.NewId()) existing := validPluginPolicy(p.ID) mockACS.On("GetPolicy", mock.Anything, p.ID).Return(existing, nil).Once() + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{{Id: actingUserID}}, int64(1), nil).Once() mockACS.On("SavePolicy", mock.Anything, mock.Anything).Return(p, nil).Once() _, appErr := th.App.SavePluginAccessControlPolicy(th.Context, testAgentsPluginID, actingUserID, p) @@ -995,7 +1004,9 @@ func auditParam(t *testing.T, rec map[string]any, key string) any { // from entry and refines it to "create"/"update" once the existence probe // resolves; Delete stamps "delete". func TestPluginAccessControlAudit(t *testing.T) { - th := Setup(t).InitBasic(t) + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.AttributeValueMasking = false + }).InitBasic(t) capture := startPluginAuditCapture(t, th) actingUserID := th.BasicUser.Id @@ -1094,6 +1105,8 @@ func TestPluginAccessControlAudit(t *testing.T) { p := validPluginPolicy(model.NewId()) mockACS.On("GetPolicy", mock.Anything, p.ID). Return(nil, model.NewAppError("GetPolicy", "app.pap.get_policy.app_error", nil, "", http.StatusNotFound)).Once() + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{{Id: actingUserID}}, int64(1), nil).Once() mockACS.On("SavePolicy", mock.Anything, mock.Anything).Return(p, nil).Once() rec := assertNextRecord(t, model.AuditEventSavePluginAccessControlPolicy, model.AuditStatusSuccess, func() { @@ -1104,6 +1117,26 @@ func TestPluginAccessControlAudit(t *testing.T) { mockACS.AssertExpectations(t) }) + t.Run("save: self-exclusion audits as fail", func(t *testing.T) { + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + p := validPluginPolicy(model.NewId()) + mockACS.On("GetPolicy", mock.Anything, p.ID). + Return(nil, model.NewAppError("GetPolicy", "app.pap.get_policy.app_error", nil, "", http.StatusNotFound)).Once() + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{}, int64(0), nil).Once() + + rec := assertNextRecord(t, model.AuditEventSavePluginAccessControlPolicy, model.AuditStatusFail, func() { + _, appErr := th.App.SavePluginAccessControlPolicy(th.Context, testAgentsPluginID, actingUserID, p) + require.NotNil(t, appErr) + assert.Equal(t, "app.pap.save_policy.self_exclusion", appErr.Id) + }) + assert.Equal(t, "create", auditParam(t, rec, "operation")) + mockACS.AssertNotCalled(t, "SavePolicy", mock.Anything, mock.Anything) + mockACS.AssertExpectations(t) + }) + t.Run("delete: service unavailable still audits as fail", func(t *testing.T) { th.App.Srv().ch.AccessControl = nil rec := assertNextRecord(t, model.AuditEventDeletePluginAccessControlPolicy, model.AuditStatusFail, func() { diff --git a/server/channels/app/team_access_control_test.go b/server/channels/app/team_access_control_test.go index f6002d1f3cfe..c015f254e460 100644 --- a/server/channels/app/team_access_control_test.go +++ b/server/channels/app/team_access_control_test.go @@ -646,10 +646,9 @@ func TestReconcilePolicyTeamScope(t *testing.T) { }) } -// TestCreateOrUpdateAccessControlPolicy_TeamSelfInclusion verifies that the -// team-specific self-inclusion guard at CreateOrUpdateAccessControlPolicy:124 -// is correctly wired into the create/update path. ValidateTeamAdminSelfInclusion -// is tested in isolation elsewhere; these tests confirm the integration. +// TestCreateOrUpdateAccessControlPolicy_TeamSelfInclusion verifies that +// enforceAccessControlPolicyWriteGuards self-inclusion applies to team policy +// saves. HTTP still uses ValidateTeamAdminSelfInclusion in the api4 handler. func TestCreateOrUpdateAccessControlPolicy_TeamSelfInclusion(t *testing.T) { t.Run("team admin excluded by own expression is rejected before save", func(t *testing.T) { // AttributeValueMasking defaults to true @@ -695,7 +694,7 @@ func TestCreateOrUpdateAccessControlPolicy_TeamSelfInclusion(t *testing.T) { result, appErr := th.App.CreateOrUpdateAccessControlPolicy(rctx, teamPolicy) require.NotNil(t, appErr) - assert.Equal(t, "app.team.access_policies.self_exclusion.app_error", appErr.Id) + assert.Equal(t, "app.pap.save_policy.self_exclusion", appErr.Id) assert.Nil(t, result) }) @@ -705,9 +704,8 @@ func TestCreateOrUpdateAccessControlPolicy_TeamSelfInclusion(t *testing.T) { cfg.FeatureFlags.AttributeValueMasking = false }).InitBasic(t) - callerID := th.BasicUser.Id rctx := th.Context.WithSession(&model.Session{ - UserId: callerID, + UserId: th.BasicUser.Id, Id: model.NewId(), Roles: model.SystemUserRoleId, }) @@ -720,15 +718,6 @@ func TestCreateOrUpdateAccessControlPolicy_TeamSelfInclusion(t *testing.T) { mockACS.AssertExpectations(t) }) - // Guard: caller satisfies the expression → guard passes. - mockACS.On("QueryUsersForExpression", - mock.AnythingOfType("*request.Context"), - "true", - mock.MatchedBy(func(opts model.SubjectSearchOptions) bool { - return opts.SubjectID == callerID - }), - ).Return([]*model.User{{Id: callerID}}, int64(1), nil).Once() - savedPolicy := &model.AccessControlPolicy{ ID: th.BasicTeam.Id, Type: model.AccessControlPolicyTypeTeam, @@ -736,7 +725,7 @@ func TestCreateOrUpdateAccessControlPolicy_TeamSelfInclusion(t *testing.T) { {Actions: []string{"membership"}, Expression: "true"}, }, } - // SavePolicy is reached after the guard passes. + // "true" skips checkSelfInclusion; SavePolicy is reached without a query. mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), mock.Anything). Return(savedPolicy, (*model.AppError)(nil)).Once() @@ -753,6 +742,59 @@ func TestCreateOrUpdateAccessControlPolicy_TeamSelfInclusion(t *testing.T) { require.NotNil(t, result) }) + t.Run("team admin included by a non-true expression runs the shared guard and saves", func(t *testing.T) { + th := SetupConfig(t, func(cfg *model.Config) { + *cfg.AccessControlSettings.EnableAttributeBasedAccessControl = true + cfg.FeatureFlags.AttributeValueMasking = false + }).InitBasic(t) + + callerID := th.BasicUser.Id + rctx := th.Context.WithSession(&model.Session{ + UserId: callerID, + Id: model.NewId(), + Roles: model.SystemUserRoleId, // not a system admin + }) + + mockACS := &mocks.AccessControlServiceInterface{} + originalACS := th.App.Srv().ch.AccessControl + th.App.Srv().ch.AccessControl = mockACS + t.Cleanup(func() { + th.App.Srv().ch.AccessControl = originalACS + mockACS.AssertExpectations(t) + }) + + expression := `user.attributes.dept == "eng"` + savedPolicy := &model.AccessControlPolicy{ + ID: th.BasicTeam.Id, + Type: model.AccessControlPolicyTypeTeam, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"membership"}, Expression: expression}, + }, + } + // Non-"true" expression must go through checkSelfInclusion. + mockACS.On("QueryUsersForExpression", + mock.AnythingOfType("*request.Context"), + expression, + mock.MatchedBy(func(opts model.SubjectSearchOptions) bool { + return opts.SubjectID == callerID + }), + ).Return([]*model.User{{Id: callerID}}, int64(1), nil).Once() + mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), mock.Anything). + Return(savedPolicy, (*model.AppError)(nil)).Once() + + teamPolicy := &model.AccessControlPolicy{ + ID: th.BasicTeam.Id, + Type: model.AccessControlPolicyTypeTeam, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"membership"}, Expression: expression}, + }, + } + + result, appErr := th.App.CreateOrUpdateAccessControlPolicy(rctx, teamPolicy) + require.Nil(t, appErr) + require.NotNil(t, result) + }) + t.Run("system admin bypasses the team self-inclusion guard", func(t *testing.T) { th := SetupConfig(t, func(cfg *model.Config) { *cfg.AccessControlSettings.EnableAttributeBasedAccessControl = true diff --git a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsx b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsx index 7a173d800f6c..79c59303fe5b 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsx @@ -203,4 +203,15 @@ describe('buildCELSchemas', () => { expect(schemas['user.attributes']).toEqual(['valid']); expect(schemas['user.session']).toEqual(['ip_address']); }); + + test('skips null or undefined attribute names without throwing', () => { + const schemas = buildCELSchemas([ + {attribute: null, values: [], objectType: 'user'}, + {attribute: undefined, values: [], isNative: true}, + {attribute: 'department', values: [], objectType: 'user'}, + ]); + + expect(schemas.user).toEqual(['attributes']); + expect(schemas['user.attributes']).toEqual(['department']); + }); }); diff --git a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx index 29732da90281..468db312f681 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx @@ -69,11 +69,14 @@ const MONACO_EDITOR_OPTIONS: monaco.editor.IStandaloneEditorConstructionOptions }; type CELUserAttribute = { - attribute: string; + + // Plugins may pass incomplete proxy rows; the host helper + // toCELEditorAttributes does not always run first. + attribute?: string | null; values: string[]; // 'session' marks a user.session.* attribute; 'user' marks a user.* / - // user.attributes.* attribute. Always populated by toCELEditorAttributes. + // user.attributes.* attribute. Plugin proxies may omit this. objectType?: string; // Native user attributes (e.g. user.email) complete directly off `user.` @@ -86,9 +89,12 @@ type CELUserAttribute = { // offered under user.session.* — the session bucket only appears when present. // Native attributes (isNative) complete directly off user.* (e.g. user.email). export function buildCELSchemas(userAttributes: CELUserAttribute[]): Record { + // Skip null/undefined/invalid names. Plugin proxies may pass incomplete + // rows that never went through toCELEditorAttributes; name.includes would + // throw during render. const cleanNames = (attrs: CELUserAttribute[]) => attrs. map((attr) => attr.attribute). - filter((name) => !name.includes(' ') && name.trim() !== ''); + filter((name): name is string => typeof name === 'string' && !name.includes(' ') && name.trim() !== ''); const sessionAttrNames = cleanNames(userAttributes.filter((attr) => attr.objectType === SESSION_ATTRIBUTES_OBJECT_TYPE)); const userAttrs = userAttributes.filter((attr) => !attr.objectType || attr.objectType === USER_OBJECT_TYPE); const nativeNames = cleanNames(userAttrs.filter((attr) => attr.isNative)); diff --git a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.test.tsx b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.test.tsx index 3fbe2ca8f2e2..a547f7730a5f 100644 --- a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.test.tsx @@ -229,6 +229,22 @@ describe('TestResultsModal', () => { }); }); + it('should treat null users as an empty list', async () => { + // Plugin RPC (gob) can serialize an empty users slice as null. + mockSearchUsers.mockReturnValue(() => Promise.resolve({ + data: { + users: null, + total: 0, + }, + })); + + renderWithContext(); + + await waitFor(() => { + expect(screen.getByTestId('user-count')).toHaveTextContent('Showing 0 of 0 users'); + }); + }); + it('should handle search error gracefully', async () => { mockSearchUsers.mockReturnValue(() => Promise.resolve({ error: 'Search failed', diff --git a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.tsx b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.tsx index 527bd641043c..24a0b2bd33f8 100644 --- a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.tsx +++ b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.tsx @@ -74,7 +74,8 @@ function TestResultsModal({ return; } if (result?.data) { - const newUsers = result.data.users; + // Plugin RPC (gob) can turn an empty users slice into null on the wire. + const newUsers = result.data.users ?? []; if (reset) { setUsers(newUsers); } else { @@ -92,7 +93,7 @@ function TestResultsModal({ // The picker step defers the initial fetch until a channel is chosen // (handled in handleChannelSelected). if (!requireChannel) { - fetchUsers('', ''); + fetchUsers(term, '', true); } }, []); diff --git a/webapp/channels/src/entry.tsx b/webapp/channels/src/entry.tsx index db0c05714814..3a9bc2675625 100644 --- a/webapp/channels/src/entry.tsx +++ b/webapp/channels/src/entry.tsx @@ -32,7 +32,9 @@ declare global { // This runs before we start to render anything. function preRenderSetup(onPreRenderSetupReady: () => void) { window.onerror = (msg, url, line, column, error) => { - if (msg === 'ResizeObserver loop limit exceeded') { + // Benign Chromium ResizeObserver noise (Monaco automaticLayout, etc.). + // Covers both "loop limit exceeded" and "loop completed with undelivered notifications." + if (typeof msg === 'string' && msg.startsWith('ResizeObserver loop')) { return; } From 235bddbae0c4c387c07b918f4bf59d0ebeb6001c Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 2 Sep 2026 11:44:15 -0300 Subject: [PATCH 3/4] Hand Cypress E2E deps to workers as a run-scoped artifact (#38253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit prep-deps wrote node_modules + the Cypress binary to a shared Actions cache keyed on the cypress lockfile, and the 40 workers read it back within the same run. The repo is pinned at the 10 GB Actions cache ceiling (100 entries, 9.99 GB), so LRU eviction runs continuously. Every distinct cypress lockfile a run happens to build costs a ~275 MB entry in that budget — four are live today, 1.10 GB total — and they evict entries other jobs depend on: the webapp node_modules cache (3.7 GB), the Playwright deps cache, and the E2E npm registry cache. Build the deps once in prep-deps and upload them as a run-scoped artifact instead. The handoff is unchanged in shape — one producer, N consumers, within a run — but it costs the shared cache nothing, and it drops the fail-on-cache-miss cliff the workers previously depended on. The artifact name is scoped by edition: e2e-tests-ci.yml invokes this template twice per workflow run (default and fips) and artifact names are run-scoped, so the two invocations would otherwise collide. prep-deps loses its cache-hit shortcut and now always installs, so it restores the master-warmed ~/.npm registry cache read-only to keep that install short, and its timeout goes 10 -> 15 minutes. Playwright still uses the cache handoff; it is a larger payload with a churnier key and follows separately. --- .github/actions/pack-cypress-deps/action.yml | 40 +++++++++++++++++ .../actions/unpack-cypress-deps/action.yml | 27 ++++++++++++ .../workflows/e2e-tests-cypress-template.yml | 43 +++++++++---------- 3 files changed, 87 insertions(+), 23 deletions(-) create mode 100644 .github/actions/pack-cypress-deps/action.yml create mode 100644 .github/actions/unpack-cypress-deps/action.yml diff --git a/.github/actions/pack-cypress-deps/action.yml b/.github/actions/pack-cypress-deps/action.yml new file mode 100644 index 000000000000..cdd970f0d052 --- /dev/null +++ b/.github/actions/pack-cypress-deps/action.yml @@ -0,0 +1,40 @@ +name: "Pack Cypress deps artifact" +description: > + Tar the Cypress node_modules and binary into a run-scoped artifact for the + workers in e2e-tests-cypress-template.yml. The payload must stay in sync with + unpack-cypress-deps. + +inputs: + fips: + description: > + Whether this is a FIPS-edition run. e2e-tests-ci.yml invokes the cypress + template twice per workflow run — once per edition — and artifact names are + run-scoped, so the two invocations would otherwise collide. Must match the + unpack step. + required: false + default: "false" + +runs: + using: "composite" + steps: + # -C so the archives hold paths relative to their extraction root, letting + # unpack extract straight into the workspace and HOME. + - name: ci/pack-cypress-deps + shell: bash + run: | + set -euo pipefail + tar -czf "${RUNNER_TEMP}/cypress-node-modules.tgz" -C "${GITHUB_WORKSPACE}" e2e-tests/cypress/node_modules + tar -czf "${RUNNER_TEMP}/cypress-binary.tgz" -C "${HOME}" .cache/Cypress + - name: ci/upload-cypress-deps + uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 + with: + name: e2e-cypress-deps${{ inputs.fips == 'true' && '-fips' || '' }} + path: | + ${{ runner.temp }}/cypress-node-modules.tgz + ${{ runner.temp }}/cypress-binary.tgz + # Only ever consumed by workers in the same run. + retention-days: 1 + if-no-files-found: error + # The tarballs are already gzipped; re-compressing them in the zip + # wrapper costs CPU for no gain. + compression-level: 0 diff --git a/.github/actions/unpack-cypress-deps/action.yml b/.github/actions/unpack-cypress-deps/action.yml new file mode 100644 index 000000000000..b52958a9f2fa --- /dev/null +++ b/.github/actions/unpack-cypress-deps/action.yml @@ -0,0 +1,27 @@ +name: "Unpack Cypress deps artifact" +description: > + Download and extract the run-scoped artifact built by pack-cypress-deps, + restoring the Cypress node_modules and binary into place. + +inputs: + fips: + description: > + Whether this is a FIPS-edition run. Must match the pack step so the correct + artifact is downloaded. + required: false + default: "false" + +runs: + using: "composite" + steps: + - name: ci/download-cypress-deps + uses: actions/download-artifact@70fc10c6e5e1ce46ad2ea6f2b72d43f7d47b13c3 # v8.0.0 + with: + name: e2e-cypress-deps${{ inputs.fips == 'true' && '-fips' || '' }} + path: ${{ runner.temp }} + - name: ci/unpack-cypress-deps + shell: bash + run: | + set -euo pipefail + tar -xzf "${RUNNER_TEMP}/cypress-node-modules.tgz" -C "${GITHUB_WORKSPACE}" + tar -xzf "${RUNNER_TEMP}/cypress-binary.tgz" -C "${HOME}" diff --git a/.github/workflows/e2e-tests-cypress-template.yml b/.github/workflows/e2e-tests-cypress-template.yml index a7da8b7bc7ca..67f3d6f0a852 100644 --- a/.github/workflows/e2e-tests-cypress-template.yml +++ b/.github/workflows/e2e-tests-cypress-template.yml @@ -198,13 +198,12 @@ jobs: echo "workers=$(jq -nc --argjson n "${INPUT_WORKERS}" '[range(1; $n+1)]')" >> $GITHUB_OUTPUT echo "start_time=$(date +%s)" >> $GITHUB_OUTPUT - # Install webapp node_modules once via the shared webapp-setup action, then - # workers restore the same stable cache. The node_modules cache is keyed only - # on webapp/package-lock.json and is shared with webapp-ci.yml jobs. + # Build the Cypress deps once and hand them to the workers as a run-scoped + # artifact. prep-deps: name: prep-deps runs-on: ubuntu-24.04 - timeout-minutes: 10 + timeout-minutes: 15 permissions: contents: read steps: @@ -214,23 +213,23 @@ jobs: persist-credentials: false ref: ${{ inputs.commit_sha }} fetch-depth: 1 + # Provides the @mattermost/eslint-plugin target the cypress install + # symlinks to. - name: ci/setup-webapp-node-modules uses: ./.github/actions/webapp-setup - - name: ci/cache-cypress-deps - # node_modules + the cypress binary (downloaded to ~/.cache/Cypress by - # cypress's postinstall, not into node_modules). Both must be cached; - # otherwise workers see "cypress npm package installed but binary missing". - id: cache-cypress - uses: actions/cache@cdf6c1fa76f9f475f3d7449005a359c84ca0f306 # v5.0.3 - with: - path: | - e2e-tests/cypress/node_modules - ~/.cache/Cypress - key: e2e-cypress-deps-${{ runner.os }}-${{ hashFiles('e2e-tests/cypress/package-lock.json') }} + # Read-only restore of the registry cache warmed daily from master, so the + # install below refetches as little as possible. + - name: ci/restore-npm-cache + uses: ./.github/actions/restore-e2e-npm-cache + # The postinstall downloads the Cypress binary to ~/.cache/Cypress rather + # than into node_modules, which is why the artifact carries both. - name: ci/install-cypress-deps - if: steps.cache-cypress.outputs.cache-hit != 'true' working-directory: e2e-tests/cypress run: npm ci + - name: ci/pack-cypress-deps + uses: ./.github/actions/pack-cypress-deps + with: + fips: ${{ inputs.server_edition == 'fips' }} # Register the Test System IO run AFTER prep-deps so workers reach # dispatch-run within Test System IO's inactivity window. @@ -317,14 +316,12 @@ jobs: uses: ./.github/actions/webapp-setup with: read-only: "true" - - name: ci/restore-cypress-deps - uses: actions/cache/restore@cdf6c1fa76f9f475f3d7449005a359c84ca0f306 # v5.0.3 + # prep-deps built these; it is an ancestor of this job via dispatch-begin, + # so the artifact is always present by now. + - name: ci/reuse-cypress-deps + uses: ./.github/actions/unpack-cypress-deps with: - path: | - e2e-tests/cypress/node_modules - ~/.cache/Cypress - key: e2e-cypress-deps-${{ runner.os }}-${{ hashFiles('e2e-tests/cypress/package-lock.json') }} - fail-on-cache-miss: true + fips: ${{ inputs.server_edition == 'fips' }} - name: ci/cloud-init working-directory: e2e-tests run: make cloud-init From f6f2719165ffb132e351342add570f3b1b6fd07a Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 2 Sep 2026 13:53:32 -0300 Subject: [PATCH 4/4] Fix props, refs and keys that React was warning about (#38238) * webapp: let Tag forward a ref WithTooltip anchors its popover by cloning its child with a ref. Tag is a function component, so the ref was dropped and React warned about it, leaving the marketplace label tooltips without a positioning reference. Co-authored-by: Jesse Hallam (cherry picked from commit 3a34196e47a9a875a8ef4b68227d9c3a94045725) * webapp: stop leaking component props onto DOM elements isReadonly was hardcoded to false, never read, and fell through a prop spread onto the channel header menu's
  • . SearchChannelSuggestion was connected with a null mapDispatchToProps, so react-redux injected dispatch, which the suggestion container then spread onto its
  • . Co-authored-by: Jesse Hallam (cherry picked from commit 959ba3b90d997be0cc0df6063a6dc07d626f2a6c) * webapp: stop connect's injected dispatch reaching suggestion list items Suggestions are connected components that forward their rest props into SuggestionContainer, which spreads them onto its
  • . A connect() without a mapDispatchToProps injects dispatch, so React saw it as an invalid DOM attribute. Drop it in the container alongside item, which covers every suggestion rather than one call site. Co-authored-by: Jesse Hallam (cherry picked from commit 41d09dfbfd694bdadcecc44b636f20630137984f) * webapp: key the add-workspace menu on a remote cluster's full identity RemoteClusters is keyed on (RemoteId, Name), so the same remote_id can appear more than once in the dropdown and React reported duplicate keys. Co-authored-by: Jesse Hallam (cherry picked from commit a4025d52718aa7504d2202101a8b9299daa27498) * fixup! webapp: let Tag forward a ref --------- Co-authored-by: Cursor Agent Co-authored-by: Jesse Hallam --- .../channel_header_menu/channel_header_menu.tsx | 3 --- .../channel_header_public_private_menu.tsx | 1 - .../add_workspace_dropdown.tsx | 5 +++-- .../channels/src/components/suggestion/suggestion.tsx | 4 ++++ webapp/channels/src/components/widgets/tag/tag.tsx | 11 ++++++++--- 5 files changed, 15 insertions(+), 9 deletions(-) diff --git a/webapp/channels/src/components/channel_header_menu/channel_header_menu.tsx b/webapp/channels/src/components/channel_header_menu/channel_header_menu.tsx index ab1a2f5fc527..c7d6f2badb16 100644 --- a/webapp/channels/src/components/channel_header_menu/channel_header_menu.tsx +++ b/webapp/channels/src/components/channel_header_menu/channel_header_menu.tsx @@ -62,8 +62,6 @@ export default function ChannelHeaderMenu({dmUser, gmMembers, isMobile, archived const isChannelBookmarksEnabled = useSelector(getIsChannelBookmarksEnabled); const isChannelAutotranslated = useSelector((state: GlobalState) => (channel?.id ? isChannelAutotranslatedSelector(state, channel.id) : false)); - const isReadonly = false; - if (!channel) { return null; } @@ -181,7 +179,6 @@ export default function ChannelHeaderMenu({dmUser, gmMembers, isMobile, archived isFavorite={isFavorite} isMobile={isMobile || false} isDefault={isDefault} - isReadonly={isReadonly} isLicensedForLDAPGroups={isLicensedForLDAPGroups} isChannelBookmarksEnabled={isChannelBookmarksEnabled} isChannelAutotranslated={isChannelAutotranslated} diff --git a/webapp/channels/src/components/channel_header_menu/channel_header_menu_items/channel_header_public_private_menu.tsx b/webapp/channels/src/components/channel_header_menu/channel_header_menu_items/channel_header_public_private_menu.tsx index 6762597792de..56203be2868c 100644 --- a/webapp/channels/src/components/channel_header_menu/channel_header_menu_items/channel_header_public_private_menu.tsx +++ b/webapp/channels/src/components/channel_header_menu/channel_header_menu_items/channel_header_public_private_menu.tsx @@ -37,7 +37,6 @@ interface Props extends Menu.FirstMenuItemProps { channel: Channel; user: UserProfile; isMuted: boolean; - isReadonly: boolean; isDefault: boolean; isMobile: boolean; isFavorite: boolean; diff --git a/webapp/channels/src/components/channel_settings_modal/share_channel_with_workspaces/add_workspace_dropdown.tsx b/webapp/channels/src/components/channel_settings_modal/share_channel_with_workspaces/add_workspace_dropdown.tsx index c2222068bef2..48b3e01b768e 100644 --- a/webapp/channels/src/components/channel_settings_modal/share_channel_with_workspaces/add_workspace_dropdown.tsx +++ b/webapp/channels/src/components/channel_settings_modal/share_channel_with_workspaces/add_workspace_dropdown.tsx @@ -97,10 +97,11 @@ export default function AddWorkspaceDropdown({ disabled={true} /> )} + {/* A remote cluster is keyed on (remote_id, name), so remote_id alone can repeat here. */} {!loading && available.map((rc) => ( {rc.display_name || rc.name}} onClick={() => handleSelect(rc)} /> diff --git a/webapp/channels/src/components/suggestion/suggestion.tsx b/webapp/channels/src/components/suggestion/suggestion.tsx index fa84052eca0c..e8b10effe973 100644 --- a/webapp/channels/src/components/suggestion/suggestion.tsx +++ b/webapp/channels/src/components/suggestion/suggestion.tsx @@ -36,6 +36,10 @@ const SuggestionContainer = React.forwardRef { e.preventDefault(); diff --git a/webapp/channels/src/components/widgets/tag/tag.tsx b/webapp/channels/src/components/widgets/tag/tag.tsx index cdedf4b7c129..33039b2deb73 100644 --- a/webapp/channels/src/components/widgets/tag/tag.tsx +++ b/webapp/channels/src/components/widgets/tag/tag.tsx @@ -127,7 +127,7 @@ const TagText = styled.span` text-overflow: ellipsis; `; -const Tag = ({ +const Tag = React.forwardRef(({ variant, onClick, className, @@ -136,7 +136,7 @@ const Tag = ({ size = 'xs', uppercase = false, ...rest -}: Props) => { +}, ref) => { const Icon = iconName ? glyphMap[iconName] : null; const element = onClick ? 'button' : 'div'; @@ -157,6 +157,10 @@ const Tag = ({ return ( } as={element} uppercase={uppercase} onClick={onClick} @@ -166,6 +170,7 @@ const Tag = ({ {text} ); -}; +}); +Tag.displayName = 'Tag'; export default memo(Tag);