[IAmJSD]

Root cause

matchStringListConstraints (internal/auth/service_group.go) enforces the AccountIDs, Providers and Services constraint dimensions for every non-strict permission check. It was any-match (containsAny): when both lists were non-empty, one shared value was enough. A permission scoped to account A therefore matched a request naming accounts A and B. It was safe only because every current caller passes a single-value request list, and nothing enforced that. The Regions dimension had already been moved to the all-match matchAllRegionsConstraint for this reason (Azure RI exchange target locations).

Fix

• containsAny is replaced by containsAll (internal/auth/service_helpers.go). It returns true only when the requested list is non-empty and every requested value is in the allowed list. Comparison stays exact (case-sensitive, no trimming), and an empty requested list returns false.
• matchStringListConstraints now calls containsAll, so all three dimensions are all-match, mirroring matchAllRegionsConstraint. The empty-list semantics (03-L6) are unchanged: an empty permission list or an empty request list still means "no restriction on this dimension".
• No behavior change for current callers. I checked every production construction of a request-side PermissionConstraints (handler_ri_exchange.go, handler_purchases.go purchaseConstraintSets, handler_purchases_revoke.go, handler_marketplace.go, handler_ladder*.go, handler_config.go). Each passes at most one value for AccountIDs, Providers and Services, and all-match gives the same result as any-match for one value. The StrictScope path already used the all-match listCovers and is untouched.
• Comment-only updates where the code named containsAny as the enforcement matcher: grant_ceiling_cover.go (the listCovers rationale; exact all-match enforcement keeps the "ceiling never looser than enforcement" argument valid), group_ceiling.go, and the doc comment on TestMatchConstraints_RegionsRequireEveryRequestedRegion.

Verification

All evidence is from unit tests that exercise the real matcher and PermissionsAllowForConstraintSets (no mocks in the decision path). No real-cloud or DB coverage applies: the change is a pure in-memory matcher. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

New tests (internal/auth/constraint_all_match_test.go):

• TestMatchConstraints_StringListsRequireEveryRequestedValue covers each of AccountIDs, Providers and Services through matchConstraints on a non-strict request. Cases: a partially permitted multi-value request is denied, including when the permitted value is listed second; a fully permitted subset is allowed; duplicates are allowed; a single value is allowed or denied as before; casing and whitespace still do not match; a blank extra value is denied; an unconstrained permission and a request that omits the dimension still match.
• TestPermissionsAllow_MultiAccountRequestDeniedOutsideScope runs the scenario from the issue through the exported PermissionsAllowForConstraintSets: a permission scoped to acct-a/aws/ec2 allows the single-value request and denies a request widened on each dimension.
• TestContainsAll (replaces TestContainsAny) also pins the fail-closed empty-request case and exact comparison.

Fail before (new tests run against the unchanged matcher on origin/main ba38120):

$ go test ./internal/auth/ -run 'TestMatchConstraints_StringListsRequireEveryRequestedValue|TestPermissionsAllow_MultiAccountRequestDeniedOutsideScope' -count=1
--- FAIL: TestPermissionsAllow_MultiAccountRequestDeniedOutsideScope/AccountIDs
--- FAIL: TestPermissionsAllow_MultiAccountRequestDeniedOutsideScope/Services
--- FAIL: TestPermissionsAllow_MultiAccountRequestDeniedOutsideScope/Providers
--- FAIL: TestMatchConstraints_StringListsRequireEveryRequestedValue/{AccountIDs,Providers,Services}/partially_permitted_multi-value_request_is_denied
--- FAIL: TestMatchConstraints_StringListsRequireEveryRequestedValue/{AccountIDs,Providers,Services}/permitted_value_listed_second_does_not_rescue_the_request
--- FAIL: TestMatchConstraints_StringListsRequireEveryRequestedValue/{AccountIDs,Providers,Services}/blank_request_value_is_not_permitted_by_a_constrained_permission
FAIL	github.com/LeanerCloud/cloud-commitments-platform/internal/auth

Pass after, plus the CI-equivalent checks:

