fix(auth): warn once per peer on untrusted X-Forwarded-Host (#2865)

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>

apps/backend/internal/auth/httpmw/middleware.go

@@ -13,10 +13,13 @@
 package httpmw
 
 import (
+	"crypto/sha256"
 	"net"
 	"net/http"
 	"net/netip"
 	"strings"
+	"sync"
+	"unicode/utf8"
 
 	"github.com/gin-gonic/gin"
 	"go.uber.org/zap"
@@ -64,26 +67,122 @@ func Middleware(svc *auth.Service) gin.HandlerFunc {
 // its own port-scoped cookie name, by setting the header directly. The same
 // trusted list that gates X-Forwarded-For (gin's ClientIP via
 // SetTrustedProxies) gates this header so the two trust decisions cannot
-// diverge; an untrusted value is dropped (with a warning) and the resolver
-// falls back to the request Host. backendapp passes the list returned by its
+// diverge; an untrusted value is dropped (warned once per distinct peer and
+// host, see forwardedHostWarnSet) and the resolver falls back to the request
+// Host. backendapp passes the list returned by its
 // configureTrustedProxies, so the two uses share one configuration.
 func StripUntrustedForwardedHost(trusted []string, log *logger.Logger) gin.HandlerFunc {
 	matcher := newTrustedProxyMatcher(trusted)
+	warned := newForwardedHostWarnSet()
 	return func(c *gin.Context) {
-		if c.GetHeader("X-Forwarded-Host") == "" {
+		forwardedHost := c.GetHeader("X-Forwarded-Host")
+		if forwardedHost == "" {
 			return
 		}
 		peer := remoteAddrHost(c.Request.RemoteAddr)
 		if matcher.contains(peer) {
 			return
 		}
-		log.Warn("ignoring X-Forwarded-Host from untrusted peer",
-			zap.String("peer", peer),
-			zap.String("forwarded_host", c.GetHeader("X-Forwarded-Host")))
+		if warned.first(peer, forwardedHost) {
+			log.Warn("ignoring X-Forwarded-Host from untrusted peer",
+				zap.String("peer", peer),
+				zap.String("forwarded_host", truncateForLog(forwardedHost)),
+				zap.String("hint", forwardedHostWarnHint))
+		}
 		c.Request.Header.Del("X-Forwarded-Host")
 	}
 }
 
+// forwardedHostWarnHint tells the operator how to fix the common cause: a real
+// reverse proxy missing from the trusted list. The warning is a configuration
+// signal, not a per-request event, so the fix travels with it.
+const forwardedHostWarnHint = "set KANDEV_TRUSTED_PROXIES to this peer's IP or CIDR if it is your reverse proxy"
+
+// forwardedHostWarnLimit bounds the distinct (peer, forwarded host) pairs
+// remembered for warn deduplication. A misconfigured proxy produces one pair;
+// the cap keeps a client that varies either value from growing the set without
+// bound, at the cost of only losing warnings once it is reached.
+const forwardedHostWarnLimit = 64
+
+// forwardedHostLogMaxLen caps the forwarded-host bytes written to the log. The
+// header is unauthenticated and may run to the server's whole header budget
+// (backendapp leaves MaxHeaderBytes at net/http's 1 MiB default), and dumping a
+// megabyte per line would recreate, in one line, the flooding this
+// deduplication exists to stop. The prefix is enough to recognize a hostname.
+const forwardedHostLogMaxLen = 256
+
+// truncateForLog shortens an attacker-controlled value to a bounded, readable
+// prefix, cutting on a rune boundary so the log field stays valid UTF-8.
+func truncateForLog(value string) string {
+	if len(value) <= forwardedHostLogMaxLen {
+		return value
+	}
+	cut := forwardedHostLogMaxLen
+	for cut > 0 && !utf8.RuneStart(value[cut]) {
+		cut--
+	}
+	return value[:cut] + "...(truncated)"
+}
+
+// forwardedHostWarnSet deduplicates the strip warning. The stripped header is
+// a static property of the deployment (an untrusted proxy keeps forwarding on
+// every request), so warning per request buries the rest of the log in
+// thousands of identical lines while adding nothing after the first. One
+// warning per distinct (peer, forwarded host) pair keeps the operator signal
+// and still surfaces a genuinely new peer or host.
+//
+// Keys are SHA-256 digests rather than the pair itself: the forwarded host is
+// unauthenticated and can approach the server's whole header budget, so
+// retaining the raw values would let 64 oversized requests pin tens of MiB for
+// the process lifetime despite the entry cap. A fixed-size digest bounds
+// retention at forwardedHostWarnLimit * 32 bytes whatever the header size.
+type forwardedHostWarnSet struct {
+	mu   sync.RWMutex
+	seen map[[sha256.Size]byte]struct{}
+}
+
+func newForwardedHostWarnSet() *forwardedHostWarnSet {
+	return &forwardedHostWarnSet{seen: make(map[[sha256.Size]byte]struct{})}
+}
+
+// forwardedHostWarnKey digests a (peer, forwarded host) pair into a fixed-size
+// dedup key. The NUL separator cannot appear in either value, so no pair can
+// forge another pair's digest by moving the boundary.
+func forwardedHostWarnKey(peer, forwardedHost string) [sha256.Size]byte {
+	digest := sha256.New()
+	digest.Write([]byte(peer))
+	digest.Write([]byte{0})
+	digest.Write([]byte(forwardedHost))
+	var key [sha256.Size]byte
+	copy(key[:], digest.Sum(nil))
+	return key
+}
+
+// first reports whether this (peer, forwarded host) pair has not been warned
+// about yet, recording it when so. It returns false once the pair is known or
+// the set is full.
+func (s *forwardedHostWarnSet) first(peer, forwardedHost string) bool {
+	key := forwardedHostWarnKey(peer, forwardedHost)
+	s.mu.RLock()
+	_, seen := s.seen[key]
+	atLimit := len(s.seen) >= forwardedHostWarnLimit
+	s.mu.RUnlock()
+	if seen || atLimit {
+		return false
+	}
+
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	if _, ok := s.seen[key]; ok {
+		return false
+	}
+	if len(s.seen) >= forwardedHostWarnLimit {
+		return false
+	}
+	s.seen[key] = struct{}{}
+	return true
+}
+
 // trustedProxyMatcher matches a peer IP against the trusted-proxy list (bare
 // IPs and CIDRs, the same entries configureTrustedProxies handed to gin).
 type trustedProxyMatcher struct {

apps/backend/internal/auth/httpmw/middleware_test.go

@@ -2,10 +2,14 @@ package httpmw
 
 import (
 	"context"
+	"crypto/sha256"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"sync"
 	"testing"
+	"unicode/utf8"
 
 	"github.com/gin-gonic/gin"
 	"github.com/jmoiron/sqlx"
@@ -525,4 +529,216 @@ func TestStripUntrustedForwardedHostWarns(t *testing.T) {
 	if !warned {
 		t.Fatal("no X-Forwarded-Host warning was logged")
 	}
+	assertWarningHints(t, logs)
+}
+
+func assertWarningHints(t *testing.T, logs *observer.ObservedLogs) {
+	t.Helper()
+	for i, entry := range logs.All() {
+		hint := ""
+		for _, field := range entry.Context {
+			if field.Key == "hint" {
+				hint = field.String
+				break
+			}
+		}
+		if hint != forwardedHostWarnHint {
+			t.Fatalf("warning %d hint = %q, want %q", i, hint, forwardedHostWarnHint)
+		}
+	}
+}
+
+// stripWarnRouter builds a strip-middleware router over an observed logger and
+// returns both, so a test can count the warnings a request sequence produces.
+func stripWarnRouter(t *testing.T) (*gin.Engine, *observer.ObservedLogs) {
+	t.Helper()
+	core, logs := observer.New(zapcore.WarnLevel)
+	log, err := logger.NewFromZap(zap.New(core))
+	if err != nil {
+		t.Fatalf("observer logger: %v", err)
+	}
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+	router.Use(StripUntrustedForwardedHost(nil, log))
+	router.GET("/", echoXFH)
+	return router, logs
+}
+
+// serveStrip sends one request with the given peer and X-Forwarded-Host,
+// asserting the header was stripped so warn counting never masks a behavior
+// regression.
+func serveStrip(t *testing.T, router *gin.Engine, peer, forwardedHost string) {
+	t.Helper()
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.RemoteAddr = peer
+	req.Header.Set("X-Forwarded-Host", forwardedHost)
+	rec := httptest.NewRecorder()
+	router.ServeHTTP(rec, req)
+	if got := rec.Body.String(); got != "stripped" {
+		t.Fatalf("peer %s host %s: body = %q, want stripped", peer, forwardedHost, got)
+	}
+}
+
+// TestStripUntrustedForwardedHostWarnsOncePerPeer pins the deduplication: an
+// untrusted proxy forwards on every request, so warning per request buries the
+// log in thousands of identical lines. The first request warns; repeats from
+// the same peer and host stay silent, and the header is still stripped.
+func TestStripUntrustedForwardedHostWarnsOncePerPeer(t *testing.T) {
+	router, logs := stripWarnRouter(t)
+	for range 50 {
+		serveStrip(t, router, "203.0.113.9:5555", "public.example:8443")
+	}
+	if got := logs.Len(); got != 1 {
+		t.Fatalf("warnings for 50 identical requests = %d, want 1", got)
+	}
+	assertWarningHints(t, logs)
+}
+
+// TestStripUntrustedForwardedHostWarnsPerDistinctPair pins the other half: a
+// new peer, or a known peer presenting a new forwarded host, is genuinely new
+// information and warns again.
+func TestStripUntrustedForwardedHostWarnsPerDistinctPair(t *testing.T) {
+	router, logs := stripWarnRouter(t)
+	serveStrip(t, router, "203.0.113.9:5555", "public.example:8443")
+	serveStrip(t, router, "203.0.113.9:5555", "public.example:8443") // repeat: silent
+	serveStrip(t, router, "198.51.100.4:5555", "public.example:8443")
+	serveStrip(t, router, "203.0.113.9:5555", "other.example:8443")
+	if got := logs.Len(); got != 3 {
+		t.Fatalf("warnings for 3 distinct pairs = %d, want 3", got)
+	}
+	assertWarningHints(t, logs)
+}
+
+func TestForwardedHostWarnSetFirstIsAtMostOnce(t *testing.T) {
+	set := newForwardedHostWarnSet()
+	const calls = 100
+	results := make(chan bool, calls)
+	var wg sync.WaitGroup
+	for range calls {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			results <- set.first("203.0.113.9", "public.example:8443")
+		}()
+	}
+	wg.Wait()
+	close(results)
+
+	first := 0
+	for result := range results {
+		if result {
+			first++
+		}
+	}
+	if first != 1 {
+		t.Fatalf("first returned true %d times, want 1", first)
+	}
+}
+
+// TestStripUntrustedForwardedHostWarnSetIsBounded pins the memory bound: a
+// client that varies the forwarded host cannot grow the dedup set without
+// limit. Warnings stop at the cap; stripping does not.
+func TestStripUntrustedForwardedHostWarnSetIsBounded(t *testing.T) {
+	router, logs := stripWarnRouter(t)
+	for i := range forwardedHostWarnLimit * 2 {
+		serveStrip(t, router, "203.0.113.9:5555", fmt.Sprintf("host-%d.example:8443", i))
+	}
+	if got := logs.Len(); got != forwardedHostWarnLimit {
+		t.Fatalf("warnings for %d distinct hosts = %d, want %d (cap)",
+			forwardedHostWarnLimit*2, got, forwardedHostWarnLimit)
+	}
+}
+
+// oversizedHost builds a forwarded host near net/http's default 1 MiB header
+// budget, which backendapp leaves unset: the largest value an unauthenticated
+// client can actually get past the server.
+func oversizedHost(seed int) string {
+	return fmt.Sprintf("%d.", seed) + strings.Repeat("a", 1<<20-16) + ".example:8443"
+}
+
+// TestStripUntrustedForwardedHostRetainsFixedSizeKeys pins the memory bound in
+// bytes, not just in entries: the forwarded host is unauthenticated and can
+// approach the whole header budget, so retaining raw pair values would let 64
+// oversized requests pin tens of MiB for the process lifetime despite the entry
+// cap. Keys are fixed-size digests, so retention does not scale with header
+// size, and distinct oversized hosts still dedup independently.
+func TestStripUntrustedForwardedHostRetainsFixedSizeKeys(t *testing.T) {
+	set := newForwardedHostWarnSet()
+	for i := range forwardedHostWarnLimit {
+		host := oversizedHost(i)
+		if !set.first("203.0.113.9", host) {
+			t.Fatalf("first oversized host %d: got false, want true", i)
+		}
+		if set.first("203.0.113.9", host) {
+			t.Fatalf("repeat oversized host %d: got true, want false", i)
+		}
+	}
+	retained := 0
+	for key := range set.seen {
+		retained += len(key)
+	}
+	if want := forwardedHostWarnLimit * sha256.Size; retained != want {
+		t.Fatalf("retained key bytes = %d, want %d (fixed-size digests)", retained, want)
+	}
+}
+
+// TestStripUntrustedForwardedHostDistinguishesLongSharedPrefixes pins what the
+// digest buys over simply retaining a fixed-size prefix of the pair: hosts that
+// differ only past the key width must still be told apart. A prefix key would
+// collide here and silently swallow the second host's warning, which is the one
+// an operator most needs when a proxy starts forwarding something new.
+func TestStripUntrustedForwardedHostDistinguishesLongSharedPrefixes(t *testing.T) {
+	shared := strings.Repeat("a", 1<<20-64)
+	router, logs := stripWarnRouter(t)
+	serveStrip(t, router, "203.0.113.9:5555", shared+"-one.example:8443")
+	serveStrip(t, router, "203.0.113.9:5555", shared+"-two.example:8443")
+	if got := logs.Len(); got != 2 {
+		t.Fatalf("warnings for 2 hosts sharing a %d-byte prefix = %d, want 2", len(shared), got)
+	}
+}
+
+// TestStripUntrustedForwardedHostTruncatesLoggedHost pins the other half of the
+// same bound: an oversized header must not be echoed into the log verbatim,
+// which would recreate in one line the flooding this deduplication exists to
+// stop. The value is still stripped and still warned about once.
+func TestStripUntrustedForwardedHostTruncatesLoggedHost(t *testing.T) {
+	router, logs := stripWarnRouter(t)
+	host := oversizedHost(0)
+	serveStrip(t, router, "203.0.113.9:5555", host)
+	serveStrip(t, router, "203.0.113.9:5555", host)
+	if got := logs.Len(); got != 1 {
+		t.Fatalf("warnings for repeated oversized host = %d, want 1", got)
+	}
+	logged := ""
+	for _, field := range logs.All()[0].Context {
+		if field.Key == "forwarded_host" {
+			logged = field.String
+		}
+	}
+	if want := forwardedHostLogMaxLen + len("...(truncated)"); len(logged) > want {
+		t.Fatalf("logged forwarded_host = %d bytes, want <= %d", len(logged), want)
+	}
+	if !strings.HasSuffix(logged, "...(truncated)") {
+		t.Fatalf("logged forwarded_host = %q, want a truncation marker", logged)
+	}
+	if !strings.HasPrefix(host, strings.TrimSuffix(logged, "...(truncated)")) {
+		t.Fatalf("logged forwarded_host %q is not a prefix of the header", logged)
+	}
+}
+
+// TestTruncateForLogPreservesShortValuesAndRuneBoundaries pins the two edges of
+// the truncation helper: a normal hostname is logged verbatim, and an oversized
+// multi-byte value is cut on a rune boundary so the log field stays valid UTF-8.
+func TestTruncateForLogPreservesShortValuesAndRuneBoundaries(t *testing.T) {
+	if got := truncateForLog("public.example:8443"); got != "public.example:8443" {
+		t.Fatalf("short value = %q, want it unchanged", got)
+	}
+	// "é" is 2 bytes, so the cap lands mid-rune for at least one repeat count.
+	for _, extra := range []int{0, 1} {
+		value := strings.Repeat("é", forwardedHostLogMaxLen/2+extra+8)
+		got := truncateForLog(value)
+		if !utf8.ValidString(got) {
+			t.Fatalf("truncated multi-byte value is not valid UTF-8: %q", got)
+		}
+	}
 }

docs/public/configuration.md

@@ -184,6 +184,14 @@ with a warning naming the bad value, and the whole variable is ignored (fail
 closed: no partial trust). A trailing or doubled comma is treated the same
 way. The backend never crashes on a bad value.
 
+The same list gates `X-Forwarded-Host`, which a proxy sends so the port-scoped
+session cookie can be resolved from the browser's original host. A proxy that
+sends it while missing from the list logs
+`ignoring X-Forwarded-Host from untrusted peer` with the peer address and the
+forwarded host. That warning is logged once per distinct peer and host, not per
+request, so it names the misconfiguration without flooding the log. Add the
+peer it names to `KANDEV_TRUSTED_PROXIES` if it is your reverse proxy.
+
 The variable is read once at startup and must reach the backend process: set
 it in the environment of the process that launches kandev, for example a
 systemd drop-in for `kandev.service` (`systemctl --user edit