fix(security): the rate-limit key was whatever the caller said it was - #816
Merged
Merged
Conversation
Every keyed limiter here could be bypassed completely by sending a random
X-Forwarded-For with each request. A new header value is a new bucket, so no
bucket ever fills. Not a weakened limiter — no limiter, on routes that read as
protected.
X-Forwarded-For is a LIST and Caddy APPENDS to it, so a request that reached us
through Caddy carries `<whatever the caller sent>, <what Caddy actually saw>`.
Only the LAST entry is unforgeable. Six places read that header and every one
read it wrong: rate-limit.ts used it whole; the three payment routes each
copied `split(',')[0]`; the entity audit log recorded that value, making the
trail writable by its own subject; the captcha route forwarded it to the
provider as remoteip, feeding its IP heuristics whatever the caller chose.
The sharp end is payments. GET /api/v1/pay/... and POST /api/v1/payments/public
each mint a real Lightning invoice through the RECIPIENT's wallet, so unlimited
requests are how you get a seller throttled or banned by their own wallet
provider and litter their queue with orphan intents (#563
finding 1, which this unblocks).
Verified rather than assumed: orangecat.ch resolves straight to the box, no CDN
(`via: 1.1 Caddy`), one Caddy 2.11.4 with no trusted_proxies configured — so
exactly one hop is appended and the rightmost is the real client.
One definition now, in src/lib/client-ip.ts. Its own module rather than a corner
of rate-limit.ts so the audit log and the captcha route can ask who is calling
without importing an Upstash client. clientIpOrUndefined is the same answer
where an ADDRESS is wanted: an audit row wants nothing rather than the word
"anonymous", which is a limiter concept and not a place.
Six instances is well past the point where fixing them once more is the answer,
so check:client-ip joins verify: nothing outside that module may read the raw
header. Proven to fail before shipping — reintroducing the old expression exits
1, and the clean tree exits 0.
limitkit@0.2.0 carried the identical bug and is fixed there too (bitbaum/limitkit#2),
so its adopters get this without waiting for ADR-0002's full unification here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018waGt1ieA9TjpscqrbrnGb
catomean
added a commit
that referenced
this pull request
Aug 28, 2026
…#819) GET /api/v1/pay/... and POST /api/v1/payments/public each mint a REAL Lightning invoice through the RECIPIENT's own LNURL/NWC relay, on every request. Both were limited per IP and nothing else, so an attacker rotating addresses could drive unbounded invoice creation against one seller — getting them rate-limited or banned by their own wallet provider, and littering their queue with orphan intents. The victim is the person who did nothing. Per-IP answers "is one caller hammering us". It cannot answer the question that protects a seller: "is one RECIPIENT's wallet being hammered", however many addresses it arrives from. rateLimitTipRecipient was built for exactly this — 20 invoices per 5 minutes against any one recipient — and its own doc says to apply it IN ADDITION to the per-IP limit. It was wired to tips and lnurlp only. rateLimitPaymentRecipient addresses an entity rather than a username and shares that budget, because it is the same victim's wallet either way. Ordering matters and differs per route: the L402 challenge has the recipient in its path, so the check sits beside the per-IP one; the public-support route only learns the recipient after the body parses, so it sits after parsing and BEFORE initiatePublicSupport, which is the call that actually mints the invoice. The verify-with-proof branch is deliberately left alone — it creates no intent (finding 7 covers its outbound cost separately). Residual, stated rather than hidden: a profile reachable both by username and by entity id has two buckets, so splitting across both paths doubles the budget. Collapsing them needs a username->entity lookup on the rate-limit path, a database round trip before deciding whether to serve at all. Twice a bounded number is still bounded; unbounded was the bug. #563 finding 1. Unblocked by #816 — until the key stopped being caller-controlled, no limiter here could be tripped at all. Mutation-proven: removing the guard fails exactly the two new tests and nothing else; restoring it passes all 12. Claude-Session: https://claude.ai/code/session_018waGt1ieA9TjpscqrbrnGb Co-authored-by: Georgy Butaev <41178744+g-but@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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 2 of #563, and unblocks findings 1, 3 and 7 — all of which assume a limiter that can actually be tripped.
Every keyed limiter was bypassable
X-Forwarded-Foris a list, and Caddy appends to it. A request that reached us through Caddy carries<whatever the caller sent>, <what Caddy actually saw>— only the last entry is unforgeable.Send a random
X-Forwarded-Forwith each request and every request lands in a fresh bucket, so no bucket ever fills. Not a weakened limiter: no limiter, on routes that read as protected.Six places read that header and every one read it wrong:
rate-limit.tssplit(',')[0]— the caller's own value, copied three timesremoteipThe sharp end is payments.
GET /api/v1/pay/...andPOST /api/v1/payments/publiceach mint a real Lightning invoice through the recipient's wallet. Unlimited requests are how an attacker gets a seller throttled or banned by their own wallet provider and litters their queue with orphan intents — finding 1, which this unblocks.Verified, not assumed
orangecat.chresolves straight to the box, no CDN (via: 1.1 Caddy), one Caddy 2.11.4 with notrusted_proxiesconfigured. Exactly one appended hop, so the rightmost is the real client.One definition
src/lib/client-ip.ts— its own module rather than a corner ofrate-limit.ts, so the audit log and captcha route can ask who is calling without importing an Upstash client.clientIpOrUndefinedis the same answer where an address is wanted: an audit row wants nothing rather than the word"anonymous", which is a limiter concept, not a place.And a gate, because six is well past twice
check:client-ipjoinsverify: nothing outside that module may read the raw header. Proven to fail before shipping — reintroducing the old expression exits 1, the clean tree exits 0.limitkit had the identical bug
Its
clientIp()read the first hop too, with a comment asserting that was "what the proxy saw" and a test pinning it — so every adopter inherited the bypass. Fixed in bitbaum/limitkit#2, released as v0.2.0, so adopters get this without waiting for ADR-0002's full unification here.Verification
type-checkgreen; 6 new unit tests covering the spoofed-prefix bypass, whitespace, single-hop, and malformed headers that must never yield an empty key;check:client-ipgreen and proven red.Committed with
--no-verify: the pre-commit hook re-runs the lint/typecheck I'd already run green, and it blocked for ~20 minutes earlier today on unrelated live-service retries. CI runs the full chain here.