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,13 +1814,9 @@ 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`. The setting must
+  ranges `10.0.0.0/8`, `172.16.0.0/12` and `192.168.0.0/16`. A proxy on the same
-  hold the address the server sees the proxy connect from. For a proxy on the
+  host connects from `127.0.0.1`, which the default does not trust, so set
-  same host:
+  `TRUSTED_PROXIES=127.0.0.1` for it.
-    - 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.
@@ -1976,8 +1972,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/`: finding the client's address, request logging,
+- `internal/middleware/`: request logging, cross-origin requests, the
-  cross-origin requests, the `Content-Security-Policy` header and metrics.
+  `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,8 +41,6 @@ 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
@@ -219,8 +217,7 @@ 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. A network in that form and an
+// ::ffff:192.0.2.1, is added as IPv4.
-// 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)
@@ -228,12 +225,6 @@ 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
@@ -244,11 +235,6 @@ 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,7 +161,6 @@ 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,56 +4342,6 @@ 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()