[IAmJSD]

Root cause

TestExecuteExchange_PlainAdminIsRefused (internal/api/ri_exchange_carveout_test.go) drives the real executeExchange handler, but it built Handler{config, auth} with no account resolver. When {ActionExecute, ResourceRIExchange} is removed from adminCarvedOuts (the mutation this test exists to catch), the handler gets past requirePermission and calls authorizeAWSExchangeExecution → resolveReshapeCloudAccountIDOrUnattributed → resolveReshapeCloudAccountID. With h.reshapeAccountResolver == nil that falls through to resolveAWSCloudAccountID → resolveAWSCallerIdentity, which runs awsconfig.LoadDefaultConfig plus a live sts:GetCallerIdentity.

The existing t.Setenv("AWS_EC2_METADATA_DISABLED", "true") only blocked the IMDS fallback. On a machine with AWS credentials in the environment, the mutated test still called STS for real. This was confirmed on main by pointing STS at a dead port:

[WARN] Failed to resolve source account ID via STS: operation error STS: GetCallerIdentity, exceeded maximum number of attempts, 3, ... request send failed, Post "http://127.0.0.1:9/": dial tcp 127.0.0.1:9: connect: connection refused
--- FAIL: TestExecuteExchange_PlainAdminIsRefused (3.17s)

Fix (test-only)

The test now injects the seams Handler already has, so no production code changes:

• reshapeAccountResolver is a stub that records it was reached and returns a sentinel errGateBypassed. A handler that has regressed stops at the first step past the gate, with no network I/O. Two new assertions name the failure: assert.False(gateBypassed) ("a plain admin got past the execute:ri-exchange gate into cloud account scope resolution") and assert.NotErrorIs(err, errGateBypassed). These come before the existing 403 ClientError identity check, which is still there.
• executeExchangeFn calls t.Error if it is ever reached. While the resolver returns an error it can't be reached. It stays as a backstop in case a future change moves scope resolution after the AWS call.
• t.Setenv("AWS_EC2_METADATA_DISABLED", ...) is dropped because it no longer guards anything. The doc comment is updated to describe the real path past the gate: scope resolution, then the AWS exchange call.

Sibling tests in the same file: I checked all of them and none needed the change. The other three (TestRIExchangeCarveOut_PlainAdminIsRefused, _ExchangerIsAllowed, _AdminKeepsNonCarvedVerbs) call requirePermission directly. Nothing in them goes past the gate or touches AWS, mutated or not.

Verification

This is unit/mock evidence only. No real cloud calls are made, and none are needed.

All runs used AWS_EC2_METADATA_DISABLED=true AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. The local ~/.aws/config default profile on this machine is invalid, and these settings also guarantee no ambient credential source.

Unmutated (go test -count=1 -v -run '^(TestExecuteExchange_PlainAdminIsRefused|TestRIExchangeCarveOut_.*)$' ./internal/api/): all 4 tests PASS.

Mutated: {ActionExecute, ResourceRIExchange}: true, was commented out in internal/auth/types.go, then reverted afterwards.

