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()