Repository navigation
fix(api): sign no-cloud-account marketplace rows as their registered AWS account - #1039
Conversation
…AWS account A purchase row with a NULL cloud_account_id (legacy rows, or rows whose account was deleted) was signed with the host's credentials only when its AWS account id was the host account, and refused otherwise, even when that id belonged to a registered member account. Resolve the id by (aws, account_id) through uniqueAccountByExternalID, as revoke (LeanerCloud#547) and RI exchange history (LeanerCloud#553) do, and sign with that account's own credentials through the same resolver as a row with a cloud account. An unregistered or ambiguous id keeps the host-only check from LeanerCloud#1031; a failed account lookup is an error, never a fallback to the host's credentials. Closes LeanerCloud#560 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 2 billable files and costs up to $0.50. Or wait 58 minutes for your next included review. View limit details
Comment |
Independent review verdict: CLEAN at 086ae2fReviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, acting as a security reviewer. No credentials and no AWS calls. The PR body was treated as a claim. Coordinator: claude-cc-reviewer. Blocking findings: none. Interplay with #1031: #1031 (merged as 4e42a35) is the direct parent of this head, and
Where the account comes from:
Notes (non-blocking):
What was attacked and how (
Gate:
|
|
Gate record for 086ae2f (origin/main CLAUDE.md "Review gate"):
Marking ready (owner-authorized) and merging at 086ae2f with --match-head-commit. |
Root cause
loadMarketplaceAWSConfigsent every row with a NULL/blankcloud_account_idstraight toloadMarketplaceHostAWSConfig(#1031). That signs with the host's ambient credentials only when the row'saccount_idpassescredentials.VerifyHostAccount, and returns a 400 for anything else. It never checked whether the 12-digitaccount_idbelonged to a registered cloud account.purchase_history.cloud_account_idhas no backfill (migration 000011) and isON DELETE SET NULL, so legacy rows owned by a registered member account could not be listed or canceled from the marketplace.Fix
This is the owner-suggested fix from the issue, built on top of #1031.
loadMarketplaceNoCloudAccountAWSConfig. Whenaccount_idis non-blank, it callsListCloudAccountsand resolves the id with the existinguniqueAccountByExternalID(accounts, "aws", accountID)helper, the same lookup fix(api): match revoke account scope by external id and name like History #547 (revoke) and fix(api): match RI exchange history scope through the deployment's registered account #553 (RI exchange history) use. No new helper is added.ResolveAWSCredentialProviderWithOptspath a row with a cloud account uses. That path now lives inmarketplaceAccountAWSConfig, split out ofloadMarketplaceAWSConfigunchanged. A role_arn account with no role ARN still getsErrNotHostAccount(400) unless it is the host.marketplaceNoCloudAccountMessage.account_id: the existing 400 from fix(api): sign no-cloud-account marketplace rows as the host only for the host account #1031, with no lookup.ListCloudAccountserror: a non-client (500) error with no credentials. It never falls back to the host.cloud_account_idset are unchanged (GetCloudAccountpath).Closes #560
Verification
All of this is unit/mock/fixture evidence. The STS responses are recorded XML fixtures served by an in-process
http.Clienttransport, credentials are static fakes, and no real AWS API was called. Gap: nothing ran a real List/Cancel against a real legacy row in a multi-account deployment.Tests were run with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default~/.aws/configprofile is invalid. This only affects local runs.New tests are in
internal/api/handler_marketplace_registered_account_test.go:TestLoadMarketplaceAWSConfig_NoCloudAccountResolvesRegisteredAccount(nil/emptycloud_account_id): signs with the member account's key. No STS call, noGetCloudAccount...._NoCloudAccountRegisteredRoleAccountAssumesRole: a registered role_arn account assumes its own role...._NoCloudAccountRegisteredRoleWithoutARNRefused: 400ErrNotHostAccount, no credentials...._NoCloudAccountUnresolvedFallsBackToHostCheck: covers unregistered member, same external id on Azure only, ambiguous AWS twins and a different registered account (all 400 with no credentials), plus an unregistered host (host credentials). This pins fix(api): sign no-cloud-account marketplace rows as the host only for the host account #1031's behavior, so it passes both before and after the fix...._NoCloudAccountLookupErrorFailsClosed: non-client error, no credentials, no STS call.TestMarketplaceHandlers_NoCloudAccountRegisteredRow(list and cancel end to end): the EC2 client is built with the member account's key.Fail before (production change stashed, new tests kept):
Pass after:
git-secrets:
git secrets --scanover all tracked files plus the new file, in a freshgit inittemp repo with an isolated HOME andscripts/setup-git-secrets.sh, was clean.scripts/test-git-secrets-allowlist.shreported 42 PASS, 0 FAIL. Fixtures use the account-id placeholders already in this test package (111111111111,222222222222), plus333333333333.Not run locally: gosec (not installed here), and the repo-wide
go test -short -race ./...hook (skipped for disk reasons; CI runs it).Mutation notes: each of these changes fails at least one new test:
ListCloudAccountserrorOut of scope / notes
Enabled. That matches the existingcloud_account_idpath, which doesn't checkEnabledeither. Whether disabled accounts should be refused for marketplace actions in both paths is a separate decision.CloudAccountFilter{}), the same as fix(api): match RI exchange history scope through the deployment's registered account #553. A provider filter would be a small optimization, but I kept the same shape as fix(api): match RI exchange history scope through the deployment's registered account #553.authorizeSessionSell).Maintainer: please mirror the issue's triage labels onto this PR:
priority/p2,severity/medium,triaged,urgency/this-quarter,impact/few,effort/s,type/bug.🤖 Generated with Claude Code