From e62b3c830d81659cf5d2687b0c8261da7d42fdab Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sun, 5 Jul 2026 23:55:37 +0100 Subject: [PATCH] fix(trusted-proxy): warn on catch-all, cover more cases, trim wording Follow-up to the review notes on #2116: - warn at startup when --trusted-proxy contains a catch-all (0.0.0.0/0 or ::/0), which trusts every peer and re-opens the bypass - mirrors the unset-case warning - realIPMiddleware tests: cover the unparseable-peer and trusted-peer-without-header branches, and make the observed values per-call so subtests don't share closure locals - trim the flag description and shorten the startup warning to the terse [WARN] style --- backend/app/cmd/server.go | 11 ++--- backend/app/rest/api/middleware.go | 11 +++++ backend/app/rest/api/middleware_test.go | 57 +++++++++++++++++-------- 3 files changed, 57 insertions(+), 22 deletions(-) diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index 6168c849..ae140f0e 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -85,7 +85,7 @@ type ServerCommand struct { Address string `long:"address" env:"REMARK_ADDRESS" default:"" description:"listening address"` WebRoot string `long:"web-root" env:"REMARK_WEB_ROOT" default:"./web" description:"web root directory"` UpdateLimit float64 `long:"update-limit" env:"UPDATE_LIMIT" default:"0.5" description:"updates/sec limit"` - TrustedProxies []string `long:"trusted-proxy" env:"TRUSTED_PROXY" description:"reverse-proxy networks (CIDR or IP) whose X-Real-IP/X-Forwarded-For headers set the client IP; if unset, these headers are trusted from any client (see docs)" env-delim:","` + TrustedProxies []string `long:"trusted-proxy" env:"TRUSTED_PROXY" description:"reverse-proxy networks (CIDR or IP) trusted to set the client IP; if unset, trusted from any client (see docs)" env-delim:","` RestrictedWords []string `long:"restricted-words" env:"RESTRICTED_WORDS" description:"words prohibited to use in comments" env-delim:","` RestrictedNames []string `long:"restricted-names" env:"RESTRICTED_NAMES" description:"names prohibited to use by user" env-delim:","` EnableEmoji bool `long:"emoji" env:"EMOJI" description:"enable emoji"` @@ -602,10 +602,11 @@ func (s *ServerCommand) newServerApp(ctx context.Context) (*serverApp, error) { if err != nil { return nil, fmt.Errorf("invalid --trusted-proxy: %w", err) } - if len(trustedProxies) == 0 { - log.Printf("[WARN] --trusted-proxy is not set: X-Real-IP/X-Forwarded-For/CF-Connecting-IP headers are trusted from " + - "any client, so a client can spoof its IP and bypass rate limiting and vote dedup. Behind a reverse proxy set " + - "--trusted-proxy to the proxy network (e.g. 172.16.0.0/12 for a proxy in the same compose); see the trusted-proxy docs.") + switch { + case len(trustedProxies) == 0: + log.Printf("[WARN] --trusted-proxy not set: forwarding headers are trusted from any client and can be spoofed to bypass rate limiting / vote dedup; set it behind a reverse proxy (see docs)") + case api.TrustsAnyPeer(trustedProxies): + log.Printf("[WARN] --trusted-proxy has a catch-all (0.0.0.0/0 or ::/0): forwarding headers are trusted from any client, re-opening the spoofing bypass; scope it to your proxy network") } storeEngine, err := s.makeDataStore() diff --git a/backend/app/rest/api/middleware.go b/backend/app/rest/api/middleware.go index c4a0c816..f86816c3 100644 --- a/backend/app/rest/api/middleware.go +++ b/backend/app/rest/api/middleware.go @@ -65,6 +65,17 @@ func directPeerIP(remoteAddr string) net.IP { return net.ParseIP(host) } +// TrustsAnyPeer reports whether the trusted-proxy list contains a catch-all (0.0.0.0/0 or ::/0), +// which trusts forwarding headers from every client and re-opens the IP-spoofing bypass. +func TrustsAnyPeer(cidrs []*net.IPNet) bool { + for _, c := range cidrs { + if ones, _ := c.Mask.Size(); ones == 0 { + return true + } + } + return false +} + // cidrsContain reports whether ip falls within any of the CIDRs. func cidrsContain(cidrs []*net.IPNet, ip net.IP) bool { for _, c := range cidrs { diff --git a/backend/app/rest/api/middleware_test.go b/backend/app/rest/api/middleware_test.go index cebe3257..57b064a7 100644 --- a/backend/app/rest/api/middleware_test.go +++ b/backend/app/rest/api/middleware_test.go @@ -51,38 +51,49 @@ func TestRouteTimeout(t *testing.T) { } func TestRealIPMiddleware(t *testing.T) { - var seenAddr, seenHdr string // what the downstream handler observes - next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - seenAddr, seenHdr = r.RemoteAddr, r.Header.Get("X-Real-IP") - }) - - call := func(mw func(http.Handler) http.Handler, remoteAddr, xRealIP string) { - seenAddr, seenHdr = "", "" + // call runs mw with the given peer and (optional) X-Real-IP header and returns what the + // downstream handler observes; state is per-call, so subtests don't share closure locals. + call := func(mw func(http.Handler) http.Handler, remoteAddr, xRealIP string) (addr, hdr string) { + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + addr, hdr = r.RemoteAddr, r.Header.Get("X-Real-IP") + }) req := httptest.NewRequest(http.MethodGet, "/", http.NoBody) req.RemoteAddr = remoteAddr - req.Header.Set("X-Real-IP", xRealIP) + if xRealIP != "" { + req.Header.Set("X-Real-IP", xRealIP) + } mw(next).ServeHTTP(httptest.NewRecorder(), req) + return addr, hdr } trusted, err := ParseTrustedProxies([]string{"172.16.0.0/12", "2001:db8::/32"}) require.NoError(t, err) t.Run("no trusted proxies trusts the header from anyone (legacy)", func(t *testing.T) { - call(realIPMiddleware(nil), "203.0.113.9:1234", "8.8.8.8") - assert.Equal(t, "8.8.8.8", seenAddr) + addr, _ := call(realIPMiddleware(nil), "203.0.113.9:1234", "8.8.8.8") + assert.Equal(t, "8.8.8.8", addr) }) t.Run("trusted v4 peer: forwarding header sets the client IP", func(t *testing.T) { - call(realIPMiddleware(trusted), "172.18.0.5:5555", "8.8.8.8") - assert.Equal(t, "8.8.8.8", seenAddr) + addr, _ := call(realIPMiddleware(trusted), "172.18.0.5:5555", "8.8.8.8") + assert.Equal(t, "8.8.8.8", addr) }) t.Run("trusted v6 peer: forwarding header honored", func(t *testing.T) { - call(realIPMiddleware(trusted), "[2001:db8::5]:5555", "8.8.8.8") - assert.Equal(t, "8.8.8.8", seenAddr) + addr, _ := call(realIPMiddleware(trusted), "[2001:db8::5]:5555", "8.8.8.8") + assert.Equal(t, "8.8.8.8", addr) + }) + t.Run("trusted peer without a forwarding header falls back to the socket IP", func(t *testing.T) { + addr, _ := call(realIPMiddleware(trusted), "172.18.0.5:5555", "") + assert.Equal(t, "172.18.0.5", addr, "no header to honor, so the bare socket IP is used") }) t.Run("untrusted peer: header stripped, RemoteAddr pinned to bare socket IP", func(t *testing.T) { - call(realIPMiddleware(trusted), "203.0.113.9:1234", "8.8.8.8") - assert.Equal(t, "203.0.113.9", seenAddr, "real socket IP with the port stripped") - assert.Empty(t, seenHdr, "spoofed forwarding header removed so nothing downstream can read it") + addr, hdr := call(realIPMiddleware(trusted), "203.0.113.9:1234", "8.8.8.8") + assert.Equal(t, "203.0.113.9", addr, "real socket IP with the port stripped") + assert.Empty(t, hdr, "spoofed forwarding header removed so nothing downstream can read it") + }) + t.Run("unparseable RemoteAddr is treated as untrusted, header stripped", func(t *testing.T) { + addr, hdr := call(realIPMiddleware(trusted), "garbage", "8.8.8.8") + assert.Equal(t, "garbage", addr, "unparseable peer left as-is, not overwritten") + assert.Empty(t, hdr, "forwarding header still stripped for a non-trusted peer") }) } @@ -116,6 +127,18 @@ func TestParseTrustedProxies(t *testing.T) { }) } +func TestTrustsAnyPeer(t *testing.T) { + catchAll := func(entries ...string) bool { + cidrs, err := ParseTrustedProxies(entries) + require.NoError(t, err) + return TrustsAnyPeer(cidrs) + } + assert.True(t, catchAll("10.0.0.0/8", "0.0.0.0/0"), "v4 catch-all") + assert.True(t, catchAll("::/0"), "v6 catch-all") + assert.False(t, catchAll("172.16.0.0/12", "10.0.0.5"), "scoped ranges are not catch-all") + assert.False(t, catchAll(), "empty is not catch-all") +} + func TestRest_rejectAnonUser(t *testing.T) { ts := httptest.NewServer(fakeAuth(rejectAnonUser(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { fmt.Fprintln(w, "Hello")