Repository navigation
fix(purchase): re-check purchase constraints on the claimed row before buying - #1036
Conversation
…e buying The execute entry points (run now, session approve, email-link approve, direct execute) checked permission Constraints against a row read by GetExecutionByID, but transitionApproveAndExecute buys the row its claim CAS returns. A repricing or edit landing between the two was bought unchecked. Thread a purchase.ClaimedRowCheck through ApproveExecution, ApproveAndExecute, DirectExecute and RunPlannedPurchaseNow. The funnel runs it on the CAS-returned row right after the claim, before any audit stamp or provider call; a refusal fails the row before submission (status failed, no_submission, plan suppressions released), matching the existing post-claim step cap and pre-execution refusals. Each handler passes a check re-running the Constraints it already ran; refusals map to the check's client error (403 for a Constraints denial). The SQS approve worker passes nil (no session Constraints). Closes LeanerCloud#415 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
priority/p2
|
Warning Review limit reached
This review includes 22 billable files and costs up to $5.50. Or wait 59 minutes for your next included review. View limit details
Comment |
Independent review verdict: CLEAN at 0497cf7Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, acting as a security reviewer focused on the purchase money path. The PR body was treated as a claim. Coordinator: claude-cc-reviewer. Blocking findings: none. No unapproved-spend bypass was found on the user-triggered execute paths, which are this PR's scope. Entry points:
Every provider call goes through What is re-checked on the claimed row: the permission constraints (max amount as total commitment, provider, service, region, account IDs, non-zero commitment). The approve-any/own verb and creator ownership are re-run live, and the approver's permission is read at claim time. Four-eyes, account existence and amount versioning were not post-claim checks before this PR either. When the re-check refuses:
Notes (non-blocking):
What was attacked and how:
Gate:
|
|
@cristim the Build Docker Image failure on this head is in 🤖 Generated with Claude Code |
Root cause
Every synchronous execute entry point checks the caller's permission Constraints (MaxPurchaseAmount, Providers, Services, Regions, AccountIDs) against a row read by
GetExecutionByID. It then calls the purchase funnel,transitionApproveAndExecute, which buys the row its claim CAS (claimRunning→TransitionExecutionStatus ... RETURNING) returns. Nothing re-checked that returned row.SavePurchaseExecutionis an unconditional full-row upsert, so a repricing or edit that lands between the check and the CAS gets bought without being checked against the cap. Confirmed ond7eb3b0d, the same shape as in the maintainer's reading at 68e0eaa.Fail-before proof: the new Postgres regression below, run with the funnel's re-check disabled, has
runPlannedPurchasereturn success for a row repriced from $300 to $5,000 under a $1,000MaxPurchaseAmountcap. The same happens on the session approve path.Fix (option (a): one funnel, fails closed)
purchase.ClaimedRowCheck(func(ctx, claimed *config.PurchaseExecution) error) is threaded throughApproveExecution,ApproveAndExecute,DirectExecuteandRunPlannedPurchaseNow, which are all on the sharedPurchaseManagerInterface.transitionApproveAndExecuteruns the check (in the newclaimAndCheck) on the CAS-returned row right after the claim and before any audit stamp or provider call.internal/api/claimed_row_check.go). The pre-CAS check stays, so an over-cap request on an unchanged row is still refused with the row untouched.status: failed.ErrAuditLosskeeps its precedence.Entry points
runPlannedPurchase)enforcePurchaseConstraints(execute)RunPlannedPurchaseNowapprovePurchaseViaSession, incl. approve-any/approve-own)authorizeSessionApproveApproveAndExecuteapproveViaToken)approveConstraintsForSession(incl. plan step cap)ApproveExecution→ApproveAndExecuteexecutePurchase,execute_mode=direct)execute+authorizeSessionExecuteDirectDirectExecutehandleApproveMessage)ApproveExecutionnil, unchangedretryPurchase)enforcePurchaseConstraints(execute)on the failed rowrecordit authorizedFailure-state semantics (needs owner sign-off)
The maintainer's comment left this open ("revert to pending/paused vs mark failed with an audit note"). This PR takes the choice that matches the existing post-claim refusals:
claimAndExecute's step cap re-check (failClaimedBeforeSubmission) andpreExecutionRefusal(stale Azure pricing, ladder, armed re-drive). A refused row ends:status = failed,no_submission = true,error= "purchase refused after claim: ... no longer passes the authorization checks: "approved_by,executed_atorpre_approval_skip_reasonstamped (the check runs before them);transitioned_byrecords who claimed itTrade-off to flag: one capped caller whose run loses this race now fails the row, where a revert would have left it pending for another approver. Reverting would need a new
running → pending/notified/pausedtransition with no existing precedent. Fail-closed was the safer default. A transient auth backend error during the re-check also fails the row, which is fail-closed.Tests
internal/purchase/claimed_row_check_test.go(mock store, recording provider): for run-now, session approve, direct execute and token approve, a row repriced between the snapshot and the CAS is refused withErrClaimedRowRefused. The check's own error stays reachable, the check saw the claimed row, the provider is never called, and the saved row isfailedwithno_submissionand no approve/execute stamps. Also covered: the unmodified row still buys exactly once, a check backend error fails closed, a terminal save failure isErrAuditLoss, a plan row releases its suppressions, and a nil check keeps the old behavior.internal/api/claimed_row_check_test.go: each handler entry point passes a non-nil check that refuses the repriced CAS-returned row with a 403 (real auth matcher) without reaching the funnel's success path. The unmodified row runs. Covers the token path with approve-own cap, approve-any cap, and plan step cap with no verb.internal/api/handler_purchases_claimed_row_integration_test.go(integrationtag, real Postgres via testcontainers, real handler and manager, counting fake provider): a second writer reprices the row with a rawUPDATEduring the handler's Constraints check, before the CAS. Run-now and session approve both answer 403, the provider has zero calls, the row isfailedwithno_submission, and there is nopurchase_history. Unmodified controls complete with exactly one provider call.TestHandler_approvePurchaseViaSession_FourEyesOn_DifferentApproverSucceedshad its CAS mock return a row with no recommendations, a different row from the one checked. That is exactly this bug's shape, and the new check refuses it. The mock now returns the checked recommendations.Mutation probes, each made and then reverted (all caught):
nil: its tests failErrClaimedRowRefusedmapping removed fromexecuteFailureError/tokenActionError: the 403 tests failVerification (local, macOS, at 0497cf7)
All tests ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. Fakes only: no real cloud API calls, no purchases.go build ./...,go vet ./...,go vet -tags integration ./...: cleango test -count=1per package:./internal/purchase/...,./internal/api/...,./internal/mocks/...,./internal/server/...,./internal/testutil/...: okgo test -tags integration -count=1 ./internal/purchase/and./internal/api/(testcontainers Postgres): okrun ./...(config includes theintegrationtag): 0 issuesgocyclo -over 10on every changed non-test.gofile: none.transitionApproveAndExecute,approvePurchaseViaSessionandapproveViaTokenstay at 10.--scan --cachedof all changed files in a freshgit initrepo with isolated HOME: clean.scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.Not verified: a real cloud account (not in scope; no purchases).
Out of scope / follow-ups
PurchaseDelayHours > 0,approveWithDelay→scheduleApprovedExecution): this path has the same check-then-CAS shape, but the CAS goes toscheduled, andfireOneDuelater buys whatever row its ownscheduled → runningclaim returns, with no session to re-check against. A check at scheduling time would only narrow the window. The fix needs a decision: store the approver's cap on the row, or re-check at fire time. A refusal there would also need ascheduledrollback state.requireExecutionAccess) is not re-checked post-claim. Recommendation-levelAccountIDsConstraints are re-checked. A change to the row-levelcloud_account_idbetween the check and the CAS is the same class and could take the same callback.recordit authorized (no Constraints on approve), so it has no check/buy mismatch of this kind.PurchaseManagerInterface(internal/api/types.go,internal/server/interfaces.go), which fix(purchase): approve and direct-execute map every failure to 409, partial runs mint no revocation token, and approved-row claim races #532 also touches. fix(purchase): approve and direct-execute map every failure to 409, partial runs mint no revocation token, and approved-row claim races #532's return-the-row part has already landed (fix(purchase): map session approve and direct execute failures by cause #644), so this applies on current main; a rebase may be needed if fix(purchase): approve and direct-execute map every failure to 409, partial runs mint no revocation token, and approved-row claim races #532 lands first.Closes #415
🤖 Generated with Claude Code