Resolve real client IP behind trusted proxies (closes #94)

RFC1918 ranges are the default trusted proxy set on an omitted key; an explicit list replaces the default; an explicit empty list trusts no one; unparseable values abort startup; forwarded headers honored only from trusted peers. Independent review passed: #127 (comment)

model: claude-opus-4-8 (implementation and review); merged by claude-fable-5

internal/clientip/clientip.go

@@ -0,0 +1,119 @@
+// Package clientip resolves the real client IP address of an HTTP request
+// when pixa runs behind a reverse proxy. Forwarding headers are believed
+// only when the immediate peer is a configured trusted proxy, so an
+// untrusted client cannot spoof its address by sending the header.
+package clientip
+
+import (
+	"context"
+	"net"
+	"net/netip"
+	"slices"
+	"strings"
+)
+
+// ForwardedForHeader is the request header carrying the proxy chain. It is
+// honored only when the immediate peer is a trusted proxy.
+const ForwardedForHeader = "X-Forwarded-For"
+
+// Resolver determines the client IP of a request against a fixed set of
+// trusted proxy networks.
+type Resolver struct {
+	trusted []netip.Prefix
+}
+
+// NewResolver returns a Resolver that trusts forwarding headers only from
+// peers inside the given CIDR ranges. A nil or empty list trusts no one,
+// so the peer address is always used.
+func NewResolver(trusted []netip.Prefix) *Resolver {
+	return &Resolver{trusted: trusted}
+}
+
+// Resolve returns the client IP for a request whose direct peer is
+// remoteAddr (a "host:port" string as in http.Request.RemoteAddr) and
+// whose X-Forwarded-For header lines are forwardedFor (as returned by
+// http.Header.Values). When the peer is not a trusted proxy, the peer
+// address is returned and the header is ignored entirely. When the peer is
+// trusted, the header is walked right to left and the first address that is
+// not itself a trusted proxy is returned; this is the client the outermost
+// trusted proxy observed, and entries an untrusted client may have prepended
+// sit to its left and are never reached.
+func (r *Resolver) Resolve(remoteAddr string, forwardedFor []string) string {
+	peer := hostOnly(remoteAddr)
+
+	peerAddr, err := netip.ParseAddr(peer)
+	if err != nil || !r.isTrusted(peerAddr) {
+		return peer
+	}
+
+	for _, hop := range slices.Backward(forwardedForChain(forwardedFor)) {
+		hopAddr, err := netip.ParseAddr(hop)
+		if err != nil || r.isTrusted(hopAddr) {
+			continue
+		}
+
+		return hopAddr.String()
+	}
+
+	return peerAddr.String()
+}
+
+// isTrusted reports whether addr falls inside one of the trusted proxy
+// ranges. Addresses are unmapped first so an IPv4-mapped IPv6 form matches
+// an IPv4 range, matching the fetcher's blocklist comparison.
+func (r *Resolver) isTrusted(addr netip.Addr) bool {
+	if !addr.IsValid() {
+		return false
+	}
+
+	unmapped := addr.Unmap()
+
+	return slices.ContainsFunc(r.trusted, func(prefix netip.Prefix) bool {
+		return prefix.Contains(unmapped)
+	})
+}
+
+// hostOnly strips the port from a "host:port" address. A value without a
+// port (already a bare host) is returned unchanged.
+func hostOnly(remoteAddr string) string {
+	host, _, err := net.SplitHostPort(remoteAddr)
+	if err != nil {
+		return remoteAddr
+	}
+
+	return host
+}
+
+// forwardedForChain flattens the comma-separated entries of every
+// X-Forwarded-For header line into a single ordered, trimmed list.
+func forwardedForChain(values []string) []string {
+	var chain []string
+
+	for _, value := range values {
+		for part := range strings.SplitSeq(value, ",") {
+			trimmed := strings.TrimSpace(part)
+			if trimmed != "" {
+				chain = append(chain, trimmed)
+			}
+		}
+	}
+
+	return chain
+}
+
+// contextKey is the private key type under which the resolved client IP is
+// stored in a request context.
+type contextKey struct{}
+
+// WithClientIP returns a copy of ctx carrying the resolved client IP.
+func WithClientIP(ctx context.Context, ip string) context.Context {
+	return context.WithValue(ctx, contextKey{}, ip)
+}
+
+// FromContext returns the resolved client IP stored in ctx, or an empty
+// string if none was set.
+func FromContext(ctx context.Context) string {
+	ip, _ := ctx.Value(contextKey{}).(string)
+
+	return ip
+}

internal/clientip/clientip_test.go

@@ -0,0 +1,189 @@
+package clientip_test
+
+import (
+	"net/netip"
+	"testing"
+
+	"sneak.berlin/go/pixa/internal/clientip"
+)
+
+// Addresses reused across the resolver cases.
+const (
+	trustedRangeV4 = "10.0.0.0/8"
+	forwardedV4    = "203.0.113.7"
+	untrustedV4    = "198.51.100.9"
+	trustedPeer    = "10.0.0.1:5000"
+)
+
+// mustPrefixes parses CIDR strings into prefixes for building a resolver.
+func mustPrefixes(t *testing.T, cidrs ...string) []netip.Prefix {
+	t.Helper()
+
+	prefixes := make([]netip.Prefix, 0, len(cidrs))
+
+	for _, c := range cidrs {
+		p, err := netip.ParsePrefix(c)
+		if err != nil {
+			t.Fatalf("netip.ParsePrefix(%q) error = %v", c, err)
+		}
+
+		prefixes = append(prefixes, p)
+	}
+
+	return prefixes
+}
+
+type resolveCase struct {
+	name         string
+	trusted      []string
+	remoteAddr   string
+	forwardedFor []string
+	want         string
+}
+
+// runResolveCases runs each case against a resolver built from its trusted
+// list and checks the resolved address.
+func runResolveCases(t *testing.T, cases []resolveCase) {
+	t.Helper()
+
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			r := clientip.NewResolver(mustPrefixes(t, tt.trusted...))
+
+			got := r.Resolve(tt.remoteAddr, tt.forwardedFor)
+			if got != tt.want {
+				t.Errorf("Resolve(%q, %v) = %q, want %q",
+					tt.remoteAddr, tt.forwardedFor, got, tt.want)
+			}
+		})
+	}
+}
+
+// TestResolvePeerTrust covers the trust decision on the direct peer: a
+// forwarded header is believed only from a trusted peer, and a client
+// connecting directly cannot spoof its address.
+func TestResolvePeerTrust(t *testing.T) {
+	t.Parallel()
+
+	runResolveCases(t, []resolveCase{
+		{
+			name:         "trusted peer honors forwarded client",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   trustedPeer,
+			forwardedFor: []string{forwardedV4},
+			want:         forwardedV4,
+		},
+		{
+			name:         "untrusted peer ignores forwarded header",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   untrustedV4 + ":33333",
+			forwardedFor: []string{forwardedV4},
+			want:         untrustedV4,
+		},
+		{
+			name:         "spoofed chain from untrusted peer cannot influence result",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   untrustedV4 + ":33333",
+			forwardedFor: []string{"1.2.3.4, 10.9.9.9, 127.0.0.1"},
+			want:         untrustedV4,
+		},
+		{
+			name:         "empty trusted list always uses peer",
+			trusted:      nil,
+			remoteAddr:   forwardedV4 + ":80",
+			forwardedFor: []string{"10.0.0.5"},
+			want:         forwardedV4,
+		},
+		{
+			name:         "trusted peer with no forwarded header uses peer",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   trustedPeer,
+			forwardedFor: nil,
+			want:         "10.0.0.1",
+		},
+		{
+			name:         "unparseable peer is returned unchanged",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   "garbage",
+			forwardedFor: []string{forwardedV4},
+			want:         "garbage",
+		},
+	})
+}
+
+// TestResolveChainWalk covers walking the X-Forwarded-For chain from a
+// trusted peer to the rightmost entry that is not itself a trusted proxy.
+func TestResolveChainWalk(t *testing.T) {
+	t.Parallel()
+
+	runResolveCases(t, []resolveCase{
+		{
+			name:         "rightmost untrusted entry across a mixed chain",
+			trusted:      []string{trustedRangeV4, "192.168.0.0/16"},
+			remoteAddr:   trustedPeer,
+			forwardedFor: []string{forwardedV4 + ", 192.168.1.1, 10.0.0.2"},
+			want:         forwardedV4,
+		},
+		{
+			name:         "spoofed client behind a trusted proxy is not believed",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   trustedPeer,
+			forwardedFor: []string{"1.2.3.4, " + untrustedV4},
+			want:         untrustedV4,
+		},
+		{
+			name:         "chain split across multiple header lines",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   trustedPeer,
+			forwardedFor: []string{forwardedV4, "10.0.0.2"},
+			want:         forwardedV4,
+		},
+		{
+			name:         "garbage entries are skipped",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   trustedPeer,
+			forwardedFor: []string{forwardedV4 + ", not-an-ip"},
+			want:         forwardedV4,
+		},
+		{
+			name:         "all-trusted chain falls back to peer",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   trustedPeer,
+			forwardedFor: []string{"10.0.0.9, 10.0.0.2"},
+			want:         "10.0.0.1",
+		},
+		{
+			name:         "trusted IPv6 peer honors forwarded client",
+			trusted:      []string{"2001:db8::/32"},
+			remoteAddr:   "[2001:db8::1]:9000",
+			forwardedFor: []string{forwardedV4},
+			want:         forwardedV4,
+		},
+		{
+			name:         "IPv4-mapped peer matches IPv4 trusted range",
+			trusted:      []string{trustedRangeV4},
+			remoteAddr:   "[::ffff:10.0.0.1]:5000",
+			forwardedFor: []string{forwardedV4},
+			want:         forwardedV4,
+		},
+	})
+}
+
+func TestContextRoundTrip(t *testing.T) {
+	t.Parallel()
+
+	ctx := clientip.WithClientIP(t.Context(), forwardedV4)
+	if got := clientip.FromContext(ctx); got != forwardedV4 {
+		t.Errorf("FromContext = %q, want %q", got, forwardedV4)
+	}
+}
+
+func TestFromContextAbsent(t *testing.T) {
+	t.Parallel()
+
+	if got := clientip.FromContext(t.Context()); got != "" {
+		t.Errorf("FromContext with no value = %q, want empty", got)
+	}
+}

