Gate forwarded-header trust behind trusted-proxy config (closes #88)

Every rate limiter keyed on httprate.KeyByRealIP, which believes
True-Client-IP, X-Real-IP and the first X-Forwarded-For entry from
any peer. A client could therefore mint a fresh bucket per request
by rotating a spoofed header, or drain another client's bucket by
claiming its address, which left the receiver, login and password
change limits with no value against a deliberate attacker.

The receiver, login and password change limiters now share one key
function: the connection's own address, unless the direct peer is
inside a network listed in the new TRUSTED_PROXIES CIDR list, in
which case the forwarded client address is used. The list is empty
by default, so nothing is trusted until an operator names their
proxy; a set-but-unparseable value aborts startup, matching the
handling of the other parsed variables. Within a trusted request
X-Forwarded-For is walked right to left and the first hop that is
not itself a trusted proxy wins, so client-prepended entries cannot
be selected.

Also folds in two cleanups from the same review: the 429 responder
shared by all three limiters is extracted, and the
RECEIVER_RATE_LIMIT error-path tests now assert that the failure
names the variable and wraps ErrNonPositiveValue rather than only
that some error occurred.

internal/config/config_test.go

@@ -179,9 +179,10 @@ func TestRetentionSweepInterval(t *testing.T) {
 	}
 }
 
-// expectStartupError asserts that fx refuses to build the app,
-// which is what a set-but-invalid environment value must cause.
-func expectStartupError(t *testing.T) {
+// startupError builds the app config.New belongs to and returns
+// the error fx reports, which is non-nil whenever an environment
+// value is set but invalid.
+func startupError(t *testing.T) error {
 	t.Helper()
 
 	var cfg *config.Config
@@ -196,7 +197,33 @@ func expectStartupError(t *testing.T) {
 		fx.Populate(&cfg),
 	)
 
-	assert.Error(t, app.Err())
+	return app.Err()
+}
+
+// expectStartupError asserts that fx refuses to build the app,
+// which is what a set-but-invalid environment value must cause.
+func expectStartupError(t *testing.T) {
+	t.Helper()
+
+	assert.Error(t, startupError(t))
+}
+
+// expectStartupErrorFor asserts that startup fails, that the error
+// names the offending variable so an operator can find it, and,
+// when sentinel is non-nil, that it wraps that sentinel.
+func expectStartupErrorFor(
+	t *testing.T,
+	key string,
+	sentinel error,
+) {
+	t.Helper()
+
+	err := startupError(t)
+	require.ErrorContains(t, err, key)
+
+	if sentinel != nil {
+		require.ErrorIs(t, err, sentinel)
+	}
 }
 
 func testRetentionSweepIntervalSuccess(
@@ -351,7 +378,11 @@ func TestReceiverRateLimit(t *testing.T) {
 		set         bool
 		value       string
 		expectError bool
-		expected    int
+		// sentinel, when set, must be wrapped by the startup
+		// error; every error case must additionally name the
+		// variable in its message.
+		sentinel error
+		expected int
 	}{
 		{
 			name:     caseUnsetUsesDefault,
@@ -375,12 +406,14 @@ func TestReceiverRateLimit(t *testing.T) {
 			set:         true,
 			value:       "0",
 			expectError: true,
+			sentinel:    config.ErrNonPositiveValue,
 		},
 		{
 			name:        "negative fails startup",
 			set:         true,
 			value:       "-5",
 			expectError: true,
+			sentinel:    config.ErrNonPositiveValue,
 		},
 	}
 
@@ -399,7 +432,9 @@ func TestReceiverRateLimit(t *testing.T) {
 			}
 
 			if tt.expectError {
-				expectStartupError(t)
+				expectStartupErrorFor(
+					t, "RECEIVER_RATE_LIMIT", tt.sentinel,
+				)
 			} else {
 				testReceiverRateLimitSuccess(t, tt.expected)
 			}
@@ -432,3 +467,107 @@ func testReceiverRateLimitSuccess(
 
 	assert.Equal(t, expected, cfg.ReceiverRateLimit)
 }
+
+func TestTrustedProxies(t *testing.T) {
+	tests := []struct {
+		name        string
+		set         bool
+		value       string
+		expectError bool
+		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{},
+		},
+		{
+			name:     "blank value trusts nothing",
+			set:      true,
+			value:    "  ",
+			expected: []string{},
+		},
+		{
+			name:  caseValidValueParsed,
+			set:   true,
+			value: "10.0.0.0/8, 192.168.1.7 ,2001:db8::/32",
+			expected: []string{
+				"10.0.0.0/8", "192.168.1.7/32", "2001:db8::/32",
+			},
+		},
+		{
+			name:     "host bits are masked off",
+			set:      true,
+			value:    "10.1.2.3/8",
+			expected: []string{"10.0.0.0/8"},
+		},
+		{
+			name:        caseUnparseableFails,
+			set:         true,
+			value:       "10.0.0.0/8,not-an-address",
+			expectError: true,
+		},
+		{
+			name:        "out-of-range prefix length fails startup",
+			set:         true,
+			value:       "10.0.0.0/33",
+			expectError: true,
+		},
+	}
+
+	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", "dev")
+
+			if tt.set {
+				t.Setenv("TRUSTED_PROXIES", tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv("TRUSTED_PROXIES"))
+			}
+
+			if tt.expectError {
+				expectStartupErrorFor(
+					t, "TRUSTED_PROXIES", config.ErrInvalidCIDR,
+				)
+			} else {
+				testTrustedProxiesSuccess(t, tt.expected)
+			}
+		})
+	}
+}
+
+func testTrustedProxiesSuccess(
+	t *testing.T,
+	expected []string,
+) {
+	t.Helper()
+
+	var cfg *config.Config
+
+	app := fxtest.New(
+		t,
+		fx.Provide(
+			globals.New,
+			logger.New,
+			config.New,
+		),
+		fx.Populate(&cfg),
+	)
+	require.NoError(t, app.Err())
+
+	app.RequireStart()
+
+	defer app.RequireStop()
+
+	got := make([]string, 0, len(cfg.TrustedProxies))
+	for _, prefix := range cfg.TrustedProxies {
+		got = append(got, prefix.String())
+	}
+
+	assert.Equal(t, expected, got)
+}