From 2532da55e9d3dd7f6fc73e3cc6bb674dbf39caa4 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 19 Oct 2026 00:28:18 +0200 Subject: [PATCH] Client address is the right-most untrusted X-Forwarded-For entry (closes #144) From a peer in TRUSTED_PROXIES, clientIP took the left-most X-Forwarded-For entry, which the client writes itself behind a proxy that appends to the header, so a client could choose the address the rate limit counts. It now walks the entries from the right past trusted proxies and takes the first that is not one. An entry with a port is read as its address; with only trusted entries, or one that is not an address at all, it uses the peer. Every X-Forwarded-For header line is read, and X-Real-IP only when there is no X-Forwarded-For entry. Model: opus-5-5 --- README.md | 11 +++- TODO.md | 16 +++++ configs/nginx.conf | 4 +- internal/middleware/middleware.go | 55 +++++++++++++---- internal/middleware/middleware_test.go | 82 ++++++++++++++++++++++++-- 5 files changed, 149 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index c664a53..4fa31fb 100644 --- a/README.md +++ b/README.md @@ -212,8 +212,15 @@ replaces the default; set and empty, the server trusts no proxy. Loopback is not in the default, so a reverse proxy on the same host has to be named, as in `TRUSTED_PROXIES=127.0.0.1/32`. A request whose direct peer is outside this set has its forwarded headers ignored, and the direct peer is logged and -rate-limited instead. The container image does not use this default; see -[Deployment](#deployment). +rate-limited instead. From a direct peer inside the set, the client address is +the right-most `X-Forwarded-For` entry that is not in the set: each proxy +appends the address it took the request from, so the entries further left were +written by the client and are not used. An entry written as an address with a +port, such as `198.51.100.7:5000` or `[2001:db8::1]:443`, is read as that +address. If the search from the right finds only addresses in the set, or meets +an entry that is not an address at all, such as `unknown`, the direct peer is +used. `X-Real-IP` is read only when `X-Forwarded-For` is missing or empty. The +container image does not use this default; see [Deployment](#deployment). A variable set to a value the server cannot use, such as `PORT=abc`, `DEBUG=maybe` or a `BIND_ADDRESS` that is not an IP address, stops it from diff --git a/TODO.md b/TODO.md index 0cfec79..83deb42 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,22 @@ Run `make frontend-viewport-test` in CI as its own step; it is not part of # Completed Steps +- 2026-10-18: from a trusted proxy, the backend takes the client address from + the right-most `X-Forwarded-For` entry that is not in `TRUSTED_PROXIES` + ([#144](https://git.eeqj.de/sneak/netwatch/issues/144)). It took the left-most + entry, which the client writes itself behind a proxy that appends to the + header, so a client could choose the address the rate limit counts and the + request log records. An entry written as an address with a port is read as + that address. With only trusted addresses in the header, or an entry that is + not an address at all (such as `unknown`) met on the way, the direct peer is + used. `X-Real-IP` is read only without `X-Forwarded-For`, and every + `X-Forwarded-For` header line is read, not only the first. The image's nginx, + and the `TRUSTED_PROXIES=127.0.0.1/32` it starts the backend with, are + unchanged; the comment in `configs/nginx.conf` that said the backend takes the + first entry now says it takes the one address nginx passes on. Unit tests + resolve the address from chains ending in an untrusted address, in trusted + ones, of only trusted ones, with ports, with an entry that is not an address, + and split over two header lines - 2026-10-18: after an interval change, the summary's Reachable slot reads `--/--` in white, as at page load, until the second round since the change ends ([#140](https://git.eeqj.de/sneak/netwatch/issues/140)). It used to read diff --git a/configs/nginx.conf b/configs/nginx.conf index f04e425..bc0f4dd 100644 --- a/configs/nginx.conf +++ b/configs/nginx.conf @@ -46,7 +46,9 @@ server { # 127.0.0.1:8081. These headers go with every request passed to it. # X-Forwarded-For carries only the client address, as resolved by # the real IP settings above, and not the chain the request came - # with: the backend takes the first entry, which a client can write. + # with. bin/entrypoint.sh starts the backend with + # TRUSTED_PROXIES=127.0.0.1/32, the address nginx connects from, so + # the backend takes that one entry as the client address. proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $remote_addr; diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index f428f10..5dd22bf 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -11,6 +11,7 @@ import ( "net/http" "net/netip" "runtime/debug" + "slices" "strings" "time" @@ -133,6 +134,17 @@ func ipFromHostPort(hostPort string) string { // X-Real-IP are honoured only when the direct peer is a // trusted proxy; otherwise the direct peer is returned so a // spoofed header cannot forge the logged address. +// +// Each proxy appends to X-Forwarded-For the address it took the +// request from, so the entries on the right are written by the +// trusted proxies and those further left by the client. The +// client's address is therefore the right-most entry that is not +// a trusted proxy. An entry written as an address with a port is +// read as that address, both for the trust check and as the +// result. When every entry is a trusted proxy, or the walk from +// the right meets an entry that is not an address at all (such as +// "unknown"), the peer is used. X-Real-IP is read only when there +// is no X-Forwarded-For entry at all. func clientIP( remoteAddr string, header http.Header, @@ -144,28 +156,49 @@ func clientIP( return peer } - if xff := firstForwardedFor(header.Get("X-Forwarded-For")); xff != "" { - return xff + forwarded := forwardedFor(header) + + if len(forwarded) == 0 { + if xr := strings.TrimSpace(header.Get("X-Real-IP")); validIP(xr) { + return xr + } + + return peer } - if xr := strings.TrimSpace(header.Get("X-Real-IP")); validIP(xr) { - return xr + for _, entry := range slices.Backward(forwarded) { + if !validIP(entry) { + return peer + } + + if !addrInAny(entry, trusted) { + return entry + } } return peer } -// firstForwardedFor returns the left-most valid address in an -// X-Forwarded-For list (the original client), or "" if none. -func firstForwardedFor(value string) string { +// forwardedFor returns the entries of X-Forwarded-For in order, +// leaving out blank ones. A proxy may add its entry as a header +// line of its own rather than append it to the line the client +// sent, so every line is read, not only the first. Some proxies +// write an entry as an address with a port, such as +// 198.51.100.7:5000 or [2001:db8::1]:443; the port is removed so +// that the entry is read as its address. +func forwardedFor(header http.Header) []string { + value := strings.Join(header.Values("X-Forwarded-For"), ",") + + var entries []string + for part := range strings.SplitSeq(value, ",") { - candidate := strings.TrimSpace(part) - if validIP(candidate) { - return candidate + entry := strings.TrimSpace(part) + if entry != "" { + entries = append(entries, ipFromHostPort(entry)) } } - return "" + return entries } func validIP(s string) bool { diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index 5d3fe51..efcd668 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -25,10 +25,16 @@ import ( const ( // loopbackPeer is a remote address inside the trusted-proxy allowlist. loopbackPeer = "127.0.0.1:5000" + // loopbackIP is the address of loopbackPeer, which ClientIP returns + // when it uses the peer. + loopbackIP = "127.0.0.1" // forwardedIP is the client address presented via X-Forwarded-For. forwardedIP = "203.0.113.7" // realIP is the client address presented via X-Real-IP. realIP = "203.0.113.9" + // appendedIP is the X-Forwarded-For entry a trusted proxy appended + // after the entries the client wrote. + appendedIP = "198.51.100.7" ) func mustPrefixes(t *testing.T, cidrs ...string) []netip.Prefix { @@ -85,10 +91,35 @@ func clientIPCases() []clientIPCase { want: forwardedIP, }, { - name: "trusted proxy uses left-most of chain", - remoteAddr: "10.1.2.3:5000", - xff: forwardedIP + ", 10.1.2.3", - want: forwardedIP, + name: "trusted proxy uses right-most of chain", + remoteAddr: loopbackPeer, + xff: "203.0.113.9, " + appendedIP, + want: appendedIP, + }, + { + name: "trusted proxy entries are skipped from the right", + remoteAddr: loopbackPeer, + xff: "203.0.113.9, 10.0.0.5", + want: "203.0.113.9", + }, + { + name: "chain of only trusted proxies uses peer", + remoteAddr: loopbackPeer, + xff: "10.0.0.5, 10.1.2.3", + want: loopbackIP, + }, + { + name: "entry that is not an address uses peer", + remoteAddr: loopbackPeer, + xff: "203.0.113.9, unknown", + want: loopbackIP, + }, + { + name: "x-real-ip ignored when forwarded-for has entries", + remoteAddr: loopbackPeer, + xff: "10.0.0.5", + xRealIP: realIP, + want: loopbackIP, }, { name: "trusted proxy falls back to x-real-ip", @@ -117,7 +148,7 @@ func clientIPCases() []clientIPCase { name: "trusted proxy with garbage header uses peer", remoteAddr: loopbackPeer, xff: "not-an-ip", - want: "127.0.0.1", + want: loopbackIP, }, } } @@ -148,6 +179,47 @@ func TestClientIP(t *testing.T) { } } +// TestClientIPReadsEveryForwardedForLine checks that an entry a proxy +// adds as an X-Forwarded-For line of its own is read, and the line the +// client sent before it is not taken. +func TestClientIPReadsEveryForwardedForLine(t *testing.T) { + t.Parallel() + + header := http.Header{} + header.Add("X-Forwarded-For", "203.0.113.9") + header.Add("X-Forwarded-For", appendedIP) + + got := middleware.ClientIP(loopbackPeer, header, + mustPrefixes(t, "127.0.0.1/32")) + if got != appendedIP { + t.Errorf("ClientIP() = %q, want %q", got, appendedIP) + } +} + +// TestClientIPReadsAddressWithPort checks that an X-Forwarded-For entry +// written as an address with a port is read as that address, both for +// the trust check and as the result. +func TestClientIPReadsAddressWithPort(t *testing.T) { + t.Parallel() + + trusted := mustPrefixes(t, "127.0.0.1/32", "10.0.0.0/8") + + for xff, want := range map[string]string{ + "203.0.113.9, " + appendedIP + ":5000": appendedIP, + "203.0.113.9, [2001:db8::1]:443": "2001:db8::1", + "203.0.113.9, 10.0.0.5:5000": "203.0.113.9", + } { + header := http.Header{} + header.Set("X-Forwarded-For", xff) + + got := middleware.ClientIP(loopbackPeer, header, trusted) + if got != want { + t.Errorf("X-Forwarded-For %q: ClientIP() = %q, want %q", + xff, got, want) + } + } +} + func TestSecurityHeaders(t *testing.T) { t.Parallel()