fix: stop wrongly refusing first-time signups, and make the cause loud - #442
Merged
Ryanmello07 merged 18 commits intoAug 23, 2026
Merged
Conversation
A first POST /auth/network-create of the day, from an address that had
created one account in ten days, was answered 429 on both production
deployments. Signup was effectively down for new users.
Root cause: session.ResolveClientAddress returns the immediate TCP peer's
address when that peer is not in trustedProxyPrefixes(). The ingress proxy
reaches the api from a docker bridge address, and
BRINGYOUR_TRUSTED_PROXY_CIDRS -- which defaults to loopback only -- appears
exactly once in this repo, in its own const, and is set by nothing. So every
request on the deployment resolved to one address and every per-address
budget in the service collapsed into a single budget for the whole fleet.
The window is sliding, so it never reset.
The four changes below do not widen a single limit. Three of them stop a
legitimate user from being charged for something that is not theirs; the
fourth makes the misconfiguration impossible to sit through silently.
1. Report an unenumerated proxy instead of failing silently
(session/client_session.go)
A request whose peer is NOT trusted but which carries X-UR-Forwarded-For
or X-Forwarded-For is exactly the wedged condition: something upstream
believes it is a proxy this service trusts, and this service disagrees.
That now emits a glog.Errorf naming the peer, the header, the current
trusted set and BRINGYOUR_TRUSTED_PROXY_CIDRS, and saying plainly that
every client behind the peer is sharing one rate-limit budget.
The peer is NOT auto-trusted. Honoring a forwarding header from an
unenumerated peer would let any client claim any source address and step
outside every per-address limit in the service, which is a strictly worse
bug than the one being fixed. The report is rate limited -- once per
distinct peer over a bounded set, then at most once per minute -- because
any client can set a forwarding header on a directly reachable api, and
an unbounded report is both a log flood and unbounded memory. The
interval path never stops firing, so a deployment that becomes wedged
later cannot go quiet again.
2. The auth-attempt limiter answers 429, not 503
(model/auth_model_attempt.go, model/rate_limit_error.go)
maxUserAuthAttemptsError returned "503 User auth attempts exceeded
limits." A 5xx tells every well-behaved client and SDK that the server is
broken and the request should be retried, and every retry records another
attempt, so the status itself consumed the remaining budget faster and
the refusal reinforced itself. The other limiter on the same endpoint
already returned 429. Rate limiting is client-attributable.
3. Input-validation failures no longer spend the budget
(model/network_model.go)
UserAuthAttempt was consumed above the terms check and above network-name
validation. An unticked terms box, a name that was too short, or a name
someone else already had each burned one of five slots in a five-minute
window -- and for a signup carrying no user auth (SSO, wallet) those
slots are shared by everyone at the client address, so one person
fumbling a form refused strangers. The limiter now runs after input
validation.
This does not make abuse easier. Nothing between the top of the function
and the limiter grants a capability that is not already available
unauthenticated and unlimited: ValidateNetworkName is pure, and
checkNetworkNameAvailability is the same read-only name lookup POST
/auth/network-check already serves to anyone with no limiter at all.
Everything that creates or mutates state stays below the limiter.
TestValidSignupsStillSpendTheAuthBudget pins the abuse side directly: it
interleaves a free pre-validation refusal before every well-formed
submission and asserts the limiter still fires on the well-formed ones at
the documented count. If the limiter moved back above validation, the
interleaved refusals would be charged too and that count would halve, so
the test discriminates the fix from the bug rather than passing either
way. Making a form mistake free does not stretch an attacker's budget.
4. Honest refusals, with Retry-After
(model/network_create_rate_limit.go, model/auth_model_attempt.go,
router/handler_utils.go)
The account-creation refusal read "You have reached the maximum number of
account creations for today" to a user who had created none: the budget
is keyed on a subnet of the client address, so the usual recipient is
being refused for what others sharing the address did and then told it
was their own doing. Both refusals now say the limit is scoped to the
network address, which is what lets support tell a wrongly-refused user
on a shared connection apart from actual abuse.
The auth-attempt wording is chosen by scope, because it is not always
address-wide: with no user auth the Redis key carries no identity and the
budget really is shared, but with a user auth it is that account's. A
blanket "address-scoped" sentence would have been false at five of the
seven call sites.
Both refusals carry Retry-After. RaiseHttpError reads it off the error
through a one-method interface so model does not have to import the
router. For account creation the value is the real remaining time on the
window, computed in the same statement and against the same clock as the
count.
Deliberately NOT changed:
- the /29 IPv4 and /56 IPv6 masking in ip.go. Widening it is an
anti-abuse policy decision, not a bug fix.
- every limit value.
- the auth-attempt sliding window never resetting under retry. A rejected
attempt is still recorded, so a client that retries keeps its own window
full. That is real, but it is the limiter's protection against a
hammering caller; removing it would weaken it. Retry-After now gives
such a client the wait it needs to actually recover.
- the 500-with-a-plain-body convention for validation refusals, which
means NetworkCreateResult.Error is unreachable over HTTP. Pinned
unchanged by TestNetworkCreateValidationRefusalIsNotReportedAsSuccess.
- the email/UserAuth branch never calling SetUserAuthAttemptSuccess.
Tests: session/client_address_ingress_test.go,
model/network_create_rate_limit_regression_test.go,
api/handlers/network_create_status_regression_test.go. The tests that
pinned the wrongful behaviour now pin the fixed behaviour and keep a note of
what the old behaviour was and why it was wrong.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
The previous commit made an unenumerated ingress proxy loud: a request whose TCP peer is not in BRINGYOUR_TRUSTED_PROXY_CIDRS but which carries a forwarding header now tells the operator to add that peer's subnet. Review found that the remedy it printed was a way to take the api down. ResolveClientAddress's TRUSTED branch required X-UR-Forwarded-For as ip:port, or X-Forwarded-For paired with X-Forwarded-Source-Port and single-hop, and returned an error for anything else. router.wrap and router.wrapWithInput both answer a session construction failure with HTTP 500, on every endpoint. Stock nginx (proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for), ALB, CloudFront and Cloudflare all send a bare X-Forwarded-For with no source port, and $proxy_add_x_forwarded_for APPENDS, so a second hop adds a comma that was also rejected. The deployment shape most likely to trigger the new report was exactly the one the remedy broke. 1. The trusted branch reads what real proxies send (session/client_session.go) A forwarding header is now parsed as a chain and walked RIGHT TO LEFT, skipping entries that are themselves trusted proxies; the first entry that is not a trusted proxy is the client. Entries may be a bare ip, ip:port or [v6]:port. X-Forwarded-Source-Port still pairs with a single-hop X-Forwarded-For exactly as before; on a longer chain the port belongs to whichever hop wrote it and is dropped rather than guessed, so the address is resolved with port 0. Every per-address limiter buckets on the ip alone (server.ClientIpHash), so an unknown port costs the client nothing. The direction is the security property. A proxy appends what it observed, so everything to the LEFT of the entry a trusted hop wrote is text the client supplied. Reading left to right -- "the original client" -- would let every client behind the proxy pick its own rate-limit bucket, which is strictly worse than the shared-budget bug being fixed. Repeated header field lines are joined before the walk, because Header.Get returns only the first line and a proxy that appends a second line is legal; reading only the first would hand back precisely the client's own value. Nothing that used to resolve resolves differently. Every shape newly accepted returned an error before, and an error is a 500, so there is no working deployment for the looser contract to take anything away from. TestEveryLoosenedShapeUsedToBeAnHttp500 checks that claim against a verbatim copy of the old implementation rather than asserting it in a comment. 2. An unreadable header degrades instead of 500ing, and says so Anything still unreadable falls back to the peer address. That cannot be a wrong attribution -- the peer is where the packets came from, the most restrictive answer available -- but it silently reinstates the fleet-wide budget collapse, so reportUnusableForwardingHeader names the trusted peer and its own remedy. The header VALUE is never logged: on any proxy that forwards client headers it is client-controlled. 3. The report's remedy is now the whole remedy It states what the proxy must send once enumerated and, more importantly, what it must not do: forward a client-supplied value through unchanged. 4. The report guard is under test, and is per-peer The report tests stubbed reportUnenumeratedProxy with a copy that re-implemented the rate-limit guard, so the production guard could be deleted with the suite still green. They now stub only glogErrorf, putting ResolveClientAddress -> the real report -> the real guard -> the log under one seam. The guard itself was one global "last reported at" instant shared by every peer, over a set that never evicted: 1024 distinct peers and the per-peer path was gone for the process lifetime, and whoever called first in each interval took the only token, so a caller who could reach the api directly could keep the genuinely wedged proxy out of the log. It is now map[(peer, condition)]time.Time, written only when a report is actually emitted, pruned when it reaches its cap, keyed on the unmapped address. 5. Impl tagging uses %w (router/handler_utils.go) WrapRequireAuth, WrapRequireClient and WrapNoAuth tagged impl errors with %s, which flattens the error and breaks the errors.As that RaiseHttpError uses to find Retry-After. The status survived (the regex peels the tag), so the symptom would have been an invisible missing header. No rate limit is reached through those wrappers today; this is for the next one. 6. The comment justifying the limiter reorder is corrected (model/network_model.go) It claimed checkNetworkNameAvailability is the same lookup /auth/network-check already serves unmetered. It is a superset: same fuzzy search plus an exact SELECT in a transaction. The reorder is still safe -- nothing above the limiter writes -- but the comment is load-bearing for anyone moving code across the limiter later and it overstated its case. No limit value, no /29 or /56 masking, and no auto-trust of an unenumerated peer is touched. Abuse does not get cheaper: relative to a WEDGED deployment per-client attribution does allow more accounts (5/24h per /29 rather than 5/24h fleet-wide), but that is nominal policy, and the wedged state was a bug that refused legitimate users. Degrading to the peer address can only tighten. Port 0 makes the wallet-auth-challenge key coarser, never finer.
The %w that keeps Retry-After reachable through errors.As was written three
times. A test that constructs its own fmt.Errorf("[impl]%w", ...) passes no
matter what the wrappers do -- the first version of the test for this did
exactly that and stayed green when the production tagging was reverted to %s.
One function, driven end to end through WrapNoAuth and directly for the two
wrappers that need a JWT, actually fails.
proxyIsTrusted unmaps and server.ClientIpHashForAddr does not, so a peer left in its [::ffff:a.b.c.d] form buckets under the ipv6 /56 rule instead of the ipv4 /29 one and holds a second, separate per-address budget.
Two gaps in the previous commit. The prune walks the whole suppression map holding the process-global report mutex, on a path any caller who can reach the api directly can trigger. While the map is full -- which is precisely the state a caller cycling source addresses creates -- it ran on every request, turning the flood guard into an O(cap) critical section aimed at whoever filled it. It now runs at most once per interval. That is safe not because a more frequent scan would find nothing (entries age out continuously) but because the prune is best-effort reclamation: a lingering entry delays reuse of one slot, memory is bounded by the cap either way, and the global-token tier keeps the report firing while a prune is deferred. The branch where every hop in the chain is itself a trusted proxy -- ordinary internal traffic between two enumerated components -- returns the peer address and emits no report, and nothing reached it. Conflating it with the unreadable-header case would fill an operator's log with alerts about traffic working exactly as intended.
TestVerifySourceIpPrecedence case 3 asserted that a trusted proxy sending a bare X-Forwarded-For is REJECTED, on the reasoning that rejecting is safer than silently re-attributing the request to the proxy. The premise was right; the conclusion was not. The rejection is an error, and router.wrap turns that into HTTP 500 on every endpoint, so it meant the api was down for any deployment that enumerated a stock nginx. Attribution is preserved by reading the client out of the header instead. The case now pins that, plus the two properties it was really protecting: an unreadable header falls back to the immediate peer rather than to anything the caller chose, and a chain resolves to the entry the trusted hop appended, not the entry a client prepended.
…794cf CORRECTION to ac794cf, which said "Nothing that used to resolve resolves differently." That is not true, and the exception is the one place this work narrows attribution rather than widening it. The previous reader never compared a forwarded VALUE to the trusted set: it took X-UR-Forwarded-For, or X-Forwarded-For + X-Forwarded-Source-Port, and returned it. The new reader walks the chain right to left skipping entries that are themselves inside an enumerated CIDR -- which is what finds the client behind two hops and what stops a client prepending an address of its own. The cost is that a forwarded address INSIDE an enumerated range is now read as a proxy hop. An operator who enumerates 172.16.0.0/12 because their docker bridge peer is 172.18.0.7 therefore takes the individual bucket away from any real caller arriving from inside 172.16.0.0/12. The direction is conservative -- those callers share the proxy's address, they are never attributed to anything a caller chose -- but it is the shared-budget failure this work exists to remove, on a narrow path, so it is now said out loud in three places rather than left to be discovered: - reportUnenumeratedProxy tells the operator to enumerate the NARROWEST range that contains only proxies and says what an address inside one costs; - the !found branch's comment states both readings of an all-enumerated chain and why neither is reported, instead of asserting the benign one; - TestForwardedAddressInsideAnEnumeratedCidrSharesTheProxyBucket pins the trade against a verbatim replica of the old branch, and TestEveryLoosenedShapeUsedToBeAnHttp500 now says in its own comment that its argument covers the loosening only. Keeping the skip rather than special-casing a single-hop chain is deliberate: a rule that takes a trusted value when the chain has one entry and skips it when the chain has two is harder to reason about and does not remove the multi-hop case, where skipping is what makes the walk correct.
… client pick one ResolveClientAddress read the FIRST forwarding header it found and ignored the rest, and forwardingHeaders lists X-UR-Forwarded-For first. A proxy overwrites the header IT sets and passes every other client header through untouched, so on any deployment whose proxy owns X-Forwarded-For -- Caddy, nginx's documented recipe, ALB, CloudFront, Cloudflare -- a client that sent its own X-UR-Forwarded-For beat the address the proxy had vouched for and chose its own rate-limit bucket, stepping outside the 5-per-24h account creation limit and the 5-per-5min auth attempt limit. The capability predates this branch, but this branch is what makes it matter: before it, enumerating a stock proxy answered HTTP 500 on every endpoint, so few operators would have got as far as reaching it. Preferring X-Forwarded-For instead is rejected, with evidence: BOTH ownership shapes exist in this project. The warp load balancer forces X-UR-Forwarded-For = $remote_addr:$remote_port (controller/verify_controller.go :139), while the beta Caddyfile has to strip X-UR-Forwarded-For by hand (header_up -X-UR-Forwarded-For) because Caddy owns X-Forwarded-For. A static preference in either direction is the forgery in one of the two shapes. So neither header is declared authentic. Every present forwarding header is read; when two of them name two DIFFERENT client addresses, NEITHER is honored and the request is attributed to the peer -- the most restrictive answer available, never an address of the client's choosing -- under a report of its own, because that fallback is the shared-budget collapse this file exists to prevent. Two deliberate limits on that rule: - only the ADDRESS is compared. A proxy that sets both headers correctly sends them in different forms (ip:port in one, a bare ip whose port reads as 0 in the other), so requiring the ports to match would call a working deployment a conflict and take its client address and port away for nothing. Port handling is therefore untouched: the first present header decides it. - the first PRESENT header still decides the value, not the first header that named somebody. Stepping past a header that names only enumerated hops is a caller inside the trusted range getting its own X-Forwarded-For honored. A malformed header still aborts the whole read, whichever header it is: the unreadable one can be the proxy's and the readable one the client's. Abuse is not cheaper. A client that reaches the new fallback lands on the proxy's shared bucket rather than one it named -- strictly more restrictive. It is a NEW ability on a warp-shaped deployment (X-UR-Forwarded-For was picked first, so a garbage X-Forwarded-For was never read) and it buys nothing: legitimate traffic resolves to real client addresses, so poisoning the peer bucket refuses only other poisoners. The report is bounded to one line per peer per condition per minute by the existing suppression, and the header VALUES are still never logged -- on this path both are client-influenced by definition, so only the two header NAMES, which come from the forwardingHeaders constants, are interpolated into a constant format string. Every operator-facing report now states the COMPLETE header contract -- OVERWRITE OR STRIP ALL THREE of X-Forwarded-For, X-UR-Forwarded-For and X-Forwarded-Source-Port -- rather than only the header that triggered it. An operator who fixes the one header a report named and passes the others through has closed nothing. This is also what closes the client-settable half of the X-Forwarded-Source-Port issue: that header sets the port component of the wallet-auth-challenge threshold key. TestUrForwardedForChainIsNotReadAsASingleHop used to end by pinning "X-UR wins over X-Forwarded-For when both are present". That pin is deliberately INVERTED here, not quietly weakened: it was the forgery wearing the other header name. The shapes where the two headers AGREE -- what a correctly configured proxy sends -- still resolve byte for byte as they did, which TestProxySettingBothForwardingHeadersConsistentlyIsUnchanged pins.
…se them Comments only, no behaviour change. Both are things review found that are correct as written but change what a bucket means on the day this ships, and neither was stated where the code does it. 1. parseRequestAddress's Unmap is a loosening at deploy. netip treats a 4-in-6 address as ipv6 (Is4 is false for it), so server.ClientIpHashForAddr masks such a client under the /56 rule -- and the first 7 bytes of the 4-in-6 form are zero, so EVERY 4-in-6 client on a deployment hashes to one bucket and shares one account-creation budget. After the Unmap they split into their real per-/29 buckets, so each gets a fresh budget once. Same wrongly-collapsed-bucket-un-collapsed as the rest of the branch, low reachability (Go renders ipv4 peers in ipv4 form), but disclosed rather than discovered. 2. the wallet-auth-challenge threshold keys on client_address_port as well as the address hash. The port is client-influenced wherever a trusted proxy passes X-Forwarded-Source-Port or X-UR-Forwarded-For through -- the remedy is the proxy's, and it is what the reports now state -- and it MOVES with the deployment: on a proxy that sends a bare X-Forwarded-For the port is 0 for everyone, so the key becomes per-/29 instead of per-connection. That makes the threshold bind for the first time and starts sharing five failed challenges across a /29. Deliberately not repaired. Dropping the port from that key closes the first half by making the second universal -- a shared budget on every deployment, which is the wrongful-refusal failure this branch exists to remove, in a limiter with no coverage here. Left alone on purpose, not by omission.
Caught by a deliberate break: gutting proxyForwardingHeaderContract down to two of the three headers left every assertion in the previous commit passing. Both reports already name all three headers incidentally -- "optionally with X-Forwarded-Source-Port", "or X-UR-Forwarded-For as ip:port" -- so Contains(report, "X-Forwarded-Source-Port") was satisfied by prose that says the opposite of what the assertion is for. The tests now pin the enumeration itself, "X-Forwarded-For, X-UR-Forwarded-For and X-Forwarded-Source-Port", in all three reports. With the same break applied, all three fail: the misconfiguration report does not mention "X-Forwarded-For, X-UR-Forwarded-For and X-Forwarded-Source-Port" the conflicting-header report does not mention ... the unusable-header report does not mention ...
…ty element Two follow-ups the full suite and a break turned up. TestVerifySourceIpPrecedence case 1 asserted "X-UR-Forwarded-For wins over everything" with the two headers naming two DIFFERENT clients, so it failed against the conflict rule -- and it was the same pin, in another package, for the same reason: on a deployment whose proxy owns X-Forwarded-For, the header the CLIENT can set is X-UR-Forwarded-For, and /verify egress identity must not be a value the caller picked. The case is split: 1 keeps the precedence /verify actually depends on (X-UR-Forwarded-For's ip:port beats the bare form plus X-Forwarded-Source-Port for the SAME client, which is what a proxy setting both correctly sends), and 1b pins the peer fallback for the disagreeing pair. observed before the split: verify_controller_test.go:168: X-UR-Forwarded-For must win, got "10.9.9.9:5555" TestAnEmptyChainElementIsNotSkipped pins the trailing-comma decision review raised as an open question. "203.0.113.9," degrades to the peer with a loud report rather than skipping the empty element, and that stays: the shape the tolerance would rescue is indistinguishable from the shape it would break -- where the header is wholly client-supplied, "6.6.6.6," would resolve to 6.6.6.6 -- and no proxy in this deployment's set emits an empty element. Both halves are asserted, so deleting the test is the deliberate act if a proxy that needs it ever turns up.
…hat does
A client behind a trusted proxy could still choose its own client address, and
therefore its own rate-limit bucket, on either deployment shape.
trusted = 172.16.0.0/12, peer 172.18.0.7:52344
X-Forwarded-For: 172.20.5.5 (the proxy's, an address inside the range)
X-UR-Forwarded-For: 6.6.6.6:1234 (the client's)
=> resolved to 6.6.6.6:1234
conflictingCandidates skipped candidates with found == false as "names no
client, so it cannot disagree", while thirty lines later the same condition on
candidates[0] meant "refuse everything". So the guard covered one ordering and
left its mirror open -- and which header a proxy owns differs by product, so a
rule that holds for one ordering holds for neither deployment.
found == false now means the same thing in both places: a PRESENT header whose
every entry is inside an enumerated CIDR is a claim that the client is a
trusted hop, it disagrees with any header naming a real client, and the request
falls back to the peer address. candidateNamesNoClient checks EVERY candidate,
not just the first.
Why this is safe in the direction it closes: the warp sidecar writes
X-UR-Forwarded-For as $remote_addr:$remote_port, the peer it sees. If X-UR
names a real client then that sidecar is the edge, so no inner enumerated hop
could have written an all-enumerated X-Forwarded-For -- that header came from
the client. The mirror direction (warp nginx behind an enumerated CDN) already
resolved to the peer via the first-candidate guard and loses nothing.
The shape is refused SILENTLY, not reported. Behind a trusted proxy the peer is
one address for every client, so the (peer, condition) suppression slot is
shared; a client can produce a no-claim header whenever it likes, so reporting
this would hand it a way to take that slot every interval and silence the
genuine conflicting-headers report. With two entries in forwardingHeaders, a
found == false candidate means at most one candidate is found == true, so
conflictingCandidates provably cannot fire for anything the new rule catches --
no currently-reporting shape changes.
Does not make abuse cheaper. The only new outcome is the peer address, which is
where the packets came from and the most restrictive answer available. A client
that wants to demote itself onto the peer bucket can already do it today with
X-Forwarded-For: garbage (a malformed header aborts the whole read), so this
adds no capability; it removes one.
Tests, each watched failing against the unfixed resolver:
session TestASecondForwardingHeaderCannotChooseTheBucket -- three new shapes
(both orderings + a two-hop enumerated chain), with wantReports pinning
that they stay quiet
session TestForwardedAddressInsideAnEnumeratedCidrSharesTheProxyBucket --
the two-header block, because "shares the proxy's bucket" is only worth
something when the caller also sends the other header
router TestSecondForwardingHeaderDoesNotReachTheHandler -- the same two
shapes at the HTTP boundary, where the ClientAddress handed to the impl is
what every per-address limit keys on
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
… raw ip
The report suppression key unmapped the peer but did not mask it, so a caller
bought a fresh report slot with every source address it owned. Measured on the
shipped code: one unenumerated-proxy report is 1754 bytes, and a caller
rotating addresses out of ONE ipv6 /64 -- an ordinary residential allocation --
sustained the full proxyReportedMax cap.
single ipv6 /64, 5000 fresh addresses per simulated minute
before: [1024 1024 1024 1024] lines/min = 2.59 GB/day, map at 1024 entries
after: [1 1 1 1] lines/min, map at 1 entry
The key is now the peer NETWORK, using the prefixes server.ClientIpHashForAddr
already uses to decide what counts as one client (/29 v4, /56 v6). ip.go is
untouched -- this reuses its answer, it does not change it. A caller that
cannot buy extra rate-limit budget by rotating inside its /29 or /56 can no
longer buy extra log volume with it either.
Coarser would be worse than the flood, so the test pins both directions: eight
addresses in one /29 are one line, and the ADJACENT /29 is still named. Two
genuinely different unenumerated proxies silencing each other would take away
the report an operator has to act on.
The liveness property is pinned separately and unchanged:
TestGenuineReportSurvivesAFloodOfDistinctPeers drives 4000 distinct /29s per
interval past the wedged proxy and requires all 5 of 5 of its reports.
Does not make abuse cheaper. It removes report slots, never adds any: every
peer that was suppressed before is still suppressed, and some that were not now
are. Nothing about address resolution or any rate-limit budget is touched.
Also states, at the format strings, that printing the peer's full unhashed
address is deliberate -- it is the one place this service writes a client
address in plaintext, and the address IS the remedy, since "enumerate this
CIDR" is not actionable from a hash. The header VALUE, the part a client
controls, is still never logged.
NOT done, deliberately: no global lines-per-interval ceiling. A caller holding
1024 distinct /29s (8192 ipv4 addresses) still reaches 626 lines/min in the
same harness. A true cap needs the ceiling on the already-seen branch, which is
exactly the path that keeps the wedged-proxy report firing every interval, so
it would trade a verified liveness property for log volume.
Test: session TestReportSlotsAreNotBoughtByRotatingSourceAddresses, watched
failing against the unbucketed key with "every address in one ipv4 /29 was
reported separately (8 lines)".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
CheckNetworkCreateRateLimit computes the hint in SQL and the comment above it promises "the real remaining time on the window: the oldest attempt still counted expires then." Nothing asserted the value. TestAccountLimitRefusalIsHonestAboutItsScope only required 0 < seconds <= window, api/handlers' assertRateLimitBody only required that the header parses and is positive, and the router tests use a test-local error type rather than model.rateLimitError -- so a flat restatement of the window, or arithmetic broken by the timestamp/timestamptz mix in that expression, passed the whole suite. The new test backdates a full budget of attempts by 20 hours and requires the hint to follow them: ~4h, not the flat 86400. The two candidate answers are 20 hours apart, so the 15-minute tolerance covers clock skew between the test process and postgres without coming near the wrong answer. The behaviour was already correct -- this is a coverage gap, not a defect, and the blast radius of a future regression here is bounded by the existing [1, window] clamp: a wrong hint, never an outage. It is worth pinning because the failure is symptomless from the server's side and lands entirely on the client, which either waits a day it did not have to or hammers the endpoint because the hint is obviously wrong. Does not make abuse cheaper: test-only, and it asserts a SMALLER hint is correct only when the attempts really are old. Watched failing against `INTERVAL '1 seconds' * $2` flattened to the full window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
TestUnenumeratedProxyReportIsRateLimited proves a second peer gets its own report line using 172.18.0.7 and 172.18.0.8. Now that the suppression key is a peer network, those two are distinct only because .7 is the last address of 172.18.0.0/29 and .8 is the first of 172.18.0.8/29 -- move either and the assertion silently becomes one peer reported twice, which the interval already forbids, and it passes for the wrong reason. Comment only; the invariant is pinned deliberately in TestReportSlotsAreNotBoughtByRotatingSourceAddresses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
TestGenuineReportSurvivesAFloodOfDistinctPeers drove 4000 iterations per
interval but only ~500 distinct /29s, because the counter varied bits the /29
mask keeps rather than the ones it does not -- so the bucketing the test sits
next to was doing part of the work the flood was supposed to do.
{n>>13, n>>5, n<<3} is the big-endian encoding of n into the 29 network bits,
so every iteration is its own /29: four thousand distinct networks per
interval, four times the map's cap. The assertion is unchanged and still holds
at 5 of 5.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
The comment claimed the wedged proxy 'wins the token on nearly every pass' and cited TestGenuineReportSurvivesAFloodOfDistinctPeers as proof. Review found both halves false: deleting the entire global-token branch left that test green, so it never reaches the tier it was cited for, and re-running the scenario with the flood arriving first after each interval boundary named the wedged proxy 0 times out of 5, not 5. The real mechanism is the seen-branch refreshing the key on every request, ahead of the prune. The prune evicts at exactly one interval, so a boundary-timed flood can starve the diagnostic -- at a cost of >=1024 sustained distinct /29s or /56s, to win fewer log lines. Never a wrong address, never a budget, never a spoof. Recorded rather than fixed: tightening the prune to buy the property back trades a real bound for a diagnostic nicety. A comment asserting a protection that does not hold is worse than one that states the limit.
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.
Signup is currently refusing first-time users on production. Reproduced on both deployments from an address that had created one account in ten days:
Root cause
ResolveClientAddressreturns the immediate peer's address when that peer is not intrustedProxyPrefixes().BRINGYOUR_TRUSTED_PROXY_CIDRSappears exactly once in the repo — its own const — and is never set, so the default is loopback only while the ingress proxy arrives from a bridge address. Every request in the deployment therefore resolves to one address and shares one budget of 5. The window is sliding, not calendar-day, so it never resets: once five creates land the deployment stays wedged.The Caddyfile sets
X-Forwarded-Source-Portspecifically so "the same address feeds rate limiting" — and the trust gate returns before that header is ever read.What this changes
1. The misconfiguration is now loud. An unenumerated peer arriving with forwarding headers is reported, naming the peer, the header, the current trusted set and the env var — and naming the other possibility (a client sending a header it is not entitled to) so an operator is not misled into whitelisting a spoofer. The peer is not auto-trusted; resolution is unchanged. Rate limited per bucketed peer, then per interval.
2. Standard proxies no longer 500. A first version of this warning told operators to enumerate the proxy — but the trusted branch required
X-UR-Forwarded-Forasip:port, orX-Forwarded-ForplusX-Forwarded-Source-Port, single-hop. Stock nginx, ALB, CloudFront and Cloudflare send a bareX-Forwarded-For, and$proxy_add_x_forwarded_forappends, so two hops add a rejected comma. That path becameHTTP 500for every endpoint — following the log line would have taken the API down. A bareX-Forwarded-Forfrom an enumerated proxy is now handled, pinned at the HTTP boundary byTestEnumeratedProxySendingOnlyBareXForwardedForIsNotA500.3.
503->429for the auth-attempt limiter. 7 call sites across login, verify, code sends and password reset. A 5xx tells every SDK the server is broken and to retry, and every attempt including rejected ones is recorded — so retries were self-reinforcing.4. Validation mistakes no longer spend budget. The limiter moved below terms, name validation and availability. A taken network name or an unticked terms box was consuming an address-wide slot. Nothing that mutates state sits above the limiter.
5. Honest refusals, with
Retry-After. The message told users who had created nothing that they had hit their limit. Wording is parameterized onuserAuth, because a blanket "address-scoped" claim is false at 5 of the 7 sites — only the nil-userAuth key is genuinely shared. Account creation computes the real remaining window.Deliberately not changed
The
/29and/56masking, every limit value, and the default trusted set. Those are anti-abuse policy, not defects — widening them is your call, not something to slip into a fix.ip.gois untouched.Verification
Three adversarial review rounds; the final one ran the suite rather than reading it, and re-ran each mutation first-hand. 8 deliberate breaks, each watched going red — including both HIGH findings from earlier rounds, proved closed at the HTTP boundary rather than by inspecting message text.
The suite shows 13 failures in
modelandapi/handlers. All 13 were re-run on a pristineupstream/maintree and fail identically there (payments/leaderboard/subsidy and a missingapple_roots.pem). Zero new failures. All 10 new model regression tests and both new handler tests pass when run explicitly.Two LOW findings are recorded rather than hidden: a comment overclaiming why the diagnostic survives a flood is corrected in the final commit, and
TestConfiguredAppleRootCertificatespanics the wholeapi/handlersbinary on a missing optional config — pre-existing on upstream, but it means this branch's own status-code tests are skipped in a full-package run unless that file is present.Deploy note
The code makes the wedge visible; the fix is to set
BRINGYOUR_TRUSTED_PROXY_CIDRSto the ingress proxy's range. The betaapiservice has noenv_filekey, so it cannot currently be injected there at all.