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.
Plan. Take option 1, which leaves the behaviour alone. It fails closed: peers with no identity share one bucket. net/http always fills RemoteAddr for a TCP listener, and rejecting outright would add a failure path for a case nothing reaches in normal serving.
Add a test that pins what an empty RemoteAddr does today: one shared rate-limit key.
Put a one-sentence comment at the fallback in the rate-limit key function (internal/middleware/ratelimit.go) that tells the empty case apart from the Unix-socket case.
Plan. Take option 1, which leaves the behaviour alone. It fails closed: peers with no identity share one bucket. `net/http` always fills `RemoteAddr` for a TCP listener, and rejecting outright would add a failure path for a case nothing reaches in normal serving.
- Add a test that pins what an empty `RemoteAddr` does today: one shared rate-limit key.
- Put a one-sentence comment at the fallback in the rate-limit key function (`internal/middleware/ratelimit.go`) that tells the empty case apart from the Unix-socket case.
- Runs after https://git.eeqj.de/sneak/webhooker/issues/270, which touches the same file.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 09:13:11 +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.
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.
clientKeyininternal/middleware/ratelimit.goparsesipFromHostPort(r.RemoteAddr);ipFromHostPortreturns""whennet.SplitHostPortfails, the parse then fails, and the fallback returnsr.RemoteAddrraw. For an emptyRemoteAddrthat 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/httpalways populatesRemoteAddrfor 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 sameRemoteAddr.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
RemoteAddris empty, on the grounds that a request with no peer identity cannot be rate limited at all.Done-criteria once decided
RemoteAddrpath has a test asserting whichever behaviour is chosen.clientKeydistinguishes the empty case from the Unix-socket case.Plan. Take option 1, which leaves the behaviour alone. It fails closed: peers with no identity share one bucket.
net/httpalways fillsRemoteAddrfor a TCP listener, and rejecting outright would add a failure path for a case nothing reaches in normal serving.RemoteAddrdoes today: one shared rate-limit key.internal/middleware/ratelimit.go) that tells the empty case apart from the Unix-socket case.Model: opus-5-5