internal/config/config.go

@@ -44,6 +44,7 @@ const (
 	keyUpstreamConnectionsPerHost = "upstream_connections_per_host"
 	keyCacheMaxBytes              = "cache_max_bytes"
 	keyBlockedNetworks            = "blocked_networks"
+	keyTrustedProxies             = "trusted_proxies"
 )
 
 // placeholderSigningKey is the dummy signing_key shipped in
@@ -117,6 +118,17 @@ type Config struct {
 	// fetcher's dialer; the built-in ranges always apply.
 	BlockedNetworks []netip.Prefix
 
+	// TrustedProxies are the CIDR ranges of reverse proxies whose
+	// forwarding headers may be believed. Forwarded headers are honored
+	// only when the immediate peer falls inside one of these ranges;
+	// otherwise the peer address is used and the headers are ignored, so
+	// an untrusted client cannot spoof its address. An omitted key
+	// defaults to the RFC 1918 private ranges (see defaultTrustedProxies),
+	// since pixa is deployed behind a proxy on a private network; an
+	// explicitly empty list trusts nothing and always uses the peer
+	// address, and an explicit list replaces the default.
+	TrustedProxies []netip.Prefix
+
 	// CacheMaxBytes is the disk cache size limit in bytes. Zero
 	// disables the disk cache entirely. When cache_max_bytes is
 	// omitted from the configuration, this holds the computed default
@@ -185,11 +197,24 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) {
 		}
 	}
 
