Repository navigation
fix: rate-lock reuse gap, overlapping webhook retries, key-rotation docs - #1287
Merged
github-actions[bot] merged 1 commit intoOct 16, 2026
Conversation
…cument key rotation windows - fiat-crypto: bind a rate lock to the user who created it and consume it on first use, so the same lock can no longer be confirmed twice (duplicate conversions) or confirmed by a different user than the one who locked it. - webhooks: add an in-flight guard around the delivery worker's 1s ticker so a slow or backlogged pass can never overlap with the next tick and deliver the same webhook twice. - WEBHOOK_VERIFICATION.md: document the overlap/retention/replay-tolerance windows WebhookKeyRegistry actually enforces during key rotation, which were implemented but never written down. Closes #1278, Closes #1279, Closes #1280, Closes #1281.
|
@maikNeta is attempting to deploy a commit to the smartdevs17's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@maikNeta Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
github-actions Bot
pushed a commit
that referenced
this pull request
Oct 17, 2026
#1184 #1185 #1186) (#1290) # Payout reliability, payment correctness and payment security (iterations 10-11) Closes #1183, #1184, #1185, #1186. These four tickets are templated "deliver the next increment" issues with no fixed scope. Following the precedent of #1286 and #1287, this PR takes one concrete, real problem per theme instead of a broad sweep, and keeps each change independently reviewable. ## Scope ### #1185 — Improve payout reliability `PayoutEngine` had **no execution path**. Auto-approved and manually approved batches were stamped `completed` without anything being sent, `executing` was unreachable dead state, there was no failure or retry path, and calendar schedules fired on every evaluation pass. - `pending -> executing -> completed | failed` is now a guarded transition table. `completed` is only reached when the rail returns a transaction hash. - An injected `executor` actually settles the batch and receives a stable `idempotencyKey` on every attempt. - Failures schedule an exponential backoff with jitter, capped by `maxAttempts`. - `daily` / `weekly` / `monthly` respect their period and key their batches by period bucket, so a replayed pass cannot pay the same period twice. - `thresholdAmount` of `0` no longer falls through to the "pay anything" branch. - `payout.created` / `pending_review` / `executing` / `completed` / `failed` / `delayed` events via a pluggable sink. A throwing subscriber cannot abort a payout decision. - Reservations are released on rejection, and on exhaustion only after an approver explicitly calls `releaseReservedFunds` — returning them automatically would mint a fresh batch and spin against a rail that is already down. - One merchant's misconfiguration is isolated from the rest of the evaluation pass. - New REST surface at `/api/v1/payouts`, mounted in `backend/src/index.ts`. ### #1184 — Fix payment correctness bugs - **Unsupported FX pairs were priced at `1.0`.** `getCurrentRate` returned par for any pair it did not know, so `GBP -> BTC` would quote one Bitcoin per pound and `confirmConversion` would record it. Unquoted pairs are now an error. - **Money was derived with binary floating point.** `33.33 * 1.08` is `35.99640000000001`, so the crypto amount stopped agreeing with the fiat amount charged. Added `backend/src/services/money.ts` (bigint minor units, half-away-from-zero rounding) and routed conversions through it. - **`lockRate` accepted `NaN`, negative and infinite amounts**, producing locks whose conversion amounts were `"NaN"`. - **`processFiatRefund` recorded `'failed'`** — indistinguishable from a real failure — returned `true` without refunding anything when no payment intent was present, and was not idempotent, so a retry issued a second Stripe refund. - **Revenue pool recipients were validated individually but not in aggregate**, so a pool could be configured to distribute more than 100% of what it received. - **`recordDistribution` had no idempotency on `txHash`**, so a reorg or retried indexer double-counted revenue. - **`getConversion` / `listConversions` returned live internal references**, letting a caller mutate a financial record. ### #1186 — Strengthen payment security - Every `/api/v1/payouts` route, **including reads**, requires an authenticated approver principal. The check runs before any lookup, so an unauthenticated caller cannot probe batch existence from the status code. - `Idempotency-Key` on every money-moving route. A key reused with a **different** body is rejected with `409 idempotency_key_reuse_mismatch` instead of silently replaying the earlier response — previously a caller could believe it created a new resource while holding the old one. - Strict allowlists (asset, schedule cadence) and bounded amounts and identifiers. - The asset allowlist and amount ceiling are enforced in the **engine** as well as the route, so a caller reaching the engine directly is not less protected. - Route-scoped rate limits: 120/min for the router, 30/min for approval decisions. - Engine errors are mapped to sanitised HTTP responses; rail text never reaches clients. ### #1183 — Improve documentation - `docs/PAYOUT_RELIABILITY.md` — lifecycle, retry policy, reservations, events, API, ops runbook. - `docs/PAYMENT_CORRECTNESS.md` — the money, FX, idempotency and allocation invariants. - `docs/security/payment-security.md` — authentication, authorization, allowlists, rate limits, audit. - `docs/README.md` index entries, and `PAYOUT_APPROVALS.md` cross-links. ## Files | File | Change | | ---- | ------ | | `backend/src/services/money.ts` | new — decimal-safe money arithmetic | | `backend/src/services/payouts/payout-engine.ts` | execution, retry, cadence, events, limits | | `backend/src/services/fiat-crypto.ts` | pair validation, amount validation, exact conversion, refund semantics | | `backend/src/services/revenue-pool/index.ts` | allocation ceiling, distribution idempotency | | `backend/src/middleware/idempotency.ts` | payload fingerprinting, bounded cache | | `backend/src/routes/payouts.ts` | new — `/api/v1/payouts` | | `backend/src/routes/stripe.ts` | unsupported-pair and settlement guards | | `backend/src/index.ts` | mount the payouts router | | `backend/src/**/__tests__/*` | new and updated tests | | `docs/PAYOUT_RELIABILITY.md`, `docs/PAYMENT_CORRECTNESS.md`, `docs/security/payment-security.md` | new | ## Tests - `services/__tests__/money.test.ts` (new) — parsing, rounding, exact rate application. - `services/payouts/__tests__/payout-engine.test.ts` — execution, backoff growth, attempt exhaustion, reservation release, cadence, cycle keys, event emission, per-merchant isolation, concurrency collapse. - `services/__tests__/fiat-crypto.test.ts` — unquoted pairs, amount validation, exact conversion, refund state and idempotency, deterministic settlement references, snapshot isolation. - `services/revenue-pool/__tests__/revenue-pool.test.ts` (new) — allocation ceiling on add and update, distribution replay. - `middleware/__tests__/idempotency.test.ts` — fingerprint mismatch, key ordering, nested payloads, cache eviction. - `routes/__tests__/payouts.test.ts` (new) — auth on every route, validation, idempotency replay and mismatch, approve/reject lifecycle, rate limiting. ## Verification `npm run test --workspace backend`, `npm run lint --workspace backend` and `npm run test:docs` are the relevant commands. **Note:** this change set was authored in an environment without a Node runtime, so the suites were not executed locally — CI is the first run. The changes were reviewed by hand against the existing suites, and the two pre-existing payout tests that assert the old "no execution" behaviour were updated to the new contract.
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.
These four issues are all templated "deliver the next increment" iteration tickets with no specific scope. Following the precedent set by #1286 (the prior iteration's equivalent batch), this PR picks one concrete, real fix per theme.
fiat-crypto.ts'sconfirmConversionhad no single-use guard on a rate lock — the samerateLockIdcould be confirmed repeatedly, producing duplicatecompletedconversions from one locked amount.userId: 'user_default'on every conversion), so any caller could confirm anyone else's lock. Rate locks are now created with and checked against the acting user's id.setIntervalhad no in-flight guard. A slow endpoint or a backlog bigger than one tick could clear meant the next tick started a second, overlapping pass that could redeliver the same still-"pending" webhook a second time before the first pass marked it "processing".WEBHOOK_VERIFICATION.mdchecked off "secret rotation" but never documented theoverlapSeconds/retentionSeconds/toleranceSecondswindowsWebhookKeyRegistryactually enforces — added that section.Testing
backend/src/services/__tests__/fiat-crypto.test.ts: confirms a conversion is tied to its lock's owner, rejects confirming another user's lock, and rejects reusing an already-confirmed lock.backend/src/services/webhooks.test.ts: uses fake timers + a stalledfetchto show the worker's ticker never callsfetcha second time while the first delivery attempt is still in flight.Closes #1278,
Closes #1279,
Closes #1280,
Closes #1281.