diff --git a/README.md b/README.md index d0e5924..3865ba3 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,28 @@ TTY detection, and security headers are always applied. | `SENTRY_DSN` | Sentry error reporting DSN | `""` | | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint | `120` | +| `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted | `""` (none) | + +#### Trusted proxies + +`TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare +address such as `10.0.0.1` is accepted and treated as a single host), +for example `10.0.0.0/8, 192.168.1.7, 2001:db8::/32`. It decides whose +`X-Forwarded-For`, `X-Real-IP`, and `True-Client-IP` headers the rate +limiters believe. + +Forwarded headers are honoured **only** when the connecting peer is +inside one of these blocks; for every other peer the client identity is +the connection's own address and the headers are ignored. The default +is the empty list, which trusts nobody — anything else would let any +client pick its own rate limit bucket, minting a fresh one per request +or draining someone else's. Set it to the address of your reverse +proxy, and to nothing wider. + +Within a trusted request, `X-Forwarded-For` is read right to left and +the first hop that is not itself a trusted proxy wins, so entries a +client prepended before reaching the proxy cannot be selected. A set +but unparseable value aborts startup. Sessions are bounded by two independent clocks, and end at whichever one runs out first: @@ -124,8 +146,9 @@ fatal configuration error: webhooker logs the offending variable and its value and refuses to start, rather than silently running with a substituted default. `PORT=eighty`, `DEBUG=ture`, and `RETENTION_SWEEP_INTERVAL=1 hour` all abort startup. `PORT` must -additionally be a number in the range 1–65535, and -`RECEIVER_RATE_LIMIT` must be at least 1. +additionally be a number in the range 1–65535, +`RECEIVER_RATE_LIMIT` must be at least 1, and every entry in +`TRUSTED_PROXIES` must be a CIDR block or a bare IP address. Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`, @@ -802,6 +825,16 @@ legitimate webhook senders). Requests over the limit receive HTTP 429 with a `Retry-After` header. A set-but-invalid `RECEIVER_RATE_LIMIT` value aborts startup rather than silently falling back to the default. +Every limiter here — receiver, login, and password change — identifies +the client the same way, through one shared key function: the +connection's own address, unless the peer is listed in +`TRUSTED_PROXIES`, in which case the forwarded client address is used +instead. See [Trusted proxies](#trusted-proxies). Deployed without that +variable set, a client behind a reverse proxy shares one bucket with +every other client behind the same proxy, which is the safe direction +to be wrong in: set `TRUSTED_PROXIES` to the proxy's address to get +per-client limits back. + Finer-grained per-webhook rate limits (configured in the web UI and enforced in the webhook handler) can layer on top of this env-level abuse limit later; they are tracked as future work. diff --git a/internal/config/config.go b/internal/config/config.go index d2511b9..a154c85 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -5,8 +5,10 @@ import ( "errors" "fmt" "log/slog" + "net/netip" "os" "strconv" + "strings" "time" "go.uber.org/fx" @@ -59,6 +61,11 @@ var ErrNonPositiveValue = errors.New("value must be positive") // TCP port number is set above the valid port range. var ErrInvalidPort = errors.New("invalid port") +// ErrInvalidCIDR is returned when an environment variable holding a +// list of CIDR blocks contains an entry that is neither a CIDR block +// nor a bare IP address. +var ErrInvalidCIDR = errors.New("invalid CIDR") + //nolint:revive // ConfigParams is a standard fx naming convention. type ConfigParams struct { fx.In @@ -90,6 +97,14 @@ type Config struct { // client IP may send to a single webhook receiver entrypoint. ReceiverRateLimit int + // TrustedProxies is the set of networks whose members are + // allowed to speak for the client with forwarded headers + // (X-Forwarded-For, X-Real-IP, True-Client-IP). It is empty + // unless TRUSTED_PROXIES is set, and empty means no peer is + // trusted: forwarded headers are then ignored entirely and + // clients are identified by the connection's own address. + TrustedProxies []netip.Prefix + params *ConfigParams log *slog.Logger } @@ -212,6 +227,60 @@ func envDuration( return d, nil } +// parseCIDR parses one trusted-proxy list entry, which may be a +// CIDR block ("10.0.0.0/8") or a bare address ("10.0.0.1", treated +// as a single-host block). +func parseCIDR(entry string) (netip.Prefix, error) { + if strings.Contains(entry, "/") { + prefix, err := netip.ParsePrefix(entry) + if err != nil { + return netip.Prefix{}, err //nolint:wrapcheck // wrapped by caller + } + + return prefix.Masked(), nil + } + + addr, err := netip.ParseAddr(entry) + if err != nil { + return netip.Prefix{}, err //nolint:wrapcheck // wrapped by caller + } + + return netip.PrefixFrom(addr.Unmap(), addr.Unmap().BitLen()), nil +} + +// envPrefixList returns the value of the named environment variable +// parsed as a comma-separated list of CIDR blocks (bare addresses +// allowed). An unset, empty, or blank value yields an empty list. A +// set value containing an unparseable entry is a hard error naming +// the key and the bad entry, so startup fails loudly rather than +// silently running with a list the operator did not intend. +func envPrefixList(key string) ([]netip.Prefix, error) { + v := strings.TrimSpace(os.Getenv(key)) + if v == "" { + return nil, nil + } + + var prefixes []netip.Prefix + + for entry := range strings.SplitSeq(v, ",") { + entry = strings.TrimSpace(entry) + if entry == "" { + continue + } + + prefix, err := parseCIDR(entry) + if err != nil { + return nil, fmt.Errorf( + "%w: %s: %q: %w", ErrInvalidCIDR, key, entry, err, + ) + } + + prefixes = append(prefixes, prefix) + } + + return prefixes, nil +} + // resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to // dev, and rejects unrecognised values. func resolveEnvironment() (string, error) { @@ -282,6 +351,11 @@ func loadFromEnv() (*Config, error) { return nil, err } + trustedProxies, err := envPrefixList("TRUSTED_PROXIES") + if err != nil { + return nil, err + } + return &Config{ DataDir: envString("DATA_DIR"), Debug: debug, @@ -294,6 +368,7 @@ func loadFromEnv() (*Config, error) { RetentionSweepInterval: retentionSweepInterval, SessionIdleTimeout: sessionIdleTimeout, ReceiverRateLimit: receiverRateLimit, + TrustedProxies: trustedProxies, }, nil } @@ -335,6 +410,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { "dataDir", s.DataDir, "retentionSweepInterval", s.RetentionSweepInterval.String(), "receiverRateLimit", s.ReceiverRateLimit, + "trustedProxies", len(s.TrustedProxies), "hasSentryDSN", s.SentryDSN != "", "hasMetricsAuth", s.MetricsUsername != "" && s.MetricsPassword != "", diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 31067db..62fa3cc 100644 --- a/internal/config/config_test.go +++ b/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) +} diff --git a/internal/middleware/ratelimit.go b/internal/middleware/ratelimit.go index 59aef7c..8f4f29e 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -2,6 +2,9 @@ package middleware import ( "net/http" + "net/netip" + "slices" + "strings" "time" "github.com/go-chi/httprate" @@ -31,13 +34,118 @@ const ( receiverRateInterval = 1 * time.Minute ) +// normalizeAddr strips the IPv4-in-IPv6 wrapper and any zone from +// addr so that comparisons and bucket keys are canonical. +func normalizeAddr(addr netip.Addr) netip.Addr { + return addr.Unmap().WithZone("") +} + +// isTrustedProxy reports whether addr belongs to a network the +// operator listed in TRUSTED_PROXIES. The list is empty by default, +// so by default nothing is trusted. +func (m *Middleware) isTrustedProxy(addr netip.Addr) bool { + for _, prefix := range m.params.Config.TrustedProxies { + if prefix.Contains(addr) { + return true + } + } + + return false +} + +// forwardedClientAddr returns the client address named by this +// request's forwarded headers. It is consulted only for requests +// whose direct peer is a trusted proxy. +// +// True-Client-IP and X-Real-IP are single-valued, and a trusted +// proxy is expected to overwrite whatever the client sent, so they +// are taken as given. X-Forwarded-For is a chain the client can +// prepend to, so it is walked right to left and the first hop that +// is not itself a trusted proxy wins: entries the client inserted +// sit to the left of the proxies' own appends and cannot be picked +// while the chain is intact. +func (m *Middleware) forwardedClientAddr( + r *http.Request, +) (netip.Addr, bool) { + for _, header := range []string{"True-Client-IP", "X-Real-IP"} { + addr, err := netip.ParseAddr( + strings.TrimSpace(r.Header.Get(header)), + ) + if err == nil { + return normalizeAddr(addr), true + } + } + + hops := strings.Split( + strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",", + ) + + for _, hop := range slices.Backward(hops) { + addr, err := netip.ParseAddr(strings.TrimSpace(hop)) + if err != nil { + continue + } + + if addr = normalizeAddr(addr); !m.isTrustedProxy(addr) { + return addr, true + } + } + + return netip.Addr{}, false +} + +// rateLimitKey is the client identity every rate limiter in this +// package buckets on. Forwarded headers are honoured only when the +// direct peer (RemoteAddr) is inside the configured trusted-proxy +// set; otherwise the peer address itself is the key. Without that +// gate any client could mint a fresh bucket per request, or starve +// another client's bucket, by picking an X-Forwarded-For value — +// which makes every limit here decorative against a deliberate +// attacker. +func (m *Middleware) rateLimitKey(r *http.Request) (string, error) { + return m.clientKey(r), nil +} + +// clientKey computes the bucket key described on rateLimitKey. +func (m *Middleware) clientKey(r *http.Request) string { + peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr)) + if err != nil { + // Not an address we can reason about; key on the raw + // value rather than collapsing such peers into one + // shared bucket. + return r.RemoteAddr + } + + peer = normalizeAddr(peer) + if !m.isTrustedProxy(peer) { + return peer.String() + } + + if addr, ok := m.forwardedClientAddr(r); ok { + return addr.String() + } + + return peer.String() +} + +// tooManyRequests returns the 429 handler shared by every limiter: +// it logs the rejection with logMessage and answers with +// responseMessage. httprate adds the Retry-After header (RFC 6585). +func (m *Middleware) tooManyRequests( + logMessage, responseMessage string, +) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + m.log.Warn(logMessage, "path", r.URL.Path) + http.Error(w, responseMessage, http.StatusTooManyRequests) + } +} + // LoginRateLimit returns middleware that enforces per-IP rate // limiting on login attempts using go-chi/httprate. Only POST // requests are rate-limited; GET requests (rendering the login // form) pass through unaffected. When the rate limit is exceeded, -// a 429 Too Many Requests response is returned. IP extraction -// honours X-Forwarded-For, X-Real-IP, and True-Client-IP headers -// for reverse-proxy setups. +// a 429 Too Many Requests response is returned. Clients are +// identified by rateLimitKey. func (m *Middleware) LoginRateLimit() func(http.Handler) http.Handler { return m.postRateLimit( loginRateLimit, @@ -66,9 +174,7 @@ func (m *Middleware) PasswordChangeRateLimit() func(http.Handler) http.Handler { // limit on POST requests only; all other methods pass through // unaffected. Requests over the limit receive a 429 with the // given response message, and each rejection is logged with the -// given log message. IP extraction honours X-Forwarded-For, -// X-Real-IP, and True-Client-IP headers for reverse-proxy -// setups. +// given log message. Clients are identified by rateLimitKey. func (m *Middleware) postRateLimit( limit int, interval time.Duration, @@ -77,19 +183,10 @@ func (m *Middleware) postRateLimit( limiter := httprate.Limit( limit, interval, - httprate.WithKeyFuncs(httprate.KeyByRealIP), - httprate.WithLimitHandler(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - m.log.Warn(logMessage, - "path", r.URL.Path, - ) - http.Error( - w, - responseMessage, - http.StatusTooManyRequests, - ) - }, - )), + httprate.WithKeyFuncs(m.rateLimitKey), + httprate.WithLimitHandler( + m.tooManyRequests(logMessage, responseMessage), + ), ) return func(next http.Handler) http.Handler { @@ -116,31 +213,19 @@ func (m *Middleware) postRateLimit( // path (the path contains the entrypoint UUID, so each sender // is limited per entrypoint without affecting other senders or // other entrypoints). The limit is Config.ReceiverRateLimit -// requests per minute. Requests over the limit receive a 429; -// httprate adds the Retry-After header (RFC 6585). IP -// extraction honours X-Forwarded-For, X-Real-IP, and -// True-Client-IP headers for reverse-proxy setups. +// requests per minute. Requests over the limit receive a 429. +// Clients are identified by rateLimitKey. func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler { return httprate.Limit( m.params.Config.ReceiverRateLimit, receiverRateInterval, httprate.WithKeyFuncs( - httprate.KeyByRealIP, + m.rateLimitKey, httprate.KeyByEndpoint, ), - httprate.WithLimitHandler(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - m.log.Warn( - "webhook receiver rate limit exceeded", - "path", r.URL.Path, - ) - http.Error( - w, - "Too many requests. "+ - "Please slow down.", - http.StatusTooManyRequests, - ) - }, + httprate.WithLimitHandler(m.tooManyRequests( + "webhook receiver rate limit exceeded", + "Too many requests. Please slow down.", )), ) } diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index 4664b30..39b4ca0 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -2,9 +2,11 @@ package middleware_test import ( "context" + "fmt" "log/slog" "net/http" "net/http/httptest" + "net/netip" "os" "testing" @@ -182,11 +184,22 @@ func TestLoginRateLimit_IndependentPerIP(t *testing.T) { ) } -// receiverLimitedHandler builds a ReceiverRateLimit-wrapped -// handler with the given per-minute limit. -func receiverLimitedHandler( - t *testing.T, limit int, -) http.Handler { +// okHandler is the terminal handler the limiter middleware wraps +// in these tests: it answers 200 to anything that reaches it. +func okHandler() http.Handler { + return http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + ) +} + +// rateLimitMiddleware builds a Middleware around cfg, whose +// TrustedProxies field is what the rate limit key function gates +// forwarded-header trust on. +func rateLimitMiddleware( + t *testing.T, cfg *config.Config, +) *middleware.Middleware { t.Helper() log := slog.New(slog.NewTextHandler( @@ -194,17 +207,53 @@ func receiverLimitedHandler( &slog.HandlerOptions{Level: slog.LevelDebug}, )) - m := middleware.NewForTest( - log, - &config.Config{ReceiverRateLimit: limit}, - nil, + return middleware.NewForTest(log, cfg, nil) +} + +// trustedProxies parses CIDR strings for a test Config. +func trustedProxies(cidrs ...string) []netip.Prefix { + prefixes := make([]netip.Prefix, 0, len(cidrs)) + for _, cidr := range cidrs { + prefixes = append(prefixes, netip.MustParsePrefix(cidr)) + } + + return prefixes +} + +// postWithHeaders sends one POST to the handler from peer with the +// given headers set and returns the recorder. +func postWithHeaders( + handler http.Handler, + peer, path string, + headers map[string]string, +) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, path, nil, + ) + req.RemoteAddr = peer + + for name, value := range headers { + req.Header.Set(name, value) + } + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + return w +} + +// receiverLimitedHandler builds a ReceiverRateLimit-wrapped +// handler with the given per-minute limit and no trusted proxies. +func receiverLimitedHandler( + t *testing.T, limit int, +) http.Handler { + t.Helper() + + m := rateLimitMiddleware( + t, &config.Config{ReceiverRateLimit: limit}, ) - return m.ReceiverRateLimit()(http.HandlerFunc( - func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - }, - )) + return m.ReceiverRateLimit()(okHandler()) } // receiverPost sends one POST to the handler from the given IP @@ -311,3 +360,182 @@ func TestReceiverRateLimit_CountsEveryMethod(t *testing.T) { "a GET over the limit must be rate-limited", ) } + +const ( + loginPath = "/pages/login" + headerXFF = "X-Forwarded-For" + headerReal = "X-Real-IP" + headerTrue = "True-Client-IP" +) + +// TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test +// this gating exists for: with no trusted proxies configured (the +// default), a client that rotates a forwarded header on every +// request must stay in one bucket. If forwarded headers were +// trusted unconditionally, each spoofed value would mint a fresh +// bucket and the limit would stop no one. +func TestRateLimitKey_SpoofedForwardedFromUntrustedPeer( + t *testing.T, +) { + t.Parallel() + + for _, header := range []string{ + headerXFF, headerReal, headerTrue, + } { + t.Run(header, func(t *testing.T) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{}) + handler := m.LoginRateLimit()(okHandler()) + + const peer = "203.0.113.9:44444" + + for i := range middleware.LoginRateLimitConst { + w := postWithHeaders( + handler, peer, loginPath, + map[string]string{ + header: fmt.Sprintf( + "198.51.100.%d", i+1, + ), + }, + ) + assert.Equal( + t, http.StatusOK, w.Code, + "request %d should pass", i, + ) + } + + w := postWithHeaders( + handler, peer, loginPath, + map[string]string{header: "198.51.100.200"}, + ) + assert.Equal( + t, http.StatusTooManyRequests, w.Code, + "a spoofed %s from an untrusted peer must "+ + "not mint a fresh bucket", header, + ) + }) + } +} + +// TestRateLimitKey_ForwardedHonouredFromTrustedPeer checks the +// other half: when the direct peer is a configured trusted proxy, +// the forwarded client address is what buckets are keyed on, so +// one sender behind the proxy cannot exhaust another's limit. +func TestRateLimitKey_ForwardedHonouredFromTrustedPeer( + t *testing.T, +) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{ + TrustedProxies: trustedProxies("10.0.0.0/8"), + }) + handler := m.LoginRateLimit()(okHandler()) + + const peer = "10.0.0.1:44444" + + first := map[string]string{headerXFF: "198.51.100.7"} + + for range middleware.LoginRateLimitConst { + postWithHeaders(handler, peer, loginPath, first) + } + + w := postWithHeaders(handler, peer, loginPath, first) + assert.Equal( + t, http.StatusTooManyRequests, w.Code, + "the forwarded client's own bucket must fill up", + ) + + w = postWithHeaders( + handler, peer, loginPath, + map[string]string{headerXFF: "198.51.100.8"}, + ) + assert.Equal( + t, http.StatusOK, w.Code, + "a forwarded header from a trusted peer must be honoured", + ) +} + +// TestRateLimitKey_ChainWalkSkipsClientPrepended covers the +// residual spoofing route behind a trusted proxy: the client +// controls the leftmost X-Forwarded-For entries, so the key is the +// rightmost hop that is not itself trusted. Rotating the prepended +// entry must not create new buckets. +func TestRateLimitKey_ChainWalkSkipsClientPrepended(t *testing.T) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{ + TrustedProxies: trustedProxies("10.0.0.0/8"), + }) + handler := m.LoginRateLimit()(okHandler()) + + const peer = "10.0.0.1:44444" + + chain := func(spoof string) map[string]string { + return map[string]string{ + headerXFF: spoof + ", 198.51.100.7, 10.0.0.2", + } + } + + for i := range middleware.LoginRateLimitConst { + w := postWithHeaders( + handler, peer, loginPath, + chain(fmt.Sprintf("9.9.9.%d", i+1)), + ) + assert.Equal( + t, http.StatusOK, w.Code, + "request %d should pass", i, + ) + } + + w := postWithHeaders( + handler, peer, loginPath, chain("9.9.9.200"), + ) + assert.Equal( + t, http.StatusTooManyRequests, w.Code, + "a client-prepended X-Forwarded-For entry must not "+ + "mint a fresh bucket", + ) +} + +// TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves +// the receiver limiter uses the same gated key function as the +// POST limiters. +func TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer( + t *testing.T, +) { + t.Parallel() + + const ( + limit = 3 + peer = "203.0.113.10:44444" + path = "/webhook/uuid-d" + ) + + handler := receiverLimitedHandler(t, limit) + + for i := range limit { + w := postWithHeaders( + handler, peer, path, + map[string]string{ + headerXFF: fmt.Sprintf( + "198.51.100.%d", i+1, + ), + }, + ) + assert.Equal( + t, http.StatusOK, w.Code, + "request %d should pass", i, + ) + } + + w := postWithHeaders( + handler, peer, path, + map[string]string{headerXFF: "198.51.100.200"}, + ) + assert.Equal( + t, http.StatusTooManyRequests, w.Code, + "a spoofed X-Forwarded-For from an untrusted peer must "+ + "not mint a fresh receiver bucket", + ) +}