-	blockedNetworks, err := getBlockedNetworks(sc)
+	blockedNetworks, err := parseCIDRList(sc, keyBlockedNetworks)
 	if err != nil {
 		return nil, err
 	}
 
+	trustedProxies, err := parseCIDRList(sc, keyTrustedProxies)
+	if err != nil {
+		return nil, err
+	}
+
+	// parseCIDRList returns a nil slice only when the key is absent; an
+	// explicitly empty list ([]) comes back non-nil and empty. An omitted
+	// key takes the RFC 1918 default, while an explicit empty list is left
+	// as trust-nothing.
+	if trustedProxies == nil {
+		trustedProxies = defaultTrustedProxies()
+	}
+
 	loader := &strictLoader{sc: sc}
 
 	c := &Config{
@@ -207,6 +232,7 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) {
 			keyUpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost),
 		CacheMaxBytes:   loader.int64Val(keyCacheMaxBytes, 0),
 		BlockedNetworks: blockedNetworks,
+		TrustedProxies:  trustedProxies,
 	}
 
 	// The computed default for cache_max_bytes needs a validated
@@ -322,7 +348,8 @@ func isKnownConfigKey(key string) bool {
 	switch key {
 	case keyDebug, keyMaintenanceMode, keyPort, keyStateDir, keySentryDSN,
 		keyDBURL, keyMetrics, keySigningKey, keyAllowlistHosts, keyAllowHTTP,
-		keyUpstreamConnectionsPerHost, keyCacheMaxBytes, keyBlockedNetworks, "env":
+		keyUpstreamConnectionsPerHost, keyCacheMaxBytes, keyBlockedNetworks,
+		keyTrustedProxies, "env":
 		return true
 	}
 
@@ -817,27 +844,42 @@ func getStringSlice(sc *smartconfig.Config) []string {
 	return nil
 }
 
-// getBlockedNetworks parses the blocked_networks value into CIDR prefixes,
-// or returns nil if the key is omitted. It accepts a YAML list of strings
-// or a comma-separated string. An explicitly null value, a wrong type, an
-// empty entry, a non-string entry, or an unparseable CIDR aborts startup
-// naming the key and the offending value; a default (the built-in
-// blocklist alone) applies only to an omitted key.
-func getBlockedNetworks(sc *smartconfig.Config) ([]netip.Prefix, error) {
+// defaultTrustedProxies returns the trusted_proxies default: the three RFC
+// 1918 private ranges. pixa is always deployed behind a TLS-terminating
+// reverse proxy, which in practice sits on a private network, so its
+// forwarding headers are believed unless the operator says otherwise.
+// Loopback is deliberately excluded: it is not an RFC 1918 range, and no
+// deployment reaches pixa over it. A fresh slice is returned on each call so
+// callers may hold it without aliasing shared state.
+func defaultTrustedProxies() []netip.Prefix {
+	return []netip.Prefix{
+		netip.MustParsePrefix("10.0.0.0/8"),
+		netip.MustParsePrefix("172.16.0.0/12"),
+		netip.MustParsePrefix("192.168.0.0/16"),
+	}
+}
+
+// parseCIDRList parses the value of the named config key into CIDR
+// prefixes, or returns nil if the key is omitted. It accepts a YAML list
+// of strings or a comma-separated string. An explicitly null value, a
+// wrong type, an empty entry, a non-string entry, or an unparseable CIDR
+// aborts startup naming the key and the offending value; the default
+// (an empty list) applies only to an omitted key.
+func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) {
 	if sc == nil {
 		return nil, nil
 	}
 
-	raw, ok := sc.Get(keyBlockedNetworks)
+	raw, ok := sc.Get(key)
 	if !ok {
 		return nil, nil
 	}
 
 	if raw == nil {
-		return nil, errNullConfigValue(keyBlockedNetworks)
+		return nil, errNullConfigValue(key)
 	}
 
-	entries, err := blockedNetworkEntries(raw)
+	entries, err := cidrListEntries(raw, key)
 	if err != nil {
 		return nil, err
 	}
@@ -848,7 +890,7 @@ func getBlockedNetworks(sc *smartconfig.Config) ([]netip.Prefix, error) {
 		prefix, err := netip.ParsePrefix(entry)
 		if err != nil {
 			return nil, fmt.Errorf("config key %q: value %q is %w",
-				keyBlockedNetworks, entry, errNotAValidCIDR)
+				key, entry, errNotAValidCIDR)
 		}
 
 		prefixes = append(prefixes, prefix)
@@ -857,10 +899,10 @@ func getBlockedNetworks(sc *smartconfig.Config) ([]netip.Prefix, error) {
 	return prefixes, nil
 }
 
-// blockedNetworkEntries extracts the raw blocked_networks entries as
+// cidrListEntries extracts the raw entries of the named CIDR-list key as
 // trimmed, non-empty strings, from either a YAML list of strings or a
 // comma-separated string. Any other shape is a configuration error.
-func blockedNetworkEntries(raw any) ([]string, error) {
+func cidrListEntries(raw any, key string) ([]string, error) {
 	switch val := raw.(type) {
 	case []any:
 		entries := make([]string, 0, len(val))
@@ -869,12 +911,12 @@ func blockedNetworkEntries(raw any) ([]string, error) {
 			str, ok := item.(string)
 			if !ok {
 				return nil, fmt.Errorf("config key %q: list entry %v (%T) is %w",
-					keyBlockedNetworks, item, item, errNotAString)
+					key, item, item, errNotAString)
 			}
 
 			if strings.TrimSpace(str) == "" {
 				return nil, fmt.Errorf("config key %q: %w",
-					keyBlockedNetworks, errEmptyListEntry)
+					key, errEmptyListEntry)
 			}
 
 			entries = append(entries, strings.TrimSpace(str))
@@ -888,7 +930,7 @@ func blockedNetworkEntries(raw any) ([]string, error) {
 			trimmed := strings.TrimSpace(part)
 			if trimmed == "" {
 				return nil, fmt.Errorf("config key %q: value %q %w",
-					keyBlockedNetworks, val, errEmptyEntry)
+					key, val, errEmptyEntry)
 			}
 
 			entries = append(entries, trimmed)
@@ -897,6 +939,6 @@ func blockedNetworkEntries(raw any) ([]string, error) {
 		return entries, nil
 	default:
 		return nil, fmt.Errorf("config key %q: value %v (%T) is %w",
-			keyBlockedNetworks, raw, raw, errNotAStringList)
+			key, raw, raw, errNotAStringList)
 	}
 }

internal/config/trusted_proxies_internal_test.go

@@ -0,0 +1,85 @@
+package config
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestTrustedProxiesConfig checks the trusted_proxies key wiring: an
+// explicit CIDR list lands in TrustedProxies in order and replaces the
+// default, an omitted key falls back to the RFC 1918 private ranges, and an
+// explicitly empty list trusts no one. The list parser itself is shared with
+// blocked_networks and is exercised in depth by that key's tests.
+func TestTrustedProxiesConfig(t *testing.T) {
+	t.Parallel()
+
+	t.Run("explicit list replaces the default in order", func(t *testing.T) {
+		t.Parallel()
+
+		c, err := configFromYAML(t,
+			signingKeyLine+`trusted_proxies: ["10.0.0.0/8", "2001:db8::/32"]`+"\n")
+		if err != nil {
+			t.Fatalf("valid trusted_proxies should load: %v", err)
+		}
+
+		got := make([]string, len(c.TrustedProxies))
+		for i, p := range c.TrustedProxies {
+			got[i] = p.String()
+		}
+
+		if joined := strings.Join(got, ","); joined != "10.0.0.0/8,2001:db8::/32" {
+			t.Errorf("TrustedProxies = %v, want the two ranges in order", got)
+		}
+	})
+
+	t.Run("omitted key defaults to the RFC 1918 ranges", func(t *testing.T) {
+		t.Parallel()
+
+		c, err := configFromYAML(t, signingKeyLine)
+		if err != nil {
+			t.Fatalf("minimal config should load: %v", err)
+		}
+
+		got := make([]string, len(c.TrustedProxies))
+		for i, p := range c.TrustedProxies {
+			got[i] = p.String()
+		}
+
+		want := "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16"
+		if joined := strings.Join(got, ","); joined != want {
+			t.Errorf("TrustedProxies = %v, want the RFC 1918 ranges %q", got, want)
+		}
+	})
+
+	t.Run("explicitly empty list trusts no one", func(t *testing.T) {
+		t.Parallel()
+
+		c, err := configFromYAML(t, signingKeyLine+"trusted_proxies: []\n")
+		if err != nil {
+			t.Fatalf("empty trusted_proxies should load: %v", err)
+		}
+
+		if len(c.TrustedProxies) != 0 {
+			t.Errorf("TrustedProxies = %v, want empty", c.TrustedProxies)
+		}
+	})
+}
+
+// TestTrustedProxiesInvalidAbortsStartup checks that an invalid or null
+// value aborts startup with an error naming the key and the offending value.
+func TestTrustedProxiesInvalidAbortsStartup(t *testing.T) {
+	t.Parallel()
+
+	runAbortCases(t, []abortCase{
+		{
+			name:              "invalid cidr",
+			yaml:              signingKeyLine + `trusted_proxies: ["999.0.0.0/8"]` + "\n",
+			wantErrSubstrings: []string{keyTrustedProxies, "999.0.0.0/8"},
+		},
+		{
+			name:              "null value",
+			yaml:              signingKeyLine + "trusted_proxies:\n",
+			wantErrSubstrings: []string{keyTrustedProxies, nullValueText},
+		},
+	})
+}

internal/handlers/auth.go

@@ -8,6 +8,7 @@ import (
 	"strconv"
 	"time"
 
+	"sneak.berlin/go/pixa/internal/clientip"
 	"sneak.berlin/go/pixa/internal/encurl"
 	"sneak.berlin/go/pixa/internal/imgcache"
 	"sneak.berlin/go/pixa/internal/templates"
@@ -47,7 +48,8 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) {
 
 	// Constant-time comparison to prevent timing attacks
 	if subtle.ConstantTimeCompare([]byte(submittedKey), []byte(s.config.SigningKey)) != 1 {
-		s.log.Warn("failed login attempt", "remote_addr", r.RemoteAddr)
+		s.log.Warn("failed login attempt",
+			"remote_addr", clientip.FromContext(r.Context()))
 		s.renderLogin(w, r, "Invalid signing key")
 
 		return
@@ -62,7 +64,8 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	s.log.Info("successful login", "remote_addr", r.RemoteAddr)
+	s.log.Info("successful login",
+		"remote_addr", clientip.FromContext(r.Context()))
 
 	// Redirect to generator page
 	http.Redirect(w, r, "/", http.StatusSeeOther)

internal/handlers/auth_clientip_internal_test.go

@@ -0,0 +1,41 @@
+package handlers
+
+import (
+	"bytes"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"testing"
+
+	"sneak.berlin/go/pixa/internal/clientip"
+	"sneak.berlin/go/pixa/internal/config"
+)
+
+// TestFailedLoginLogsResolvedClientIP verifies the failed-login record
+// carries the resolved client IP from the request context, not the raw
+// proxy peer address.
+func TestFailedLoginLogsResolvedClientIP(t *testing.T) {
+	t.Parallel()
+
+	var buf bytes.Buffer
+
+	h := &Handlers{
+		log:    slog.New(slog.NewJSONHandler(&buf, nil)),
+		config: &config.Config{SigningKey: testSigningKey},
+	}
+
+	form := url.Values{loginKeyField: {"wrong-key"}}
+	req := httptest.NewRequestWithContext(
+		t.Context(), http.MethodPost, "/",
+		strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req = req.WithContext(clientip.WithClientIP(req.Context(), "203.0.113.7"))
+
+	h.handleLoginPost(httptest.NewRecorder(), req)
+
+	if !strings.Contains(buf.String(), `"remote_addr":"203.0.113.7"`) {
+		t.Errorf("failed-login log missing resolved client IP; got %q", buf.String())
+	}
+}

internal/middleware/client_ip_internal_test.go

@@ -0,0 +1,118 @@
+package middleware
+
+import (
+	"bytes"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"net/netip"
+	"strings"
+	"testing"
+
+	"sneak.berlin/go/pixa/internal/clientip"
+	"sneak.berlin/go/pixa/internal/config"
+)
+
+// testForwardedClient is the client address the proxy forwards.
+const testForwardedClient = "203.0.113.7"
+
+// newTestMiddleware builds a Middleware whose resolver trusts the given
+// CIDRs and whose logger writes JSON to buf.
+func newTestMiddleware(t *testing.T, buf *bytes.Buffer, trusted ...string) *Middleware {
+	t.Helper()
+
+	prefixes := make([]netip.Prefix, 0, len(trusted))
+
+	for _, c := range trusted {
+		p, err := netip.ParsePrefix(c)
+		if err != nil {
+			t.Fatalf("netip.ParsePrefix(%q) error = %v", c, err)
+		}
+
+		prefixes = append(prefixes, p)
+	}
+
+	return &Middleware{
+		log:      slog.New(slog.NewJSONHandler(buf, nil)),
+		config:   &config.Config{TrustedProxies: prefixes},
+		clientIP: clientip.NewResolver(prefixes),
+	}
+}
+
+// TestClientIPMiddlewareStoresResolvedIP verifies the ClientIP middleware
+// puts the resolved address into the request context for a trusted and an
+// untrusted peer.
+func TestClientIPMiddlewareStoresResolvedIP(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		remoteAddr string
+		forwarded  string
+		want       string
+	}{
+		{
+			name:       "trusted peer honors forwarded client",
+			remoteAddr: "10.0.0.1:5000",
+			forwarded:  testForwardedClient,
+			want:       testForwardedClient,
+		},
+		{
+			name:       "untrusted peer ignores forwarded header",
+			remoteAddr: "198.51.100.9:5000",
+			forwarded:  testForwardedClient,
+			want:       "198.51.100.9",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			mw := newTestMiddleware(t, &bytes.Buffer{}, "10.0.0.0/8")
+
+			var got string
+
+			handler := mw.ClientIP()(http.HandlerFunc(
+				func(_ http.ResponseWriter, r *http.Request) {
+					got = clientip.FromContext(r.Context())
+				}))
+
+			req := httptest.NewRequestWithContext(
+				t.Context(), http.MethodGet, "/", nil)
+			req.RemoteAddr = tt.remoteAddr
+			req.Header.Set("X-Forwarded-For", tt.forwarded)
+
+			handler.ServeHTTP(httptest.NewRecorder(), req)
+
+			if got != tt.want {
+				t.Errorf("client IP in context = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
+// TestLoggingUsesResolvedClientIP verifies the logging middleware records
+// the resolved forwarded client IP rather than the proxy peer address.
+func TestLoggingUsesResolvedClientIP(t *testing.T) {
+	t.Parallel()
+
+	var buf bytes.Buffer
+
+	mw := newTestMiddleware(t, &buf, "10.0.0.0/8")
+
+	handler := mw.ClientIP()(mw.Logging()(http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusOK)
+		})))
+
+	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
+	req.RemoteAddr = "10.0.0.1:5000"
+	req.Header.Set("X-Forwarded-For", testForwardedClient)
+
+	handler.ServeHTTP(httptest.NewRecorder(), req)
+
+	if !strings.Contains(buf.String(), `"remoteIP":"`+testForwardedClient+`"`) {
+		t.Errorf("log output missing resolved client IP; got %q", buf.String())
+	}
+}

internal/middleware/middleware.go

@@ -3,7 +3,6 @@ package middleware
 
 import (
 	"log/slog"
-	"net"
 	"net/http"
 	"time"
 
@@ -14,6 +13,7 @@ import (
 	ghmm "github.com/slok/go-http-metrics/middleware"
 	"github.com/slok/go-http-metrics/middleware/std"
 	"go.uber.org/fx"
+	"sneak.berlin/go/pixa/internal/clientip"
 	"sneak.berlin/go/pixa/internal/config"
 	"sneak.berlin/go/pixa/internal/logger"
 )
@@ -58,31 +58,34 @@ type Params struct {
 
 // Middleware provides HTTP middleware functions.
 type Middleware struct {
-	log    *slog.Logger
-	config *config.Config
+	log      *slog.Logger
+	config   *config.Config
+	clientIP *clientip.Resolver
 }
 
 // New creates a new Middleware instance.
 func New(_ fx.Lifecycle, params Params) (*Middleware, error) {
 	s := &Middleware{
-		log:    params.Logger.Get(),
-		config: params.Config,
+		log:      params.Logger.Get(),
+		config:   params.Config,
+		clientIP: clientip.NewResolver(params.Config.TrustedProxies),
 	}
 
 	return s, nil
 }
 
-func ipFromHostPort(hp string) string {
-	h, _, err := net.SplitHostPort(hp)
-	if err != nil {
-		return ""
+// ClientIP returns a middleware that resolves the real client IP,
+// honoring X-Forwarded-For only from trusted proxies, and stores it in
+// the request context for the logging middleware and handlers to read.
+func (s *Middleware) ClientIP() func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			ip := s.clientIP.Resolve(
+				r.RemoteAddr, r.Header.Values(clientip.ForwardedForHeader))
+			ctx := clientip.WithClientIP(r.Context(), ip)
+			next.ServeHTTP(w, r.WithContext(ctx))
+		})
 	}
-
-	if len(h) > 0 && h[0] == '[' {
-		return h[1 : len(h)-1]
-	}
-
-	return h
 }
 
 type loggingResponseWriter struct {
@@ -127,7 +130,7 @@ func (s *Middleware) Logging() func(http.Handler) http.Handler {
 					"request_id", reqID,
 					"referer", r.Referer(),
 					"proto", r.Proto,
-					"remoteIP", ipFromHostPort(r.RemoteAddr),
+					"remoteIP", clientip.FromContext(ctx),
 					"status", lrw.statusCode,
 					"response_bytes", lrw.bytesWritten,
 					"latency_ms", latency.Milliseconds(),

internal/server/routes.go

@@ -18,6 +18,7 @@ func (s *Server) SetupRoutes() {
 
 	s.router.Use(middleware.Recoverer)
 	s.router.Use(middleware.RequestID)
+	s.router.Use(s.mw.ClientIP())
 	s.router.Use(s.mw.SecurityHeaders())
 	s.router.Use(s.mw.Logging())