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`.
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
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 next2026-08-12 12:19:14 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #133.
What changed
forwardedClientAddrno longer joins and splits the chain. It walksthe
X-Forwarded-Forheader values in reverse and cuts one entry at atime off the right end of each value with
strings.LastIndexByte,stopping after
maxForwardedHopsentries. Allocation is now bounded bythe 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.mdisuntouched, 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(ClientKeyForTesthook).Allocation evidence
New test
TestRateLimitKey_LongChainAllocationIsBoundedkeys a requestcarrying a 1,000,007-byte chain (100k trusted hops) and measures
runtime.MemStats.TotalAllocover 50 calls. Bytes rather thanallocation count, because
strings.Splitof a 1 MB chain is a singleallocation and
testing.AllocsPerRunwould score it as cheap. Notiming assertion.
[]string)Mutation evidence
With
ratelimit.goreverted to thestrings.Splitimplementation andthe new test unchanged:
The test passes only against the scanning implementation.
Gate evidence
make checkexits 0: all packages pass (including the unchanged 50k-hoptest from #124),
0 issues.from the linter,
fmt-checkclean.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.PASS.
Differential-tested
forwardedClientAddragainstnext'sstrings.Splitimplementation 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. Revertinginternal/middleware/ratelimit.gotostrings.Splitfails 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 lint57.3s →0 issues.,#23 [builder 8/10] RUN make test55.5s with no(cached)markers;make checkexit 0. Fast-forwards ontonext, andnextstill merges tomainwithout conflict.Disclosures: the differential harness is a standalone Go module outside this repo, since the old implementation no longer exists here and no
maketarget can host the comparison; every check of this repo was run only throughmake/script.TestRateLimitKey_LongChainAllocationIsBoundedreads process-wideTotalAlloc, so it is noise-sensitive in the fail direction only — 8/8 passes under-race(6 repeatmake testruns,make check, and the Docker builder stage) against a 256x margin (16 bytes measured vs 4096 asserted), and the=== PAUSE/=== CONTordering 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-AllocsPerRunreasoning is correct: thestrings.Splitof a 1 MB chain is one allocation, so a count assertion would have scored the defect as cheap.