diff --git a/README.md b/README.md index c664a53..75269dd 100644 --- a/README.md +++ b/README.md @@ -212,7 +212,13 @@ 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 +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. If the search from the right finds only +addresses in the set, or meets an entry that is not an IP address, 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`, diff --git a/TODO.md b/TODO.md index 0cfec79..6725a0f 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,18 @@ 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. With only trusted addresses in the header, or an entry + that is not an IP address 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. Unit + tests resolve the address from chains ending in an untrusted address, in + trusted ones, of only trusted ones, 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/internal/middleware/middleware.go b/internal/middleware/middleware.go index f428f10..c7ab207 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,15 @@ 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. When every entry is a trusted proxy, or the +// walk from the right meets an entry that is not an address, 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 +154,46 @@ 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. +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, 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..0b4b3ab 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -25,6 +25,9 @@ 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. @@ -85,10 +88,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, 198.51.100.7", + want: "198.51.100.7", + }, + { + 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, 198.51.100.7:5000", + 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 +145,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 +176,23 @@ 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", "198.51.100.7") + + got := middleware.ClientIP(loopbackPeer, header, + mustPrefixes(t, "127.0.0.1/32")) + if got != "198.51.100.7" { + t.Errorf("ClientIP() = %q, want %q", got, "198.51.100.7") + } +} + func TestSecurityHeaders(t *testing.T) { t.Parallel()