1. Offline env as above:

   ri_exchange_carveout_test.go:186: Should be false
       Messages: a plain admin got past the execute:ri-exchange gate into cloud account scope resolution (#1644)
   ri_exchange_carveout_test.go:187: Target error should not be in err chain ... the refusal must come from the permission gate, not from a step behind it
   ri_exchange_carveout_test.go:189: Should be true ... a carve-out denial must be a client error, not a downstream failure
   --- FAIL: TestExecuteExchange_PlainAdminIsRefused (0.00s)
   

2. Same, plus AWS_ENDPOINT_URL_STS=http://127.0.0.1:9 AWS_REGION=us-east-1 and static dummy credentials (AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE), so any real STS call would show up as a connection-refused error with retries. The result is the same three assertion failures, (0.00s). There is no Failed to resolve source account ID via STS log line, while main logs one (3.17s, refused POST to 127.0.0.1:9). STS is not reached.

Other checks:

• go test -count=1 ./internal/api/: ok
• go test ./internal/mocks/ -count=1: ok (TestNoUnfailableMockAssertions and the other guards)
• go vet ./internal/api/: ok
• golangci-lint v2.10.1 run ./internal/api/: 0 issues
• gofmt: clean
• gocyclo -over 10 -ignore "_test\.go": silent (only a test file changed)
• git secrets --scan in an isolated temp repo: clean
• scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL

Out of scope

• Production code still builds AWS clients from ambient credentials past the gate. The broader "no injected seam for exchange.ExecuteExchange" point is the follow-up the test's existing comment calls "#1760". That number is from the old tracker and doesn't resolve in this repo or in cloud-commitments-cli, so I didn't link it. This PR only makes the carve-out test hermetic, using seams that already exist.
• No frontend changes.

Maintainer: please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/s.

Closes #175

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 1 billable file and costs up to $0.25.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 4170465
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5a19c0b5-f903-46a8-a34c-c73bddbe17b7

Commits

Reviewing files that changed from the base of the PR and between 4170465 and 900c742.

Files selected for processing: 1

• internal/api/ri_exchange_carveout_test.go

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

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

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 900c742

Scope: test-only, in internal/api/ri_exchange_carveout_test.go (+39/-17). It closes #175.

Root cause: the code matches the issue, and injecting the existing seam fixes the root cause, not a symptom.

• With the carve-out removed, the handler passes requirePermission (handler_ri_exchange.go:1934).
• It then reaches requireReshapeAccountScope (:1941) and authorizeAWSExchangeExecution (:2556, resolver at :2569).
• Both reach resolveReshapeCloudAccountID (:1418-1422), which with a nil reshapeAccountResolver makes a live STS call.
• I reproduced the live STS attempt on base. No production code changes and no migration.

Findings (none blocking):

• [non-blocking] ri_exchange_carveout_test.go:139-141 - The comment names "authorizeAWSExchangeExecution -> resolveReshapeCloudAccountID" as the first step past the gate.
  • For a session with restricted allowed_accounts, requireReshapeAccountScope -> reshapeCloudAccountInScope (:1361) reaches the resolver first.
  • Both use the same seam, so the test stays offline either way. Only the wording is off.
• [non-blocking] ri_exchange_carveout_test.go:172-175 - The executeExchangeFn backstop is not what catches a "scope resolution skipped" regression.
  • In probe M4, the 403 assertion fails first, and the backstop's t.Error never fires.
  • It still stops a future path from making a live EC2 exchange call, so keeping it is reasonable, but the comment slightly overstates its role.
• [non-blocking, pre-existing] ri_exchange_carveout_test.go:119 - The comment cites "handler_ri_exchange.go:1713-1716". The actual lines are 1934-1937.

Attacked:

• Fail-before (base b6355a8, with the carve-out removed, STS pointed at 127.0.0.1:9 and dummy static keys):
  • The old test logs "Failed to resolve source account ID via STS ... connection refused" and fails in 2.10 s, so a real STS attempt happened.
  • The head test under the same mutation fails all three new assertions in 0.00 s, with no STS line.
  • Base plus the head test, unmutated, passes.
• Mutation probes, each followed by a cmp check that the files match the head and base blobs exactly:
  • M1, the carve-out removed: the target test FAILS on gateBypassed, NotErrorIs and IsClientError with no network. The sibling TestRIExchangeCarveOut_PlainAdminIsRefused FAILS too.
  • M2, a scope-resolution call inserted before the gate with its error masked as a 403: only the new gateBypassed assertion FAILS. The old 403-identity check alone would have passed, so this assertion adds real discrimination.
  • M3, the executeExchange denial returned as errors.New: FAILS on IsClientError. This was rerun anchored to executeExchange, because the first attempt hit the same pattern in another handler.
  • M4, the carve-out removed and scope resolution replaced by a constant: FAILS on the 403 assertion (finding 2).
  • M5, the carve-out removed and the resolver seam ignored: the STS attempt returns (4.86 s) and the test FAILS. So hermeticity depends on the seam, as claimed.
• Blast radius: only the existing Handler fields reshapeAccountResolver (handler.go:105) and executeExchangeFn (:141) are touched. No exported API changes.
• Strictness: the test asserts the 403 more strictly than before and weakens nothing.

Verified locally, with no cloud credentials, under the heavy lock:

• Head: go build ./... OK, go vet ./internal/api/ OK, go test -count=1 ./internal/api/ ./internal/auth/ ok, and gofmt -l clean.
• Merged with current origin/main f5ad8c6:
  • merge-tree is clean.
  • Main changed handler.go, handler_ri_exchange.go and ri_exchange_detached.go, but not on the executeExchange path.
  • With GOTOOLCHAIN=go1.26.10: build OK, vet OK, the api and auth tests ok, and gofmt clean.
  • M1 on the merged tree fails offline on all three new assertions.
• Integration tests were not run, since this is a unit test with mocks and there is no migration or store code.

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

[cristim]

VERDICT: APPROVE 900c742

Reviewed head 900c742 against origin/main 6776e27. git merge-tree is clean (merged tree 63a0404c). All checks below ran on the merged tree with Go 1.26.10.

Findings (none blocking):

• nit internal/api/ri_exchange_carveout_test.go:119 - the retained comment cites handler_ri_exchange.go:1713-1716. executeExchange is now at handler_ri_exchange.go:1934 on main. The reference was stale before this PR, but the PR rewrites the surrounding paragraph, so it could fix it too.
• nit internal/api/ri_exchange_carveout_test.go:164 - atomic.Bool is more than needed. The handler runs synchronously and a plain bool would do. Harmless.

Correctness: both paths past the gate resolve the account through resolveReshapeCloudAccountID (handler_ri_exchange.go:1419). The first is requireReshapeAccountScope -> reshapeCloudAccountInScope (:1362), which is only reached for non-AllowsAll sessions. The second is authorizeAWSExchangeExecution -> resolveReshapeCloudAccountIDOrUnattributed (:2620). That function returns the injected reshapeAccountResolver before resolveAWSCloudAccountID/STS, so the stub cuts off every network path that follows the gate. executeExchangeFn sits further downstream and serves as a backstop. The fix addresses the issue's root cause: issue #175 asks for an injectable scope resolver, and the test now uses that seam.

Attacked:

• Unmutated, -race -count=3 over the carve-out tests: all pass. go vet passes and gofmt reports nothing.
• Mutation (carve-out line commented out in internal/auth/types.go, scratch copy): the test fails in 0.00s on the gateBypassed, NotErrorIs and IsClientError assertions. This ran with AWS_ENDPOINT_URL_STS=http://127.0.0.1:9, dummy static credentials and IMDS disabled, and logged no STS line.
• Fail-before: main's version of the test under the same mutation logs "Failed to resolve source account ID via STS", so the live STS path is confirmed on main and gone on the branch.
• CI on 900c742: every check passes (Snyk skipped).
• Commit size: one commit with 56 changed lines.

[IAmJSD]

Local verification at 900c742b95d3e646b34f0c7036badb1691d5978e merged onto current main (6a4a506e), together with #1140, #1170, #1197, #1201, #1202, #1204, #1205, #1208 and #1209 in that order (combined tree 8b907eb6, Linux, Go 1.26.10):

• go build ./..., go vet ./..., go vet -tags integration ./..., (cd tests/e2e && go vet -tags e2e ./...): clean.
• go test -race -short -count=1 ./...: 36 packages ok, no failures.
• go test -race -tags integration -count=1 on every package the nine PRs change (analytics, api, auth, config, database/postgres/migrations, mocks, server), against Docker Postgres: all ok.

CI on this head: CI Success and pre-commit green. cristim's independent review approved this head. Migrations in this batch (000136-000139) are applied in numeric order. Nothing was run against any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), after cristim's independent review approving 900c742b and the combined-tree verification above. Squash-merged at exactly 900c742b95d3e646b34f0c7036badb1691d5978e.