MM-68978 - Harden ABAC masking guards and fix sentinel detection (#36740)

* MM-68978 - Harden ABAC masking guards and fix sentinel detection

* address pr feedback

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/app/access_control.go

@@ -134,7 +134,8 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model.
 		}
 
 		// Merge hidden values back in and block deletion of masked conditions.
-		if appErr := a.mergeStoredPolicyExpressions(rctx, policy, callerID); appErr != nil {
+		mergedHidden, appErr := a.mergeStoredPolicyExpressions(rctx, policy, callerID)
+		if appErr != nil {
 			return nil, appErr
 		}
 
@@ -144,7 +145,7 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model.
 		// clearance themselves). Masking and write-path value validation still
 		// apply to system admins above.
 		if !a.HasPermissionTo(callerID, model.PermissionManageSystem) {
-			if appErr := a.checkSelfInclusion(rctx, policy, callerID); appErr != nil {
+			if appErr := a.checkSelfInclusion(rctx, policy, callerID, mergedHidden); appErr != nil {
 				return nil, appErr
 			}
 		}
@@ -168,7 +169,6 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model.
 
 // policyHasMaskedValuesForCaller returns true if policy contains any attribute values
 // that are not visible to callerID under the current masking rules.
-// A nil policy is treated as "no hidden values" — there's nothing to protect.
 func (a *App) policyHasMaskedValuesForCaller(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) (bool, *model.AppError) {
 	if policy == nil {
 		return false, nil
@@ -182,6 +182,9 @@ func (a *App) policyHasMaskedValuesForCaller(rctx request.CTX, policy *model.Acc
 		if appErr != nil {
 			return false, appErr
 		}
+		if maskedAST == nil {
+			continue
+		}
 		for _, cond := range maskedAST.Conditions {
 			if cond.HasMaskedValues {
 				return true, nil
@@ -193,21 +196,21 @@ func (a *App) policyHasMaskedValuesForCaller(rctx request.CTX, policy *model.Acc
 }
 
 // mergeStoredPolicyExpressions re-injects hidden values from the stored policy into the
-// submitted one, and blocks the save if the caller removed a condition that contained
-// values they cannot see (which would silently widen access beyond what they could audit).
-// No-op for new policies (not found in store).
-func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError {
+// submitted one, and blocks the save if the caller removed a condition with values they
+// cannot see. No-op for new policies (not found in store).
+// Returns true when at least one hidden value was re-injected.
+func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) (bool, *model.AppError) {
 	acs := a.Srv().ch.AccessControl
 	if acs == nil {
-		return nil
+		return false, nil
 	}
 
 	existingPolicy, appErr := acs.GetPolicy(rctx, policy.ID)
 	if appErr != nil {
 		if appErr.StatusCode == http.StatusNotFound {
-			return nil
+			return false, nil
 		}
-		return appErr
+		return false, appErr
 	}
 
 	// Pair submitted and stored rules by Name so that a reorder /
@@ -232,6 +235,7 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces
 
 	pairedNames := make(map[string]bool, len(existingPolicy.Rules))
 	membershipPaired := false
+	mergedHidden := false
 
 	for i := range policy.Rules {
 		rule := &policy.Rules[i]
@@ -267,7 +271,7 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces
 		submittedExpr := rule.Expression
 		mergedExpr, appErr := a.mergeExpressionWithMaskedValues(rctx, policy.ID, submittedExpr, stored.Expression, callerID)
 		if appErr != nil {
-			return appErr
+			return false, appErr
 		}
 		rule.Expression = mergedExpr
 		// Hidden values were re-injected → caller was working from a
@@ -277,6 +281,7 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces
 		if mergedExpr != submittedExpr {
 			rule.Actions = stored.Actions
 			rule.Role = stored.Role
+			mergedHidden = true
 		}
 	}
 
@@ -308,15 +313,14 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces
 		}
 		hasMasked, appErr := a.expressionHasMaskedValuesForCaller(rctx, stored.Expression, callerID)
 		if appErr != nil {
-			return appErr
+			return false, appErr
 		}
 		if hasMasked {
-			return model.NewAppError("MergeStoredPolicyExpressions", "app.pap.save_policy.masked_rule_deleted", nil,
-				"cannot remove a rule that contains attribute values you do not hold", http.StatusForbidden)
+			return false, saveForbiddenError(rctx, "MergeStoredPolicyExpressions", "masked_rule_deleted: cannot remove a rule that contains attribute values you do not hold")
 		}
 	}
 
-	return nil
+	return mergedHidden, nil
 }
 
 // isMembershipRule reports whether a rule fills the policy's
@@ -339,6 +343,9 @@ func (a *App) expressionHasMaskedValuesForCaller(rctx request.CTX, storedExpr, c
 	if appErr != nil {
 		return false, appErr
 	}
+	if maskedAST == nil {
+		return false, nil
+	}
 	for _, cond := range maskedAST.Conditions {
 		if cond.HasMaskedValues {
 			return true, nil
@@ -390,10 +397,7 @@ func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submit
 			mlog.String("policy_id", policyID),
 			mlog.String("caller_id", callerID),
 		)
-		return "", model.NewAppError("mergeExpressionWithMaskedValues",
-			"app.pap.save_policy.advanced_expression_blocked", nil,
-			"this rule expression cannot be safely edited while restricted values are present",
-			http.StatusForbidden)
+		return "", saveForbiddenError(rctx, "mergeExpressionWithMaskedValues", "advanced_expression_blocked: stored rule expression cannot be safely edited while restricted values are present")
 	}
 
 	cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName)
@@ -437,8 +441,7 @@ func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submit
 		}
 		attr := storedAST.Conditions[i].Attribute
 		if submittedCounts[attr] < storedCounts[attr] {
-			return "", model.NewAppError("mergeExpressionWithMaskedValues", "app.pap.save_policy.masked_condition_deleted", nil,
-				"cannot remove a rule condition that contains attribute values you do not hold", http.StatusForbidden)
+			return "", saveForbiddenError(rctx, "mergeExpressionWithMaskedValues", "masked_condition_deleted: cannot remove a rule condition that contains attribute values you do not hold")
 		}
 	}
 
@@ -498,8 +501,19 @@ func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submit
 	return buildCELFromConditions(mergedConditions), nil
 }
 
+// saveForbiddenError logs the rejection reason internally and returns a generic 403.
+func saveForbiddenError(rctx request.CTX, where, internalReason string) *model.AppError {
+	rctx.Logger().Info("save policy refused (see internal_reason for details)",
+		mlog.String("where", where),
+		mlog.String("internal_reason", internalReason),
+	)
+	return model.NewAppError(where, "app.pap.save_policy.forbidden", nil, "", http.StatusForbidden)
+}
+
 // checkSelfInclusion verifies the caller satisfies all policy rules after their edit.
-func (a *App) checkSelfInclusion(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError {
+// 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.
+func (a *App) checkSelfInclusion(rctx request.CTX, policy *model.AccessControlPolicy, callerID string, mergedHidden bool) *model.AppError {
 	for _, rule := range policy.Rules {
 		if rule.Expression == "" || rule.Expression == "true" {
 			continue
@@ -510,9 +524,10 @@ func (a *App) checkSelfInclusion(rctx request.CTX, policy *model.AccessControlPo
 			return appErr
 		}
 		if !matches {
-			return model.NewAppError("CreateOrUpdateAccessControlPolicy",
-				"app.pap.save_policy.self_exclusion", nil,
-				"You do not satisfy one or more conditions in this policy.", http.StatusForbidden)
+			if mergedHidden {
+				return saveForbiddenError(rctx, "checkSelfInclusion", "self_exclusion: you do not satisfy one or more conditions in this policy")
+			}
+			return model.NewAppError("checkSelfInclusion", "app.pap.save_policy.self_exclusion", nil, "", http.StatusForbidden)
 		}
 	}
 
@@ -541,8 +556,7 @@ func (a *App) DeleteAccessControlPolicy(rctx request.CTX, id string) *model.AppE
 			if hasMasked, appErr := a.policyHasMaskedValuesForCaller(rctx, policy, callerID); appErr != nil {
 				return appErr
 			} else if hasMasked {
-				return model.NewAppError("DeleteAccessControlPolicy", "app.pap.delete_policy.masked_values", nil,
-					"policy contains attribute values you do not hold; you cannot delete this policy", http.StatusForbidden)
+				return model.NewAppError("DeleteAccessControlPolicy", "app.pap.delete_policy.masked_values", nil, "", http.StatusForbidden)
 			}
 		}
 	}
@@ -646,11 +660,46 @@ func (a *App) SimulateAccessControlPolicyForUsers(rctx request.CTX, params model
 	// save-only invariant. The merge alone is what makes the
 	// simulator see the unmasked policy.
 	if a.Config().FeatureFlags.AttributeValueMasking {
-		if appErr := a.mergeStoredPolicyExpressions(rctx, params.Policy, rctx.Session().UserId); appErr != nil {
+		// Resolve the caller once here so merge and response masking use the same
+		// identity. An empty callerID means there is no session; we must not let
+		// unmasked literals propagate to the response without knowing who is asking.
+		callerID := ""
+		if s := rctx.Session(); s != nil {
+			callerID = s.UserId
+		}
+		if callerID == "" {
+			return nil, model.NewAppError("SimulateAccessControlPolicyForUsers", "api.context.session_expired.app_error", nil, "session required for simulation when attribute value masking is enabled", http.StatusUnauthorized)
+		}
+
+		if _, appErr := a.mergeStoredPolicyExpressions(rctx, params.Policy, callerID); appErr != nil {
 			return nil, appErr
 		}
+
+		resp, appErr := acs.SimulatePolicyForUsers(rctx, params)
+		if appErr != nil {
+			return nil, appErr
+		}
+
+		if resp != nil {
+			enrichBlameForDraftScope(rctx, acs, params.Policy, resp)
+			if isThisRuleScope(params.EvaluationScope) {
+				filterResponseToEditingRuleScope(resp, params.RuleName)
+			}
+
+			// mergeStoredPolicyExpressions re-injected the stored hidden
+			// values so the simulator could evaluate the real CEL — and
+			// enrichBlameForDraftScope just copied those unmasked
+			// expressions into Blame.Expression / MergedRules / the
+			// evaluation tree. Re-mask every literal-bearing surface
+			// before the response leaves the server so the caller never
+			// sees a value they couldn't see via the policy GET path.
+			a.MaskSimulationPolicyLiteralsForCaller(rctx, resp, callerID)
+		}
+
+		return resp, nil
 	}
 
+	// Masking disabled: simulate without merge or response masking.
 	resp, appErr := acs.SimulatePolicyForUsers(rctx, params)
 	if appErr != nil {
 		return nil, appErr
@@ -661,20 +710,6 @@ func (a *App) SimulateAccessControlPolicyForUsers(rctx request.CTX, params model
 		if isThisRuleScope(params.EvaluationScope) {
 			filterResponseToEditingRuleScope(resp, params.RuleName)
 		}
-
-		// mergeStoredPolicyExpressions re-injected the stored hidden
-		// values so the simulator could evaluate the real CEL — and
-		// enrichBlameForDraftScope just copied those unmasked
-		// expressions into Blame.Expression / MergedRules / the
-		// evaluation tree. Re-mask every literal-bearing surface
-		// before the response leaves the server so the caller never
-		// sees a value they couldn't see via the policy GET path.
-		// Same flag gate as the merge above: either both run or
-		// neither does, so the response always matches the policy
-		// state that produced it.
-		if a.Config().FeatureFlags.AttributeValueMasking {
-			a.MaskSimulationPolicyLiteralsForCaller(rctx, resp, rctx.Session().UserId)
-		}
 	}
 
 	return resp, nil
@@ -1657,13 +1692,47 @@ func (a *App) GetAccessControlFieldsAutocomplete(rctx request.CTX, after string,
 func (a *App) UpdateAccessControlPoliciesActive(rctx request.CTX, updates []model.AccessControlPolicyActiveUpdate) ([]*model.AccessControlPolicy, *model.AppError) {
 	acs := a.Srv().ch.AccessControl
 	if acs == nil {
-		return nil, model.NewAppError("ExpressionToVisualAST", "app.pap.update_access_control_policies_active.app_error", nil, "Policy Administration Point is not initialized", http.StatusNotImplemented)
+		return nil, model.NewAppError("UpdateAccessControlPoliciesActive", "app.pap.update_access_control_policies_active.app_error", nil, "Policy Administration Point is not initialized", http.StatusNotImplemented)
+	}
+
+	// Deactivating a policy is enforcement-equivalent to deleting it: the policy stops
+	// filtering membership. Mirror the delete-path guard so a caller blocked from
+	// deleting a policy with hidden values cannot achieve the same effect via deactivation.
+	if a.Config().FeatureFlags.AttributeValueMasking {
+		session := rctx.Session()
+		if session != nil {
+			callerID := session.UserId
+			for _, u := range updates {
+				if u.Active {
+					continue // activation never widens access
+				}
+				policy, appErr := acs.GetPolicy(rctx, u.ID)
+				if appErr != nil {
+					return nil, appErr
+				}
+				if hasMasked, appErr := a.policyHasMaskedValuesForCaller(rctx, policy, callerID); appErr != nil {
+					return nil, appErr
+				} else if hasMasked {
+					return nil, model.NewAppError("UpdateAccessControlPoliciesActive", "app.pap.delete_policy.masked_values", nil, "", http.StatusForbidden)
+				}
+			}
+		}
 	}
 
 	policies, err := a.Srv().Store().AccessControlPolicy().SetActiveStatusMultiple(rctx, updates)
 	if err != nil {
 		return nil, model.NewAppError("UpdateAccessControlPoliciesActive", "app.pap.update_access_control_policies_active.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
+
+	for _, policy := range policies {
+		switch policy.Type {
+		case model.AccessControlPolicyTypeChannel:
+			a.publishChannelPolicyEnforcedUpdate(rctx, policy.ID)
+		case model.AccessControlPolicyTypeParent:
+			a.publishChannelPolicyEnforcedForChannelPoliciesWithImport(rctx, policy.ID)
+		}
+	}
+
 	return policies, nil
 }
 

server/channels/app/access_control_masking.go

@@ -624,7 +624,6 @@ func celValueLiteral(value any) string {
 const maskedTokenValue = "--------"
 
 // validatePolicyExpressionValues checks that all submitted literal values are held by the caller.
-// Returns the same generic error for every rejection to prevent value enumeration.
 func (a *App) validatePolicyExpressionValues(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError {
 	cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName)
 	if appErr != nil {
@@ -775,9 +774,7 @@ func (a *App) GetMaskedExpression(rctx request.CTX, expression string, callerID
 // maskConditionValuesWithToken replaces non-held values with the masked token in place,
 // preserving expression structure so the visual AST endpoint can still parse it.
 // fieldsByName is pre-fetched by the caller to avoid N+1 lookups; a missing entry
-// is treated as fail-closed (whole value masked).
-// maskConditionValuesWithToken replaces non-held values with the masked token in place.
-// Returns true if any value was masked.
+// is treated as fail-closed (whole value masked). Returns true if any value was masked.
 func (a *App) maskConditionValuesWithToken(rctx request.CTX, callerID string, condition *model.Condition, cpaGroupID string, fieldsByName map[string]*model.PropertyField) bool {
 	if condition.ValueType == model.AttrValue {
 		return false
@@ -1215,8 +1212,15 @@ func clearEvaluationTreeLiterals(node *model.PolicySimulationEvaluationNode) {
 	}
 }
 
+// maskFailClosedSentinel is the CEL expression written into a response rule when masking
+// cannot safely produce a redacted version (parse failure or CPA group unavailable).
+// "false" is used because it is deny-all if ever evaluated literally, matching the
+// fail-closed intent. This value only ever appears in API responses — the stored DB
+// expression is never overwritten by this path.
+const maskFailClosedSentinel = "false"
+
 // MaskPolicyExpressions masks non-held literal values in all policy rule expressions, in place.
-// Fails closed (sets a rule to "true") if its expression cannot be parsed or masked.
+// Fails closed (sets a rule to maskFailClosedSentinel) if its expression cannot be parsed or masked.
 func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) {
 	cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName)
 	if appErr != nil {
@@ -1227,7 +1231,7 @@ func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessContro
 			if rule.Expression == "" || rule.Expression == "true" {
 				continue
 			}
-			policy.Rules[i].Expression = "true"
+			policy.Rules[i].Expression = maskFailClosedSentinel
 		}
 		return
 	}
@@ -1245,7 +1249,7 @@ func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessContro
 		}
 		ast, appErr := a.ExpressionToVisualAST(rctx, rule.Expression)
 		if appErr != nil {
-			policy.Rules[i].Expression = "true" // fail closed
+			policy.Rules[i].Expression = maskFailClosedSentinel // fail closed: deny-all sentinel, response-only
 			continue
 		}
 		asts[i] = ast

server/channels/app/access_control_masking_test.go

@@ -833,11 +833,9 @@ func TestMaskSimulationPolicyLiteralsForCaller_GuardClauses(t *testing.T) {
 	})
 
 	t.Run("empty callerID is a no-op", func(t *testing.T) {
-		// A session-less caller reaching this code path would be a
-		// caller-context bug; the function must refuse rather than
-		// run with an empty caller (which the property service would
-		// resolve to "no holdings" and therefore mask everything,
-		// effectively a stealthy DoS).
+		// The API layer rejects empty callerID before reaching this function.
+		// Pinned to ensure MaskSimulationPolicyLiteralsForCaller is safe to call
+		// with an empty callerID (no panic, no masking applied).
 		resp := &model.PolicySimulationResponse{
 			Results: []model.PolicySimulationUserResult{{
 				Decisions: map[string]model.PolicySimulationActionDecision{

server/channels/app/access_control_test.go

@@ -428,12 +428,12 @@ func TestCheckSelfInclusion(t *testing.T) {
 		mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything).
 			Return([]*model.User{{Id: callerID}}, int64(1), nil).Once()
 
-		appErr := th.App.checkSelfInclusion(th.Context, policy, callerID)
+		appErr := th.App.checkSelfInclusion(th.Context, policy, callerID, false)
 		require.Nil(t, appErr)
 		mockACS.AssertExpectations(t)
 	})
 
-	t.Run("caller who does not satisfy the policy is rejected with 403", func(t *testing.T) {
+	t.Run("caller who does not satisfy the policy is rejected with specific self_exclusion error when no hidden values were merged", func(t *testing.T) {
 		th := Setup(t).InitBasic(t)
 		callerID := th.BasicUser.Id
 
@@ -445,17 +445,40 @@ func TestCheckSelfInclusion(t *testing.T) {
 
 		mockACS := &mocks.AccessControlServiceInterface{}
 		th.App.Srv().ch.AccessControl = mockACS
-		// No users returned → caller does not satisfy → expect self_exclusion 403.
+		// No users returned → caller does not satisfy → expect specific self_exclusion error.
 		mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything).
 			Return([]*model.User{}, int64(0), nil).Once()
 
-		appErr := th.App.checkSelfInclusion(th.Context, policy, callerID)
+		appErr := th.App.checkSelfInclusion(th.Context, policy, callerID, false)
 		require.NotNil(t, appErr)
 		require.Equal(t, http.StatusForbidden, appErr.StatusCode)
 		require.Equal(t, "app.pap.save_policy.self_exclusion", appErr.Id)
 		mockACS.AssertExpectations(t)
 	})
 
+	t.Run("caller who does not satisfy the policy gets opaque 403 when hidden values were merged", func(t *testing.T) {
+		th := Setup(t).InitBasic(t)
+		callerID := th.BasicUser.Id
+
+		policy := &model.AccessControlPolicy{
+			Rules: []model.AccessControlPolicyRule{
+				{Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.clearance == "TopSecret"`},
+			},
+		}
+
+		mockACS := &mocks.AccessControlServiceInterface{}
+		th.App.Srv().ch.AccessControl = mockACS
+		// No users returned → caller does not satisfy → mergedHidden=true returns generic forbidden.
+		mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything).
+			Return([]*model.User{}, int64(0), nil).Once()
+
+		appErr := th.App.checkSelfInclusion(th.Context, policy, callerID, true)
+		require.NotNil(t, appErr)
+		require.Equal(t, http.StatusForbidden, appErr.StatusCode)
+		require.Equal(t, "app.pap.save_policy.forbidden", appErr.Id)
+		mockACS.AssertExpectations(t)
+	})
+
 	t.Run("trivial rules (empty / 'true') are skipped without querying", func(t *testing.T) {
 		th := Setup(t).InitBasic(t)
 		callerID := th.BasicUser.Id
@@ -472,7 +495,7 @@ func TestCheckSelfInclusion(t *testing.T) {
 		// No query should fire for trivial expressions — if it does, the mock will fail
 		// the test by returning the default zero-value response.
 
-		appErr := th.App.checkSelfInclusion(th.Context, policy, callerID)
+		appErr := th.App.checkSelfInclusion(th.Context, policy, callerID, false)
 		require.Nil(t, appErr)
 		mockACS.AssertNotCalled(t, "QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything)
 	})
@@ -4475,7 +4498,7 @@ func TestMergeStoredPolicyExpressions_ActionsLocked(t *testing.T) {
 		},
 	}, nil).Maybe()
 
-	mergeErr := th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID)
+	_, mergeErr := th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID)
 	require.Nil(t, mergeErr)
 
 	require.Len(t, submittedPolicy.Rules, 1)
@@ -4486,31 +4509,17 @@ func TestMergeStoredPolicyExpressions_ActionsLocked(t *testing.T) {
 	mockACS.AssertExpectations(t)
 }
 
-// TestMergeStoredPolicyExpressions_FailClosedTrueRejectedOnResubmit verifies the
-// claim from the PR review: if MaskPolicyExpressions emitted "true" for a rule
-// because the stored expression could not be parsed (fail-closed), a caller who
-// re-submits that "true" unchanged will be blocked on the save path.
-//
-// How it works:
-//  1. MaskPolicyExpressions (GET path) calls ExpressionToVisualAST on the stored
-//     expression; on parse failure it sets the rule to "true" (fail-closed).
-//  2. The caller sees "true" in the GET response and re-submits it.
-//  3. On the save path, mergeExpressionWithMaskedValues calls
-//     expressionHasMaskedValuesForCaller, which calls GetMaskedVisualAST, which
-//     calls ExpressionToVisualAST on the *stored* expression again.
-//  4. That second parse also fails → error propagates → save is blocked.
-//     "true" is never written to the DB.
-func TestMergeStoredPolicyExpressions_FailClosedTrueRejectedOnResubmit(t *testing.T) {
+// TestMergeStoredPolicyExpressions_FailClosedSentinelRejectedOnResubmit verifies that
+// re-submitting the fail-closed sentinel as the rule expression is rejected on save
+// when the stored expression is unparseable.
+func TestMergeStoredPolicyExpressions_FailClosedSentinelRejectedOnResubmit(t *testing.T) {
 	th := Setup(t).InitBasic(t)
 	th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise))
 
 	callerID := model.NewId()
 	policyID := model.NewId()
 
-	// A stored expression that ExpressionToVisualAST cannot parse.
-	// In production this is guarded by save-time validation, but defensive
-	// code paths must still protect against it.
-	storedExpr := `user.attributes.TopSecret == "Value"`
+	storedExpr := `user.attributes.TopSecret == "Value"` // ExpressionToVisualAST cannot parse this
 
 	storedPolicy := &model.AccessControlPolicy{
 		ID:   policyID,
@@ -4520,13 +4529,11 @@ func TestMergeStoredPolicyExpressions_FailClosedTrueRejectedOnResubmit(t *testin
 		},
 	}
 
-	// Caller re-submits "true" — what MaskPolicyExpressions emitted as the
-	// fail-closed value when it could not parse the stored expression on GET.
 	submittedPolicy := &model.AccessControlPolicy{
 		ID:   policyID,
 		Type: model.AccessControlPolicyTypeParent,
 		Rules: []model.AccessControlPolicyRule{
-			{Actions: []string{model.AccessControlPolicyActionMembership}, Expression: "true"},
+			{Actions: []string{model.AccessControlPolicyActionMembership}, Expression: maskFailClosedSentinel},
 		},
 	}
 
@@ -4534,16 +4541,12 @@ func TestMergeStoredPolicyExpressions_FailClosedTrueRejectedOnResubmit(t *testin
 	th.App.Srv().ch.AccessControl = mockACS
 
 	mockACS.On("GetPolicy", mock.Anything, policyID).Return(storedPolicy, nil).Once()
-	// Simulate the same parse failure that would have triggered fail-closed on GET.
 	parseErr := model.NewAppError("ExpressionToVisualAST", "app.pap.expression_to_visual_ast.app_error", nil, "simulated parse failure", http.StatusInternalServerError)
 	mockACS.On("ExpressionToVisualAST", mock.Anything, storedExpr).Return(nil, parseErr).Maybe()
 
-	mergeErr := th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID)
+	_, mergeErr := th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID)
 
-	// Save must be blocked. The error returned here causes UpdateAccessControlPolicy
-	// to abort before any DB write — the in-memory struct may still hold "true"
-	// but it never reaches the store.
-	require.NotNil(t, mergeErr, "expected mergeStoredPolicyExpressions to return an error when stored expression is unparseable")
+	require.NotNil(t, mergeErr)
 	mockACS.AssertExpectations(t)
 }
 
@@ -4602,7 +4605,7 @@ func TestMergeStoredPolicyExpressions_ActionsEditableWhenNoMasking(t *testing.T)
 		},
 	}, nil).Maybe()
 
-	appErr = th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID)
+	_, appErr = th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID)
 	require.Nil(t, appErr)
 
 	require.Len(t, submittedPolicy.Rules, 1)
@@ -4612,3 +4615,222 @@ func TestMergeStoredPolicyExpressions_ActionsEditableWhenNoMasking(t *testing.T)
 	assert.Equal(t, []string{model.AccessControlPolicyActionUploadFileAttachment}, submittedPolicy.Rules[0].Actions)
 	mockACS.AssertExpectations(t)
 }
+
+// TestUpdateAccessControlPoliciesActive_MaskingGuard verifies deactivation follows the
+// same masking rules as delete, while activation is always allowed.
+func TestUpdateAccessControlPoliciesActive_MaskingGuard(t *testing.T) {
+	t.Run("deactivation blocked when caller has masked values", func(t *testing.T) {
+		// policyHasMaskedValuesForCaller resolves the property group from the store,
+		// so this subtest uses SetupConfig + InitBasic rather than a mock store.
+		th := SetupConfig(t, func(cfg *model.Config) {
+			cfg.FeatureFlags.AttributeBasedAccessControl = true
+			cfg.FeatureFlags.AttributeValueMasking = true
+		}).InitBasic(t)
+
+		callerID := model.NewId()
+		th.Context = th.Context.WithSession(&model.Session{UserId: callerID, Id: model.NewId()}).(*request.Context)
+
+		policyID := model.NewId()
+		sensitivePolicy := &model.AccessControlPolicy{
+			ID:   policyID,
+			Type: model.AccessControlPolicyTypeChannel,
+			Rules: []model.AccessControlPolicyRule{
+				{Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.f_unknown == "Secret"`},
+			},
+		}
+
+		mockACS := &mocks.AccessControlServiceInterface{}
+		th.App.Srv().ch.AccessControl = mockACS
+		mockACS.On("GetPolicy", th.Context, policyID).Return(sensitivePolicy, nil).Once()
+		// Unknown field → fail-closed → HasMaskedValues=true.
+		mockACS.On("ExpressionToVisualAST", mock.Anything, mock.Anything).Return(&model.VisualExpression{
+			Conditions: []model.Condition{
+				{Attribute: "user.attributes.f_unknown", Operator: "==", Value: "Secret", ValueType: model.LiteralValue},
+			},
+		}, nil).Maybe()
+
+		_, appErr := th.App.UpdateAccessControlPoliciesActive(th.Context, []model.AccessControlPolicyActiveUpdate{
+			{ID: policyID, Active: false},
+		})
+
+		require.NotNil(t, appErr)
+		require.Equal(t, http.StatusForbidden, appErr.StatusCode)
+		require.Equal(t, "app.pap.delete_policy.masked_values", appErr.Id)
+		mockACS.AssertExpectations(t)
+	})
+
+	t.Run("activation always allowed even when caller has masked values", func(t *testing.T) {
+		// The guard skips Active=true updates, so no property store access is needed.
+		thMock := SetupWithStoreMock(t)
+		thMock.App.UpdateConfig(func(cfg *model.Config) {
+			cfg.FeatureFlags.AttributeBasedAccessControl = true
+			cfg.FeatureFlags.AttributeValueMasking = true
+		})
+
+		callerID := model.NewId()
+		thMock.Context = thMock.Context.WithSession(&model.Session{UserId: callerID, Id: model.NewId()}).(*request.Context)
+
+		channelID := model.NewId()
+		policy := &model.AccessControlPolicy{
+			ID:   channelID,
+			Type: model.AccessControlPolicyTypeChannel,
+			Rules: []model.AccessControlPolicyRule{
+				{Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.f_unknown == "Secret"`},
+			},
+		}
+
+		mockStore := thMock.App.Srv().Store().(*storemocks.Store)
+		mockACPStore := storemocks.AccessControlPolicyStore{}
+		mockStore.On("AccessControlPolicy").Return(&mockACPStore)
+		mockACPStore.On("SetActiveStatusMultiple", thMock.Context, mock.Anything).Return([]*model.AccessControlPolicy{policy}, nil).Once()
+
+		// Channel cache & WS broadcast
+		mockChannelStore := storemocks.ChannelStore{}
+		mockStore.On("Channel").Return(&mockChannelStore).Maybe()
+		mockChannelStore.On("InvalidateChannel", channelID).Maybe()
+		mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Maybe()
+
+		mockACS := &mocks.AccessControlServiceInterface{}
+		thMock.App.Srv().ch.AccessControl = mockACS
+
+		_, appErr := thMock.App.UpdateAccessControlPoliciesActive(thMock.Context, []model.AccessControlPolicyActiveUpdate{
+			{ID: channelID, Active: true},
+		})
+
+		require.Nil(t, appErr)
+		mockACPStore.AssertExpectations(t)
+		mockACS.AssertNotCalled(t, "GetPolicy", mock.Anything, mock.Anything)
+	})
+
+	t.Run("deactivation allowed when masking flag is off", func(t *testing.T) {
+		thMock := SetupWithStoreMock(t)
+		thMock.Context = thMock.Context.WithSession(&model.Session{UserId: model.NewId(), Id: model.NewId()}).(*request.Context)
+
+		channelID := model.NewId()
+		policy := &model.AccessControlPolicy{
+			ID:   channelID,
+			Type: model.AccessControlPolicyTypeChannel,
+		}
+
+		mockStore := thMock.App.Srv().Store().(*storemocks.Store)
+		mockACPStore := storemocks.AccessControlPolicyStore{}
+		mockStore.On("AccessControlPolicy").Return(&mockACPStore)
+		mockACPStore.On("SetActiveStatusMultiple", thMock.Context, mock.Anything).Return([]*model.AccessControlPolicy{policy}, nil).Once()
+
+		mockChannelStore := storemocks.ChannelStore{}
+		mockStore.On("Channel").Return(&mockChannelStore).Maybe()
+		mockChannelStore.On("InvalidateChannel", channelID).Maybe()
+		mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Maybe()
+
+		mockACS := &mocks.AccessControlServiceInterface{}
+		thMock.App.Srv().ch.AccessControl = mockACS
+
+		_, appErr := thMock.App.UpdateAccessControlPoliciesActive(thMock.Context, []model.AccessControlPolicyActiveUpdate{
+			{ID: channelID, Active: false},
+		})
+
+		require.Nil(t, appErr)
+		// GetPolicy must never be called when masking is off.
+		mockACS.AssertNotCalled(t, "GetPolicy", mock.Anything, mock.Anything)
+		mockACPStore.AssertExpectations(t)
+	})
+}
+
+// TestUpdateAccessControlPoliciesActive_BroadcastsWebsocketEvents verifies that
+// activate/deactivate fires cache invalidation and WS events for channel and parent policies.
+func TestUpdateAccessControlPoliciesActive_BroadcastsWebsocketEvents(t *testing.T) {
+	t.Run("channel policy triggers publishChannelPolicyEnforcedUpdate", func(t *testing.T) {
+		thMock := SetupWithStoreMock(t)
+		thMock.Context = thMock.Context.WithSession(&model.Session{UserId: model.NewId(), Id: model.NewId()}).(*request.Context)
+
+		channelID := model.NewId()
+		policy := &model.AccessControlPolicy{
+			ID:   channelID,
+			Type: model.AccessControlPolicyTypeChannel,
+		}
+
+		mockStore := thMock.App.Srv().Store().(*storemocks.Store)
+		mockACPStore := storemocks.AccessControlPolicyStore{}
+		mockStore.On("AccessControlPolicy").Return(&mockACPStore)
+		mockACPStore.On("SetActiveStatusMultiple", thMock.Context, mock.Anything).Return([]*model.AccessControlPolicy{policy}, nil).Once()
+
+		mockChannelStore := storemocks.ChannelStore{}
+		mockStore.On("Channel").Return(&mockChannelStore)
+		mockChannelStore.On("InvalidateChannel", channelID).Once()
+		mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Once()
+
+		mockACS := &mocks.AccessControlServiceInterface{}
+		thMock.App.Srv().ch.AccessControl = mockACS
+
+		_, appErr := thMock.App.UpdateAccessControlPoliciesActive(thMock.Context, []model.AccessControlPolicyActiveUpdate{
+			{ID: channelID, Active: true},
+		})
+
+		require.Nil(t, appErr)
+		mockChannelStore.AssertExpectations(t)
+	})
+}
+
+// TestSaveForbiddenErrorHidesInternalID verifies that saveForbiddenError always returns
+// app.pap.save_policy.forbidden with an empty DetailedError, regardless of the internal reason.
+func TestSaveForbiddenErrorHidesInternalID(t *testing.T) {
+	rctx := request.TestContext(t)
+
+	internalReasons := []string{
+		"masked_condition_deleted: detail",
+		"masked_rule_deleted: detail",
+		"self_exclusion: detail",
+		"advanced_expression_blocked: detail",
+	}
+
+	for _, reason := range internalReasons {
+		appErr := saveForbiddenError(rctx, "testWhere", reason)
+		require.NotNil(t, appErr)
+		assert.Equal(t, "app.pap.save_policy.forbidden", appErr.Id, "reason %q must not appear in error ID", reason)
+		assert.Equal(t, http.StatusForbidden, appErr.StatusCode)
+		assert.Empty(t, appErr.DetailedError)
+	}
+}
+
+// TestMaskPolicyExpressions_FailClosedUsesDenyAllSentinel verifies that MaskPolicyExpressions
+// emits maskFailClosedSentinel ("false") on parse failure, not the open-access "true".
+func TestMaskPolicyExpressions_FailClosedUsesDenyAllSentinel(t *testing.T) {
+	th := SetupWithStoreMock(t)
+	callerID := model.NewId()
+
+	t.Run("CPA group lookup failure masks all rules to deny-all sentinel", func(t *testing.T) {
+		policy := &model.AccessControlPolicy{
+			Rules: []model.AccessControlPolicyRule{
+				{Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.secret == "X"`},
+			},
+		}
+
+		// Inject a mock ACS that makes ExpressionToVisualAST fail, triggering fail-closed.
+		mockACS := &mocks.AccessControlServiceInterface{}
+		th.App.Srv().ch.AccessControl = mockACS
+		mockACS.On("ExpressionToVisualAST", mock.Anything, mock.Anything).Return(nil,
+			model.NewAppError("ExpressionToVisualAST", "app_error", nil, "parse fail", http.StatusInternalServerError)).Maybe()
+
+		th.App.MaskPolicyExpressions(th.Context, policy, callerID)
+
+		require.Len(t, policy.Rules, 1)
+		assert.Equal(t, maskFailClosedSentinel, policy.Rules[0].Expression)
+	})
+
+	t.Run("trivial true and empty rules are untouched by fail-closed path", func(t *testing.T) {
+		policy := &model.AccessControlPolicy{
+			Rules: []model.AccessControlPolicyRule{
+				{Actions: []string{model.AccessControlPolicyActionMembership}, Expression: ""},
+				{Actions: []string{model.AccessControlPolicyActionMembership}, Expression: "true"},
+			},
+		}
+
+		mockACS := &mocks.AccessControlServiceInterface{}
+		th.App.Srv().ch.AccessControl = mockACS
+
+		th.App.MaskPolicyExpressions(th.Context, policy, callerID)
+
+		assert.Equal(t, "", policy.Rules[0].Expression, "empty expression must stay empty")
+		assert.Equal(t, "true", policy.Rules[1].Expression, "open-access rule must stay \"true\"")
+	})
+}

server/i18n/en.json

@@ -7700,26 +7700,18 @@
     "id": "app.pap.query_expression.app_error",
     "translation": "Could not query for expression."
   },
-  {
-    "id": "app.pap.save_policy.advanced_expression_blocked",
-    "translation": "This rule expression cannot be safely edited while restricted values are present."
-  },
   {
     "id": "app.pap.save_policy.app_error",
     "translation": "Unable to save access control policy."
   },
+  {
+    "id": "app.pap.save_policy.forbidden",
+    "translation": "You do not have permission to make this change to the policy."
+  },
   {
     "id": "app.pap.save_policy.invalid_value",
     "translation": "Invalid value."
   },
-  {
-    "id": "app.pap.save_policy.masked_condition_deleted",
-    "translation": "You cannot remove a condition that contains attribute values you do not have permission to view."
-  },
-  {
-    "id": "app.pap.save_policy.masked_rule_deleted",
-    "translation": "You cannot remove a rule that contains attribute values you do not have permission to view."
-  },
   {
     "id": "app.pap.save_policy.name_exists.app_error",
     "translation": "A policy with this name already exists. Please choose a different name."

webapp/channels/src/components/admin_console/access_control/editors/shared.tsx

@@ -12,6 +12,9 @@ import Markdown from 'components/markdown';
 
 import './shared.scss';
 
+// Sentinel emitted by the server in masked CEL expressions for values the caller cannot see.
+export const MASKED_VALUE_TOKEN_LITERAL = '"--------"';
+
 // CEL operator constants
 export enum CELOperator {
     EQUALS = '==',

webapp/channels/src/components/admin_console/access_control/policies.tsx

@@ -17,14 +17,9 @@ import SectionNotice from 'components/section_notice';
 
 import {getHistory} from 'utils/browser_history';
 
-import './policies.scss';
+import {MASKED_VALUE_TOKEN_LITERAL} from './editors/shared';
 
-// The server emits the eight-dash masked-token sentinel inside raw CEL expressions
-// when masking values the caller cannot see (e.g. `attr == "--------"`). The full
-// visual AST carries a typed `has_masked_values` flag per condition, but on the
-// policies list page we only have the raw expression strings — so we detect masking
-// by the quoted token substring.
-const MASKED_VALUE_TOKEN_LITERAL = '"--------"';
+import './policies.scss';
 
 function policyHasMaskedValues(policy: AccessControlPolicy): boolean {
     return policy.rules?.some((rule) => rule.expression?.includes(MASKED_VALUE_TOKEN_LITERAL)) ?? false;

webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx

@@ -32,7 +32,7 @@ import Constants from 'utils/constants';
 import ChannelList from './channel_list';
 
 import CELEditor from '../editors/cel_editor/editor';
-import {hasUsableAttributes, isSimpleExpression} from '../editors/shared';
+import {hasUsableAttributes, isSimpleExpression, MASKED_VALUE_TOKEN_LITERAL} from '../editors/shared';
 import TableEditor from '../editors/table_editor/table_editor';
 import PolicyConfirmationModal from '../modals/confirmation/confirmation_modal';
 
@@ -82,15 +82,15 @@ function PolicyDetails({
     const [addChannelOpen, setAddChannelOpen] = useState(false);
     const [editorMode, setEditorMode] = useState<'cel' | 'table'>('table');
 
-    // Derive masked-rows state directly from the expression rather than relying
-    // on a callback from TableEditor: TableEditor unmounts on mode switches, and
-    // on remount its rows-derived flag flickers false-then-true while the async
-    // AST round-trip is in flight, briefly opening gates (CEL read-only, delete,
-    // banner) that should stay closed. The "--------" sentinel is what the
-    // server emits in raw CEL for any value the caller can't see, so its
-    // presence in the expression is a stable signal independent of editor
-    // lifecycle.
-    const hasMaskedRows = useMemo(() => expression.includes('"--------"'), [expression]);
+    // Check for masked values using existingRules when available (updated after each fetch),
+    // falling back to the prop. The "--------" sentinel may be absent from a locally rebuilt
+    // CEL string even when masked values are present, so we rely on the server-sourced rules.
+    const hasMaskedRows = useMemo(
+        () => (existingRules.length > 0 ? existingRules : policy?.rules ?? []).some(
+            (rule) => rule.expression?.includes(MASKED_VALUE_TOKEN_LITERAL),
+        ),
+        [existingRules, policy],
+    );
     const [channelChanges, setChannelChanges] = useState<ChannelChanges>({
         removed: {},
         added: {},
@@ -211,10 +211,8 @@ function PolicyDetails({
                         setServerError(formatMessage({id: 'admin.access_control.edit_policy.invalid_value', defaultMessage: 'Invalid value.'}));
                     } else if (result.error.server_error_id === 'app.pap.save_policy.self_exclusion') {
                         setServerError(formatMessage({id: 'admin.access_control.edit_policy.self_exclusion', defaultMessage: 'You do not satisfy one or more conditions in this policy. Contact a System Admin for assistance.'}));
-                    } else if (result.error.server_error_id === 'app.pap.save_policy.masked_condition_deleted') {
-                        setServerError(formatMessage({id: 'admin.access_control.edit_policy.masked_condition_deleted', defaultMessage: 'You cannot remove a condition that contains attribute values you do not have permission to view.'}));
-                    } else if (result.error.server_error_id === 'app.pap.save_policy.masked_rule_deleted') {
-                        setServerError(formatMessage({id: 'admin.access_control.edit_policy.masked_rule_deleted', defaultMessage: 'You cannot remove a rule that contains attribute values you do not have permission to view.'}));
+                    } else if (result.error.server_error_id === 'app.pap.save_policy.forbidden') {
+                        setServerError(formatMessage({id: 'admin.access_control.edit_policy.forbidden', defaultMessage: 'You do not have permission to make this change to the policy. Contact a System Admin for assistance.'}));
                     } else {
                         setServerError(result.error.message);
                     }

webapp/channels/src/i18n/en.json

@@ -278,9 +278,8 @@
   "admin.access_control.edit": "Edit",
   "admin.access_control.edit_policy.apply_policy": "Apply policy",
   "admin.access_control.edit_policy.cancel": "Cancel",
+  "admin.access_control.edit_policy.forbidden": "You do not have permission to make this change to the policy. Contact a System Admin for assistance.",
   "admin.access_control.edit_policy.invalid_value": "Invalid value.",
-  "admin.access_control.edit_policy.masked_condition_deleted": "You cannot remove a condition that contains attribute values you do not have permission to view.",
-  "admin.access_control.edit_policy.masked_rule_deleted": "You cannot remove a rule that contains attribute values you do not have permission to view.",
   "admin.access_control.edit_policy.name_exists": "A policy with this name already exists. Please choose a different name.",
   "admin.access_control.edit_policy.save": "Save",
   "admin.access_control.edit_policy.save_policy": "Save policy",