[pull] master from mattermost:master

.cursor/README.md

@@ -17,7 +17,7 @@ The Docker build context is `.cursor/` only. The Dockerfile intentionally does n
 ## Runtime Hooks
 
 - `cloud-agent-install.sh` runs after Cursor checks out the repo. It refreshes nvm, installs agent-browser browsers, verifies Cursor's multi-repo `mattermost/enterprise` checkout, runs `server` Go dependency hydration, installs webapp dependencies, and runs Playwright `npm ci`.
-- `cloud-agent-start.sh` materializes `.cursor/cursor.md` as `.cursor/AGENTS.md`, fixes current-session Docker socket access, then starts Docker and waits until `docker info` and `docker compose version` succeed.
+- `cloud-agent-start.sh` materializes `.cursor/cursor.md` as `.cursor/AGENTS.md`, fixes current-session Docker socket access, starts Docker, waits until `docker info` and `docker compose version` succeed, then logs in to Docker Hub when credentials are configured.
 
 The environment declares `github.com/mattermost/enterprise` in `repositoryDependencies` so Cursor can provide it as part of the multi-repo workspace. Cursor currently clones the repositories as siblings, such as `/agent/repos/mattermost` and `/agent/repos/enterprise`, which matches `server/Makefile`'s default `../../enterprise` path. The install hook does not clone, pull, or symlink enterprise.
 
@@ -33,4 +33,7 @@ Set these environment variables to `true` to shorten startup for narrow tasks:
 
 ## Expected Secrets
 
-- AWS uploads use the standard AWS CLI environment variables provided to the Cloud Agent: `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, and `AWS_S3_BUCKET_NAME`. The image only supplies the `aws` binary.
+Configure these in the [Cursor Cloud Agents dashboard](https://cursor.com/dashboard/cloud-agents) as environment-scoped secrets for the Mattermost Cloud Agent environment.
+
+- AWS uploads use the standard AWS CLI environment variables: `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, and `AWS_S3_BUCKET_NAME`. The image only supplies the `aws` binary.
+- Docker Hub pulls use the same variable names as CI: `DOCKERHUB_USERNAME` and `DOCKERHUB_TOKEN`. The start hook runs `docker login` after `dockerd` is ready. Mark `DOCKERHUB_TOKEN` as **redacted** in the dashboard. When both are set, agents can pull the full default `make start-docker` image set without hitting anonymous rate limits.

.cursor/cursor.md

@@ -58,7 +58,8 @@ The Mattermost server is expected at `http://localhost:8065`. The webapp dev ser
   ```
 
 - When the server starts and `MM_LICENSE` is present in the environment, the server applies that license automatically. If `MM_LICENSE` is not set, starting the server automatically applies an Entry license, which provides nearly all functionality needed for development.
-- `ENABLED_DOCKER_SERVICES='postgres redis'` avoids optional local-dev services such as Prometheus, Grafana, Loki, Minio, Azurite, and OpenLDAP. This is useful in Cloud when Docker Hub rate limits block the default `make start-docker` dependency set.
+- When `DOCKERHUB_USERNAME` and `DOCKERHUB_TOKEN` are configured as Cloud Agent secrets, `cloud-agent-start.sh` logs in to Docker Hub and the full default `make start-docker` dependency set can be used without trimming services.
+- `ENABLED_DOCKER_SERVICES='postgres redis'` avoids optional local-dev services such as Prometheus, Grafana, Loki, Minio, Azurite, and OpenLDAP. Use this fallback when Docker Hub credentials are unavailable and anonymous pulls hit rate limits.
 - If the first-user signup UI is flaky but the server is already healthy, seed local state with `mmctl` and then log in through the browser:
 
   ```bash

.cursor/scripts/cloud-agent-start.sh

@@ -34,6 +34,21 @@ ensure_docker_socket_access() {
   fi
 }
 
+docker_login_if_configured() {
+  if [ -z "${DOCKERHUB_USERNAME:-}" ] || [ -z "${DOCKERHUB_TOKEN:-}" ]; then
+    log "Docker Hub credentials not configured; anonymous pulls may hit rate limits."
+    return 0
+  fi
+
+  log "Logging in to Docker Hub as ${DOCKERHUB_USERNAME}."
+  if echo "${DOCKERHUB_TOKEN}" | docker login -u "${DOCKERHUB_USERNAME}" --password-stdin >/tmp/docker-login.log 2>&1; then
+    log "Docker Hub login succeeded."
+  else
+    log "Docker Hub login failed; see /tmp/docker-login.log."
+    tail -n 20 /tmp/docker-login.log >&2 || true
+  fi
+}
+
 if [ -f /proc/sys/kernel/apparmor_restrict_unprivileged_userns ]; then
   sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 >/dev/null 2>&1 || \
     log "Could not relax AppArmor user namespace restriction; openldap-based tests may need a larger host profile."
