mirrors/remark42
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(trusted-proxy): warn on catch-all, cover more cases, trim wording

Browse Source 
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
This commit is contained in:
Dmitry Verkhoturov
2026-07-09 15:05:05 -05:00
committed by Umputun
parent b1502801fa
commit e62b3c830d
3 changed files with 57 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/app/cmd/server.go
+6 -5
View File
@@ -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()
backend/app/rest/api/middleware.go
+11
View File
@@ -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 {
backend/app/rest/api/middleware_test.go
+40 -17
View File
@@ -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")