From 2a47fb9a8e8aaba0ffc8153994930890cb82d52d Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 14 Oct 2026 14:01:37 +0200 Subject: [PATCH] Unset TRUSTED_PROXIES trusts RFC 1918, empty trusts none (closes #129) The backend's own TRUSTED_PROXIES now follows the ruling for every app: unset, it trusts 10.0.0.0/8, 172.16.0.0/12 and 192.168.0.0/16, and no longer loopback; a list replaces that; set and empty, it trusts no proxy. viper reads an empty environment variable as unset, so an empty value got the default. config.New now reads this one setting with os.LookupEnv first. viper's AllowEmptyEnv would apply to every setting, so an empty PORT or DEBUG would stop the start instead of counting as unset, as README.md says it does. bin/entrypoint.sh still starts the backend with TRUSTED_PROXIES=127.0.0.1/32. Model: opus-5-5 --- README.md | 18 ++++++----- TODO.md | 8 +++++ internal/config/config.go | 19 +++++++---- internal/config/config_test.go | 58 ++++++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 320b0f7..7956481 100644 --- a/README.md +++ b/README.md @@ -199,23 +199,25 @@ image, `make run` builds it and runs it on port 8080. | `DATA_DIR` | `./data/reports` | Directory for compressed reports | | `DATA_DIR_MAX_BYTES` | `1073741824` (1 GiB) | Most bytes of report files kept in `DATA_DIR`, oldest deleted first; see [Report limits](#report-limits) | | `DEBUG` | `false` | Enable debug logging | -| `TRUSTED_PROXIES` | loopback + RFC1918 | Comma-separated CIDRs whose `X-Forwarded-For` / `X-Real-IP` headers are trusted for client IP resolution | +| `TRUSTED_PROXIES` | RFC 1918 ranges | Comma-separated CIDRs whose `X-Forwarded-For` / `X-Real-IP` headers are trusted for client IP resolution | | `REPORTS_PER_MINUTE` | `60` | Reports each client address may send a minute; see [Report limits](#report-limits) | | `CORS_ALLOWED_ORIGINS` | empty | Comma-separated origins whose pages may call the API; see [CORS](#cors) | | `METRICS_USERNAME` | empty | Basic auth user name for `/metrics`; see [Metrics](#metrics) | | `METRICS_PASSWORD` | empty | Basic auth password for `/metrics`; see [Metrics](#metrics) | | `SENTRY_DSN` | empty | DSN of the Sentry project to send errors to; see [Sentry](#sentry) | -`TRUSTED_PROXIES` defaults to -`127.0.0.1/32,::1/128,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16`. The loopback -entries cover a reverse proxy on the same host. A request whose direct peer is -outside this set has its forwarded headers ignored, and the direct peer is -logged and rate-limited instead. The container image does not use this default; -see [Deployment](#deployment). +`TRUSTED_PROXIES` defaults to `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16`. A list +replaces the default; set and empty, the server trusts no proxy. Loopback is not +in the default, so a reverse proxy on the same host has to be named, as in +`TRUSTED_PROXIES=127.0.0.1/32`. A request whose direct peer is outside this set +has its forwarded headers ignored, and the direct peer is logged and +rate-limited instead. The container image does not use this default; see +[Deployment](#deployment). A variable set to a value the server cannot use, such as `PORT=abc`, `DEBUG=maybe` or a `BIND_ADDRESS` that is not an IP address, stops it from -starting, with an error naming the variable. An empty variable counts as unset. +starting, with an error naming the variable. An empty variable counts as unset, +except `TRUSTED_PROXIES`. #### Report storage diff --git a/TODO.md b/TODO.md index 158bfd6..de845ba 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,14 @@ Run `make frontend-viewport-test` in CI as its own step; it is not part of # Completed Steps +- 2026-10-14: unset, the backend's own `TRUSTED_PROXIES` trusts `10.0.0.0/8`, + `172.16.0.0/12` and `192.168.0.0/16`, and no longer loopback; a list replaces + that; set and empty, it trusts no proxy, where it used to get the default, as + viper reads an empty environment variable as unset + ([#129](https://git.eeqj.de/sneak/netwatch/issues/129)). A value it cannot + parse still stops the start, and the image still starts the backend with + `TRUSTED_PROXIES=127.0.0.1/32`. Tests load the config with it unset, empty and + set to a list - 2026-10-14: while report files cannot be written, `POST /api/v1/reports` answers 500, and storing recovers once writing works again, with no restart ([#123](https://git.eeqj.de/sneak/netwatch/issues/123)). A failed write puts diff --git a/internal/config/config.go b/internal/config/config.go index f92d0f1..69cca58 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -9,6 +9,7 @@ import ( "math" "net/netip" "net/url" + "os" "strconv" "strings" @@ -21,13 +22,11 @@ import ( ) // defaultTrustedProxies lists the networks whose forwarded -// headers are honoured by default: IPv4 and IPv6 loopback, -// for a reverse proxy on the same host, and the RFC1918 -// ranges. The container image does not use it: +// headers are honoured when TRUSTED_PROXIES is unset: the +// RFC 1918 ranges. The container image does not use it: // bin/entrypoint.sh gives the server 127.0.0.1/32, since // nginx is its only client there. -const defaultTrustedProxies = "127.0.0.1/32,::1/128," + - "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" +const defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" // Default limits on stored reports; README.md gives the reasons for // these values. @@ -135,6 +134,14 @@ func New( viper.GetString("DEBUG"), errNotBool) } + // An empty TRUSTED_PROXIES trusts no proxy, but viper reads an + // empty environment variable as unset and would give the default, + // so the environment is read first. + trustedProxies, set := os.LookupEnv("TRUSTED_PROXIES") + if !set { + trustedProxies = viper.GetString("TRUSTED_PROXIES") + } + s := &Config{ BindAddress: viper.GetString("BIND_ADDRESS"), CORSAllowedOrigins: splitList(viper.GetString("CORS_ALLOWED_ORIGINS")), @@ -146,7 +153,7 @@ func New( Port: port, ReportsPerMinute: viper.GetInt("REPORTS_PER_MINUTE"), SentryDSN: viper.GetString("SENTRY_DSN"), - TrustedProxies: splitList(viper.GetString("TRUSTED_PROXIES")), + TrustedProxies: splitList(trustedProxies), log: log, params: ¶ms, } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a6a42e5..7de411a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,8 @@ package config_test import ( + "os" + "slices" "strings" "testing" @@ -29,6 +31,62 @@ func requireConfigError(t *testing.T, setting string) { } } +// requireTrustedProxies builds the config as main does and fails the +// test unless its TrustedProxies are want. +func requireTrustedProxies(t *testing.T, want []string) { + t.Helper() + + var cfg *config.Config + + app := fx.New( + fx.NopLogger, + fx.Provide(globals.New, logger.New, config.New), + fx.Populate(&cfg), + ) + + err := app.Err() + if err != nil { + t.Fatalf("config error = %v", err) + } + + if !slices.Equal(cfg.TrustedProxies, want) { + t.Fatalf("TrustedProxies = %q, want %q", cfg.TrustedProxies, want) + } +} + +// TestTrustedProxiesUnsetTrustsRFC1918: unset, the server trusts the +// RFC 1918 ranges and nothing else, loopback included. +func TestTrustedProxiesUnsetTrustsRFC1918(t *testing.T) { + // t.Setenv first, so the variable is put back as it was after the + // test. + t.Setenv("TRUSTED_PROXIES", "") + + err := os.Unsetenv("TRUSTED_PROXIES") + if err != nil { + t.Fatal(err) + } + + requireTrustedProxies(t, + []string{"10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"}) +} + +// TestTrustedProxiesEmptyTrustsNothing: set and empty, the server +// trusts no proxy. viper alone would read the empty variable as unset +// and give the default. +func TestTrustedProxiesEmptyTrustsNothing(t *testing.T) { + t.Setenv("TRUSTED_PROXIES", "") + + requireTrustedProxies(t, []string{}) +} + +// TestTrustedProxiesListReplacesDefault: a list is used as given, with +// none of the default ranges added. +func TestTrustedProxiesListReplacesDefault(t *testing.T) { + t.Setenv("TRUSTED_PROXIES", "172.17.0.1/32, 2001:db8::/32") + + requireTrustedProxies(t, []string{"172.17.0.1/32", "2001:db8::/32"}) +} + // TestSettingsLoadAsGiven: valid values pass the checks and are used // as given. bin/entrypoint.sh starts the server with these // BIND_ADDRESS and PORT values.