fix(ratelimit): key rate limiters on real client IP behind trusted proxy (bookshelf-t582g.9) #1247

Merged

zombor merged 2 commits from bd-bookshelf-t582g.9 into main 2026-07-27 02:58:02 +00:00

Conversation 2 Commits 2 Files changed 9 +145 -33

zombor commented 2026-07-26 19:05:16 +00:00

Owner

Copy link

Summary

• All rate limiters (login, refresh, OIDC backchannel, device-code) were keying on r.RemoteAddr, which is the reverse proxy's IP when TrustProxyHeaders=true. This collapsed every real user into one bucket — one attacker could exhaust the burst and lock out all users.
• Exported audit.ExtractIPWithPolicy (already had the correct XFF/X-Real-IP extraction logic) and wired it into clientIP(r, trustProxyHeaders) in the users package.
• Added TrustProxyHeaders bool to users.Deps (mirroring OIDCDeps and DeviceDeps) and threaded it through all four call sites.
• When TrustProxyHeaders=false (default), behavior is identical to before — only RemoteAddr is used, preventing header-forgery bypasses.

Test plan

• New clientIP specs: trustProxy=false ignores XFF/X-Real-IP (preserves existing behavior)
• New clientIP specs: trustProxy=true uses first XFF hop, prefers X-Real-IP over XFF, falls back to RemoteAddr when no proxy headers present
• Smoke spec: two XFF clients with different IPs get distinct rate-limit keys
• audit.ExtractIPWithPolicy covered by two new specs in the audit package
• Coverage gate: check-coverage: OK — zero uncovered statement blocks
• make test green; make lint clean on affected packages

Closes bead bookshelf-t582g.9 on merge.

## Summary - All rate limiters (login, refresh, OIDC backchannel, device-code) were keying on `r.RemoteAddr`, which is the reverse proxy's IP when `TrustProxyHeaders=true`. This collapsed every real user into one bucket — one attacker could exhaust the burst and lock out all users. - Exported `audit.ExtractIPWithPolicy` (already had the correct XFF/X-Real-IP extraction logic) and wired it into `clientIP(r, trustProxyHeaders)` in the users package. - Added `TrustProxyHeaders bool` to `users.Deps` (mirroring `OIDCDeps` and `DeviceDeps`) and threaded it through all four call sites. - When `TrustProxyHeaders=false` (default), behavior is identical to before — only `RemoteAddr` is used, preventing header-forgery bypasses. ## Test plan - [x] New `clientIP` specs: `trustProxy=false` ignores XFF/X-Real-IP (preserves existing behavior) - [x] New `clientIP` specs: `trustProxy=true` uses first XFF hop, prefers X-Real-IP over XFF, falls back to RemoteAddr when no proxy headers present - [x] Smoke spec: two XFF clients with different IPs get distinct rate-limit keys - [x] `audit.ExtractIPWithPolicy` covered by two new specs in the audit package - [x] Coverage gate: `check-coverage: OK — zero uncovered statement blocks` - [x] `make test` green; `make lint` clean on affected packages Closes bead bookshelf-t582g.9 on merge.

zombor added 1 commit 2026-07-26 19:05:16 +00:00

fix(ratelimit): key rate limiters on real client IP when TrustProxyHeaders=true ... db7fd27b32

