Repository navigation
fix(api/purchases): guard web purchase execution (closes #643, #644, #647) - #655
Merged
Merged
Conversation
…cs and double-submit Two related guards on the web executePurchase path: - #643: validate each client-supplied recommendation's Provider/Service/ Term/Payment/Count at the API boundary (validatePurchaseRecommendation in validation.go), wired into validateExecutePurchaseRequest after the account scope check. Per-provider Term (1/3) and Payment whitelists reject malformed values (Term:7, Payment:"foo", non-positive Count, empty/all provider) before they reach the cloud SDK with a silent default substituted. Scoped to the web execute path only; the retry path replays already-validated recs and is not re-gated. - #644: submit-time idempotency. purchaseIdempotencyKey hashes actor + sorted rec tuples + capacity; findDuplicatePendingExecution recomputes the key for recent web-sourced pending executions (no schema change) and collapses a double-click/retry within a 2-minute window onto the existing execution instead of minting a second approvable row (double-spend). The frontend execute button is now disabled before the confirm dialog and network call in both the single and fan-out paths, and re-enabled on cancel. Updated existing executePurchase / per-account-perms tests to carry valid recs and the new GetPendingExecutions lookup; added table-driven validation tests, idempotency-key and duplicate-lookup tests, and frontend button-disable tests. #647 (server-side capacity_percent consistency) requires plumbing the pre-scaling count through the frontend and rec type; split into a follow-up to keep this change focused.
…ts (#647) capacity_percent was a decorative audit-only field: the frontend scaled rec counts client-side, sent the percent alongside, and the backend stored it without ever cross-checking that the scaled counts agreed with the recorded percent. The audit trail for a financial action could claim e.g. "50% capacity" while the recs summed to 100%. Fix: stamp the pre-scaling recommended count onto each rec at submit time (RecommendedCount in config.RecommendationRecord, recommended_count on the TS Recommendation/LocalRecommendation wire types) and add validateCapacityConsistency, wired into validateExecutePurchaseRequest after the per-rec content validation. It rejects any rec where floor(RecommendedCount*pct/100) != Count with a 400. The check is opt-in per rec: recs that carry no recommended_count (legacy callers, single-rec full-capacity purchases, retry replays) are skipped, so the change is backward-compatible and never produces a false rejection for callers that predate the field. JSONB persistence means no schema change. The frontend stamps recommended_count = r.count on the scaled copy in handleBulkPurchaseClick before the counts are floored, so the value the backend verifies always describes the recs it accompanies. Tests: table-driven validateCapacityConsistency cases (full/partial/floor/ mismatch/absent/mixed); a frontend round-trip test that recommended_count survives into the executePurchase POST body on the single-rec path.
cristim
added
bug
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR addresses duplicate purchase submissions and capacity consistency validation across frontend and backend. Frontend disables the execute button while awaiting confirmation and restores it on cancel. Backend validates per-recommendation fields, detects duplicate submissions via stable idempotency keys within a 2-minute window, and verifies capacity-scaling math using preserved pre-scaled counts. ChangesPurchase Execution Guards and Validations
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/app.ts (2)
320-332:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle confirm-dialog rejection so the execute button can’t get stuck disabled.
If
confirmDialog()throws/rejects, this path exits before the existingfinally, leaving the button disabled until reload.Suggested fix
- const ok = await confirmDialog({ - title: `Send ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} for approval?`, - body: 'This will email an approval request to the configured approver. Cloud commitments are charged only after the approver clicks the link in that email.', - confirmLabel: 'Send for approval', - destructive: false, - }); + let ok = false; + try { + ok = await confirmDialog({ + title: `Send ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} for approval?`, + body: 'This will email an approval request to the configured approver. Cloud commitments are charged only after the approver clicks the link in that email.', + confirmLabel: 'Send for approval', + destructive: false, + }); + } catch (error) { + const err = error as Error; + showToast({ message: `Unable to open confirmation dialog: ${err.message}`, kind: 'error' }); + if (executeBtn) { + executeBtn.disabled = false; + executeBtn.textContent = 'Send for Approval'; + } + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app.ts` around lines 320 - 332, confirmDialog may throw/reject and currently can exit early leaving executeBtn disabled; wrap the await confirmDialog(...) call in a try/catch/finally (or use .catch()) so any rejection is handled and executeBtn is re-enabled in the finally block before returning. Specifically update the block around confirmDialog in frontend/src/app.ts to catch errors from confirmDialog and ensure executeBtn.disabled = false and executeBtn.textContent = 'Send for Approval' in the finally, then bail out if the dialog was not confirmed or an error occurred.
421-546:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap fan-out execution in
try/finallyso button reset is guaranteed on all failures.This function disables the button up-front but only reenables on explicit cancel or the normal success path. Any thrown error (e.g.,
loadDashboard()failure) can leave the button permanently disabled.Suggested fix
async function handleFanOutExecute(buckets: FanOutBucket[]): Promise<void> { const executeBtn = document.getElementById('execute-purchase-btn') as HTMLButtonElement | null; - if (executeBtn) { - executeBtn.disabled = true; - executeBtn.textContent = `Sending 0/${buckets.length}…`; - } + const resetExecuteBtn = () => { + if (executeBtn) { + executeBtn.disabled = false; + executeBtn.textContent = 'Send for Approval'; + } + }; + if (executeBtn) { + executeBtn.disabled = true; + executeBtn.textContent = `Sending 0/${buckets.length}…`; + } - const ok = await confirmDialog({ - title: `Send ${buckets.length} bulk purchase${buckets.length === 1 ? '' : 's'} for approval?`, - body: `This will submit ${buckets.length} separate purchase request${buckets.length === 1 ? '' : 's'} and email ${buckets.length} approval request${buckets.length === 1 ? '' : 's'}. Each must be approved individually before its commitments are charged.`, - confirmLabel: 'Send all for approval', - destructive: false, - }); - if (!ok) { - if (executeBtn) { - executeBtn.disabled = false; - executeBtn.textContent = 'Send for Approval'; - } - return; - } + try { + const ok = await confirmDialog({ + title: `Send ${buckets.length} bulk purchase${buckets.length === 1 ? '' : 's'} for approval?`, + body: `This will submit ${buckets.length} separate purchase request${buckets.length === 1 ? '' : 's'} and email ${buckets.length} approval request${buckets.length === 1 ? '' : 's'}. Each must be approved individually before its commitments are charged.`, + confirmLabel: 'Send all for approval', + destructive: false, + }); + if (!ok) return; - // existing fan-out logic ... - await loadDashboard(); + // existing fan-out logic ... + await loadDashboard(); + } catch (error) { + const err = error as Error; + showToast({ message: `Failed to send bulk purchases for approval: ${err.message}`, kind: 'error' }); + } finally { + resetExecuteBtn(); + } - - if (executeBtn) { - executeBtn.disabled = false; - executeBtn.textContent = 'Send for Approval'; - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app.ts` around lines 421 - 546, The button disable/reset must be guaranteed even if later awaits throw — move the re-enable/reset into a finally block that executes after the user confirms; specifically, after the confirmDialog ok check, wrap the fan-out and subsequent processing (the promises/allSettled/results handling, closePurchaseModal/clear..., showToast branches, openArcheraOfferModal('purchase'), and await loadDashboard()) in try { ... } finally { if (executeBtn) { executeBtn.disabled = false; executeBtn.textContent = 'Send for Approval'; } } so any exception from api.executePurchase, loadDashboard, or other async work still re-enables the executeBtn; keep the existing early-cancel branch that re-enables and returns as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler_purchases.go`:
- Around line 1253-1278: The current duplicate check in
findDuplicatePendingExecution (which reads pending rows via
h.config.GetPendingExecutions and computes purchaseIdempotencyKey) is racy
because the insert that persists a new pending execution occurs later; make the
idempotency check atomic by enforcing it at the DB level and handling it in the
same flow that creates executions. Add a persisted idempotency key column (or a
dedicated idempotency table) and a UNIQUE constraint on the idempotency key (and
creatorID/scope as needed), then change the create-pending-execution path (the
code that inserts the new pending execution) to either perform the lookup+insert
inside a single DB transaction/row lock or simply attempt the insert and, on
unique-constraint violation, return the existing execution; reference
purchaseIdempotencyKey, findDuplicatePendingExecution, GetPendingExecutions and
the creation/insert method used around Line ~1388 to locate where to implement
the DB constraint + upsert/unique-error handling.
In `@internal/api/validation.go`:
- Around line 510-529: validatePurchaseRecommendation currently normalizes
provider/payment into local vars but takes rec by value so changes aren't
persisted; change its signature to accept a pointer
(*config.RecommendationRecord) and, after computing provider :=
strings.ToLower(strings.TrimSpace(rec.Provider)) and payment :=
strings.ToLower(strings.TrimSpace(rec.Payment)), assign those canonical strings
back to rec.Provider and rec.Payment before returning; keep the existing
whitelist checks (purchasePaymentWhitelist, purchaseTermWhitelist) but ensure
all callers are updated to pass a pointer to RecommendationRecord.
---
Outside diff comments:
In `@frontend/src/app.ts`:
- Around line 320-332: confirmDialog may throw/reject and currently can exit
early leaving executeBtn disabled; wrap the await confirmDialog(...) call in a
try/catch/finally (or use .catch()) so any rejection is handled and executeBtn
is re-enabled in the finally block before returning. Specifically update the
block around confirmDialog in frontend/src/app.ts to catch errors from
confirmDialog and ensure executeBtn.disabled = false and executeBtn.textContent
= 'Send for Approval' in the finally, then bail out if the dialog was not
confirmed or an error occurred.
- Around line 421-546: The button disable/reset must be guaranteed even if later
awaits throw — move the re-enable/reset into a finally block that executes after
the user confirms; specifically, after the confirmDialog ok check, wrap the
fan-out and subsequent processing (the promises/allSettled/results handling,
closePurchaseModal/clear..., showToast branches,
openArcheraOfferModal('purchase'), and await loadDashboard()) in try { ... }
finally { if (executeBtn) { executeBtn.disabled = false; executeBtn.textContent
= 'Send for Approval'; } } so any exception from api.executePurchase,
loadDashboard, or other async work still re-enables the executeBtn; keep the
existing early-cancel branch that re-enables and returns as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d377b6b-8f77-40ff-a1fe-a710fc4afba1
📒 Files selected for processing (11)
frontend/src/__tests__/purchase-execution-toast.test.tsfrontend/src/api/types.tsfrontend/src/app.tsfrontend/src/recommendations.tsfrontend/src/types.tsinternal/api/handler_per_account_perms_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_guards_test.gointernal/api/handler_purchases_test.gointernal/api/validation.gointernal/config/types.go
…in tx (#643) Move the pending-execution scan inside the WithTx block so the duplicate check and the INSERT are a single atomic operation, closing the TOCTOU race that the pre-tx duplicatePurchaseResponse call could not prevent. - Add GetPendingExecutionsTx to StoreInterface (SELECT ... FOR UPDATE) - Extract scanExecutionRows helper to share row-scan logic between the pool path and the new tx path - Extract matchDuplicateInList to keep persistExecutionAndSuppressions and executePurchase under the gocyclo threshold - Add derefStringOrEmpty to eliminate the nil-check branch in executePurchase - Change validatePurchaseRecommendation to *RecommendationRecord receiver so provider/service/payment normalisation is written back to the caller - Add GetPendingExecutionsTx fallback to all six test mock types Closes #643
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 22, 2026
This was referenced Jul 21, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three guards on the web
executePurchasepath (#643, #644, #647).validatePurchaseRecommendationvalidates each rec's Term/Payment/Count/Provider/Service at the web execute boundary (previously only UpfrontCost/Savings sign + $10M cap + count/capacity/scope). RejectsTerm:7/Term:0,Payment:"foo", negative/zero count, empty provider.purchaseIdempotencyKey+findDuplicatePendingExecutioncollapse a duplicate submit within a 2-minute window onto the existing execution (keyed by rec set + account + creator); frontend disables the button before the confirm dialog / network call (single + fan-out). Prevents double-spend from double-click / retry.RecommendedCountfield (+recommended_countwire type); frontend stamps the pre-scaling count;validateCapacityConsistencyrejects any rec wherefloor(RecommendedCount*pct/100) != Count(opt-in, skipped when absent for backward compat).Test plan
internal/api1215 pass (+9 new); table-driven validation tests; idempotency-key stability + window/creator/recset discrimination + duplicate-response + button-disable; capacity round-tripgo build,go vet, gofmt,tsc --noEmitclean; cancel functions untouched (bug(purchases): token-path cancel allows cancelling running/approved/paused executions with no in-flight guard (session path is stricter) #645 boundary respected)Closes #643, #644, #647.
Note:
TestPGXMock_GetExecutionByID_WithTimestampsfails on the unmodified base (already tracked as #624/#627) — unrelated to this PR.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests