TRUSTED_PROXY_CIDRS: per-IP rate limits that work behind a proxy

[distronode-com]

First of the split you asked for in #30, against main, with no dialect layer under it. This is the one you said you most wanted. Nothing else from that pull request is in here: one setting, one resolver, one test file.

What it does

TRUSTED_PROXY_CIDRS is a comma-separated list of networks (a bare address means one host). For a peer inside one of them, TrustClientIP resolves the client IP from X-Forwarded-For and puts it in the request context; remoteIP then keys the rate limiter on that instead of the TCP peer. Empty by default, and an unconfigured instance behaves exactly as it does today: the headers are not read at all, so the default cannot be weakened by a header.

One change from the version you reviewed

The #30 implementation preferred CF-Connecting-IP when a trusted peer sent one. That was spoofable and is gone.

Trusting a single-value vendor header means trusting it from every network in the list, and the list names networks, not CDNs. An ordinary reverse proxy inside it forwards whatever headers the client sent, and the header carries nothing that says which hop wrote it, so there is no way to tell the value Cloudflare set from the value a visitor typed. A client behind such a proxy could name its own rate-limit bucket with one header, which is exactly what the right-to-left walk exists to prevent.

It costs nothing to remove. A CDN sets X-Forwarded-For too, and its own ranges belong in TRUSTED_PROXY_CIDRS anyway, so the walk steps over its edge address and lands on the same visitor the vendor header would have named. The difference is that the answer now comes from a chain the code can reason about rather than from a header believed for a reason it cannot check.

TestTrustClientIP_ignoresVendorHeadersFromATrustedPeer pins it for CF-Connecting-IP, X-Real-IP and True-Client-IP. Against the old implementation it fails exactly as you would want:

--- FAIL: TestTrustClientIP_ignoresVendorHeadersFromATrustedPeer/CF-Connecting-IP
    client IP = "1.2.3.4"; want 198.51.100.7 from the chain, never CF-Connecting-IP

1.2.3.4 there is the value the client sent.

The two details that make the walk safe

Right to left, not the leftmost entry. The left of X-Forwarded-For is whatever the original client sent; every well-behaved proxy prepends to it and preserves it, so the leftmost value is chosen by the party being rate limited. The rightmost non-trusted hop is the last address one of your own proxies actually observed.

A hop that does not parse ends the walk and falls back to the peer rather than being skipped, so a client cannot inject one malformed entry to push the walk past the real hop onto a value it chose. This also means a proxy appending ip:port rather than a bare address lands on the peer, which is the safe direction to be wrong in.

Also updated

audit/claims.yaml's rate-limit-keys-on-tcp-source-address claim said the proxy headers are never read. That is now conditional, so the claim is rewritten rather than left to rot: it states the condition, records that an empty list means the original claim still holds verbatim, and names the test that holds each half.

gofmt, go vet and go test ./... are clean on main + this branch.

🤖 Generated with Claude Code

[pullfrog]

Important

Two fixes before merge: multi-value X-Forwarded-For only reads the first field line (spoofable rate-limit key behind a trusted peer), and the Unreleased CHANGELOG entry swallowed the Duplicate event-type body.

Reviewed changes

Initial review of TRUSTED_PROXY_CIDRS: opt-in trusted-proxy CIDRs so per-IP rate limits key on the real client behind a proxy/CDN instead of the shared TCP peer.

• Config + outermost middleware — TRUSTED_PROXY_CIDRS loads as CSV CIDRs/bare IPs; TrustClientIP resolves once into request context; remoteIP prefers that over peerIP.
• Safe XFF walk — trusted peer only; right-to-left past trusted hops; malformed hop falls back to peer; vendor single-value headers deliberately ignored.
• Tests + claims/docs — trustedproxy_test.go covers spoof, walk, malformed, vendor, IPv6, and parse partial-failure; audit claim and ARCHITECTURE/DEPLOY updated for the conditional trust model.

ℹ️ Nitpicks

• RateLimit's godoc still says the IP is the TCP remote address only (middleware.go ~167–169); remoteIP already documents the context path — worth aligning so the limiter comment does not contradict the new behavior.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Grok ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes (since prior pullfrog review on c554c62)

Follow-up ca9d074 addresses both must-fix findings from the initial review.

• Join all X-Forwarded-For field lines — resolveClientIP now uses Header.Values + strings.Join instead of Get, with TestTrustClientIP_joinsRepeatedForwardedForLines using two Header.Add lines so the client-seeded first line cannot hide proxy hops.
• CHANGELOG structure restored — Duplicate event-type body is back under the #17 bullet; TRUSTED_PROXY_CIDRS is a self-contained entry that also documents the multi-line join.
• Docs aligned — DEPLOY/ARCHITECTURE note the multi-line XFF read alongside the existing RTL / no-vendor-header rules.

  ^{｜ View workflow run ｜ Using Grok ｜ 𝕏}

[distronode-com]

Both review findings are fixed in ca9d074, and the second one is worth flagging on its own: Header.Get returns only the first X-Forwarded-For field line, so a client sending its own line in front of a proxy that adds a second could still have chosen its own rate-limit key. It reads as Values joined per RFC 9110 now. Every earlier test passed because the shared helper uses Header.Set, which collapses to one line and cannot express the case, so the new test calls Add twice.

Since #30 is locked, the rest of the split plan is here rather than there. Roughly in the order of appetite you described:

• fr-CA, frame ancestors, sign-out-everywhere — the ones you said you would likely take close to as-is
• metrics endpoint — worth saying in advance that it will not arrive as-is: calnode_jobs_failed_total is declared a counter and is really a gauge (COUNT(*) of the failed backlog, which falls when those rows are cleared), so rate() over it would read the decrease as a counter reset. It will come as calnode_jobs_failed, typed gauge
• STT base URL, booking.reminder webhook
• SSO sign-in last and on its own thread, as you asked. It will also arrive changed: the owner bootstrap in the version you saw was check-then-act, so two concurrent hand-offs claiming owner on an unowned instance could both become one. That wants a partial unique index and a claim-by-writing, which is a schema change and deserves its own discussion rather than being smuggled in beside seven other features

Happy to reorder if that is not the sequence you want, and happy to hold any of them if you would rather see fewer open at once.