Honour forwarded headers only from trusted proxies (closes #259)

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

README.md

@@ -1814,9 +1814,13 @@ speaks plain HTTP, and the auth cookie is `Secure`, so browsers send it only
 over HTTPS. The proxy must:
 
 - Connect from an address in `TRUSTED_PROXIES`. The default trusts the private
-  ranges `10.0.0.0/8`, `172.16.0.0/12` and `192.168.0.0/16`. 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.
+  ranges `10.0.0.0/8`, `172.16.0.0/12` and `192.168.0.0/16`. The setting must
+  hold the address the server sees the proxy connect from. For a proxy on the
+  same host:
+    - With the server outside Docker, that address is `127.0.0.1` or `::1`. The
+      default does not trust them, so set `TRUSTED_PROXIES=127.0.0.1,::1`.
+    - Through a published Docker port, that address is the Docker bridge
+      gateway, such as `172.17.0.1`. The default trusts it.
 - Add the client's address to `X-Forwarded-For`, or set `X-Real-IP`.
 - Wait at least 60 seconds for a response. A long poll can take 30 seconds, and
   the server's own write timeout is 60 seconds.
@@ -1972,8 +1976,8 @@ and their sessions to end. Then it closes the database.
   gets a message.
 - `internal/hashcash/`: stamp checking and minting.
 - `internal/ratelimit/`: the per-address login rate limiter.
-- `internal/middleware/`: request logging, cross-origin requests, the
-  `Content-Security-Policy` header and metrics.
+- `internal/middleware/`: finding the client's address, request logging,
+  cross-origin requests, the `Content-Security-Policy` header and metrics.
 - `internal/healthcheck/` and `internal/stats/`: the health check and its
   counters.
 - `internal/logger/` and `internal/globals/`: logging setup and build

internal/config/config.go

@@ -41,6 +41,8 @@ const defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16"
 
 var errOutOfRange = errors.New("setting out of range")
 
+var errNeverMatches = errors.New("entry can never match a connection")
+
 // Params defines the dependencies for creating a Config.
 type Params struct {
 	fx.In
@@ -217,7 +219,8 @@ func parseTrustedProxies(value **netipx.IPSet) error {
 
 // addTrustedProxy adds entry, a network with a slash or a single
 // address, to builder. An IPv4 address written in IPv6 form, such as
-// ::ffff:192.0.2.1, is added as IPv4.
+// ::ffff:192.0.2.1, is added as IPv4. A network in that form and an
+// address with a zone are errors, since no connection could match them.
 func addTrustedProxy(builder *netipx.IPSetBuilder, entry string) error {
 	if strings.Contains(entry, "/") {
 		prefix, err := netip.ParsePrefix(entry)
@@ -225,6 +228,12 @@ func addTrustedProxy(builder *netipx.IPSetBuilder, entry string) error {
 			return err //nolint:wrapcheck // the caller names the setting
 		}
 
+		// A connection from an IPv4 address is checked in IPv4 form.
+		if prefix.Addr().Is4In6() {
+			return fmt.Errorf("%w: %s is an IPv4 network in IPv6 form",
+				errNeverMatches, entry)
+		}
+
 		builder.AddPrefix(prefix)
 
 		return nil
@@ -235,6 +244,11 @@ func addTrustedProxy(builder *netipx.IPSetBuilder, entry string) error {
 		return err //nolint:wrapcheck // the caller names the setting
 	}
 
+	// IPSet.Contains is false for every address with a zone.
+	if addr.Zone() != "" {
+		return fmt.Errorf("%w: %s has a zone", errNeverMatches, entry)
+	}
+
 	builder.Add(addr.Unmap())
 
 	return nil

internal/config/config_test.go

@@ -161,6 +161,7 @@ func TestBadSettingStopsStartup(t *testing.T) {
 		{key: "SESSION_IDLE_TIMEOUT", values: []string{"a month", "0", "-1m"}},
 		{key: "TRUSTED_PROXIES", values: []string{
 			"proxy.example.com", "10.0.0.0/33", "192.0.2.1:8080",
+			"::ffff:10.0.0.0/104", "fe80::1%eth0",
 		}},
 	}
 

internal/handlers/api_test.go

@@ -4342,6 +4342,56 @@ func TestLoginRateLimitIgnoresForwardedHeaders(t *testing.T) {
 	}
 }
 
+func TestLoginRateLimitPerClientBehindTrustedProxy(t *testing.T) {
+	// The test client connects from 127.0.0.1. Trusted as a proxy, it
+	// passes each client's address in X-Forwarded-For. t.Setenv rules
+	// out t.Parallel.
+	t.Setenv("TRUSTED_PROXIES", "127.0.0.1")
+
+	tserver := newTestServer(t)
+
+	// An empty JSON object is refused after the rate limit counts the
+	// attempt, so no attempt checks a password or looks up a hostname.
+	loginFrom := func(forwardedFor string) int {
+		t.Helper()
+
+		request, err := http.NewRequestWithContext(
+			t.Context(), http.MethodPost,
+			tserver.url("/api/v1/login"), strings.NewReader("{}"),
+		)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		request.Header.Set("Content-Type", "application/json")
+		request.Header.Set("X-Forwarded-For", forwardedFor)
+
+		resp, err := http.DefaultClient.Do(request)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		_ = resp.Body.Close()
+
+		return resp.StatusCode
+	}
+
+	// The default burst is 5 attempts per address.
+	for range 5 {
+		loginFrom("203.0.113.1")
+	}
+
+	status := loginFrom("203.0.113.1")
+	if status != http.StatusTooManyRequests {
+		t.Errorf("sixth attempt from 203.0.113.1 got %d, want 429", status)
+	}
+
+	status = loginFrom("203.0.113.2")
+	if status == http.StatusTooManyRequests {
+		t.Error("first attempt from 203.0.113.2 got 429")
+	}
+}
+
 func TestLoginRateLimitAllowsNormalUse(t *testing.T) {
 	t.Parallel()