[cristim]

Summary

executeExchange (POST /api/ri-exchange/execute) was the only one of the four RI-exchange endpoints that did not enforce the session's allowed_accounts scope, allowing a scoped user with an unconstrained execute:ri-exchange grant to execute an irreversible AWS RI exchange against a cloud account outside their session's scope.

Root cause

The three sibling read endpoints (listConvertibleRIs, getRIUtilization, getReshapeRecommendations) each call reshapeCloudAccountInScope(ctx, session) and return an empty list when the deployment's registered AWS cloud account falls outside the session's allowed_accounts. executeExchange only checked requirePermissionConstraints (the SEC-01/#1141 gate on the granting permission's own Constraints.AccountIDs), a different, independent gate that a permission with no AccountIDs constraint satisfies unconditionally. A user scoped to allowed_accounts=[acct-A] holding such an unconstrained permission could therefore execute against a deployment whose ambient AWS account is acct-B, even though the read endpoints correctly hid acct-B's data from that same user.

Fix

Call the existing reshapeCloudAccountInScope helper at the top of executeExchange (via a new requireReshapeAccountScope wrapper) and return 403 (not an empty list, since this is a mutating, financially irreversible endpoint) when out of scope. Extracted resolveReshapeCloudAccountIDOrUnattributed (identical logic to the pre-existing inline resolution, just named) so the added branch keeps executeExchange at or under the project's CI-enforced gocyclo gate of 10 (it measured exactly 10 pre-fix).

Regression test

Added TestExecuteExchange_SessionAccountScopeMismatchReturns403 in internal/api/handler_ri_exchange_test.go, replaying the issue's exact scenario: a session scoped to allowed_accounts=[acct-A] with an unconstrained execute:ri-exchange grant attempting to execute against a deployment whose ambient account resolves to acct-B.

Proved it fails pre-fix: stashed only the internal/api/handler_ri_exchange.go change (keeping the new test) and reran the test, which failed because the code never reached GetAllowedAccountsAPI (2 of 3 expected mock calls met). Restored the fix and reran: passes, with HasPermissionForConstraintsAPI (the SEC-01 gate) asserted as never called, proving the session-scope gate refuses before that check.

Also updated five existing tests (TestExecuteExchange_PermissionConstraintsDenied, TestExecuteExchange_AccountResolutionErrorFailsClosed, TestExecuteExchange_UnattributedAccountStillConstrained, TestExecuteExchange_InvalidBodyNeverClaims, TestExecuteExchange_DuplicateSubmitRefusedBeforeAWS, TestExecuteExchange_PermissionGate) to stub the session's account scope via the existing allowAnyAccountScope helper, since they exercise unrelated paths that now also reach the new check.

Verification

All commands run from the repo root with GOWORK=off and the CI-pinned toolchain (go1.26.6, matching go.mod's go directive and .github/workflows/ci.yml's GO_VERSION) and the CI-pinned golangci-lint v2.10.1:

• go build ./... -- clean
• go vet ./... -- clean
• go test ./internal/api/... -race -short -- 2217 passed
• go mod tidy -diff -- empty
• golangci-lint run ./internal/api/... (v2.10.1, matching .github/workflows/ci.yml) -- 0 issues
• gocyclo -over 10 -ignore "_test\.go" . -- 0 functions over 10 (repo-wide, matching the CI step)

Docker is unavailable on this host; no testcontainers-based tests are affected by this change (all touched tests are handler-level unit tests using mocks), so this does not change CI's container-suite coverage.

Notes

• Restricted sessions now resolve the deployment's cloud account ID twice per executeExchange request: once inside reshapeCloudAccountInScope for the new scope check, once again in resolveReshapeCloudAccountIDOrUnattributed for the permission-constraints check. This is unchanged for unrestricted sessions (the common case), where the first resolution is skipped entirely. Threading the resolved ID through from the scope check would remove the duplicate call but pushes executeExchange back over the gocyclo gate; the extra STS/ListCloudAccounts round trip on this low-QPS, already-network-bound endpoint was judged an acceptable cost for the simpler diff.

Follow-up commit

Addressed an independent adversarial review (no blocking findings) with a second commit:

• Added TestExecuteExchange_SessionAccountScopeMatchReachesConstraintsCheck: a session whose allowed_accounts includes the deployment's own account must clear the new gate and reach the existing constraints check, proving the fix does not over-deny in-scope requests.
• Added TestExecuteExchange_SessionScopeLookupErrorFailsClosed: a failure resolving the session's scope propagates as a plain error, not a 403.
• Trimmed the two new helper doc comments and updated reshapeCloudAccountInScope's doc comment to list executeExchange among its callers.

Closes #93

Summary by CodeRabbit

• Bug Fixes
  • Exchange requests now respect the account scope assigned to the session. Requests involving deployments outside that scope are rejected with a 403 response, even if other permission checks would otherwise allow the exchange.
  • Permission checks can handle deployments without a registered cloud account, while still returning errors when account resolution fails.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 32f1f1fd-149d-4099-b6ac-c6bc1fe8b210

📥 Commits

Reviewing files that changed from the base of the PR and between 4b7bc93 and 0421afe.

📒 Files selected for processing (5)

• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• internal/api/ri_exchange_idempotency_guards_test.go
• internal/api/ri_exchange_idempotency_test.go
• internal/api/router_660_permission_flips_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

executeExchange now checks the session’s allowed-account scope and returns 403 when the deployment is outside that scope. It also resolves the cloud account ID for permission constraints, using an unattributed-account sentinel when no ID is registered. Tests cover the scope rejection and preserve other targeted checks.

Changes

RI exchange account-scope enforcement

Layer / File(s)	Summary
Enforce account scope and resolve permission constraints internal/api/handler_ri_exchange.go	The handler checks allowed-account scope before request parsing. It resolves the cloud account ID for permission constraints and uses the unattributed sentinel when no ID is registered.
Test account scope and preserve targeted execution paths internal/api/handler_ri_exchange_test.go, internal/api/ri_exchange_idempotency_guards_test.go, internal/api/ri_exchange_idempotency_test.go, internal/api/router_660_permission_flips_test.go	A regression test verifies that an out-of-scope deployment returns 403 before permission-constraint evaluation. Related tests allow unrestricted account scope to reach the behavior each test checks.

Priority: ⚪ Not assessed

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: High

Merge Risk: ⚪ Minimal · up to 0421a

The exchange now checks the session’s account scope before submission. No remaining issue was established that would prevent merging after normal checks.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: enforcing the session's allowed_accounts scope in executeExchange.
Linked Issues check	✅ Passed	Issue #93 requires a separate session allowed_accounts check for executeExchange and a 403 response for an out-of-scope deployment account. The PR calls requireReshapeAccountScope before request…
Out of Scope Changes check	✅ Passed	The changed production code implements issue #93. The account-resolution helper preserves the existing unattributed-account constraint behavior and supports the handler complexity limit. The test upda…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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

[cristim]

Independent adversarial review + local verification: MERGE at 0421afe. executeExchange now enforces the session's allowed_accounts via the same reshapeCloudAccountInScope helper its three sibling reads use, before the constraint check: out-of-scope -> 403, scope-lookup error -> 500 (verified), admin/unrestricted unaffected, and a name-scoped in-scope session still reaches the constraint check (verified). Regression test fails at merge-base and passes on head; go test -race ./internal/api 2217 passed; golangci-lint 0 issues; checks green. Follow-ups filed: quote endpoint scoping, two extra tests.