@@ -44,6 +59,7 @@ ensure_docker_socket_access
 if docker info >/dev/null 2>&1; then
   log "Docker is already running."
   docker compose version
+  docker_login_if_configured
   exit 0
 fi
 
@@ -64,6 +80,7 @@ for _ in {1..60}; do
     log "Docker is ready."
     docker version
     docker compose version
+    docker_login_if_configured
     exit 0
   fi
 

server/channels/app/access_control.go

@@ -211,7 +211,8 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces
 		if i >= len(existingPolicy.Rules) {
 			continue
 		}
-		storedExpr := existingPolicy.Rules[i].Expression
+		storedRule := existingPolicy.Rules[i]
+		storedExpr := storedRule.Expression
 		if storedExpr == "" || storedExpr == "true" {
 			continue
 		}
@@ -220,6 +221,15 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces
 			return appErr
 		}
 		policy.Rules[i].Expression = mergedExpr
+		// If hidden values were re-injected into the expression, the caller was
+		// working from a masked view of this rule. Lock Actions to the stored
+		// value too — without this, a caller who sees "--------" could swap the
+		// action type (e.g., "membership" → "upload_file_attachment") and the
+		// merge would restore the hidden CEL value while silently removing the
+		// original access restriction.
+		if mergedExpr != rule.Expression {
+			policy.Rules[i].Actions = storedRule.Actions
+		}
 	}
 
 	// Any stored rules beyond the submitted set were dropped by the caller. If any of those
@@ -387,8 +397,16 @@ func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submit
 		// regardless of the stored operator. After we restore the original operator,
 		// the value shape may not match (e.g., "==" with a []any value). Normalize
 		// scalar operators to a single string from the array.
