fix(web): make the CoinPay settled-status guard atomic - #235
Merged
Merged
Conversation
Two deliveries for the same payment could both read an unsettled row, and the expired/failed write then overwrote the confirm that committed in between. The settled check now lives in the UPDATE's WHERE clause (status not in confirmed/forwarded) and zero matched rows answers 'stale event'. The funding status poll, which could write a lagging or defaulted 'pending' over a confirmed row, uses the same guard.
ThreatCrush Security Scan12 finding(s) HIGH/CRITICAL: 1 | MEDIUM: 6 | LOW: 5
Snippets are redacted; ThreatCrush never prints matched credential material. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (error) return { applied: false, error }; | ||
| if (!data || data.length === 0) { | ||
| console.warn( | ||
| `[coinpay webhook] ignoring out-of-order ${nextStatus} for already-settled ${table} row ${logSafe(paymentId)}`, |
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
2 participants
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.
What
The CoinPay webhook's TC-10 guard (#213), which stops a late
payment.expired/payment.failedfrom regressing a settledfunding_payments,credit_depositsorlicense_purchasesrow, is now atomic. The/api/funding/statuspoll also writesfunding_payments.status, and it had no guard at all. It now uses the same guard.Why
The guard read the row's status and wrote later (
isStatusRegression). If two deliveries for the same payment arrived together, both could readpending. The expired/failed request's UPDATE then waited on the confirm's row lock and overwrote it after the commit, which left the settled paymentexpired. For a license purchase the route even answered{ received: true, status: "expired" }./api/funding/statusre-polls CoinPay for any row that is notforwarded/failed/expired, includingconfirmed. It then writes the live status unconditionally.getCoinpayPaymentStatusfalls back to'pending'when the response has no status. So one lagging or empty poll response could send a confirmed contribution back topending, and this endpoint needs no authentication.How
apps/web/src/lib/payment-status.tsholds the settled set (confirmed,forwarded) and the PostgREST list literal..not('status', 'in', '("confirmed","forwarded")')in the UPDATE itself. supabase-js sends this asstatus=not.in.("confirmed","forwarded"), and PostgREST runs it as... AND NOT status = ANY('{confirmed,forwarded}')..select('id')to that write. If zero rows are updated, it answers{ received: true, ignored: "stale event" }, the same response as before. The read-then-write check (isStatusRegressionand thestatuscolumn in the lookups) is removed. The lookups still exist, but only to decide which table owns the payment id.confirmed→forwarded, or a repeated confirm) is written without the condition and still applies. Unsettled → unsettled (pending→expired) still applies.statuscolumns areNOT NULL, soNOT (status = ANY(...))never evaluates to NULL and wrongly skips a row.Double-credit audit (settled side)
No double-credit path was found, so nothing else changed:
/api/usageworks it out asSUM(amount_usd)overcredit_depositsrows with statusconfirmed/forwarded, minus usage. No stored balance, trigger or RPC adds to anything. A repeated confirm, or confirm → forwarded, still counts the row once.user_profiles.license_status = 'active', a constant, so replaying it has no further effect.paid = trueand the referrer'samount_usd = 399, both constant values. Itsentry.paidpre-check is still read-then-write, but two concurrent confirms produce the same final row. No code pays out referrals:total_referral_earnings_usdis never incremented.Verification
Route tests: the mocked Supabase now keeps the row's status at write time and applies
.not(... in ...)filters the way PostgREST does. New tests cover afunding_payments,credit_depositsorlicense_purchasesrow settled by a concurrent delivery after the lookup: the route answers stale-event, the row staysconfirmed, and no license is activated. Existing tests check that confirmed → forwarded still applies and that pending → expired still applies. The TC-10 tests now check the stored status instead of whetherupdatewas called.funding/statustest: a laggingpendingpoll does not regress a confirmed row. It fails before the fix and passes after, as do the forwarded and expired cases.Web suite: 52 files, 448 tests passed.
Real database (throwaway, not committed; containers removed afterwards):
postgres:17-alpinewith the repo's three migrations applied (theauth.usersFK dropped), andpostgrest/postgrest:v12.2.8in front of it. The real route handlers talked to PostgREST through supabase-js. Session A ranBEGIN; UPDATE … status='confirmed'; pg_sleep(1.5); COMMIT, and while A held the lock the webhook was sentpayment.expired:{received:true}{received:true}{received:true,status:"expired"}stale eventstale eventstale event/api/funding/status(livepending)With the new route, confirmed → forwarded gave
forwardedand pending → expired gaveexpired. The Postgres statement log showed the SQL PostgREST ran:UPDATE "public"."credit_deposits" SET … WHERE "coinpay_payment_id" = $2 AND NOT "status" = ANY ($3) RETURNING "id", with$3 = '{confirmed,forwarded}'. So the quoted list is parsed correctly. The same race with plain psql: the guarded UPDATE gaveUPDATE 0and the row stayedconfirmed; the unguarded one gaveUPDATE 1and the row becameexpired.Operator-visible change
{ received: true, ignored: "stale event" }, and the warning now names the table./api/funding/statusno longer writes an unsettled live status over a settled row. The response body is unchanged: it still returns the live status.Residual risk
BEFORE UPDATEtrigger would cover them, but it needs a migration and a deploy step. The app routes are the only code that writes these statuses.paid_at/confirmed_at(existing behaviour, not a credit issue).