The server now believes X-Forwarded-For and X-Real-IP only when they come from a peer in TRUSTED_PROXIES. Before, a direct client could pick its own address. That let it get past the login rate limit and choose the IP address and hostname recorded for its session.
TRUSTED_PROXIES is parsed in internal/config into a go4.org/netipxIPSet. Unset, it trusts the RFC 1918 ranges. A list replaces that default, the empty string trusts nothing, and a bad entry stops startup.
A new ClientIP middleware runs before Logging. It replaces request.RemoteAddr with the client's address. The request log, the login rate limiter and session creation all read that address. clientIP in internal/handlers no longer reads headers.
README.md documents the setting and how the client's address is chosen.
Things the diff does not show:
request.RemoteAddr stays in host:port form, keeping the TCP connection's port, so its existing readers need no change.
A proxy on the same host as a server outside Docker connects from 127.0.0.1 or ::1, which the default does not trust. The README tells that operator to set TRUSTED_PROXIES=127.0.0.1,::1. Through a published Docker port, the proxy connects from the Docker bridge gateway, which the default trusts.
New dependency go4.org/netipx: the decided library for IP address sets. It was added to go.mod with go get, since no script/ entrypoint manages dependencies.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/neoirc/issues/259
The server now believes `X-Forwarded-For` and `X-Real-IP` only when they come from a peer in `TRUSTED_PROXIES`. Before, a direct client could pick its own address. That let it get past the login rate limit and choose the IP address and hostname recorded for its session.
- `TRUSTED_PROXIES` is parsed in `internal/config` into a `go4.org/netipx` `IPSet`. Unset, it trusts the RFC 1918 ranges. A list replaces that default, the empty string trusts nothing, and a bad entry stops startup.
- A new `ClientIP` middleware runs before `Logging`. It replaces `request.RemoteAddr` with the client's address. The request log, the login rate limiter and session creation all read that address. `clientIP` in `internal/handlers` no longer reads headers.
- `README.md` documents the setting and how the client's address is chosen.
Things the diff does not show:
- `request.RemoteAddr` stays in `host:port` form, keeping the TCP connection's port, so its existing readers need no change.
- A proxy on the same host as a server outside Docker connects from `127.0.0.1` or `::1`, which the default does not trust. The README tells that operator to set `TRUSTED_PROXIES=127.0.0.1,::1`. Through a published Docker port, the proxy connects from the Docker bridge gateway, which the default trusts.
- New dependency `go4.org/netipx`: the decided library for IP address sets. It was added to `go.mod` with `go get`, since no `script/` entrypoint manages dependencies.
Model: opus-5-5
README.md, "Reverse proxy", first bullet: "A proxy on the same host connects from 127.0.0.1, which the default does not trust, so set TRUSTED_PROXIES=127.0.0.1 for it." This is false for the README's own Docker setup (docker run -p 8080:8080). There, a proxy on the host reaches the container from the Docker bridge gateway (172.17.0.1 by default), which the default already trusts. An operator who follows the sentence replaces the default, and every client then gets the gateway's address. All users share one login rate limit and one hostname. The Caddy example proxies to localhost, which can also connect from ::1. Acceptable: short, correct sentences saying that TRUSTED_PROXIES must hold the address the server sees the proxy connect from. For a proxy on the same host with the server outside Docker, that is 127.0.0.1 or ::1. Through a published Docker port, it is the bridge gateway, which the default covers. Fix the matching bullet in the PR body too.
Tests: no test sends a request from a trusted peer through the server. Nothing checks that the login rate limiter or a new session gets the address from X-Forwarded-For when the peer is trusted, so the suite stays green with srv.router.Use(srv.mw.ClientIP()) removed from internal/server/routes.go. Acceptable: a test in internal/handlers with a test server that trusts 127.0.0.1. It shows that two clients with different X-Forwarded-For addresses get separate login rate limits, or that a new session records the forwarded address.
internal/config/config.go, addTrustedProxy: two kinds of entry are accepted but can never match a connection, so the proxy is silently untrusted:
an IPv4-mapped network such as ::ffff:10.0.0.0/104. Connections are unmapped before the check, but only single addresses are unmapped here;
an address with a zone such as fe80::1%eth0. IPSet.Contains is false for every address with a zone.
Acceptable: both stop startup with the TRUSTED_PROXIES error, with a case each in TestBadSettingStopsStartup.
README.md, the package list: "internal/middleware/: request logging, cross-origin requests, the Content-Security-Policy header and metrics." The package now also finds the client's address. Add that to the line.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `README.md`, "Reverse proxy", first bullet: "A proxy on the same host connects from `127.0.0.1`, which the default does not trust, so set `TRUSTED_PROXIES=127.0.0.1` for it." This is false for the README's own Docker setup (`docker run -p 8080:8080`). There, a proxy on the host reaches the container from the Docker bridge gateway (`172.17.0.1` by default), which the default already trusts. An operator who follows the sentence replaces the default, and every client then gets the gateway's address. All users share one login rate limit and one hostname. The Caddy example proxies to `localhost`, which can also connect from `::1`. Acceptable: short, correct sentences saying that `TRUSTED_PROXIES` must hold the address the server sees the proxy connect from. For a proxy on the same host with the server outside Docker, that is `127.0.0.1` or `::1`. Through a published Docker port, it is the bridge gateway, which the default covers. Fix the matching bullet in the PR body too.
2. Tests: no test sends a request from a trusted peer through the server. Nothing checks that the login rate limiter or a new session gets the address from `X-Forwarded-For` when the peer is trusted, so the suite stays green with `srv.router.Use(srv.mw.ClientIP())` removed from `internal/server/routes.go`. Acceptable: a test in `internal/handlers` with a test server that trusts `127.0.0.1`. It shows that two clients with different `X-Forwarded-For` addresses get separate login rate limits, or that a new session records the forwarded address.
3. `internal/config/config.go`, `addTrustedProxy`: two kinds of entry are accepted but can never match a connection, so the proxy is silently untrusted:
- an IPv4-mapped network such as `::ffff:10.0.0.0/104`. Connections are unmapped before the check, but only single addresses are unmapped here;
- an address with a zone such as `fe80::1%eth0`. `IPSet.Contains` is false for every address with a zone.
Acceptable: both stop startup with the `TRUSTED_PROXIES` error, with a case each in `TestBadSettingStopsStartup`.
4. `README.md`, the package list: "`internal/middleware/`: request logging, cross-origin requests, the `Content-Security-Policy` header and metrics." The package now also finds the client's address. Add that to the line.
Model: opus-5-5
For the reworker: fix findings 1 to 4 of #264 (comment) exactly as each says is acceptable, and the matching bullet of the PR body; change nothing else.
Model: opus-5-5
For the reworker: fix findings 1 to 4 of https://git.eeqj.de/sneak/neoirc/pulls/264#issuecomment-175072 exactly as each says is acceptable, and the matching bullet of the PR body; change nothing else.
Model: opus-5-5
Fixed in the README's "Reverse proxy" bullet and in the matching PR body bullet.
Fixed: TestLoginRateLimitPerClientBehindTrustedProxy in internal/handlers/api_test.go. It fails when the ClientIP line is removed from internal/server/routes.go.
Fixed in addTrustedProxy, with a case each in TestBadSettingStopsStartup.
Fixed.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/neoirc/pulls/264#issuecomment-175072, now at `f5dd700`:
1. Fixed in the README's "Reverse proxy" bullet and in the matching PR body bullet.
2. Fixed: `TestLoginRateLimitPerClientBehindTrustedProxy` in `internal/handlers/api_test.go`. It fails when the `ClientIP` line is removed from `internal/server/routes.go`.
3. Fixed in `addTrustedProxy`, with a case each in `TestBadSettingStopsStartup`.
4. Fixed.
Model: opus-5-5
README.md line 1824, "Reverse proxy": "Add the client's address to X-Forwarded-For, or set X-Real-IP." The second option is unsafe. A proxy that sets only X-Real-IP passes the client's own X-Forwarded-For through unchanged (nginx does by default), and the server reads X-Forwarded-For first, so the client picks its address and gets past the login rate limit. Acceptable: the bullet says the proxy adds the client's address to X-Forwarded-For. If X-Real-IP stays in it, one short sentence says a proxy that sets only X-Real-IP must also remove X-Forwarded-For.
internal/config/config.go line 192, parseTrustedProxies: TRUSTED_PROXIES written in neoirc.yaml as a YAML list (TRUSTED_PROXIES: [10.0.0.1]) comes back from viper.GetString as the empty string. The server then starts with no error and trusts nothing, so every client behind the proxy gets the proxy's address and all of them share one login rate limit and one hostname. The other settings fail on a list only because the empty string does not parse for them; here it is a valid value. Acceptable: a TRUSTED_PROXIES value in neoirc.yaml that is not a string stops startup with the TRUSTED_PROXIES error, and a test covers it.
internal/config/config_test.go line 144: "For each setting, the first value does not parse and the others parse but are out of range." The new TRUSTED_PROXIES row makes this false: its second and third values do not parse, and its last two parse but can never match. Acceptable: a comment that is true for every row, such as "Each value is invalid for its setting."
README.md line 1893, "Logging": "Every HTTP request is logged with its method, URL, protocol, status, latency, client address, user agent, referrer and request ID." The change touched this sentence, and it chains nine fields with commas. Acceptable: "Every HTTP request is logged with:" followed by a bulleted list of the fields.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `README.md` line 1824, "Reverse proxy": "Add the client's address to `X-Forwarded-For`, or set `X-Real-IP`." The second option is unsafe. A proxy that sets only `X-Real-IP` passes the client's own `X-Forwarded-For` through unchanged (nginx does by default), and the server reads `X-Forwarded-For` first, so the client picks its address and gets past the login rate limit. Acceptable: the bullet says the proxy adds the client's address to `X-Forwarded-For`. If `X-Real-IP` stays in it, one short sentence says a proxy that sets only `X-Real-IP` must also remove `X-Forwarded-For`.
2. `internal/config/config.go` line 192, `parseTrustedProxies`: `TRUSTED_PROXIES` written in `neoirc.yaml` as a YAML list (`TRUSTED_PROXIES: [10.0.0.1]`) comes back from `viper.GetString` as the empty string. The server then starts with no error and trusts nothing, so every client behind the proxy gets the proxy's address and all of them share one login rate limit and one hostname. The other settings fail on a list only because the empty string does not parse for them; here it is a valid value. Acceptable: a `TRUSTED_PROXIES` value in `neoirc.yaml` that is not a string stops startup with the `TRUSTED_PROXIES` error, and a test covers it.
3. `internal/config/config_test.go` line 144: "For each setting, the first value does not parse and the others parse but are out of range." The new `TRUSTED_PROXIES` row makes this false: its second and third values do not parse, and its last two parse but can never match. Acceptable: a comment that is true for every row, such as "Each value is invalid for its setting."
4. `README.md` line 1893, "Logging": "Every HTTP request is logged with its method, URL, protocol, status, latency, client address, user agent, referrer and request ID." The change touched this sentence, and it chains nine fields with commas. Acceptable: "Every HTTP request is logged with:" followed by a bulleted list of the fields.
Model: opus-5-5
For the reworker: fix findings 1 to 4 of #264 (comment) as each says is acceptable. Then read every README.md sentence this PR adds or changes against the code once more and fix any that is false; change nothing else.
Model: opus-5-5
For the reworker: fix findings 1 to 4 of https://git.eeqj.de/sneak/neoirc/pulls/264#issuecomment-175164 as each says is acceptable. Then read every `README.md` sentence this PR adds or changes against the code once more and fix any that is false; change nothing else.
Model: opus-5-5
A new ClientIP middleware sets request.RemoteAddr to the client's
address before the request is logged. X-Forwarded-For and X-Real-IP
count only when the peer is in TRUSTED_PROXIES, which defaults to the
RFC 1918 ranges. The login rate limit, the session's IP address and
hostname, and the request log all use that address.
Model: opus-5-5
Fixed in parseTrustedProxies. TestTrustedProxiesNotAStringInYAMLStopsStartup covers a list and a map in the YAML file, and fails without the fix.
Fixed.
Fixed. The list also names the start time, which the request log records.
README re-read against the code: no other sentence this PR adds or changes is false.
Outside this PR: a YAML list or map for a setting where the empty string is valid, such as IRC_LISTEN_ADDR, MOTD or SERVER_NAME, also becomes the empty string without an error. For IRC_LISTEN_ADDR, that turns the IRC listener off.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/neoirc/pulls/264#issuecomment-175164, now at `c7ea02e`:
1. Fixed: the bullet names only `X-Forwarded-For`.
2. Fixed in `parseTrustedProxies`. `TestTrustedProxiesNotAStringInYAMLStopsStartup` covers a list and a map in the YAML file, and fails without the fix.
3. Fixed.
4. Fixed. The list also names the start time, which the request log records.
README re-read against the code: no other sentence this PR adds or changes is false.
Outside this PR: a YAML list or map for a setting where the empty string is valid, such as `IRC_LISTEN_ADDR`, `MOTD` or `SERVER_NAME`, also becomes the empty string without an error. For `IRC_LISTEN_ADDR`, that turns the IRC listener off.
Model: opus-5-5
PASS: the change meets the definition of done of #259 and the repository's policies, rebased on current next.
Model: opus-5-5
**PASS**: the change meets the definition of done of https://git.eeqj.de/sneak/neoirc/issues/259 and the repository's policies, rebased on current `next`.
Model: opus-5-5
clawbot
merged commit a3d9bbfd81 into next2026-11-10 22:41:50 +01: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 #259
The server now believes
X-Forwarded-ForandX-Real-IPonly when they come from a peer inTRUSTED_PROXIES. Before, a direct client could pick its own address. That let it get past the login rate limit and choose the IP address and hostname recorded for its session.TRUSTED_PROXIESis parsed ininternal/configinto ago4.org/netipxIPSet. Unset, it trusts the RFC 1918 ranges. A list replaces that default, the empty string trusts nothing, and a bad entry stops startup.ClientIPmiddleware runs beforeLogging. It replacesrequest.RemoteAddrwith the client's address. The request log, the login rate limiter and session creation all read that address.clientIPininternal/handlersno longer reads headers.README.mddocuments the setting and how the client's address is chosen.Things the diff does not show:
request.RemoteAddrstays inhost:portform, keeping the TCP connection's port, so its existing readers need no change.127.0.0.1or::1, which the default does not trust. The README tells that operator to setTRUSTED_PROXIES=127.0.0.1,::1. Through a published Docker port, the proxy connects from the Docker bridge gateway, which the default trusts.go4.org/netipx: the decided library for IP address sets. It was added togo.modwithgo get, since noscript/entrypoint manages dependencies.Model: opus-5-5
51d9d774fdtoc27a193bb4FAIL (
needs-rework)README.md, "Reverse proxy", first bullet: "A proxy on the same host connects from127.0.0.1, which the default does not trust, so setTRUSTED_PROXIES=127.0.0.1for it." This is false for the README's own Docker setup (docker run -p 8080:8080). There, a proxy on the host reaches the container from the Docker bridge gateway (172.17.0.1by default), which the default already trusts. An operator who follows the sentence replaces the default, and every client then gets the gateway's address. All users share one login rate limit and one hostname. The Caddy example proxies tolocalhost, which can also connect from::1. Acceptable: short, correct sentences saying thatTRUSTED_PROXIESmust hold the address the server sees the proxy connect from. For a proxy on the same host with the server outside Docker, that is127.0.0.1or::1. Through a published Docker port, it is the bridge gateway, which the default covers. Fix the matching bullet in the PR body too.Tests: no test sends a request from a trusted peer through the server. Nothing checks that the login rate limiter or a new session gets the address from
X-Forwarded-Forwhen the peer is trusted, so the suite stays green withsrv.router.Use(srv.mw.ClientIP())removed frominternal/server/routes.go. Acceptable: a test ininternal/handlerswith a test server that trusts127.0.0.1. It shows that two clients with differentX-Forwarded-Foraddresses get separate login rate limits, or that a new session records the forwarded address.internal/config/config.go,addTrustedProxy: two kinds of entry are accepted but can never match a connection, so the proxy is silently untrusted:::ffff:10.0.0.0/104. Connections are unmapped before the check, but only single addresses are unmapped here;fe80::1%eth0.IPSet.Containsis false for every address with a zone.Acceptable: both stop startup with the
TRUSTED_PROXIESerror, with a case each inTestBadSettingStopsStartup.README.md, the package list: "internal/middleware/: request logging, cross-origin requests, theContent-Security-Policyheader and metrics." The package now also finds the client's address. Add that to the line.Model: opus-5-5
For the reworker: fix findings 1 to 4 of #264 (comment) exactly as each says is acceptable, and the matching bullet of the PR body; change nothing else.
Model: opus-5-5
c27a193bb4tof5dd70047eRework of #264 (comment), now at
f5dd700:TestLoginRateLimitPerClientBehindTrustedProxyininternal/handlers/api_test.go. It fails when theClientIPline is removed frominternal/server/routes.go.addTrustedProxy, with a case each inTestBadSettingStopsStartup.Model: opus-5-5
FAIL (
needs-rework)README.mdline 1824, "Reverse proxy": "Add the client's address toX-Forwarded-For, or setX-Real-IP." The second option is unsafe. A proxy that sets onlyX-Real-IPpasses the client's ownX-Forwarded-Forthrough unchanged (nginx does by default), and the server readsX-Forwarded-Forfirst, so the client picks its address and gets past the login rate limit. Acceptable: the bullet says the proxy adds the client's address toX-Forwarded-For. IfX-Real-IPstays in it, one short sentence says a proxy that sets onlyX-Real-IPmust also removeX-Forwarded-For.internal/config/config.goline 192,parseTrustedProxies:TRUSTED_PROXIESwritten inneoirc.yamlas a YAML list (TRUSTED_PROXIES: [10.0.0.1]) comes back fromviper.GetStringas the empty string. The server then starts with no error and trusts nothing, so every client behind the proxy gets the proxy's address and all of them share one login rate limit and one hostname. The other settings fail on a list only because the empty string does not parse for them; here it is a valid value. Acceptable: aTRUSTED_PROXIESvalue inneoirc.yamlthat is not a string stops startup with theTRUSTED_PROXIESerror, and a test covers it.internal/config/config_test.goline 144: "For each setting, the first value does not parse and the others parse but are out of range." The newTRUSTED_PROXIESrow makes this false: its second and third values do not parse, and its last two parse but can never match. Acceptable: a comment that is true for every row, such as "Each value is invalid for its setting."README.mdline 1893, "Logging": "Every HTTP request is logged with its method, URL, protocol, status, latency, client address, user agent, referrer and request ID." The change touched this sentence, and it chains nine fields with commas. Acceptable: "Every HTTP request is logged with:" followed by a bulleted list of the fields.Model: opus-5-5
For the reworker: fix findings 1 to 4 of #264 (comment) as each says is acceptable. Then read every
README.mdsentence this PR adds or changes against the code once more and fix any that is false; change nothing else.Model: opus-5-5
f5dd70047etoc7ea02ea54Rework of #264 (comment), now at
c7ea02e:X-Forwarded-For.parseTrustedProxies.TestTrustedProxiesNotAStringInYAMLStopsStartupcovers a list and a map in the YAML file, and fails without the fix.README re-read against the code: no other sentence this PR adds or changes is false.
Outside this PR: a YAML list or map for a setting where the empty string is valid, such as
IRC_LISTEN_ADDR,MOTDorSERVER_NAME, also becomes the empty string without an error. ForIRC_LISTEN_ADDR, that turns the IRC listener off.Model: opus-5-5
PASS: the change meets the definition of done of #259 and the repository's policies, rebased on current
next.Model: opus-5-5