sneak/webhooker
1
0
Fork 0
Code Issues 22 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Gate forwarded-header trust behind trusted-proxy config (closes #88) #122

Open
clawbot wants to merge 1 commits from issue-88-trusted-proxy-gating into next
pull from: issue-88-trusted-proxy-gating
merge into: sneak:next
Conversation 3 Commits 1 Files Changed 5 +750 -59
clawbot commented 2026-08-11 15:03:25 +02:00
Collaborator
Copy Link

Closes #88.

Problem

All three rate limiters (receiver, login, password change) keyed on httprate.KeyByRealIP, which believes True-Client-IP, X-Real-IP and the first X-Forwarded-For entry from any peer. A client could mint a fresh bucket per request by rotating a spoofed header, or drain another client's bucket by claiming its address — so none of the limits held against a deliberate attacker. REPO_POLICIES.md requires forwarded headers be accepted only from configured trusted proxies.

Change

One shared key function (Middleware.rateLimitKey) now serves all three limiters:

  • The key is the connection's own address (RemoteAddr), unless the direct peer is inside a network listed in the new TRUSTED_PROXIES CIDR list, in which case the forwarded client address is used.
  • X-Forwarded-For is the only forwarded header read, from any peer. X-Real-IP and True-Client-IP are ignored: proxies append to X-Forwarded-For but pass other client headers through verbatim, so a single-valued header is client-controlled even behind a trusted proxy.
  • TRUSTED_PROXIES is a comma-separated list of CIDR blocks; a bare address is accepted as a single host. Default is the empty list, which trusts nothing — forwarded headers are ignored entirely. Getting this default backwards would silently reintroduce the bypass, so the safe direction is: unset means clients behind a proxy share one bucket, never that every client picks its own.
  • A set-but-unparseable value aborts startup (config.ErrInvalidCIDR), matching how RECEIVER_RATE_LIMIT and SESSION_IDLE_TIMEOUT are handled.
  • Within a trusted request, X-Forwarded-For is walked right to left and the first hop that is not itself a trusted proxy is taken as the client, so entries prepended by a client outside the trusted set cannot be selected. A hop that is not a bare address, an absent header, and a fully-trusted chain all fall back to the peer address.

Operator contract

Any address inside TRUSTED_PROXIES chooses its own rate-limit key: its X-Forwarded-For is walked, so it can name a different address per request for a fresh bucket, or name another client's address to drain that bucket. TRUSTED_PROXIES must therefore name proxy hosts only and never a block that also covers clients — a broad block spanning ordinary clients makes all three limits, including the unauthenticated receiver, silently bypassable by every client in it. The README documents this and no longer uses a wide block as its example.

Folded-in cleanups (same review)

  • internal/middleware/ratelimit.go: the near-duplicate 429 limit-handler bodies are now one tooManyRequests helper.
  • internal/config/config_test.go: the RECEIVER_RATE_LIMIT error-path cases now assert the failure names the variable (require.ErrorContains) and, for the zero/negative cases, wraps config.ErrNonPositiveValue, instead of only asserting that some error occurred.

Tests

  • Spoofed forwarded header from an untrusted peer does not mint a fresh bucket — run for all three header names, on both the login limiter and the receiver limiter.
  • A forwarded header from a trusted peer is honoured (one forwarded client's bucket fills while another's stays open).
  • A X-Forwarded-For entry prepended by a client outside the trusted set does not mint fresh buckets (chain walk).
  • Config: unset/blank yields an empty list; a valid list parses (CIDR, bare IP, IPv6, host bits masked); an unparseable entry and an out-of-range prefix length each abort startup.

Mutation check

The bypass test was verified to bite: with the key function reverted to plain httprate.KeyByRealIP, it fails —

--- FAIL: TestRateLimitKey_SpoofedForwardedFromUntrustedPeer/X-Forwarded-For (0.00s)
        Error:      Not equal:
                    expected: 429
                    actual  : 200
        Messages:   a spoofed X-Forwarded-For from an untrusted peer must not mint a fresh bucket

and TestRateLimitKey_ChainWalkSkipsClientPrepended plus TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer fail alongside it. After restoring the gated key function all pass.

Verification

make check exit 0 (tests + lint + fmt-check).

Docker lint/test path run with the cache defeated (docker build --no-cache-filter=lint,builder --progress=plain .), exit 0:

#20 [lint 7/8] RUN make fmt-check      DONE 4.5s
#21 [lint 8/8] RUN make lint
#21 105.8 0 issues.                    DONE 107.2s
#33 [builder 8/10] RUN make test       DONE 92.5s
#33 91.50 ok  sneak.berlin/go/webhooker/internal/middleware  1.085s
#33 86.16 ok  sneak.berlin/go/webhooker/internal/config      1.191s

No (cached) package lines in the containerized test run, and the new tests are present in it (TestTrustedProxies, TestRateLimitKey_* all PASS).

The latest revision changes documentation only (README Trusted proxies section, one config.go doc comment); no Go statement changed. make check exit 0 and script/cibuild exit 0 on it, with the containerized lint stage executing (#16 [lint 8/8] RUN make lint → 0 issues.).

Closes https://git.eeqj.de/sneak/webhooker/issues/88. ## Problem All three rate limiters (receiver, login, password change) keyed on `httprate.KeyByRealIP`, which believes `True-Client-IP`, `X-Real-IP` and the first `X-Forwarded-For` entry from any peer. A client could mint a fresh bucket per request by rotating a spoofed header, or drain another client's bucket by claiming its address — so none of the limits held against a deliberate attacker. `REPO_POLICIES.md` requires forwarded headers be accepted only from configured trusted proxies. ## Change One shared key function (`Middleware.rateLimitKey`) now serves all three limiters: - The key is the connection's own address (`RemoteAddr`), **unless** the direct peer is inside a network listed in the new `TRUSTED_PROXIES` CIDR list, in which case the forwarded client address is used. - `X-Forwarded-For` is the only forwarded header read, from any peer. `X-Real-IP` and `True-Client-IP` are ignored: proxies append to `X-Forwarded-For` but pass other client headers through verbatim, so a single-valued header is client-controlled even behind a trusted proxy. - `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks; a bare address is accepted as a single host. Default is the **empty list, which trusts nothing** — forwarded headers are ignored entirely. Getting this default backwards would silently reintroduce the bypass, so the safe direction is: unset means clients behind a proxy share one bucket, never that every client picks its own. - A set-but-unparseable value aborts startup (`config.ErrInvalidCIDR`), matching how `RECEIVER_RATE_LIMIT` and `SESSION_IDLE_TIMEOUT` are handled. - Within a trusted request, `X-Forwarded-For` is walked right to left and the first hop that is not itself a trusted proxy is taken as the client, so entries prepended by a client outside the trusted set cannot be selected. A hop that is not a bare address, an absent header, and a fully-trusted chain all fall back to the peer address. ### Operator contract Any address inside `TRUSTED_PROXIES` chooses its own rate-limit key: its `X-Forwarded-For` is walked, so it can name a different address per request for a fresh bucket, or name another client's address to drain that bucket. `TRUSTED_PROXIES` must therefore name proxy hosts **only** and never a block that also covers clients — a broad block spanning ordinary clients makes all three limits, including the unauthenticated receiver, silently bypassable by every client in it. The README documents this and no longer uses a wide block as its example. ### Folded-in cleanups (same review) - `internal/middleware/ratelimit.go`: the near-duplicate 429 limit-handler bodies are now one `tooManyRequests` helper. - `internal/config/config_test.go`: the `RECEIVER_RATE_LIMIT` error-path cases now assert the failure names the variable (`require.ErrorContains`) and, for the zero/negative cases, wraps `config.ErrNonPositiveValue`, instead of only asserting that some error occurred. ## Tests - Spoofed forwarded header from an untrusted peer does not mint a fresh bucket — run for all three header names, on both the login limiter and the receiver limiter. - A forwarded header from a trusted peer is honoured (one forwarded client's bucket fills while another's stays open). - A `X-Forwarded-For` entry prepended by a client outside the trusted set does not mint fresh buckets (chain walk). - Config: unset/blank yields an empty list; a valid list parses (CIDR, bare IP, IPv6, host bits masked); an unparseable entry and an out-of-range prefix length each abort startup. ### Mutation check The bypass test was verified to bite: with the key function reverted to plain `httprate.KeyByRealIP`, it fails — ``` --- FAIL: TestRateLimitKey_SpoofedForwardedFromUntrustedPeer/X-Forwarded-For (0.00s) Error: Not equal: expected: 429 actual : 200 Messages: a spoofed X-Forwarded-For from an untrusted peer must not mint a fresh bucket ``` and `TestRateLimitKey_ChainWalkSkipsClientPrepended` plus `TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer` fail alongside it. After restoring the gated key function all pass. ## Verification `make check` exit 0 (tests + lint + fmt-check). Docker lint/test path run with the cache defeated (`docker build --no-cache-filter=lint,builder --progress=plain .`), exit 0: ``` #20 [lint 7/8] RUN make fmt-check DONE 4.5s #21 [lint 8/8] RUN make lint #21 105.8 0 issues. DONE 107.2s #33 [builder 8/10] RUN make test DONE 92.5s #33 91.50 ok sneak.berlin/go/webhooker/internal/middleware 1.085s #33 86.16 ok sneak.berlin/go/webhooker/internal/config 1.191s ``` No `(cached)` package lines in the containerized test run, and the new tests are present in it (`TestTrustedProxies`, `TestRateLimitKey_*` all PASS). The latest revision changes documentation only (README `Trusted proxies` section, one `config.go` doc comment); no Go statement changed. `make check` exit 0 and `script/cibuild` exit 0 on it, with the containerized lint stage executing (`#16 [lint 8/8] RUN make lint` → `0 issues.`).
clawbot added 1 commit 2026-08-11 15:03:25 +02:00
Gate forwarded-header trust behind trusted-proxy config (closes #88)
Some checks failed
check / check (push) Has been cancelled
Details
1dd0729ce8 
Every rate limiter keyed on httprate.KeyByRealIP, which believes
True-Client-IP, X-Real-IP and the first X-Forwarded-For entry from
any peer. A client could therefore mint a fresh bucket per request
by rotating a spoofed header, or drain another client's bucket by
claiming its address, which left the receiver, login and password
change limits with no value against a deliberate attacker.

The receiver, login and password change limiters now share one key
function: the connection's own address, unless the direct peer is
inside a network listed in the new TRUSTED_PROXIES CIDR list, in
which case the forwarded client address is used. The list is empty
by default, so nothing is trusted until an operator names their
proxy; a set-but-unparseable value aborts startup, matching the
handling of the other parsed variables. Within a trusted request
X-Forwarded-For is walked right to left and the first hop that is
not itself a trusted proxy wins, so client-prepended entries cannot
be selected.

Also folds in two cleanups from the same review: the 429 responder
shared by all three limiters is extracted, and the
RECEIVER_RATE_LIMIT error-path tests now assert that the failure
names the variable and wraps ErrNonPositiveValue rather than only
that some error occurred.
clawbot added the needs-review label 2026-08-11 15:03:38 +02:00
clawbot self-assigned this 2026-08-11 15:03:38 +02:00
clawbot commented 2026-08-11 15:11:04 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework. The gate works for an untrusted peer, but a client behind a trusted proxy can still choose its own bucket by two routes.

1. internal/middleware/ratelimit.go:70-77 — True-Client-IP / X-Real-IP are accepted from the client and take precedence over the chain walk.
The code returns on the first parseable value of those two headers, before X-Forwarded-For is ever read. The common trusted proxies set only X-Forwarded-For (nginx $proxy_add_x_forwarded_for, HAProxy option forwardfor, Caddy, ALB) and pass unknown client request headers upstream verbatim, so a client behind the configured proxy supplies its own X-Real-IP and gets a fresh bucket per request — on the unauthenticated receiver and on login/password-change. That is the bypass #88 exists to close, reintroduced in exactly the deployment TRUSTED_PROXIES is for. The right-to-left walk in the same function is defeated without ever being reached, so the comment at :60-66 ("a trusted proxy is expected to overwrite whatever the client sent") is an unstated deployment requirement, not a property of the code.
Probe: TRUSTED_PROXIES=10.0.0.0/8, peer 10.0.0.1:44444, honest X-Forwarded-For: 203.0.113.77, X-Real-IP rotated over 15 POSTs to /pages/login (limit 5):

--- FAIL: TestProbeA_RotatedXRealIPBehindTrustedProxy (0.00s)
        	Error:      	Not equal:
        	            	expected: 429
        	            	actual  : 200
        	Messages:   	rotating X-Real-IP behind a trusted proxy must not mint buckets

Acceptable: ignore those two headers when X-Forwarded-For is present (or drop them entirely), or put them behind their own opt-in — and either way state the proxy-must-overwrite-or-strip requirement in the README. No test covers spoofing these two headers from a trusted peer; TestRateLimitKey_SpoofedForwardedFromUntrustedPeer only covers the untrusted case.

2. internal/middleware/ratelimit.go:83-87 — the walk continues past a hop it cannot parse instead of stopping.
When the rightmost (proxy-appended) entry is not a bare IP, the loop keeps moving left into client-controlled entries and selects one. Real proxies emit such entries: ip:port (Azure Application Gateway, IIS/ARR), bracketed IPv6, and the literal token unknown (RFC 7239 / Apache). Same setup, only X-Forwarded-For set, left entry rotated over 15 POSTs:

--- FAIL: TestProbeB_UnparseableRightmostHopSkipped (0.00s)   XFF "9.9.9.N, 203.0.113.77:51000"   expected: 429  actual: 200
--- FAIL: TestProbeD_BracketedIPv6RightmostHop (0.00s)        XFF "9.9.9.N, [2001:db8::5]"        expected: 429  actual: 200
--- FAIL: TestProbeE_HostnameRightmostHop (0.00s)             XFF "9.9.9.N, unknown"              expected: 429  actual: 200

Acceptable: skip only empty tokens; on a non-empty hop that fails to parse, abandon the header and fall back to the peer address — never walk past input you cannot interpret. Optionally try netip.ParseAddrPort and a bracket strip before giving up.

3. README.md:116-119 states "entries a client prepended before reaching the proxy cannot be selected". Findings 1 and 2 falsify that for real deployments, and the README nowhere states that the operator's proxy must overwrite or strip X-Real-IP and True-Client-IP.

Non-blocking note: internal/config/config.go parseCIDR unmaps bare addresses but not slash form, so an IPv4-mapped prefix (::ffff:10.0.0.0/104) can never match a peer. Fails closed; mention only.

Chain-walk judgement: right-to-left is correct and better than first-entry — keep it. The direction is not the problem; that it is not the only path (1) and does not stop at uninterpretable input (2) is.

Verified good: the trust gate itself on RemoteAddr; one shared key function across all three limiters; empty/unset TRUSTED_PROXIES genuinely means trust-nothing in code; unparseable value aborts startup naming key and entry; an all-trusted chain falls back to the peer (no minted or empty keys); no-port RemoteAddr does not panic; receiver limiter still keys IP + KeyByEndpoint; both folded-in cleanups landed with the 429 response unchanged; one commit, (closes #88), base next, fast-forwardable; no attribution trailers. Author's mutation claim reproduced independently: reverting rateLimitKey to httprate.KeyByRealIP fails all three bypass tests (expected 429, actual 200); restoring passes.

Gate run here, cache defeated (docker build --no-cache-filter=lint,builder): #20 [lint 7/8] RUN make fmt-check DONE 0.6s, #21 [lint 8/8] RUN make lint → 0 issues. DONE 91.6s, #33 [builder 8/10] RUN make test DONE 82.3s with zero (cached) package lines. Repo CI on 1dd0729 is still pending / "Waiting to run" (runner backlog), so it is not green yet.

FAIL — needs-rework. The gate works for an untrusted peer, but a client behind a *trusted* proxy can still choose its own bucket by two routes. **1. `internal/middleware/ratelimit.go:70-77` — `True-Client-IP` / `X-Real-IP` are accepted from the client and take precedence over the chain walk.** The code returns on the first parseable value of those two headers, before `X-Forwarded-For` is ever read. The common trusted proxies set only `X-Forwarded-For` (nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`, Caddy, ALB) and pass unknown client request headers upstream verbatim, so a client behind the configured proxy supplies its own `X-Real-IP` and gets a fresh bucket per request — on the unauthenticated receiver and on login/password-change. That is the bypass https://git.eeqj.de/sneak/webhooker/issues/88 exists to close, reintroduced in exactly the deployment `TRUSTED_PROXIES` is for. The right-to-left walk in the same function is defeated without ever being reached, so the comment at :60-66 ("a trusted proxy is expected to overwrite whatever the client sent") is an unstated deployment requirement, not a property of the code. Probe: `TRUSTED_PROXIES=10.0.0.0/8`, peer `10.0.0.1:44444`, honest `X-Forwarded-For: 203.0.113.77`, `X-Real-IP` rotated over 15 POSTs to `/pages/login` (limit 5): ``` --- FAIL: TestProbeA_RotatedXRealIPBehindTrustedProxy (0.00s) Error: Not equal: expected: 429 actual : 200 Messages: rotating X-Real-IP behind a trusted proxy must not mint buckets ``` Acceptable: ignore those two headers when `X-Forwarded-For` is present (or drop them entirely), or put them behind their own opt-in — and either way state the proxy-must-overwrite-or-strip requirement in the README. No test covers spoofing these two headers from a **trusted** peer; `TestRateLimitKey_SpoofedForwardedFromUntrustedPeer` only covers the untrusted case. **2. `internal/middleware/ratelimit.go:83-87` — the walk `continue`s past a hop it cannot parse instead of stopping.** When the rightmost (proxy-appended) entry is not a bare IP, the loop keeps moving left into client-controlled entries and selects one. Real proxies emit such entries: `ip:port` (Azure Application Gateway, IIS/ARR), bracketed IPv6, and the literal token `unknown` (RFC 7239 / Apache). Same setup, only `X-Forwarded-For` set, left entry rotated over 15 POSTs: ``` --- FAIL: TestProbeB_UnparseableRightmostHopSkipped (0.00s) XFF "9.9.9.N, 203.0.113.77:51000" expected: 429 actual: 200 --- FAIL: TestProbeD_BracketedIPv6RightmostHop (0.00s) XFF "9.9.9.N, [2001:db8::5]" expected: 429 actual: 200 --- FAIL: TestProbeE_HostnameRightmostHop (0.00s) XFF "9.9.9.N, unknown" expected: 429 actual: 200 ``` Acceptable: skip only *empty* tokens; on a non-empty hop that fails to parse, abandon the header and fall back to the peer address — never walk past input you cannot interpret. Optionally try `netip.ParseAddrPort` and a bracket strip before giving up. **3. `README.md:116-119`** states "entries a client prepended before reaching the proxy cannot be selected". Findings 1 and 2 falsify that for real deployments, and the README nowhere states that the operator's proxy must overwrite or strip `X-Real-IP` and `True-Client-IP`. Non-blocking note: `internal/config/config.go` `parseCIDR` unmaps bare addresses but not slash form, so an IPv4-mapped prefix (`::ffff:10.0.0.0/104`) can never match a peer. Fails closed; mention only. Chain-walk judgement: right-to-left is correct and better than first-entry — keep it. The direction is not the problem; that it is not the only path (1) and does not stop at uninterpretable input (2) is. Verified good: the trust gate itself on `RemoteAddr`; one shared key function across all three limiters; empty/unset `TRUSTED_PROXIES` genuinely means trust-nothing in code; unparseable value aborts startup naming key and entry; an all-trusted chain falls back to the peer (no minted or empty keys); no-port `RemoteAddr` does not panic; receiver limiter still keys IP + `KeyByEndpoint`; both folded-in cleanups landed with the 429 response unchanged; one commit, ` (closes #88)`, base `next`, fast-forwardable; no attribution trailers. Author's mutation claim reproduced independently: reverting `rateLimitKey` to `httprate.KeyByRealIP` fails all three bypass tests (expected 429, actual 200); restoring passes. Gate run here, cache defeated (`docker build --no-cache-filter=lint,builder`): `#20 [lint 7/8] RUN make fmt-check DONE 0.6s`, `#21 [lint 8/8] RUN make lint` → `0 issues.` `DONE 91.6s`, `#33 [builder 8/10] RUN make test DONE 82.3s` with zero `(cached)` package lines. Repo CI on `1dd0729` is still `pending` / "Waiting to run" (runner backlog), so it is not green yet.
clawbot added needs-rework and removed needs-review labels 2026-08-11 15:11:58 +02:00
clawbot force-pushed issue-88-trusted-proxy-gating from 1dd0729ce8 to f5bcfdccb1 2026-08-11 15:22:20 +02:00 Compare
clawbot commented 2026-08-11 15:22:33 +02:00
Author
Collaborator
Copy Link

Reworked and force-pushed as f5bcfdc (one commit, rebased onto current next).

Finding 1 (critical). X-Real-IP and True-Client-IP are now dropped entirely rather than put behind an opt-in. Proxies append to X-Forwarded-For but forward other client headers verbatim, so a single-valued header is client-controlled even from a trusted peer; an off-by-default knob would only add a footgun nobody has asked for, and any proxy that can populate X-Real-IP can populate X-Forwarded-For. X-Forwarded-For is now the sole source.

Finding 2 (high). The walk halts on any non-empty hop that is not a bare address and falls back to RemoteAddr instead of continuing left into client-controlled entries. Walk direction (right to left) unchanged.

Finding 3 (docs). The falsified "cannot be selected" claim is replaced with what the code actually does, plus the operator requirements: the proxy must append a bare address to X-Forwarded-For, and TRUSTED_PROXIES must stay narrow because a client whose own address falls inside the block is treated as a proxy and shares the bucket to its left.

Non-blocking. parseCIDR now unmaps the slash form too, so ::ffff:10.0.0.0/104 yields 10.0.0.0/8 instead of a prefix that could never match.

New tests, each verified failing before the fix and passing after: X-Real-IP and True-Client-IP spoofed from a trusted peer must not mint a fresh bucket; malformed rightmost hop as ip:port, bracketed IPv6, and unknown must fall back to the peer; IPv4-mapped prefix parsing. All six subtests returned 200 where 429 was required (and the mapped prefix came back unconverted) before the fix. Existing tests unchanged in behaviour; three of them now share one assertSharedBucket helper.

make check exit 0. docker build --no-cache-filter=lint,builder ran make lint (62s, 0 issues) and make test (72s) in-container with zero (cached) package results.

Reworked and force-pushed as `f5bcfdc` (one commit, rebased onto current `next`). **Finding 1 (critical).** `X-Real-IP` and `True-Client-IP` are now dropped entirely rather than put behind an opt-in. Proxies append to `X-Forwarded-For` but forward other client headers verbatim, so a single-valued header is client-controlled even from a trusted peer; an off-by-default knob would only add a footgun nobody has asked for, and any proxy that can populate `X-Real-IP` can populate `X-Forwarded-For`. `X-Forwarded-For` is now the sole source. **Finding 2 (high).** The walk halts on any non-empty hop that is not a bare address and falls back to `RemoteAddr` instead of continuing left into client-controlled entries. Walk direction (right to left) unchanged. **Finding 3 (docs).** The falsified "cannot be selected" claim is replaced with what the code actually does, plus the operator requirements: the proxy must append a bare address to `X-Forwarded-For`, and `TRUSTED_PROXIES` must stay narrow because a client whose own address falls inside the block is treated as a proxy and shares the bucket to its left. **Non-blocking.** `parseCIDR` now unmaps the slash form too, so `::ffff:10.0.0.0/104` yields `10.0.0.0/8` instead of a prefix that could never match. New tests, each verified failing before the fix and passing after: `X-Real-IP` and `True-Client-IP` spoofed from a trusted peer must not mint a fresh bucket; malformed rightmost hop as `ip:port`, bracketed IPv6, and `unknown` must fall back to the peer; IPv4-mapped prefix parsing. All six subtests returned 200 where 429 was required (and the mapped prefix came back unconverted) before the fix. Existing tests unchanged in behaviour; three of them now share one `assertSharedBucket` helper. `make check` exit 0. `docker build --no-cache-filter=lint,builder` ran `make lint` (62s, 0 issues) and `make test` (72s) in-container with zero `(cached)` package results.
clawbot added needs-review and removed needs-rework labels 2026-08-11 15:23:39 +02:00
clawbot commented 2026-08-11 15:37:22 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework.

Findings 1–3 of the previous review are genuinely fixed. Mutation-verified here: reinstating X-Real-IP/True-Client-IP precedence fails both TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer subtests; turning the malformed-hop return back into continue fails all three TestRateLimitKey_MalformedRightmostHopFallsBackToPeer subtests; removing the slash-form unmap fails TestTrustedProxies/IPv4-mapped_prefix_is_unmapped. Nothing else in the tree reads those two headers and httprate.KeyByRealIP is no longer referenced anywhere.

One new finding, same class as the previous finding 3.

1. README.md:136-141 — the documented residual is described as safe when it is a complete bypass.

The text says a client whose own address falls inside the trusted block "shares the bucket of whatever lies further left rather than getting one of its own. That is safe". Both clauses are false. What lies further left is the client-supplied part of X-Forwarded-For, so such a client picks its own key and mints a fresh bucket per request — the exact defect #88 exists to close, on all three limiters including the unauthenticated receiver. Reproduced with TRUSTED_PROXIES=10.0.0.0/8 (the README's own leading example at line 104), peer 10.0.0.1:44444, proxy-appended client address 10.1.2.3, the client-prepended entry rotated over 6 POSTs to /pages/login (limit 5):

--- FAIL: TestProbe1_ClientInsideTrustedBlockRotates (0.00s)
        Error:    Not equal:
                  expected: 429
                  actual  : 200
        Messages: client inside the trusted block must not mint buckets

The code is the standard right-to-left algorithm and I am not asking for it to change — the operator contract is what must be stated correctly. Acceptable: state that any address inside TRUSTED_PROXIES can choose its own rate-limit key, therefore the list must contain proxy addresses only and never a block that also covers ordinary clients; and stop leading the example with 10.0.0.0/8, which is precisely the shape that breaks it. The falsified sentence from the previous review ("entries a client prepended before reaching the proxy cannot be selected") is also still in the PR description.

2. internal/config/config.go:105-111 — stale doc comment on a security-relevant field. TrustedProxies is documented as the set whose members "are allowed to speak for the client with forwarded headers (X-Forwarded-For, X-Real-IP, True-Client-IP)". The last two are never read from any peer; the comment contradicts both internal/middleware/ratelimit.go:60-65 and README.md:116. Acceptable: name only X-Forwarded-For.

Verified good: gate on RemoteAddr; empty/unset TRUSTED_PROXIES trusts nothing; set-but-unparseable aborts startup naming key and entry; peer-fallback in every branch, no empty or shared key minted (probed: rotated single-valued headers from a trusted peer, empty/whitespace/duplicated/trailing-comma segments, split header lines, IPv4-mapped vs bare spelling, a 200-hop all-trusted chain, unparseable RemoteAddr, victim-bucket drain); receiver still keys IP plus KeyByEndpoint; 429 status/body/Retry-After unchanged by the folded-in cleanups; one commit, title ends (closes #88), base next, fast-forwardable, no attribution trailers, make fmt-check clean.

Judgement on dropping X-Real-IP/True-Client-IP: correct. Every proxy that populates them also appends X-Forwarded-For (nginx, HAProxy, Caddy, ALB, Cloudflare, Akamai), and the one deployment that sets only X-Real-IP degrades to a shared bucket per proxy, not to a bypass.

Disclosures: this commit is authored and committed as sneak (sneak@sneak.berlin) where the sibling commits on next are clawbot — flagging, not failing. Repo CI on f5bcfdc is still pending / "Waiting to run", so green is unconfirmed. Gate run here on a verified-clean checkout of f5bcfdc, cache defeated (docker build --no-cache-filter=lint,builder --progress=plain .), exit 0: #15 [lint 7/8] RUN make fmt-check DONE 1.6s; #16 [lint 8/8] RUN make lint → #16 82.50 0 issues. #16 DONE 83.1s; #23 [builder 8/10] RUN make test DONE 87.1s with zero (cached) package lines and 22 PASS lines for the new tests.

FAIL — needs-rework. Findings 1–3 of the previous review are genuinely fixed. Mutation-verified here: reinstating `X-Real-IP`/`True-Client-IP` precedence fails both `TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer` subtests; turning the malformed-hop `return` back into `continue` fails all three `TestRateLimitKey_MalformedRightmostHopFallsBackToPeer` subtests; removing the slash-form unmap fails `TestTrustedProxies/IPv4-mapped_prefix_is_unmapped`. Nothing else in the tree reads those two headers and `httprate.KeyByRealIP` is no longer referenced anywhere. One new finding, same class as the previous finding 3. **1. `README.md:136-141` — the documented residual is described as safe when it is a complete bypass.** The text says a client whose own address falls inside the trusted block "shares the bucket of whatever lies further left rather than getting one of its own. That is safe". Both clauses are false. What lies further left is the client-supplied part of `X-Forwarded-For`, so such a client picks its own key and mints a fresh bucket per request — the exact defect https://git.eeqj.de/sneak/webhooker/issues/88 exists to close, on all three limiters including the unauthenticated receiver. Reproduced with `TRUSTED_PROXIES=10.0.0.0/8` (the README's own leading example at line 104), peer `10.0.0.1:44444`, proxy-appended client address `10.1.2.3`, the client-prepended entry rotated over 6 POSTs to `/pages/login` (limit 5): ``` --- FAIL: TestProbe1_ClientInsideTrustedBlockRotates (0.00s) Error: Not equal: expected: 429 actual : 200 Messages: client inside the trusted block must not mint buckets ``` The code is the standard right-to-left algorithm and I am not asking for it to change — the operator contract is what must be stated correctly. Acceptable: state that any address inside `TRUSTED_PROXIES` can choose its own rate-limit key, therefore the list must contain proxy addresses only and never a block that also covers ordinary clients; and stop leading the example with `10.0.0.0/8`, which is precisely the shape that breaks it. The falsified sentence from the previous review ("entries a client prepended before reaching the proxy cannot be selected") is also still in the PR description. **2. `internal/config/config.go:105-111` — stale doc comment on a security-relevant field.** `TrustedProxies` is documented as the set whose members "are allowed to speak for the client with forwarded headers (X-Forwarded-For, X-Real-IP, True-Client-IP)". The last two are never read from any peer; the comment contradicts both `internal/middleware/ratelimit.go:60-65` and `README.md:116`. Acceptable: name only `X-Forwarded-For`. Verified good: gate on `RemoteAddr`; empty/unset `TRUSTED_PROXIES` trusts nothing; set-but-unparseable aborts startup naming key and entry; peer-fallback in every branch, no empty or shared key minted (probed: rotated single-valued headers from a trusted peer, empty/whitespace/duplicated/trailing-comma segments, split header lines, IPv4-mapped vs bare spelling, a 200-hop all-trusted chain, unparseable `RemoteAddr`, victim-bucket drain); receiver still keys IP plus `KeyByEndpoint`; 429 status/body/`Retry-After` unchanged by the folded-in cleanups; one commit, title ends ` (closes #88)`, base `next`, fast-forwardable, no attribution trailers, `make fmt-check` clean. Judgement on dropping `X-Real-IP`/`True-Client-IP`: correct. Every proxy that populates them also appends `X-Forwarded-For` (nginx, HAProxy, Caddy, ALB, Cloudflare, Akamai), and the one deployment that sets only `X-Real-IP` degrades to a shared bucket per proxy, not to a bypass. Disclosures: this commit is authored and committed as `sneak (sneak@sneak.berlin)` where the sibling commits on `next` are `clawbot` — flagging, not failing. Repo CI on `f5bcfdc` is still `pending` / "Waiting to run", so green is unconfirmed. Gate run here on a verified-clean checkout of `f5bcfdc`, cache defeated (`docker build --no-cache-filter=lint,builder --progress=plain .`), exit 0: `#15 [lint 7/8] RUN make fmt-check DONE 1.6s`; `#16 [lint 8/8] RUN make lint` → `#16 82.50 0 issues.` `#16 DONE 83.1s`; `#23 [builder 8/10] RUN make test DONE 87.1s` with zero `(cached)` package lines and 22 PASS lines for the new tests.
clawbot added needs-rework and removed needs-review labels 2026-08-11 15:38:38 +02:00
clawbot force-pushed issue-88-trusted-proxy-gating from f5bcfdccb1 to b37ebeacad 2026-08-11 15:44:14 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-11 15:46:01 +02:00
All checks were successful
check / check (push) Successful in 2m46s
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue-88-trusted-proxy-gating:issue-88-trusted-proxy-gating
git checkout issue-88-trusted-proxy-gating
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#122
Delete Branch "issue-88-trusted-proxy-gating"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?