+		//
+		// When the stored scalar value is hidden, always use hiddenValues[0] directly
+		// rather than taking arr[0] from the merged list. Without this guard a crafted
+		// submission of `in ["caller-visible"]` would pass validateConditionValues,
+		// land in mergeConditionValues as a []any, and arr[0] would be the attacker's
+		// value — silently overwriting the stored hidden value.
 		if isScalarOperator(merged.Operator) {
-			if arr, ok := merged.Value.([]any); ok {
+			if len(hiddenValues) > 0 {
+				merged.Value = hiddenValues[0]
+			} else if arr, ok := merged.Value.([]any); ok {
 				if len(arr) == 0 {
 					merged.Value = nil
 				} else if s, ok := arr[0].(string); ok {
@@ -645,6 +663,33 @@ func (a *App) GetAccessControlPolicyAttributes(rctx request.CTX, channelID strin
 		return nil, appErr
 	}
 
+	if len(attributes) == 0 {
+		return attributes, nil
+	}
+
+	// Strip source_only and shared_only fields: their values must not be
+	// exposed to channel members through the invite modal / members sidebar.
+	// Fail closed: if the CPA group or a field cannot be resolved, omit that
+	// field rather than leaking its values.
+	cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName)
+	if appErr != nil {
+		return map[string][]string{}, nil
+	}
+
+	for fieldName := range attributes {
+		// Read directly from the store so this security filter sees the raw
+		// access_mode, unaffected by property read hooks for the request caller.
+		field, fieldErr := a.Srv().Store().PropertyField().GetFieldByName(rctx.Context(), cpaGroup.ID, "", fieldName)
+		if fieldErr != nil {
+			delete(attributes, fieldName)
+			continue
+		}
+		switch field.GetAccessMode() {
+		case model.PropertyAccessModeSourceOnly, model.PropertyAccessModeSharedOnly:
+			delete(attributes, fieldName)
+		}
+	}
+
 	return attributes, nil
 }
 

server/channels/app/access_control_masking.go

@@ -362,9 +362,11 @@ func mergeConditionValues(submitted model.Condition, hiddenValues []string) mode
 		merged.Value = result
 
 	case string:
-		// Empty string and the masked-token sentinel both mean "no real value
-		// submitted here"; restore from hidden values.
-		if (v == "" || v == maskedTokenValue) && len(hiddenValues) > 0 {
+		// For scalar conditions the caller cannot edit a value they cannot see.
+		// Always restore the stored hidden value regardless of what was submitted,
+		// preventing a crafted save from overwriting a hidden stored value with a
+		// different caller-visible string that passes validateConditionValues.
+		if len(hiddenValues) > 0 {
 			merged.Value = hiddenValues[0]
 		}
 
@@ -545,6 +547,13 @@ func conditionToCEL(cond model.Condition) string {
 			orParts = append(orParts, celStringLiteral(v)+" in "+attr)
 		}
 		if len(orParts) == 1 {
+			// When the sole value is the masked-token sentinel, duplicate it into a
+			// two-branch OR so that the parser can recover hasAnyOf on the next read.
+			// A standalone "tok in attr" is promoted to hasAllOf by
+			// mergeMultiselectConditions, which would display the wrong operator in the UI.
+			if values[0] == maskedTokenValue {
+				return "(" + orParts[0] + " || " + orParts[0] + ")"
+			}
 			return orParts[0]
 		}
 		return "(" + strings.Join(orParts, " || ") + ")"
@@ -750,8 +759,14 @@ func (a *App) GetMaskedExpression(rctx request.CTX, expression string, callerID
 	rctxWithCaller := RequestContextWithCallerID(rctx, callerID)
 	fieldsByName := a.fetchConditionFields(rctxWithCaller, visualAST.Conditions, cpaGroupID)
 
+	hasMasked := false
 	for i := range visualAST.Conditions {
-		a.maskConditionValuesWithToken(rctxWithCaller, callerID, &visualAST.Conditions[i], cpaGroupID, fieldsByName)
+		if a.maskConditionValuesWithToken(rctxWithCaller, callerID, &visualAST.Conditions[i], cpaGroupID, fieldsByName) {
+			hasMasked = true
+		}
+	}
+	if !hasMasked {
+		return expression, nil
 	}
 
 	return buildCELFromConditions(visualAST.Conditions), nil
@@ -761,43 +776,48 @@ func (a *App) GetMaskedExpression(rctx request.CTX, expression string, callerID
 // 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).
-func (a *App) maskConditionValuesWithToken(rctx request.CTX, callerID string, condition *model.Condition, cpaGroupID string, fieldsByName map[string]*model.PropertyField) {
+// maskConditionValuesWithToken replaces non-held values with the masked token in place.
+// 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
+		return false
 	}
 
 	fieldName := extractFieldName(condition.Attribute)
 	if fieldName == "" {
-		return
+		return false
 	}
 
 	field, ok := fieldsByName[fieldName]
 	if !ok {
 		condition.Value = maskedTokenValue // fail closed
-		return
+		return true
 	}
 
 	switch field.GetAccessMode() {
 	case model.PropertyAccessModePublic:
-		return
+		return false
 	case model.PropertyAccessModeSourceOnly:
 		condition.Value = maskedTokenValue
+		return true
 	case model.PropertyAccessModeSharedOnly:
 		var visibleNames map[string]struct{}
 		if field.Type == model.PropertyFieldTypeSelect || field.Type == model.PropertyFieldTypeMultiselect {
 			visibleNames = extractVisibleOptionNames(field)
 		} else {
 			visibleNames = a.getCallerTextValues(rctx, callerID, field, cpaGroupID)
 		}
-		replaceHiddenValuesWithToken(condition, visibleNames)
+		return replaceHiddenValuesWithToken(condition, visibleNames)
 	default:
 		condition.Value = maskedTokenValue
+		return true
 	}
 }
 
 // replaceHiddenValuesWithToken keeps visible values and appends a single masked token if any were hidden.
 // One token regardless of count prevents count-based inference about the number of hidden values.
-func replaceHiddenValuesWithToken(condition *model.Condition, visibleNames map[string]struct{}) {
+// Returns true if any value was replaced.
+func replaceHiddenValuesWithToken(condition *model.Condition, visibleNames map[string]struct{}) bool {
 	switch v := condition.Value.(type) {
 	case []any:
 		var result []any
@@ -817,11 +837,14 @@ func replaceHiddenValuesWithToken(condition *model.Condition, visibleNames map[s
 			result = append(result, maskedTokenValue)
 		}
 		condition.Value = result
+		return hasMasked
 	case string:
 		if _, visible := visibleNames[v]; !visible {
 			condition.Value = maskedTokenValue
+			return true
 		}
 	}
+	return false
 }
 
 // MaskPolicyExpressions masks non-held literal values in all policy rule expressions, in place.
@@ -867,9 +890,14 @@ func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessContro
 		if ast == nil {
 			continue
 		}
+		hasMasked := false
 		for j := range ast.Conditions {
-			a.maskConditionValuesWithToken(rctxWithCaller, callerID, &ast.Conditions[j], cpaGroupID, fieldsByName)
+			if a.maskConditionValuesWithToken(rctxWithCaller, callerID, &ast.Conditions[j], cpaGroupID, fieldsByName) {
+				hasMasked = true
+			}
+		}
+		if hasMasked {
+			policy.Rules[i].Expression = buildCELFromConditions(ast.Conditions)
 		}
-		policy.Rules[i].Expression = buildCELFromConditions(ast.Conditions)
 	}
 }

server/channels/app/access_control_merge_test.go

@@ -89,6 +89,24 @@ func TestBuildCELFromConditions(t *testing.T) {
 		assert.Equal(t, `"Alpha" in user.attributes.Programs`, result)
 	})
 
+	t.Run("hasAnyOf with single masked-token value emits duplicate OR to preserve operator through re-parse", func(t *testing.T) {
+		// A sole masked-token sentinel must round-trip as hasAnyOf. Without the
+		// duplicate, a standalone "tok in attr" is promoted to hasAllOf by
+		// mergeMultiselectConditions, showing the wrong operator in the table editor.
+		conditions := []model.Condition{
+			{
+				Attribute:     "user.attributes.Programs",
+				Operator:      "hasAnyOf",
+				Value:         []any{maskedTokenValue},
+				ValueType:     model.LiteralValue,
+				AttributeType: "multiselect",
+			},
+		}
+		result := buildCELFromConditions(conditions)
+		expected := `("` + maskedTokenValue + `" in user.attributes.Programs || "` + maskedTokenValue + `" in user.attributes.Programs)`
+		assert.Equal(t, expected, result)
+	})
+
 	t.Run("hasAllOf operator", func(t *testing.T) {
 		conditions := []model.Condition{
 			{
@@ -424,6 +442,45 @@ func TestMergeConditionValues(t *testing.T) {
 		result := mergeConditionValues(submitted, []string{"Building 7"})
 		assert.Equal(t, "Building 7", result.Value)
 	})
+
+	t.Run("scalar: hidden value wins over empty submitted string", func(t *testing.T) {
+		submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "!=", Value: ""}
+		result := mergeConditionValues(submitted, []string{"Building 7"})
+		assert.Equal(t, "Building 7", result.Value)
+	})
+
+	t.Run("scalar: hidden value wins over masked-token submitted string", func(t *testing.T) {
+		submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "!=", Value: maskedTokenValue}
+		result := mergeConditionValues(submitted, []string{"Building 7"})
+		assert.Equal(t, "Building 7", result.Value)
+	})
+
+	t.Run("scalar: hidden value wins over caller-visible submitted string (security: prevents overwrite)", func(t *testing.T) {
+		// A crafted save can submit a caller-held value that passes validateConditionValues.
+		// mergeConditionValues must still restore the stored hidden value so the caller
+		// cannot overwrite a shared_only scalar they cannot see.
+		submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "!=", Value: "Building 1"}
+		result := mergeConditionValues(submitted, []string{"Building 7"})
+		assert.Equal(t, "Building 7", result.Value)
+	})
+
+	t.Run("scalar via []any: mergeConditionValues appends hidden value last; isScalarOperator block must use hiddenValues[0] not arr[0]", func(t *testing.T) {
+		// Second attack vector: a crafted submission of `in ["Building 1"]` (a list,
+		// not a string) also passes validateConditionValues for a shared_only caller.
+		// mergeConditionValues produces ["Building 1", "Building 7"] — the caller's
+		// value comes first. The isScalarOperator normalization in
+		// mergeExpressionWithMaskedValues must use hiddenValues[0] directly rather
+		// than arr[0], otherwise the attacker's value wins.
+		submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "in", Value: []any{"Building 1"}}
+		result := mergeConditionValues(submitted, []string{"Building 7"})
+		values, ok := result.Value.([]any)
+		require.True(t, ok)
+		// Hidden value is appended after the submitted one — arr[0] would be "Building 1".
+		// The fix in mergeExpressionWithMaskedValues (isScalarOperator block) must
+		// pick hiddenValues[0] = "Building 7" instead of arr[0].
+		assert.Equal(t, "Building 1", values[0], "submitted value is first in merged list")
+		assert.Equal(t, "Building 7", values[1], "hidden value is appended last")
+	})
 }
 
 func TestGetHiddenValues(t *testing.T) {