Rate-limit key collapses every peer with an empty RemoteAddr into one bucket #168

New Issue

clawbot · 2026-08-17T23:38:48+02:00

clawbot commented

2026-08-17 23:38:48 +02:00

Raised by the independent review of #162 and left out of scope there, since it predates that change and is not what the change touches.

clientKey in internal/middleware/ratelimit.go parses ipFromHostPort(r.RemoteAddr); ipFromHostPort returns "" when net.SplitHostPort fails, the parse then fails, and the fallback returns r.RemoteAddr raw. For an empty RemoteAddr that raw value is "", so every such peer keys on the same empty string and shares one rate-limit bucket across all four limiter instances.

Severity is low. It is fail-closed — peers collapse into one bucket rather than each minting their own — and net/http always populates RemoteAddr for a TCP listener, so it is not reachable in the normal serving path. It is reachable for a hand-built *http.Request, and the same collapse is the documented behaviour for a Unix-socket listener, where every peer legitimately carries the same RemoteAddr.

Worth deciding rather than leaving silent, because the empty case is currently indistinguishable from the Unix-socket case in the code and is untested either way.

Options

Leave the behaviour and add a test pinning it, so the shared bucket is a recorded decision rather than an accident.
Reject the request outright when RemoteAddr is empty, on the grounds that a request with no peer identity cannot be rate limited at all.

Done-criteria once decided

The empty-RemoteAddr path has a test asserting whichever behaviour is chosen.
If option 1, a comment at the fallback in clientKey distinguishes the empty case from the Unix-socket case.

Raised by the independent review of https://git.eeqj.de/sneak/webhooker/pulls/162 and left out of scope there, since it predates that change and is not what the change touches. `clientKey` in `internal/middleware/ratelimit.go` parses `ipFromHostPort(r.RemoteAddr)`; `ipFromHostPort` returns `""` when `net.SplitHostPort` fails, the parse then fails, and the fallback returns `r.RemoteAddr` raw. For an empty `RemoteAddr` that raw value is `""`, so every such peer keys on the same empty string and shares one rate-limit bucket across all four limiter instances. Severity is low. It is fail-closed — peers collapse into one bucket rather than each minting their own — and `net/http` always populates `RemoteAddr` for a TCP listener, so it is not reachable in the normal serving path. It is reachable for a hand-built `*http.Request`, and the same collapse is the documented behaviour for a Unix-socket listener, where every peer legitimately carries the same `RemoteAddr`. Worth deciding rather than leaving silent, because the empty case is currently indistinguishable from the Unix-socket case in the code and is untested either way. ## Options 1. Leave the behaviour and add a test pinning it, so the shared bucket is a recorded decision rather than an accident. 2. Reject the request outright when `RemoteAddr` is empty, on the grounds that a request with no peer identity cannot be rate limited at all. ## Done-criteria once decided - The empty-`RemoteAddr` path has a test asserting whichever behaviour is chosen. - If option 1, a comment at the fallback in `clientKey` distinguishes the empty case from the Unix-socket case.

clawbot referenced this issue

2026-08-17 23:39:32 +02:00

Bucket IPv6 rate-limit keys by /64 (closes #125) #162

clawbot referenced this issue

2026-08-17 23:50:35 +02:00

Milestone 1.0.0: internet-facing readiness #111

clawbot referenced this issue

2026-08-17 23:51:12 +02:00

Bucket IPv6 rate-limit keys by /64 (closes #125) #162

Sign in to join this conversation.