fix(payments): bound L402 verify retries per payment intent - #839
Merged
Conversation
The verify-retry branch sits deliberately outside the per-IP challenge budget, and that part is right: a payer who has genuinely paid must be able to retry until settlement is seen, and sharing the challenge budget would lock them out of their own purchase. But unbounded is not unbudgeted. Every verify on a non-terminal intent drives an outbound call — the recipient's NWC or LNURL relay, or mempool — so one valid token bought unlimited traffic aimed at someone else's infrastructure, and the per-IP limiter could not see it by design, because the thing being replayed is a token, not an address. rateLimitL402Verify gives it a budget of its own: 60 checks per minute, far more than an honest client polling a payment needs and far less than a loop, applied before the outbound call so a refused check never leaves the box. Keyed on the INTENT, not the preimage — the preimage is caller-supplied, so keying on it would have allowed exactly the bucket-rotation the per-IP limiter already fell to. The route crossed the 150-line gate on the way, so both branches moved to lib/api/l402-handlers: they carry different rate budgets and different failure surfaces, and neither is the route's business. Route is 36 lines. Refs #563 finding 7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018waGt1ieA9TjpscqrbrnGb
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.
Closes finding 7 from the #563 audit.
The bug
The L402 verify-retry branch sits deliberately outside the per-IP challenge
budget, and that part is right: a payer who has genuinely paid must be able to
retry until settlement is seen, and sharing the challenge budget would lock them
out of their own purchase.
But unbounded is not the same as unbudgeted. Every verify on a non-terminal
intent drives an outbound call — the recipient's NWC or LNURL relay, or
mempool. So one valid token bought unlimited traffic aimed at someone else's
infrastructure, and the per-IP limiter could not see it by design, because the
thing being replayed is a token, not an address.
The fix
rateLimitL402Verify(paymentIntentId)— 60 checks per minute, far more than anhonest client polling a payment needs and far less than a loop. Applied before
the outbound call, so a refused check is one that never leaves the box.
Keyed on the intent, not the preimage. The preimage is caller-supplied, so
keying on it would have allowed exactly the bucket-rotation trick the per-IP
limiter already fell to. The intent id cannot be varied without a valid status
token for a different payment, and it is not a secret (it is in the status
route's own URL), so no hashing is needed.
The budget covers terminal intents too, even though those short-circuit in
refreshPaymentStatusbefore any rail call — a cheap check is still a check,and one bound is easier to reason about than two.
Verification
4 new tests: the budget is keyed on the intent; a refused check returns 429
and never reaches
verifyL402Payment; verify still does not spend thechallenge budget; and the challenge branch still does not spend the verify one.
22 suites / 174 tests green across the api + payments suites, and a full
non-incremental
type-checkclean. The box was too loaded to finish onenpm run verify, so it was verified by parts instead: eslint on the changedfiles, plus check:sizes / audit:routes / duplication / dead-fields /
client-ip / user-scoped-deletes / one-current-user / rpc-exists /
currency-units all green. CI runs the whole bundle.
Refs #563 finding 7.