[pull] master from mattermost:master

.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

.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}"

.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

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
 }
 

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.

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)

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
 }