Found by a TLS deployment audit running real nginx in front of the app.
With TRUSTED_PROXIES=127.0.0.1/32 correctly set, and the rate limiter demonstrably keying on the true client address, every access log line still reads "remoteIP":"127.0.0.1".
Verified: two genuinely distinct clients (172.17.0.2 and 172.17.0.3, separate containers) hammered the receiver into 429. grep '"remoteIP"' | sort | uniq -c returned 15 "remoteIP":"127.0.0.1" — the proxy, every time. Rate-limit rejection lines carry no client identity at all, and the CSRF-failure WARN's remote_addr is likewise the proxy.
So the app already computes the real client address for rate limiting and then throws it away before logging. In the deployed shape, abuse forensics from webhooker's own logs are impossible: an operator seeing a flood of 429s cannot tell whether it is one attacker or a thousand.
Mitigated because nginx's own access log has the client IP — but correlating the two requires the operator to configure and log X-Request-Id, which the README never mentions. Not milestoned: the information exists at the proxy, so this is a convenience and forensics gap rather than a hole.
Definition of done:
The access log carries the client address the rate limiter already computed, not the peer address, whenever a trusted proxy supplied it. Keep the peer address available too — distinguishing "who connected" from "who the request is attributed to" is the point, and collapsing them loses information.
Rate-limit rejection lines carry the same client identity, since those are the lines an operator investigating abuse reads first.
The CSRF-failure WARN likewise.
Document the field's meaning, including that it is only trustworthy when TRUSTED_PROXIES is set correctly — an attributed address derived from an untrusted X-Forwarded-For is attacker-controlled and must not be presented as authoritative.
Found by a TLS deployment audit running real nginx in front of the app.
With `TRUSTED_PROXIES=127.0.0.1/32` correctly set, and the rate limiter demonstrably keying on the true client address, every access log line still reads `"remoteIP":"127.0.0.1"`.
Verified: two genuinely distinct clients (`172.17.0.2` and `172.17.0.3`, separate containers) hammered the receiver into `429`. `grep '"remoteIP"' | sort | uniq -c` returned `15 "remoteIP":"127.0.0.1"` — the proxy, every time. Rate-limit rejection lines carry no client identity at all, and the CSRF-failure WARN's `remote_addr` is likewise the proxy.
So the app already computes the real client address for rate limiting and then throws it away before logging. In the deployed shape, abuse forensics from webhooker's own logs are impossible: an operator seeing a flood of 429s cannot tell whether it is one attacker or a thousand.
Mitigated because nginx's own access log has the client IP — but correlating the two requires the operator to configure and log `X-Request-Id`, which the README never mentions. Not milestoned: the information exists at the proxy, so this is a convenience and forensics gap rather than a hole.
Definition of done:
- The access log carries the client address the rate limiter already computed, not the peer address, whenever a trusted proxy supplied it. Keep the peer address available too — distinguishing "who connected" from "who the request is attributed to" is the point, and collapsing them loses information.
- Rate-limit rejection lines carry the same client identity, since those are the lines an operator investigating abuse reads first.
- The CSRF-failure WARN likewise.
- Document the field's meaning, including that it is only trustworthy when `TRUSTED_PROXIES` is set correctly — an attributed address derived from an untrusted `X-Forwarded-For` is attacker-controlled and must not be presented as authoritative.
Plan. Runs after #333 lands, which makes the RFC 1918 ranges the default trusted set. The access log (internal/middleware/middleware.go) and the CSRF warning (internal/middleware/csrf.go) still log the peer address. The receiver's rejection log in internal/handlers/webhook.go does too.
One address, two fields. Work out the client address once per request, from the same code the rate limiters key on (internal/middleware/ratelimit.go): the first hop of X-Forwarded-For that is not a trusted proxy, when the peer is a trusted proxy; otherwise the peer itself. Do not write a second parser. Every line the issue names carries two fields:
remoteIP, the connecting peer, which keeps its current meaning;
one new field for the client address, named plainly.
A rate-limit rejection line that carries no address today gets both.
README: document the new field where the access log is described. It is only as trustworthy as TRUSTED_PROXIES: a peer inside that set can write any address it likes into it.
Tests:
with a trusted peer and a forwarded chain, each named line carries the forwarded client and the peer;
with an untrusted peer, both fields are the peer;
each test fails when its change is removed.
Model: opus-5-5
Plan. Runs after https://git.eeqj.de/sneak/webhooker/issues/333 lands, which makes the RFC 1918 ranges the default trusted set. The access log (`internal/middleware/middleware.go`) and the CSRF warning (`internal/middleware/csrf.go`) still log the peer address. The receiver's rejection log in `internal/handlers/webhook.go` does too.
- **One address, two fields.** Work out the client address once per request, from the same code the rate limiters key on (`internal/middleware/ratelimit.go`): the first hop of `X-Forwarded-For` that is not a trusted proxy, when the peer is a trusted proxy; otherwise the peer itself. Do not write a second parser. Every line the issue names carries two fields:
- `remoteIP`, the connecting peer, which keeps its current meaning;
- one new field for the client address, named plainly.
A rate-limit rejection line that carries no address today gets both.
- **README:** document the new field where the access log is described. It is only as trustworthy as `TRUSTED_PROXIES`: a peer inside that set can write any address it likes into it.
- **Tests:**
- with a trusted peer and a forwarded chain, each named line carries the forwarded client and the peer;
- with an untrusted peer, both fields are the peer;
- each test fails when its change is removed.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 09:13:11 +02:00
#440 adds clientIP next to remoteIP on the access log, the rate-limit rejection lines, the CSRF warning and the receiver's webhook request received line. remoteIP is still the connecting peer; clientIP is the address the rate limiters key on, worked out once per request by the same code. The README documents the field and that it is only as trustworthy as TRUSTED_PROXIES.
Judgement call: the CSRF and receiver lines' remote_addr (peer with port) is now remoteIP (peer without port), like the rest.
Judgement call: "the receiver's rejection log" is read as webhook request received, the only receiver line that logged the peer.
Judgement call: the login failure limit's warning gets both fields as a rate-limit rejection line; login limiting itself is untouched.
Judgement call: the README's measured widest access log line (1,972 bytes) predates the new field and is removed; the stated 2,560-byte ceiling holds with the field charged.
Model: opus-5-5
https://git.eeqj.de/sneak/webhooker/pulls/440 adds `clientIP` next to `remoteIP` on the access log, the rate-limit rejection lines, the CSRF warning and the receiver's `webhook request received` line. `remoteIP` is still the connecting peer; `clientIP` is the address the rate limiters key on, worked out once per request by the same code. The README documents the field and that it is only as trustworthy as `TRUSTED_PROXIES`.
- Judgement call: the CSRF and receiver lines' `remote_addr` (peer with port) is now `remoteIP` (peer without port), like the rest.
- Judgement call: "the receiver's rejection log" is read as `webhook request received`, the only receiver line that logged the peer.
- Judgement call: the login failure limit's warning gets both fields as a rate-limit rejection line; login limiting itself is untouched.
- Judgement call: the README's measured widest access log line (1,972 bytes) predates the new field and is removed; the stated 2,560-byte ceiling holds with the field charged.
Model: opus-5-5
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.
Found by a TLS deployment audit running real nginx in front of the app.
With
TRUSTED_PROXIES=127.0.0.1/32correctly set, and the rate limiter demonstrably keying on the true client address, every access log line still reads"remoteIP":"127.0.0.1".Verified: two genuinely distinct clients (
172.17.0.2and172.17.0.3, separate containers) hammered the receiver into429.grep '"remoteIP"' | sort | uniq -creturned15 "remoteIP":"127.0.0.1"— the proxy, every time. Rate-limit rejection lines carry no client identity at all, and the CSRF-failure WARN'sremote_addris likewise the proxy.So the app already computes the real client address for rate limiting and then throws it away before logging. In the deployed shape, abuse forensics from webhooker's own logs are impossible: an operator seeing a flood of 429s cannot tell whether it is one attacker or a thousand.
Mitigated because nginx's own access log has the client IP — but correlating the two requires the operator to configure and log
X-Request-Id, which the README never mentions. Not milestoned: the information exists at the proxy, so this is a convenience and forensics gap rather than a hole.Definition of done:
TRUSTED_PROXIESis set correctly — an attributed address derived from an untrustedX-Forwarded-Foris attacker-controlled and must not be presented as authoritative.Plan. Runs after #333 lands, which makes the RFC 1918 ranges the default trusted set. The access log (
internal/middleware/middleware.go) and the CSRF warning (internal/middleware/csrf.go) still log the peer address. The receiver's rejection log ininternal/handlers/webhook.godoes too.One address, two fields. Work out the client address once per request, from the same code the rate limiters key on (
internal/middleware/ratelimit.go): the first hop ofX-Forwarded-Forthat is not a trusted proxy, when the peer is a trusted proxy; otherwise the peer itself. Do not write a second parser. Every line the issue names carries two fields:remoteIP, the connecting peer, which keeps its current meaning;A rate-limit rejection line that carries no address today gets both.
README: document the new field where the access log is described. It is only as trustworthy as
TRUSTED_PROXIES: a peer inside that set can write any address it likes into it.Tests:
Model: opus-5-5
#440 adds
clientIPnext toremoteIPon the access log, the rate-limit rejection lines, the CSRF warning and the receiver'swebhook request receivedline.remoteIPis still the connecting peer;clientIPis the address the rate limiters key on, worked out once per request by the same code. The README documents the field and that it is only as trustworthy asTRUSTED_PROXIES.remote_addr(peer with port) is nowremoteIP(peer without port), like the rest.webhook request received, the only receiver line that logged the peer.Model: opus-5-5