go build ./...                                                    # ok
go vet ./internal/auth/ ./internal/api/                           # ok
go test ./internal/auth/ -count=1                                 # ok
go test -race -short ./internal/auth/ -count=1                    # ok
go test ./internal/api/ -count=1                                  # ok
go test ./internal/server/ ./internal/mocks/ -count=1             # ok (incl. TestNoUnfailableMockAssertions)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/auth/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/auth/               # silent
bash scripts/gosec-hook.sh <changed files>                        # clean
git-secrets --scan (fresh isolated repo, scripts/setup-git-secrets.sh)   # clean
scripts/test-git-secrets-allowlist.sh                              # 42 PASS, 0 FAIL
gofmt -l internal/auth/ ; git diff --check                         # clean

Overlap with open PRs

• fix(auth): enforce constraints on an admin-scoped permission on every check #1096 (admin-scoped constraints, issue sec(auth): an admin-scoped API key ignores its account and amount constraints on non-strict checks #956) edits permissionsAllow in service_group.go (around lines 323-345). This PR edits matchStringListConstraints and the matchAllRegionsConstraint doc comment (around lines 411-460). The hunks don't overlap and should merge cleanly. The two changes compose: once fix(auth): enforce constraints on an admin-scoped permission on every check #1096 lands, an {admin, *} permission's AccountIDs/Providers/Services constraints are also all-match.
• fix(api): bound update-any by its Constraints on planned-purchase DELETE #1097 (planned-purchase DELETE update-any) touches only internal/api and builds single-value strict constraint sets, so it is unaffected.
• test(api): answer region-scoped Azure exchange tests from the real matcher #1148 (Azure region test helper, issue test(api): newAzureRegionScopedHandler re-implements the all-match region rule instead of asserting it #208) edits only internal/api/handler_ri_exchange_azure_test.go. Its comments refer to matchAllRegionsConstraint (not renamed here) and to "containsAny's any-match rule" as a historical regression, which is still accurate.
• fix(auth): bind group account scope entries to account IDs #1116 (group account scope) edits group_ceiling.go near line 376 and removes tests near line 806 of service_group_test.go. This PR's edits there are one comment line near 229 and the region test's doc comment near 1436, so they don't overlap.

Out of scope

• An empty request list still matches a constrained permission on the non-strict path ("no restriction specified", 03-L6). Strict callers use StrictScope/listCovers, which deny this case. docs/audits/codebase-audit-2026-09-02.md flags it as a separate concern, and changing it would be a wider behavior change than this issue asks for.
• Regions keeps its separate case-insensitive, trimmed matcher. AccountIDs, Providers and Services stay exact on purpose, because listCovers (the grant ceiling) relies on exact enforcement there.

Closes #138

A maintainer, please mirror the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/xs, type/security.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

VERDICT: APPROVE 28f283b

Reviewed: full diff origin/main...28f283ba (1 commit, 7 files, +174/-39). Rebase check: git merge-tree --write-tree origin/main 28f283ba against origin/main f4387e4 is clean.

Issue #138 asks for all-match (or a len==1 assertion) on AccountIDs/Providers/Services. The PR switches matchStringListConstraints to containsAll (internal/auth/service_group.go:435-437, internal/auth/service_helpers.go:135-149). That closes the latent any-match bypass the issue describes.

Findings (none blocking):

• nit internal/auth/service_group.go:329 - semantics note. All-match is evaluated per permission inside permissionsAllow. A user holding two separate permissions, one for acct-a and one for acct-b, is now denied a request naming [acct-a, acct-b], where any-match would have allowed it. This fails closed and no current caller sends multi-value lists, but the doc comment could say that a multi-value request needs one permission covering every value, so a future multi-account caller knows to split into per-account constraint sets.
• nit internal/auth/service_helpers.go:136 - the len(requested) == 0 fail-closed guard can't be reached from enforcement, because matchStringListConstraints handles empty lists first. Only TestContainsAll pins it. It's harmless defense in depth.

Out of scope, pre-existing, and called out in the PR body: on the non-strict path an empty request list still matches a constrained permission (03-L6).

Attacked:

