Trust the RFC 1918 ranges as proxies when TRUSTED_PROXIES is unset (closes #333)
Unset or empty, TRUSTED_PROXIES now defaults to 10.0.0.0/8, 172.16.0.0/12 and 192.168.0.0/16, so a reverse proxy reaching the app over a Docker network or a private LAN gets per-client rate-limit buckets without configuration. A set value replaces the default; an unparseable one still fails startup. The startup warning for an empty list goes, with its test hook and test, since the default is no longer empty. The README's configuration table, Trusted proxies, upaas and reverse-proxy sections describe the new default and when to narrow it to the proxy alone. Model: opus-5-5
This commit is contained in:
+14
-101
@@ -551,6 +551,11 @@ func testReceiverRateLimitSuccess(
|
||||
}
|
||||
|
||||
func TestTrustedProxies(t *testing.T) {
|
||||
// Unset, the RFC 1918 private ranges are trusted, so a reverse
|
||||
// proxy on a Docker network or a private LAN is covered without
|
||||
// configuration.
|
||||
defaultProxies := []string{cidrPrivateV4, "172.16.0.0/12", "192.168.0.0/16"}
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
set bool
|
||||
@@ -559,18 +564,21 @@ func TestTrustedProxies(t *testing.T) {
|
||||
expected []string
|
||||
}{
|
||||
{
|
||||
// The default must be "trust nobody": an empty list
|
||||
// means forwarded headers are ignored, never that
|
||||
// every peer may speak for the client.
|
||||
name: caseUnsetUsesDefault,
|
||||
set: false,
|
||||
expected: []string{},
|
||||
expected: defaultProxies,
|
||||
},
|
||||
{
|
||||
name: "blank value trusts nothing",
|
||||
name: "blank value uses default",
|
||||
set: true,
|
||||
value: " ",
|
||||
expected: []string{},
|
||||
expected: defaultProxies,
|
||||
},
|
||||
{
|
||||
name: "set value replaces the default entirely",
|
||||
set: true,
|
||||
value: "203.0.113.7",
|
||||
expected: []string{"203.0.113.7/32"},
|
||||
},
|
||||
{
|
||||
name: caseValidValueParsed,
|
||||
@@ -845,101 +853,6 @@ func TestEgressAllowlistWarning(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestSharedRateLimitBucketWarning covers the startup warning that
|
||||
// tells an operator a deployment behind a reverse proxy shares one
|
||||
// rate-limit bucket between every client, which turns the receiver
|
||||
// limits into service-wide ceilings and collapses login failure
|
||||
// counting. It must fire whenever TRUSTED_PROXIES is empty, in any
|
||||
// environment, because behind a proxy every client shares one bucket
|
||||
// in dev and prod alike. It stays quiet once proxies are named.
|
||||
func TestSharedRateLimitBucketWarning(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
environment string
|
||||
trustedProxies string
|
||||
expectWarning bool
|
||||
}{
|
||||
{
|
||||
name: "prod without trusted proxies warns",
|
||||
environment: config.EnvironmentProd,
|
||||
expectWarning: true,
|
||||
},
|
||||
{
|
||||
name: "prod with trusted proxies is quiet",
|
||||
environment: config.EnvironmentProd,
|
||||
trustedProxies: cidrPrivateV4,
|
||||
expectWarning: false,
|
||||
},
|
||||
{
|
||||
name: "dev without trusted proxies warns",
|
||||
environment: config.EnvironmentDev,
|
||||
expectWarning: true,
|
||||
},
|
||||
{
|
||||
name: "dev with trusted proxies is quiet",
|
||||
environment: config.EnvironmentDev,
|
||||
trustedProxies: cidrPrivateV4,
|
||||
expectWarning: false,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
// Cannot use t.Parallel() here because t.Setenv
|
||||
// is incompatible with parallel subtests.
|
||||
t.Setenv("WEBHOOKER_ENVIRONMENT", tt.environment)
|
||||
|
||||
if tt.trustedProxies == "" {
|
||||
require.NoError(
|
||||
t, os.Unsetenv("TRUSTED_PROXIES"),
|
||||
)
|
||||
} else {
|
||||
t.Setenv("TRUSTED_PROXIES", tt.trustedProxies)
|
||||
}
|
||||
|
||||
var buf bytes.Buffer
|
||||
|
||||
log := slog.New(slog.NewJSONHandler(
|
||||
&buf, &slog.HandlerOptions{
|
||||
Level: slog.LevelDebug,
|
||||
},
|
||||
))
|
||||
|
||||
require.NoError(
|
||||
t,
|
||||
config.WarnSharedRateLimitBucketForTest(log),
|
||||
)
|
||||
|
||||
if !tt.expectWarning {
|
||||
assert.Empty(t, buf.String())
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
logged := buf.String()
|
||||
|
||||
assert.Contains(t, logged, `"level":"WARN"`)
|
||||
assert.Contains(t, logged, "TRUSTED_PROXIES")
|
||||
assert.Contains(t, logged, "share one bucket")
|
||||
assert.Contains(
|
||||
t, logged, "throttle every other client's failed logins",
|
||||
)
|
||||
// The warning must not claim a lockout the login
|
||||
// endpoint no longer permits: credentials are verified
|
||||
// before any budget is spent.
|
||||
assert.Contains(
|
||||
t, logged, "a correct password still gets in",
|
||||
)
|
||||
// The text must stay accurate for a developer with
|
||||
// nothing in front of the process, where an empty
|
||||
// list costs nothing.
|
||||
assert.Contains(
|
||||
t, logged, "nothing proxying to this process",
|
||||
)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// metricsEnv describes what one subtest below puts in the
|
||||
// environment for a single METRICS_ variable. A variable that is
|
||||
// set to the empty string and one that is not set at all are
|
||||
|
||||
Reference in New Issue
Block a user