fix(purchase): gate scheduler/SQS on typed web source constant (follow-up to #1456) - #1458
Conversation
…w-up to #1456) executableByScheduler gated on the bare literal exec.Source == "web", but web-originated executions are persisted with Source = common.PurchaseSourceWeb ("cudly-web"). The literal never matched, so a pending/notified web row on a plan with AutoPurchase=true could be auto-executed by the cron sweep and the SQS execute_purchase path WITHOUT the required token-link human approval -- a fail-open on a money path (CodeRabbit CRITICAL thread on #1456, left unaddressed). Fix: - manager.go: compare against the typed common.PurchaseSourceWeb constant, not a stringly-typed literal (repo enum convention). - manager_test.go / messages_test.go: the prior regression tests set Source: "web" (matching the buggy gate) and relied on AssertNotCalled("GetPurchasePlan"); the mock's default GetPurchasePlan stub does not record the call, so those tests could never detect the bypass. Use the real persisted value and assert the invariant via a GetPurchasePlanFn sentinel: web rows must short-circuit before the AutoPurchase plan fetch. Verified the tests FAIL on the buggy literal and PASS on the fix.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Follow-up to #1456 — unaddressed CodeRabbit CRITICAL
Part of the adversarial-sweep over PRs merged in the last 2 weeks. #1456 merged
with an unresolved CodeRabbit CRITICAL review thread; this implements the fix
and hardens the regression tests that could never have caught it.
Bug (money-path fail-open)
executableBySchedulergated on the bare literalexec.Source == "web", butweb-originated executions are persisted with
Source = common.PurchaseSourceWeb(
"cudly-web", set inhandler_purchases.go). The literal never matched, so apending/notified web row on a plan with
AutoPurchase=truecould beauto-executed by both the cron sweep (
ProcessScheduledPurchases) and theSQS
execute_purchasepath without the required token-link human approval.Confirmed to reproduce on current
main.Fix
manager.go: compare against the typedcommon.PurchaseSourceWebconstantinstead of a stringly-typed literal (repo enum convention).
manager_test.go/messages_test.go: the prior regression tests setSource: "web"(matching the buggy gate) and relied onAssertNotCalled("GetPurchasePlan"). The mock's defaultGetPurchasePlanstub does not record the call, so those tests could never detect the
bypass. They now use the real persisted value and assert the invariant via a
GetPurchasePlanFnsentinel: web rows must short-circuit before theAutoPurchase plan fetch. Verified the tests fail on the buggy literal and
pass on the fix.
Verification
go build ./...,go vet,go test ./internal/purchase/...all green."web"value → both regression tests fail.