• Callers. I grepped every production request-side PermissionConstraints construction (handler_ri_exchange.go:1116,2544, handler_purchases.go:578,2955, handler_purchases_revoke.go:356, handler_marketplace.go:420, handler_ladder*.go, handler_config.go:154). All are single-value or empty, so production behavior is unchanged. I found no other containsAny users, and internal/mocks has no separate matcher.
• API-key path. service_apikeys_api.go:446-449 checks both the key permissions and the owner permissions through permissionsAllow, so both get all-match.
• Admin wildcard. In permissionsAllow, {admin,*} short-circuits unless StrictScope is set, and the StrictScope path uses listCovers. Neither is changed.
• Grant ceiling. listCovers (grant_ceiling_cover.go:45) is already exact all-match with a non-empty requirement, so after this PR the ceiling is still never looser than enforcement. Only a comment changed there.
• Empty, nil and blank inputs, case, and whitespace are covered by the new table test across all three dimensions.
• Mutation M1: reverting containsAll to any-match fails TestContainsAll, TestMatchConstraints_StringListsRequireEveryRequestedValue and TestPermissionsAllow_MultiAccountRequestDeniedOutsideScope.
• Mutation M2: dropping the empty-requested guard fails TestContainsAll only (expected, see the nit above).
• Mutation M3: re-wiring only the Providers dimension to an any-match helper in matchConstraints fails both new integration-style tests.
• Mutation M4: wiring Regions to matchStringListConstraints fails the casing and whitespace cases of TestMatchConstraints_RegionsRequireEveryRequestedRegion, which confirms the PR's rewritten test comment.

Local results (fresh git archive export of 28f283b): go build ./... ok; go vet ./internal/auth/ ok; go vet -tags integration ./internal/auth/ ./internal/api/ ok; go test ./internal/auth/ 985 passed; go test ./internal/api/ ./internal/mocks/ 3929 passed. I did not run the DB-tagged integration tests. They don't touch the matcher.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 28f283b

Findings:

• [non-blocking] internal/api/handler_purchases.go:2926-2929 - The purchaseConstraintSets doc comment still says single-value lists make the "any-overlap matcher equivalent to strict containment". After this PR the matcher is all-match, so the comment is stale. No runtime effect.
• [non-blocking] internal/auth/service_helpers.go:136-138 - The len(requested)==0 fail-closed guard in containsAll cannot be reached from production code, because its only caller (matchStringListConstraints, service_group.go:435-437) already handles an empty request list. A deletion probe is caught only by the TestContainsAll unit test. It is a documented, tested defensive guard, so it can stay.

Against the issue: the PR takes the issue's second option (all-match) and applies it to all three dimensions, not only AccountIDs. That fixes the root cause in the shared matcher instead of adding per-call-site len==1 asserts. The StrictScope path (listCovers) and Regions (matchAllRegionsConstraint) are untouched (service_group.go:396-408).

Attacked:

• Over-denial: every non-test request-side PermissionConstraints construction that reaches permissionsAllow passes an AccountIDs/Providers/Services list of 0 or 1 element. Only Regions is multi-value (Azure exchange), and it is unchanged. For one value, all-match equals any-match, so no current caller changes behaviour. The call sites checked:
  • handler_purchases.go:2955 and :578
  • handler_ri_exchange.go:1116 and :2544
  • handler_purchases_revoke.go:356
  • handler_marketplace.go:420
  • handler_ladder.go:164, handler_ladder_amend.go:71, handler_ladder_timeline.go:40, handler_ladder_resolve.go:110
  • handler_config.go:153
• Under-denial:
  • The API-key path (service_apikeys_api.go:445-451) runs the same permissionsAllow against both the key's and the owner's permissions.
  • No second matcher exists: AuthContext.HasPermission (types.go:174) checks only action and resource, and nothing else uses containsAny.
  • An empty request list still matches on the non-strict path, which is unchanged and out of scope.
• Composition: permissions scoped to A and to B no longer satisfy a request for [A,B]. That is the intended semantics, and no current caller sends such a request.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  1. containsAll skips unpermitted values: 4 tests FAIL.
  2. The empty-request guard dropped: only TestContainsAll FAILS (finding 2).
  3. Only the first requested value checked: the new tests FAIL.
  4. Services uses the normalizing regions matcher: 2 tests FAIL.
  5. A full revert to any-match: TestContainsAll and both new tests FAIL.

Verified locally, on a git archive export of HEAD with no cloud credentials:

• go build ./... and go vet ./internal/auth/ ./internal/api/: ok.
• gofmt -l internal/auth/: clean.
• go test -count=1 on ./internal/auth/, ./internal/api/ and ./internal/server/: ok.
• Fail-before: copied into the base export, TestPermissionsAllow_MultiAccountRequestDeniedOutsideScope and TestMatchConstraints_StringListsRequireEveryRequestedValue both FAIL.
• git merge-tree against origin/main: clean.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.