Behind a reverse proxy, all requests share one RemoteAddr (the proxy's IP),
collapsing every user into a single login/refresh/device-code bucket. One
attacker could exhaust the burst and lock out all users.

Fix: export audit.ExtractIPWithPolicy (already implements XFF/X-Real-IP
trust-proxy extraction) and wire it into clientIP(r, trustProxyHeaders).
Thread TrustProxyHeaders into users.Deps (mirroring OIDCDeps / DeviceDeps)
and pass it at every clientIP call site in handler, oidc_handler, and
device_handler. When the flag is false (default) behavior is unchanged.

TDD: new clientIP specs covering trust=false ignores XFF/X-Real-IP,
trust=true uses first XFF hop, prefers X-Real-IP, falls back to RemoteAddr,
and two different XFF IPs get distinct keys. Coverage gate: green.

Closes bead bookshelf-t582g.9 on merge.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-26 19:34:54 +00:00

Author

Owner

Copy link

Security Review — PR #1247 (rate limiters key on trusted-proxy client IP)

Adversarial focus: trust model of the rate-limit key. Reviewed git diff origin/main...origin/bd-bookshelf-t582g.9 plus extractIPWithPolicy in internal/audit/recorder.go and the config/wiring flow.

Trust-model verdict per the four concerns:

1. Untrusted mode (default TrustProxyHeaders=false) — SAFE. clientIP → audit.ExtractIPWithPolicy skips the whole header block and keys on r.RemoteAddr only. Forged X-Forwarded-For / X-Real-IP are ignored (covered by new tests). Flag defaults false (config.go:548).
2. Trusted mode hop selection — see [MAJOR] below.
3. Consistency with audit middleware — CONFIRMED. The rate limiter now literally calls the same audit.ExtractIPWithPolicy, driven by the same single cfg.TrustProxyHeaders flag that feeds the audit Middleware (app.go:938) and the users Deps (app.go:398). One extraction function, one config source. This is exactly the consistency the PR aimed for.
4. Attacker-chosen key — no path in untrusted mode; the only exposure is the trusted-mode leftmost-XFF case below, which requires the operator to have opted in.

---

[MAJOR] internal/audit/recorder.go:148 — trusted mode keys on the leftmost (client-supplied) X-Forwarded-For hop
In trusted mode, when no X-Real-IP is present, extraction takes xff[:idx] — the leftmost XFF value. A reverse proxy that appends the real client IP (RFC-style: X-Forwarded-For: <client-sent>, <real-ip>) leaves the leftmost value fully attacker-controlled. An attacker can then (a) send a fresh forged leftmost value per request to mint unlimited rate-limit buckets and bypass throttling on /login, /auth/refresh, device-grant, and OIDC backchannel; or (b) pin a victim's IP as the leftmost value to burn that victim's bucket and DoS-lock them out. The safe hop when a proxy appends is the rightmost entry (or a proxy-hop count), not the leftmost. This is safe only if the operator configures the proxy to set X-Real-IP or to replace XFF with the true client IP — a requirement that is neither enforced nor documented.
Suggested fix: for XFF, take the rightmost hop (or add a trusted-proxy-hop-count config and index from the right); and/or document in the trust-proxy-headers flag help + ops docs that the proxy MUST set X-Real-IP or replace XFF (proxy_set_header X-Forwarded-For $remote_addr). Note this leftmost behavior is pre-existing in the audit middleware (bookshelf-kxyu) and the PR intentionally reuses it for consistency; the fix should land in extractIPWithPolicy so both audit and rate-limiting are corrected together. A follow-up bead on the shared helper is acceptable if the proxy-config requirement is documented in this PR.

[MINOR] internal/users/ratelimiter.go:171 — stale doc comment above clientIP still says headers are never trusted
The block comment at 171-178 is the new accurate one, but line 173's parenthetical ("never trust forwarded headers from untrusted clients") reads as a contradiction now that trusted mode consults them. Tighten to "never trust forwarded headers when TrustProxyHeaders is false" to match the actual policy.

---

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Security Review — PR #1247 (rate limiters key on trusted-proxy client IP) Adversarial focus: trust model of the rate-limit key. Reviewed `git diff origin/main...origin/bd-bookshelf-t582g.9` plus `extractIPWithPolicy` in `internal/audit/recorder.go` and the config/wiring flow. **Trust-model verdict per the four concerns:** 1. Untrusted mode (default `TrustProxyHeaders=false`) — **SAFE.** `clientIP` → `audit.ExtractIPWithPolicy` skips the whole header block and keys on `r.RemoteAddr` only. Forged `X-Forwarded-For` / `X-Real-IP` are ignored (covered by new tests). Flag defaults false (`config.go:548`). 2. Trusted mode hop selection — see [MAJOR] below. 3. Consistency with audit middleware — **CONFIRMED.** The rate limiter now literally calls the same `audit.ExtractIPWithPolicy`, driven by the same single `cfg.TrustProxyHeaders` flag that feeds the audit `Middleware` (`app.go:938`) and the users `Deps` (`app.go:398`). One extraction function, one config source. This is exactly the consistency the PR aimed for. 4. Attacker-chosen key — no path in untrusted mode; the only exposure is the trusted-mode leftmost-XFF case below, which requires the operator to have opted in. --- [MAJOR] internal/audit/recorder.go:148 — trusted mode keys on the leftmost (client-supplied) X-Forwarded-For hop In trusted mode, when no `X-Real-IP` is present, extraction takes `xff[:idx]` — the **leftmost** XFF value. A reverse proxy that *appends* the real client IP (RFC-style: `X-Forwarded-For: <client-sent>, <real-ip>`) leaves the leftmost value fully attacker-controlled. An attacker can then (a) send a fresh forged leftmost value per request to mint unlimited rate-limit buckets and bypass throttling on `/login`, `/auth/refresh`, device-grant, and OIDC backchannel; or (b) pin a *victim's* IP as the leftmost value to burn that victim's bucket and DoS-lock them out. The safe hop when a proxy appends is the **rightmost** entry (or a proxy-hop count), not the leftmost. This is safe only if the operator configures the proxy to set `X-Real-IP` or to *replace* XFF with the true client IP — a requirement that is neither enforced nor documented. Suggested fix: for XFF, take the rightmost hop (or add a trusted-proxy-hop-count config and index from the right); and/or document in the `trust-proxy-headers` flag help + ops docs that the proxy MUST set `X-Real-IP` or replace XFF (`proxy_set_header X-Forwarded-For $remote_addr`). Note this leftmost behavior is pre-existing in the audit middleware (bookshelf-kxyu) and the PR intentionally reuses it for consistency; the fix should land in `extractIPWithPolicy` so both audit and rate-limiting are corrected together. A follow-up bead on the shared helper is acceptable if the proxy-config requirement is documented in this PR. [MINOR] internal/users/ratelimiter.go:171 — stale doc comment above clientIP still says headers are never trusted The block comment at 171-178 is the new accurate one, but line 173's parenthetical ("never trust forwarded headers from untrusted clients") reads as a contradiction now that trusted mode consults them. Tighten to "never trust forwarded headers when TrustProxyHeaders is false" to match the actual policy. --- REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor force-pushed bd-bookshelf-t582g.9 from db7fd27b32 to 0c0de796a7 2026-07-26 19:39:15 +00:00 Compare

zombor commented 2026-07-26 21:59:34 +00:00

Author

Owner

Copy link

Security re-review — PR #1247 (bd-bookshelf-t582g.9)

Re-review of the XFF-hop MAJOR fix (leftmost -> rightmost X-Forwarded-For hop). The MAJOR is CLOSED. Verification against the four criteria:

(1) Trusted mode now keys on the rightmost (proxy-appended) hop — CONFIRMED.
internal/audit/recorder.go:167 switched strings.Index(xff, ",") (leftmost) to strings.LastIndex(xff, ",") returning xff[idx+1:] (rightmost). X-Real-IP is still preferred when present. A client-supplied leftmost value no longer influences the key, and a spoof-resistance test proves it: internal/users/ratelimiter_test.go — "a spoofed leftmost XFF value does not change the rate-limit key" asserts "attacker-spoof, 10.0.0.2" and "different-spoof, 10.0.0.2" produce the same key, plus "gives different real client IPs separate rate-limit buckets" proves distinct rightmost hops still separate. Audit-side mirror test updated to 10.0.0.1 (rightmost) in recorder_test.go.

(2) Untrusted mode still ignores XFF entirely — CONFIRMED.
extractIPWithPolicy only consults headers inside if trustProxyHeaders; otherwise it falls straight to net.SplitHostPort(r.RemoteAddr). Covered by "ignores X-Forwarded-For when trust is disabled" and "ignores X-Real-IP when trust is disabled" (ratelimiter_test.go).

(3) Shared-helper change did not break audit trust semantics — CONFIRMED.
The audit Middleware call site (recorder.go:96) is unchanged and still passes its own trustProxyHeaders param, sourced from the same cfg.TrustProxyHeaders (app.go:938) that now feeds the rate limiter (wire.go:99/158/249). Audit and rate-limit trust are unified. Audit tests updated consistently (Describe renamed to "rightmost", assertion flipped to the appended hop). No leftover single-arg clientIP(r) / ExportClientIP(r) callers remain.

(4) Single-trusted-hop assumption documented + multi-proxy limitation noted — CONFIRMED.
extractIPWithPolicy doc block (recorder.go:147-160) states the single-trusted-proxy rationale and calls out the multi-hop (CDN + reverse proxy) limitation as separately tracked. Config flag help (config.go:548) scopes it to "ONLY behind a trusted reverse proxy". Follow-up-acceptable per the task.

IPv6 note: LastIndex(",") is safe — XFF entries are comma-separated addresses with no intra-address comma, so the rightmost hop is extracted correctly.

Findings

[MINOR] internal/audit/recorder.go:64 — stale doc comment on Middleware
The Middleware doc's "IP extraction policy" still reads "trustProxyHeaders=true: X-Real-IP preferred, then first X-Forwarded-For hop." The behavior is now the rightmost (proxy-appended) hop. Update "first" -> "rightmost (proxy-appended)" to match the implementation and the extractIPWithPolicy doc. Doc-only, no correctness impact.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security re-review — PR #1247 (`bd-bookshelf-t582g.9`) Re-review of the XFF-hop MAJOR fix (leftmost -> rightmost X-Forwarded-For hop). **The MAJOR is CLOSED.** Verification against the four criteria: **(1) Trusted mode now keys on the rightmost (proxy-appended) hop — CONFIRMED.** `internal/audit/recorder.go:167` switched `strings.Index(xff, ",")` (leftmost) to `strings.LastIndex(xff, ",")` returning `xff[idx+1:]` (rightmost). X-Real-IP is still preferred when present. A client-supplied leftmost value no longer influences the key, and a spoof-resistance test proves it: `internal/users/ratelimiter_test.go` — "a spoofed leftmost XFF value does not change the rate-limit key" asserts `"attacker-spoof, 10.0.0.2"` and `"different-spoof, 10.0.0.2"` produce the *same* key, plus "gives different real client IPs separate rate-limit buckets" proves distinct rightmost hops still separate. Audit-side mirror test updated to `10.0.0.1` (rightmost) in `recorder_test.go`. **(2) Untrusted mode still ignores XFF entirely — CONFIRMED.** `extractIPWithPolicy` only consults headers inside `if trustProxyHeaders`; otherwise it falls straight to `net.SplitHostPort(r.RemoteAddr)`. Covered by "ignores X-Forwarded-For when trust is disabled" and "ignores X-Real-IP when trust is disabled" (`ratelimiter_test.go`). **(3) Shared-helper change did not break audit trust semantics — CONFIRMED.** The audit `Middleware` call site (`recorder.go:96`) is unchanged and still passes its own `trustProxyHeaders` param, sourced from the same `cfg.TrustProxyHeaders` (`app.go:938`) that now feeds the rate limiter (`wire.go:99/158/249`). Audit and rate-limit trust are unified. Audit tests updated consistently (Describe renamed to "rightmost", assertion flipped to the appended hop). No leftover single-arg `clientIP(r)` / `ExportClientIP(r)` callers remain. **(4) Single-trusted-hop assumption documented + multi-proxy limitation noted — CONFIRMED.** `extractIPWithPolicy` doc block (`recorder.go:147-160`) states the single-trusted-proxy rationale and calls out the multi-hop (CDN + reverse proxy) limitation as separately tracked. Config flag help (`config.go:548`) scopes it to "ONLY behind a trusted reverse proxy". Follow-up-acceptable per the task. IPv6 note: `LastIndex(",")` is safe — XFF entries are comma-separated addresses with no intra-address comma, so the rightmost hop is extracted correctly. ### Findings [MINOR] internal/audit/recorder.go:64 — stale doc comment on `Middleware` The `Middleware` doc's "IP extraction policy" still reads "trustProxyHeaders=true: X-Real-IP preferred, then **first** X-Forwarded-For hop." The behavior is now the **rightmost** (proxy-appended) hop. Update "first" -> "rightmost (proxy-appended)" to match the implementation and the `extractIPWithPolicy` doc. Doc-only, no correctness impact. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor referenced this pull request 2026-07-27 02:19:27 +00:00

chore(sqlc): delete 21 dead sqlc queries + companion dead code (bookshelf-9snmz.1.1) #1254

zombor referenced this pull request 2026-07-27 02:19:35 +00:00

chore(sqlc): delete 21 dead sqlc queries + companion dead code (bookshelf-9snmz.1.1) #1254

zombor force-pushed bd-bookshelf-t582g.9 from 0c0de796a7 to 544ee2d523 2026-07-27 02:48:41 +00:00 Compare

zombor merged commit 2e786a1ea1 into main 2026-07-27 02:58:02 +00:00

zombor referenced this pull request from a commit 2026-07-27 02:58:06 +00:00

Merge pull request 'fix(ratelimit): key rate limiters on real client IP behind trusted proxy (bookshelf-t582g.9)' (#1247) from bd-bookshelf-t582g.9 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1247

No description provided.