Repository navigation
sec(purchase): refuse a capped purchase whose price is not provably USD - #1296
Conversation
The purchases MaxPurchaseAmount cap (group permission and API-key max_amount) is USD, but the recommendation's currency was dropped at the scheduler boundary, so enforcePurchaseConstraints compared any currency's raw amount against it: 2,400 EUR cleared a 2,400 USD cap. - Keep common.Recommendation.CommitmentCurrency on RecommendationRecord (JSONB payload, no DDL); empty stays unknown, never a defaulted USD. - planstep takes the currency from the stored row it prices from. - enforcePurchaseConstraints checks a batch with any non-USD or unknown rec against an unmatchable amount: a grant with any spend cap refuses it with a 403 naming the currency; an uncapped grant still allows it. This is the rule checkAzureExecuteConstraints already applies. Refs LeanerCloud#361 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB22. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB24. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review 1 (cold, fresh context): VERDICT: APPROVE ac1dda6 Reviewed: git diff origin/main...ac1dda6 (one non-merge commit 89ac220 plus a merge of main 1c5787a). PR OPEN. CI: 34 pass, 1 skipping. Findings:
Attacked:
One further finding, pre-existing and not introduced by this PR, has been reported privately to the maintainers. |
|
Independent review 2 (claude-fleet, cold): REQUEST_CHANGES at ac1dda6 Blocking Non-blocking
Entry point and per-purchase cap (MaxPurchaseAmount on a group permission or API-key max_amount): does the head refuse a price that is not provably USD?
Currency inputs. The currency flows from the go parser (scheduler.go:1755), into the JSONB payload, then through size or priceFromStored.
Fail-closed
Callers and consumers
Migrations: none. Probes (head export; each restored with cp and checked with cmp; the final diff against
Commands (GOTOOLCHAIN=go1.26.10, no cloud credentials)
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
|
Merged current main (6b586cc) into the branch. The merge was clean, with no conflicts. New head: Checks on the merged tree, with no cloud credentials:
|
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 3 approving independent review(s) on |
Summary
This PR fixes one boundary of #361: the purchases spend cap.
MaxPurchaseAmounton a group permission, ormax_amounton an API key, is denominated in USD. Before this PR it was compared against the raw amount of a purchase in any currency. The other boundaries are listed in the table below.Refs #361
Root cause
The cloud-commitments-go side of the class (go#124, closed) added
common.Recommendation.CommitmentCurrency. The pinned AWS parsers set it for RIs and Savings Plans, and so do the pinned GCP Compute Engine, Cloud SQL and Memorystore clients. The platform lost it at the first boundary:scheduler.convertRecommendationsbuilt aconfig.RecommendationRecordwith no currency field, so the stored recommendation, the execution row and every rec handed toenforcePurchaseConstraintscarried bare numbers.purchaseConstraintSetsthen put the batch's total commitment intoMaxPurchaseAmount. The auth matcher compared it against the USD cap.The result: a purchase priced at 2,400 EUR passed an approve-any cap of 2,400 USD (
2400 <= 2400) and was bought. In general, whichever way the exchange rate points, the comparison either lets through a purchase over the operator's ceiling or refuses a legal one.Fix
internal/config/types.go: newRecommendationRecord.CommitmentCurrency(commitment_currency). It is persisted inside the recommendations row's JSONBpayloadand the execution'srecommendationsJSONB. No DDL and no migration. An empty value means unknown. It is never defaulted to USD.internal/scheduler/scheduler.go:convertRecommendationscopies the provider'sCommitmentCurrencyonto the record.internal/planstep/resolve.go:sizetakes the currency from the stored row it prices from, not from the displayed row. The costs are the stored row's, so their currency must be too.internal/api/purchase_currency.go(new) andenforcePurchaseConstraints: if any rec in the batch is not provably USD (another currency, or empty or whitespace), the constraint sets are checked with an amount the matcher can never satisfy (math.MaxFloat64).MaxPurchaseAmountrefuses the batch with a 403 that names the currency. The message still contains "constraints".This is the same rule
checkAzureExecuteConstraintsalready applies on the Azure RI exchange path. BecauseenforcePurchaseConstraintsis the single choke point, the rule covers every money path that calls it:claimed_row_check.go)Where the currency comes from.
priceFromStoredcopies the stored row (out := *stored), so the currency used on the web execute path is the stored one. A request body that says"commitment_currency":"USD"cannot lift the refusal, and a test pins this.I chose fail-closed over conversion because the issue rules out FX conversion and a currency field on the cap ("deliberately not in scope", PR #1515). A guardrail that cannot establish the denomination of the amount it checks has not checked anything.
Boundaries of #361: status after this PR
MaxPurchaseAmount/ API-keymax_amountvs rec amountsenforcePurchaseConstraints(all purchase money paths)scheduler.convertRecommendationsplanstep.sizemax_payment_due_usdvs quote currencyExecuteExchange(go#203)fix/issue-521) mapsErrQuoteNotUSDto 422. Not duplicated hereMaxPurchaseAmountvs exchange currencycheckAzureExecuteConstraintsmaxPayment, _ := maxRat.Float64()drops the exactness flaghandler_ri_exchange.gofirstNonEmptyCurrencydefaults reshape output to"USD"handler_ri_exchange.gofix/issue-1206) edits the function that calls itmax_step_commitmentvs step total (auto-purchase, and the email-token approver with no approve verb)purchase.CheckStepCommitmentCap,requirePlanStepCapCommitmentCurrency. Failing closed here would stop every Azure auto-purchase plan until the go side populates Azure currency. The edit is small (about 20 lines instep_cap.go, separate from #1245's hunk in the same file)handler_purchases_revoke.gocheckConfirmedRefundalready refuses a currency mismatchri_exchange_history.payment_duehas no currency columnproviders/azure3def3f89Behavior change to review
CommitmentCurrency. That is fail-closed by design: the cap cannot be checked against an amount of unknown currency.Verification
All evidence below is unit tests against mocks/fixtures, plus real-Postgres integration tests on the shared testcontainers Postgres. No real cloud API was called and nothing was purchased. Tests ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.New tests:
internal/api/purchase_currency_test.go(unit; the mock answers with the realauth.PermissionsAllowForConstraintSets):TestEnforcePurchaseConstraints_NonUSDUnderCapRefused: 900 EUR / JPY / empty / whitespace against a 1,000 USD cap, for execute, execute-any, approve-any and approve-own...._MixedBatchRefused..._USDStillComparedAgainstCap:USD,usd,USD. Over the cap is still refused...._NonUSDWithoutCapAllowed..._NonUSDOtherDimensionKeepsItsError..._NonUSDBackendErrorPropagates: exactly one auth call.TestHandler_executePurchase_NonUSDStoredPriceUnderCapRefused: the full web execute path with a stored EUR row and a request claiming USD. Nothing is saved.internal/planstep/resolve_test.go:TestResolveTakesCurrencyFromStoredRow.internal/scheduler/scheduler_test.go:TestScheduler_ConvertRecommendations_CommitmentCurrency.internal/api/combined_plan_purchase_integration_test.go(real Postgres):TestCombinedPlanPurchase_NonUSDApprovalRefusedUnderCap. A 2,400 EUR stored recommendation goes throughUpsertRecommendations, then the scheduler's plan-step attach (SendUpcomingPurchaseNotifications), then thepurchase_executionsrow. The test assertsEURround-trips to the execution row. ThenPOST /api/purchases/approve/{id}with an approve-any cap of 2,400 and of 1,000,000 must be 403 naming EUR, with nothing bought. With no cap it must be 200 and complete.Existing fixtures that model USD recs under a cap now say
CommitmentCurrency: "USD"(they had no currency, which now means unknown).fanoutRecs()and the combined-plan fixture default to USD, so the existing claimed-row and combined-plan integration tests still exercise the USD cap. No assertion was weakened.Fail before. I put back
origin/main'shandler_purchases.go,planstep/resolve.goandscheduler/scheduler.goand kept the new tests:TestEnforcePurchaseConstraints_NonUSDUnderCapRefused,..._MixedBatchRefused,TestHandler_executePurchase_NonUSDStoredPriceUnderCapRefused,TestResolveTakesCurrencyFromStoredRowandTestScheduler_ConvertRecommendations_CommitmentCurrencyall fail.TestCombinedPlanPurchase_NonUSDApprovalRefusedUnderCapfails withexpected: 403, actual: 200and body{"status":"completed"}. The 2,400 EUR step was approved under the 2,400 USD cap and bought:f.service.recorded()is non-empty.Pass after. Commands:
The machine was heavily loaded (load average about 24, about 50 containers from parallel suites). Two integration runs hit testcontainers start-up flakes: a reaper container in "removing" state, and 90-second subtests. Each test passed when re-run. The full run of the targeted set passed once with all green. I did not run whole integration packages or git-secrets here; the combined-tree run covers them.
Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Not in scope
max_step_commitmentboundary and the twohandler_ri_exchange.goitems. See the table for why each was deferred.CommitmentCurrency. That belongs to cloud-commitments-go.Moneytype, or a currency field on the caps. The issue rules both out.Frontend follow-up for the UI owner
No frontend files are changed. The recommendations API now returns
commitment_currencyon each rec, and the field is omitted when unknown. Two UI changes would help:$. They could show the currency, or flag a rec with an unknown or non-USD currency.The new 403 is a plain
{"error": ...}message, so the existing error toast already shows it.Labels
A maintainer, please mirror the issue's triage labels:
priority/p2,severity/high,urgency/this-quarter,impact/few,effort/l,type/bug,triaged.🤖 Generated with Claude Code