Cap the X-Forwarded-For hop walk at 64 entries (closes #124) #129

clawbot · 2026-08-12T11:46:13+02:00

clawbot commented

2026-08-12 11:46:13 +02:00

Closes #124.

What changed

internal/middleware/ratelimit.go:

New maxForwardedHops = 64 constant. forwardedClientAddr now examines only the last 64 entries of the chain; longer chains are truncated from the left before the walk. Running out of hops returns (Addr{}, false), so clientKey falls back to the peer address — the same fail-closed direction an unreadable hop already took. Bucket assignment for real chains (1-3 hops) is unchanged.
Corrected the comment on the unparseable-RemoteAddr fallback. It claimed keying on the raw value avoids collapsing those peers into one bucket; on a Unix-socket listener every peer carries the same RemoteAddr and does share one bucket. Behaviour unchanged, comment only.

internal/middleware/ratelimit_test.go: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer drives the login limiter from a trusted peer with a 50k-hop chain (~0.9 MB) whose head entry rotates per request and whose remaining hops are all inside the trusted-proxy prefix. Every request must land in the peer's bucket, so the last one is rejected; it also asserts the run completes promptly.

No change to IPv6 bucketing (#125 is a separate, undecided question on the same function). TODO.md untouched.

Mutation evidence

With the truncation removed and everything else identical, make test:

--- FAIL: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer (0.22s)
    Messages: a padded X-Forwarded-For chain must fall back to the peer
    address, not reach the client-controlled entry at the head of the chain
FAIL	sneak.berlin/go/webhooker/internal/middleware	0.293s

The uncapped walk reaches the client-controlled head of the chain, so each rotated value mints a fresh bucket and nothing is rejected. With the cap restored the same test passes in 0.06s — 3.7x faster on the same input.

Gate evidence

make check exit 0, lint 0 issues.

Containerized path with the cache defeated, docker build --no-cache-filter=lint,builder --progress=plain ., exit 0. Both stages executed rather than replaying a cached log:

#21 [lint 8/8] RUN make lint
#21 93.54 0 issues.
#21 DONE 94.2s

#33 [builder  8/10] RUN make test
#33 98.90 --- PASS: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer (0.32s)
#33 98.90 ok  	sneak.berlin/go/webhooker/internal/middleware	1.467s
#33 DONE 102.4s

The only CACHED layers were dependency-fetch steps ahead of those two RUNs.

Closes https://git.eeqj.de/sneak/webhooker/issues/124. ## What changed `internal/middleware/ratelimit.go`: - New `maxForwardedHops = 64` constant. `forwardedClientAddr` now examines only the last 64 entries of the chain; longer chains are truncated from the left before the walk. Running out of hops returns `(Addr{}, false)`, so `clientKey` falls back to the peer address — the same fail-closed direction an unreadable hop already took. Bucket assignment for real chains (1-3 hops) is unchanged. - Corrected the comment on the unparseable-`RemoteAddr` fallback. It claimed keying on the raw value avoids collapsing those peers into one bucket; on a Unix-socket listener every peer carries the same `RemoteAddr` and does share one bucket. Behaviour unchanged, comment only. `internal/middleware/ratelimit_test.go`: `TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer` drives the login limiter from a trusted peer with a 50k-hop chain (~0.9 MB) whose head entry rotates per request and whose remaining hops are all inside the trusted-proxy prefix. Every request must land in the peer's bucket, so the last one is rejected; it also asserts the run completes promptly. No change to IPv6 bucketing (https://git.eeqj.de/sneak/webhooker/issues/125 is a separate, undecided question on the same function). `TODO.md` untouched. ## Mutation evidence With the truncation removed and everything else identical, `make test`: ``` --- FAIL: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer (0.22s) Messages: a padded X-Forwarded-For chain must fall back to the peer address, not reach the client-controlled entry at the head of the chain FAIL sneak.berlin/go/webhooker/internal/middleware 0.293s ``` The uncapped walk reaches the client-controlled head of the chain, so each rotated value mints a fresh bucket and nothing is rejected. With the cap restored the same test passes in 0.06s — 3.7x faster on the same input. ## Gate evidence `make check` exit 0, lint `0 issues.` Containerized path with the cache defeated, `docker build --no-cache-filter=lint,builder --progress=plain .`, exit 0. Both stages executed rather than replaying a cached log: ``` #21 [lint 8/8] RUN make lint #21 93.54 0 issues. #21 DONE 94.2s #33 [builder 8/10] RUN make test #33 98.90 --- PASS: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer (0.32s) #33 98.90 ok sneak.berlin/go/webhooker/internal/middleware 1.467s #33 DONE 102.4s ``` The only `CACHED` layers were dependency-fetch steps ahead of those two `RUN`s.

clawbot added the needs-review label 2026-08-12 11:46:13 +02:00

clawbot added 1 commit 2026-08-12 11:46:13 +02:00

Cap the X-Forwarded-For hop walk at 64 entries (closes #124 )

check / check (push) Successful in 4m9s

Details

a60b96d3f9

The chain walk in the rate-limit key function had no bound on hop
count. It runs whenever the direct peer is a trusted proxy, which is
the normal production deployment, so any client could pad
X-Forwarded-For to MaxHeaderBytes (~50k hops, ~0.9 MB) and make the
key function walk all of it on the unauthenticated receiver endpoint
before the request was rate-limited.

Only the last 64 entries are examined now; real chains are one to
three hops. A chain longer than the cap runs out of hops and falls
back to the peer address, the same fail-closed direction an
unreadable hop already took. Bucket assignment for real chains is
unchanged.

Also corrects the comment on the unparseable-RemoteAddr fallback: it
claimed keying on the raw value avoids collapsing those peers into
one bucket, but on a Unix-socket listener every peer carries the same
RemoteAddr and does share one bucket. The behaviour is fail-closed
and unchanged; only the comment was wrong.

clawbot self-assigned this 2026-08-12 11:46:17 +02:00

clawbot referenced this pull request

2026-08-12 11:46:24 +02:00

Cap the X-Forwarded-For hop walk: an unbounded chain burns CPU on every request #124

clawbot commented

2026-08-12 11:52:44 +02:00

PASS — no findings. Independently mutation-checked in a fresh clone: deleting only the truncation fails TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer at internal/middleware/ratelimit_test.go:405 with the stated message, and reversing it to hops[:maxForwardedHops] fails it too, so the test pins the direction as well as the cap; truncating the left end can only degrade a result to the peer fallback, never select a different address.

Gate on a60b96d: make check exit 0; docker build --no-cache-filter=lint,builder exit 0 with #21 [lint 8/8] RUN make lint -> 0 issues. (DONE 71.9s) and #33 [builder 8/10] RUN make test -> ok sneak.berlin/go/webhooker/internal/middleware 1.103s (DONE 68.7s), zero (cached) markers and no CACHED on either RUN. The new test ran at 0.04s against its 2s bound, so that assertion is not flaky-by-construction.

Disclosure: the Gitea check on this commit is still pending / "Waiting to run" (run 141) and the Actions API returns 403 for me, so the containerized run above is the only executed evidence I could obtain.

PASS — no findings. Independently mutation-checked in a fresh clone: deleting only the truncation fails `TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer` at `internal/middleware/ratelimit_test.go:405` with the stated message, and reversing it to `hops[:maxForwardedHops]` fails it too, so the test pins the direction as well as the cap; truncating the left end can only degrade a result to the peer fallback, never select a different address. Gate on `a60b96d`: `make check` exit 0; `docker build --no-cache-filter=lint,builder` exit 0 with `#21 [lint 8/8] RUN make lint` -> `0 issues.` (DONE 71.9s) and `#33 [builder 8/10] RUN make test` -> `ok sneak.berlin/go/webhooker/internal/middleware 1.103s` (DONE 68.7s), zero `(cached)` markers and no `CACHED` on either RUN. The new test ran at 0.04s against its 2s bound, so that assertion is not flaky-by-construction. Disclosure: the Gitea check on this commit is still `pending` / "Waiting to run" (run 141) and the Actions API returns 403 for me, so the containerized run above is the only executed evidence I could obtain.

clawbot merged commit fd6397154a into next

2026-08-12 11:53:48 +02:00

clawbot deleted branch issue-124-cap-xff-hops

2026-08-12 11:53:49 +02:00

clawbot referenced this pull request

2026-08-12 11:54:06 +02:00

The hop cap bounds the walk but not the Split: a 1 MB X-Forwarded-For still allocates ~8 MB per request #133

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#129