Scan the X-Forwarded-For chain without splitting it (closes #133) #136

clawbot · 2026-08-12T12:05:17+02:00

clawbot commented

2026-08-12 12:05:17 +02:00

Closes #133.

What changed

forwardedClientAddr no longer joins and splits the chain. It walks
the X-Forwarded-For header values in reverse and cuts one entry at a
time off the right end of each value with strings.LastIndexByte,
stopping after maxForwardedHops entries. Allocation is now bounded by
the cap instead of by header length.

Bucket assignment is unchanged. The walk still counts every
comma-separated entry (including empty ones) against the 64-hop cap,
skips empty and whitespace-only hops, returns the rightmost hop that is
not itself a trusted proxy, and fails closed to the peer address on an
unreadable hop or an exhausted cap. IPv6 bucketing is untouched
(#125), TODO.md is
untouched, and no existing rate-limit assertion was edited.

Diff: internal/middleware/ratelimit.go (the walk),
internal/middleware/ratelimit_test.go (new test),
internal/middleware/export_test.go (ClientKeyForTest hook).

Allocation evidence

New test TestRateLimitKey_LongChainAllocationIsBounded keys a request
carrying a 1,000,007-byte chain (100k trusted hops) and measures
runtime.MemStats.TotalAlloc over 50 calls. Bytes rather than
allocation count, because strings.Split of a 1 MB chain is a single
allocation and testing.AllocsPerRun would score it as cheap. No
timing assertion.

before: 1,606,043 bytes per call (100k-element []string)
after: 16 bytes per call
threshold asserted: < 4096 bytes per call

Mutation evidence

With ratelimit.go reverted to the strings.Split implementation and
the new test unchanged:

--- FAIL: TestRateLimitKey_LongChainAllocationIsBounded
    "1606043" is not less than "4096"

The test passes only against the scanning implementation.

Gate evidence

make check exits 0: all packages pass (including the unchanged 50k-hop
test from #124), 0 issues.
from the linter, fmt-check clean.
docker build --no-cache-filter=lint,builder --progress=plain . exits 0.
Both stages executed, not replayed:
#16 [lint 8/8] RUN make lint → #16 85.76 0 issues., and
#23 [builder 8/10] RUN make test →
--- PASS: TestRateLimitKey_LongChainAllocationIsBounded (0.02s),
ok sneak.berlin/go/webhooker/internal/middleware 1.117s.

Closes https://git.eeqj.de/sneak/webhooker/issues/133. ## What changed `forwardedClientAddr` no longer joins and splits the chain. It walks the `X-Forwarded-For` header values in reverse and cuts one entry at a time off the right end of each value with `strings.LastIndexByte`, stopping after `maxForwardedHops` entries. Allocation is now bounded by the cap instead of by header length. Bucket assignment is unchanged. The walk still counts every comma-separated entry (including empty ones) against the 64-hop cap, skips empty and whitespace-only hops, returns the rightmost hop that is not itself a trusted proxy, and fails closed to the peer address on an unreadable hop or an exhausted cap. IPv6 bucketing is untouched (https://git.eeqj.de/sneak/webhooker/issues/125), `TODO.md` is untouched, and no existing rate-limit assertion was edited. Diff: `internal/middleware/ratelimit.go` (the walk), `internal/middleware/ratelimit_test.go` (new test), `internal/middleware/export_test.go` (`ClientKeyForTest` hook). ## Allocation evidence New test `TestRateLimitKey_LongChainAllocationIsBounded` keys a request carrying a 1,000,007-byte chain (100k trusted hops) and measures `runtime.MemStats.TotalAlloc` over 50 calls. Bytes rather than allocation count, because `strings.Split` of a 1 MB chain is a *single* allocation and `testing.AllocsPerRun` would score it as cheap. No timing assertion. - before: 1,606,043 bytes per call (100k-element `[]string`) - after: 16 bytes per call - threshold asserted: < 4096 bytes per call ## Mutation evidence With `ratelimit.go` reverted to the `strings.Split` implementation and the new test unchanged: ``` --- FAIL: TestRateLimitKey_LongChainAllocationIsBounded "1606043" is not less than "4096" ``` The test passes only against the scanning implementation. ## Gate evidence - `make check` exits 0: all packages pass (including the unchanged 50k-hop test from https://git.eeqj.de/sneak/webhooker/issues/124), `0 issues.` from the linter, `fmt-check` clean. - `docker build --no-cache-filter=lint,builder --progress=plain .` exits 0. Both stages executed, not replayed: `#16 [lint 8/8] RUN make lint` → `#16 85.76 0 issues.`, and `#23 [builder 8/10] RUN make test` → `--- PASS: TestRateLimitKey_LongChainAllocationIsBounded (0.02s)`, `ok sneak.berlin/go/webhooker/internal/middleware 1.117s`.

clawbot added the needs-review label 2026-08-12 12:05:17 +02:00

clawbot added 1 commit 2026-08-12 12:05:17 +02:00

Scan the X-Forwarded-For chain without splitting it (closes #133 )

check / check (push) Successful in 3m20s

Details

b1eb2466f6

The 64-hop cap bounded the walk but not the allocation: the chain was
split before it was capped, so a padded header cost a []string
proportional to its whole length on every request to the
unauthenticated receiver.

Cut hops off the right end of each header value in place with
strings.LastIndexByte instead, and walk multiple header values in
reverse rather than joining them. Bucket assignment is unchanged: the
walk still counts every comma-separated entry against the cap, skips
empty ones, stops at the first hop that is not a trusted proxy, and
falls back to the peer on an unreadable hop or an exhausted cap.

Measured over a 1 MB chain: 1,606,043 bytes allocated per call before,
16 bytes after.

clawbot self-assigned this 2026-08-12 12:05:20 +02:00

clawbot referenced this pull request

2026-08-12 12:05:27 +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

clawbot commented

2026-08-12 12:18:26 +02:00

PASS.

Differential-tested forwardedClientAddr against next's strings.Split implementation over ~7.2M inputs (multiple header lines, leading/trailing/stray commas, whitespace-only entries, ports, bracketed and zoned IPv6, mapped addresses, unparseable hops, and chains of 55-75 entries split across lines so the 64-hop cap runs out mid-line rather than at a line boundary): identical result for every input. The agreement is not vacuous — injected mutations that let empty entries skip the cap budget, that shift the cap by one, and that reverse the scan direction are each caught by that harness, and the comma-padding mutation reproduces exactly the #88 regression it would have to. Reverting internal/middleware/ratelimit.go to strings.Split fails the new test at 1,606,133 bytes/call while the 50k-hop test still passes there, so the new test is the one carrying the fix.

Gate: docker build --no-cache-filter=lint,builder --progress=plain . exit 0 with both stages genuinely executed — #16 [lint 8/8] RUN make lint 57.3s → 0 issues., #23 [builder 8/10] RUN make test 55.5s with no (cached) markers; make check exit 0. Fast-forwards onto next, and next still merges to main without conflict.

Disclosures: the differential harness is a standalone Go module outside this repo, since the old implementation no longer exists here and no make target can host the comparison; every check of this repo was run only through make/script. TestRateLimitKey_LongChainAllocationIsBounded reads process-wide TotalAlloc, so it is noise-sensitive in the fail direction only — 8/8 passes under -race (6 repeat make test runs, make check, and the Docker builder stage) against a 256x margin (16 bytes measured vs 4096 asserted), and the === PAUSE/=== CONT ordering in the test log independently confirms its claim that the package's parallel tests do not run alongside it. Judged sound, and the bytes-not-AllocsPerRun reasoning is correct: the strings.Split of a 1 MB chain is one allocation, so a count assertion would have scored the defect as cheap.

PASS. Differential-tested `forwardedClientAddr` against `next`'s `strings.Split` implementation over ~7.2M inputs (multiple header lines, leading/trailing/stray commas, whitespace-only entries, ports, bracketed and zoned IPv6, mapped addresses, unparseable hops, and chains of 55-75 entries split across lines so the 64-hop cap runs out mid-line rather than at a line boundary): identical result for every input. The agreement is not vacuous — injected mutations that let empty entries skip the cap budget, that shift the cap by one, and that reverse the scan direction are each caught by that harness, and the comma-padding mutation reproduces exactly the https://git.eeqj.de/sneak/webhooker/issues/88 regression it would have to. Reverting `internal/middleware/ratelimit.go` to `strings.Split` fails the new test at 1,606,133 bytes/call while the 50k-hop test still passes there, so the new test is the one carrying the fix. Gate: `docker build --no-cache-filter=lint,builder --progress=plain .` exit 0 with both stages genuinely executed — `#16 [lint 8/8] RUN make lint` 57.3s → `0 issues.`, `#23 [builder 8/10] RUN make test` 55.5s with no `(cached)` markers; `make check` exit 0. Fast-forwards onto `next`, and `next` still merges to `main` without conflict. Disclosures: the differential harness is a standalone Go module outside this repo, since the old implementation no longer exists here and no `make` target can host the comparison; every check of this repo was run only through `make`/`script`. `TestRateLimitKey_LongChainAllocationIsBounded` reads process-wide `TotalAlloc`, so it is noise-sensitive in the fail direction only — 8/8 passes under `-race` (6 repeat `make test` runs, `make check`, and the Docker builder stage) against a 256x margin (16 bytes measured vs 4096 asserted), and the `=== PAUSE`/`=== CONT` ordering in the test log independently confirms its claim that the package's parallel tests do not run alongside it. Judged sound, and the bytes-not-`AllocsPerRun` reasoning is correct: the `strings.Split` of a 1 MB chain is one allocation, so a count assertion would have scored the defect as cheap.

clawbot merged commit 9bfd033a29 into next

2026-08-12 12:19:14 +02:00

clawbot deleted branch issue-133-bounded-xff-scan

2026-08-12 12:19:14 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#136