From b99e1bdb27bbc99397f399fc4a3922fd545a6a15 Mon Sep 17 00:00:00 2001 From: fen Date: Thu, 17 Sep 2026 20:45:54 -0500 Subject: [PATCH] Fix #280 follow-up: remove PALETTE_TRUSTED_IP_HEADER Per owner decision the optional proxy-header escape hatch is dead config: remove the env var, its plumbing (Config.TrustedIPHeader, SetTrustedIPHeader), and the README row. Rate-limit keying is always the peer address; no client-supplied IP header is ever trusted. Tests updated to assert headers (CF-Connecting-IP included) never influence clientIP. --- README.md | 1 - internal/api/clientip.go | 36 +++--------------------------- internal/api/ratelimit_xff_test.go | 32 +++++++++++++------------- internal/api/server.go | 5 ----- 4 files changed, 18 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 9a57fb9..19707fc 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,6 @@ go build -o palette ./cmd/palette | `PALETTE_ADMIN_KEY` | generated | Admin key; if unset a 32-char hex key is generated and persisted to `/admin-key` (0600) | | `PALETTE_DEFAULT_DARK` | dark on | Default dark mode for new visitors. Set `false`, `0`, or `off` to default to light mode. Visitors who toggle dark mode keep their choice in their browser. | | `PALETTE_UNLOCK_SECRET` | random per start | HMAC secret for password-unlock cookies. Set a fixed value to keep unlock sessions across restarts or across replicas. | -| `PALETTE_TRUSTED_IP_HEADER` | unset | Name of a proxy-controlled client-IP header to key API rate limits on (e.g. `CF-Connecting-IP` when Cloudflare is the ingress; Cloudflare strips any client-supplied value). Unset: rate limits key on the peer address only, and all client-supplied IP headers (X-Forwarded-For, X-Real-Ip) are ignored. (#280) | An `/admin` page exists for runtime settings, protected by a key set at install (`PALETTE_ADMIN_KEY` env var) and resettable locally. See diff --git a/internal/api/clientip.go b/internal/api/clientip.go index 82ae13b..d0e3291 100644 --- a/internal/api/clientip.go +++ b/internal/api/clientip.go @@ -7,48 +7,18 @@ // per request and the limit was unenforceable (pentest H1: 6x201 across 8 // rotating-XFF creates). // -// Default: key on the actual peer address (RemoteAddr) only. Behind any +// The bucket key is always the actual peer address (RemoteAddr). Behind any // reverse proxy this is the proxy's address, so all clients share one bucket -// per endpoint — coarse, but safe. -// -// Proxy-honoring mode: a deployment in front of a proxy that OVERWRITES (not -// appends to) a client-IP header can set PALETTE_TRUSTED_IP_HEADER (e.g. -// CF-Connecting-IP when Cloudflare is the ingress; Cloudflare strips any -// client-supplied value). The header is honored ONLY when explicitly -// configured at startup, and X-Forwarded-For / X-Real-Ip are never trusted. +// per endpoint — coarse, but safe. Client-supplied IP headers +// (X-Forwarded-For, X-Real-Ip, and any others) are never trusted. package api import ( "net" "net/http" - "sync" ) -var ( - trustedIPMu sync.RWMutex - trustedIPHeader string // empty = never trust any client-IP header -) - -// SetTrustedIPHeader configures the single proxy-controlled header whose -// value may key rate-limit buckets. Called at startup; tests may reset it. -func SetTrustedIPHeader(name string) { - trustedIPMu.Lock() - defer trustedIPMu.Unlock() - trustedIPHeader = name -} - -func getTrustedIPHeader() string { - trustedIPMu.RLock() - defer trustedIPMu.RUnlock() - return trustedIPHeader -} - func clientIP(r *http.Request) string { - if name := getTrustedIPHeader(); name != "" { - if v := r.Header.Get(name); v != "" { - return v - } - } host := r.RemoteAddr if h, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { host = h diff --git a/internal/api/ratelimit_xff_test.go b/internal/api/ratelimit_xff_test.go index 680d5d0..75334da 100644 --- a/internal/api/ratelimit_xff_test.go +++ b/internal/api/ratelimit_xff_test.go @@ -7,8 +7,6 @@ import ( ) func TestClientIPUsesRemoteAddrNotXFF(t *testing.T) { - SetTrustedIPHeader("") - defer SetTrustedIPHeader("") r := httptest.NewRequest("POST", "/api/pastes", nil) r.RemoteAddr = "203.0.113.7:4432" r.Header.Set("X-Forwarded-For", "1.2.3.4, 1.2.3.5, 203.0.113.9") @@ -18,18 +16,20 @@ func TestClientIPUsesRemoteAddrNotXFF(t *testing.T) { } } -func TestClientIPTrustedHeaderOnlyWhenConfigured(t *testing.T) { - SetTrustedIPHeader("") - defer SetTrustedIPHeader("") - r := httptest.NewRequest("POST", "/api/pastes", nil) - r.RemoteAddr = "10.0.1.47:9999" - r.Header.Set("CF-Connecting-IP", "198.51.100.9") - if got := clientIP(r); got != "10.0.1.47" { - t.Fatalf("unconfigured: clientIP = %q, want peer 10.0.1.47", got) - } - SetTrustedIPHeader("CF-Connecting-IP") - if got := clientIP(r); got != "198.51.100.9" { - t.Fatalf("configured: clientIP = %q, want CF-Connecting-IP value", got) +// No client-controlled IP header is ever honored, including proxy-typical +// ones when set by an attacker. +func TestClientIPNeverTrustsHeaders(t *testing.T) { + for _, h := range []struct{ name, val string }{ + {"CF-Connecting-IP", "198.51.100.9"}, + {"X-Forwarded-For", "198.51.100.1"}, + {"X-Real-Ip", "198.51.100.2"}, + } { + r := httptest.NewRequest("POST", "/api/pastes", nil) + r.RemoteAddr = "10.0.1.47:9999" + r.Header.Set(h.name, h.val) + if got := clientIP(r); got != "10.0.1.47" { + t.Fatalf("%s header: clientIP = %q, want peer 10.0.1.47", h.name, got) + } } } @@ -37,9 +37,7 @@ func TestClientIPTrustedHeaderOnlyWhenConfigured(t *testing.T) { // repro was 8 creates with rotating XFF -> 6x201. func TestRotatingXFFDoesNotResetBucket(t *testing.T) { globalLimiter = newLimiter() - defer SetTrustedIPHeader("") - SetTrustedIPHeader("") - s := defaultSettings(Config{}) // burst/limit defaults; any header values are ignored anyway + s := defaultSettings(Config{}) var allowed, limited int for i := 0; i < 8; i++ { r := httptest.NewRequest("POST", "/api/pastes", nil) diff --git a/internal/api/server.go b/internal/api/server.go index b5d30d3..787e637 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -28,10 +28,6 @@ type Config struct { DBPath string MaxTextBytes int64 MaxItemBytes int64 - // TrustedIPHeader optionally names a proxy-controlled client-IP header - // (e.g. CF-Connecting-IP behind Cloudflare) to key rate limits on. Empty - // (default) keys on the peer address only. See clientip.go (#280). - TrustedIPHeader string } type apiServer struct { @@ -43,7 +39,6 @@ type apiServer struct { } func NewServer(st *store.Store, cfg Config, ui *web.UI, ss *settingsStore, adminKey string) *apiServer { - SetTrustedIPHeader(cfg.TrustedIPHeader) // #280 return &apiServer{store: st, cfg: cfg, ui: ui, settings: ss, adminKey: adminKey} }