From 7697822c53cfe8c2119d312ca4c2f833ad7a2ba1 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 23:23:47 +0000 Subject: [PATCH 1/6] test: add failing config tests for trusted_proxies Check that a valid CIDR list lands in TrustedProxies in order, an omitted key trusts no one, and an invalid or null value aborts startup naming the key and value. The list parser is shared with blocked_networks, whose tests exercise the remaining shapes. Model: opus-4-8 --- .../config/trusted_proxies_internal_test.go | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 internal/config/trusted_proxies_internal_test.go diff --git a/internal/config/trusted_proxies_internal_test.go b/internal/config/trusted_proxies_internal_test.go new file mode 100644 index 0000000..b2593c0 --- /dev/null +++ b/internal/config/trusted_proxies_internal_test.go @@ -0,0 +1,65 @@ +package config + +import ( + "strings" + "testing" +) + +// TestTrustedProxiesConfig checks the trusted_proxies key wiring: a valid +// CIDR list lands in TrustedProxies in order, and an omitted key 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("valid list is parsed 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 trusts no one", func(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine) + if err != nil { + t.Fatalf("minimal config 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}, + }, + }) +} -- 2.54.0 From c5f4682b0b2063420abb110e47e9c981fa9f3cf5 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 23:23:47 +0000 Subject: [PATCH 2/6] feat: add trusted_proxies config key Add a trusted_proxies CIDR-list config key alongside blocked_networks. Generalize the blocked_networks parser into parseCIDRList and cidrListEntries, which take the key name as a parameter, so both keys share one parser rather than a second copy. An invalid entry aborts startup naming the key and value; an omitted or empty key leaves the list empty. Model: opus-4-8 --- internal/config/config.go | 54 +++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 19 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index c8c1d60..324f026 100644 --- a/internal/config/config.go +++ b/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,14 @@ 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. Empty means trust + // nothing and always use the peer address. + 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,7 +194,12 @@ 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 } @@ -207,6 +221,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 +337,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 +833,27 @@ 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) { +// 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 +864,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 +873,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 +885,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 +904,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 +913,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) } } -- 2.54.0 From 4f14cd86c3fb86455eb0ac2d425adb44980749a5 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 23:23:47 +0000 Subject: [PATCH 3/6] feat: add trusted-proxy-aware client IP resolver New internal/clientip package. Resolve honors X-Forwarded-For only when the direct peer falls inside a configured trusted-proxy range; otherwise it returns the peer address and ignores the header, so a client connecting directly cannot spoof its address. When the peer is trusted, the chain is walked right to left and the first entry that is not itself a trusted proxy is returned. Also provides request-context helpers so the resolved address can be shared across middleware and handlers. Tests cover the honored, ignored, and spoofed-chain cases, rightmost selection across a mixed chain, multi-line headers, IPv6 and IPv4-mapped peers, and the context round-trip. Model: opus-4-8 --- internal/clientip/clientip.go | 119 ++++++++++++++++++ internal/clientip/clientip_test.go | 189 +++++++++++++++++++++++++++++ 2 files changed, 308 insertions(+) create mode 100644 internal/clientip/clientip.go create mode 100644 internal/clientip/clientip_test.go diff --git a/internal/clientip/clientip.go b/internal/clientip/clientip.go new file mode 100644 index 0000000..49c2ea7 --- /dev/null +++ b/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 +} diff --git a/internal/clientip/clientip_test.go b/internal/clientip/clientip_test.go new file mode 100644 index 0000000..ef3a839 --- /dev/null +++ b/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) + } +} -- 2.54.0 From a280fd2c345d449d44c9ee59e550a8ae590d743b Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 23:23:55 +0000 Subject: [PATCH 4/6] test: cover resolved client IP in request and auth logs Assert the ClientIP middleware stores the resolved address in the request context for trusted and untrusted peers, that the request log records the forwarded client rather than the proxy peer, and that a failed-login record carries the resolved address. Model: opus-4-8 --- .../handlers/auth_clientip_internal_test.go | 41 ++++++ .../middleware/client_ip_internal_test.go | 118 ++++++++++++++++++ 2 files changed, 159 insertions(+) create mode 100644 internal/handlers/auth_clientip_internal_test.go create mode 100644 internal/middleware/client_ip_internal_test.go diff --git a/internal/handlers/auth_clientip_internal_test.go b/internal/handlers/auth_clientip_internal_test.go new file mode 100644 index 0000000..91d3fd2 --- /dev/null +++ b/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()) + } +} diff --git a/internal/middleware/client_ip_internal_test.go b/internal/middleware/client_ip_internal_test.go new file mode 100644 index 0000000..7200f46 --- /dev/null +++ b/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()) + } +} -- 2.54.0 From 20a18c734563f1b1a89c47ecce7795caab874ce5 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 23:23:56 +0000 Subject: [PATCH 5/6] feat: use resolved client IP in request and auth logs Add a ClientIP middleware that resolves the client address once per request and stores it in the context, placed ahead of logging in the route chain. The request-logging middleware and both login-attempt logs now read the resolved address instead of the raw peer. Behind a trusted proxy these records show the real client; a direct, untrusted client still shows its own address and cannot forge one. Removes the local host:port helper now that the clientip package owns that parsing. Model: opus-4-8 --- internal/handlers/auth.go | 7 +++++-- internal/middleware/middleware.go | 35 +++++++++++++++++-------------- internal/server/routes.go | 1 + 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 5e74e1f..5ea2a32 100644 --- a/internal/handlers/auth.go +++ b/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) diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 00d59f7..916e3c8 100644 --- a/internal/middleware/middleware.go +++ b/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(), diff --git a/internal/server/routes.go b/internal/server/routes.go index 3b7dfc5..8e41fd7 100644 --- a/internal/server/routes.go +++ b/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()) -- 2.54.0 From 518e84ce67ef01dd37e72e8beb74e3fb6e6999e0 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 23:23:56 +0000 Subject: [PATCH 6/6] docs: document trusted_proxies and record in TODO.md Describe the trusted_proxies key in README.md and config.example.yml, including the rightmost-non-proxy selection rule and the safe default of trusting no one. Record the unit in TODO.md. Model: opus-4-8 --- README.md | 9 +++++++++ TODO.md | 11 +++++++++++ config.example.yml | 12 ++++++++++++ 3 files changed, 32 insertions(+) diff --git a/README.md b/README.md index ac27ec4..bdc97f4 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,15 @@ Configured via YAML file (`--config`). Key settings: added to the always-enforced built-in ranges (loopback, private, link-local, CGNAT, benchmark, NAT64, and the like); an invalid CIDR aborts startup +- `trusted_proxies` — list of CIDR ranges of the reverse proxies in front + of pixa. `X-Forwarded-For` is believed only when the direct peer falls + inside one of these ranges; the logged and login-recorded client + address is then the rightmost forwarded entry that is not itself a + trusted proxy. Otherwise the direct peer address is used and the header + is ignored, so a client connecting directly cannot spoof its address. + Omitted or empty trusts no one; an invalid CIDR aborts startup. Set + this to your proxy's address range when deploying behind a reverse + proxy - `upstream_fetch_timeout` — timeout for origin requests - `upstream_max_response_size` — max origin response size - `downstream_timeout` — client response timeout diff --git a/TODO.md b/TODO.md index c345355..fa231ad 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,17 @@ exhaustion # Completed Steps +- 2026-09-21 trusted-proxy client IP resolution (closes #94): a + `trusted_proxies` config key taking a list of CIDRs, parsed by the same + `net/netip` list parser as `blocked_networks` (an invalid entry aborts + startup naming the key and value; omitted or empty trusts no one); a new + `internal/clientip` package resolves the client address by honoring + `X-Forwarded-For` only when the direct peer is a trusted proxy, walking + the chain right-to-left to the rightmost non-proxy entry, so a client + connecting directly cannot spoof its address; the resolved address is + stored in the request context by a new middleware and used by the + request-logging middleware and the login-attempt logs in place of the + raw peer address; documented in `README.md` and `config.example.yml`. - 2026-09-21 blocked networks configuration extending SSRF protection: a `blocked_networks` config key taking a list of CIDRs (parsed with `net/netip`, an invalid entry aborts startup naming the key and value), diff --git a/config.example.yml b/config.example.yml index e3fbd4a..fd41c86 100644 --- a/config.example.yml +++ b/config.example.yml @@ -31,6 +31,18 @@ allowlist_hosts: # - 100.64.0.0/10 # - 2001:db8::/32 +# CIDR ranges of the reverse proxies in front of pixa. X-Forwarded-For +# is believed only when the direct peer is inside one of these ranges; +# the client address in the access log and login records is then the +# rightmost forwarded entry that is not itself a trusted proxy. A client +# connecting directly (peer outside these ranges) cannot spoof its +# address: the header is ignored and the peer address is used. Omitted or +# empty trusts no one; an invalid CIDR aborts startup. Set this when +# deploying behind a proxy. +# trusted_proxies: +# - 10.0.0.0/8 +# - 2001:db8::/32 + # Allow HTTP upstream (only for testing, always use HTTPS in production) allow_http: false -- 2.54.0