From b37ebeacad65c4bfa8f5429748322d91b88a27f5 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 11 Aug 2026 13:02:55 +0000 Subject: [PATCH] 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. X-Forwarded-For is the only forwarded header read, from any peer. Reverse proxies append to it but pass other client headers through verbatim, so believing a single-valued X-Real-IP or True-Client-IP would hand a client behind the trusted proxy a fresh bucket per request - the same bypass, inside the deployment TRUSTED_PROXIES exists to serve. Within a trusted request the chain is walked right to left, since the rightmost entry is the one the nearest proxy appended, and the first hop that is not itself a trusted proxy is taken as the client. A hop that is not a bare address - ip:port, a bracketed IPv6 literal, the token unknown - ends the walk and the peer address is used, rather than continuing left into entries the client controls. Trusted-proxy prefixes written in IPv4-mapped form are unmapped at parse time, since peer addresses are unmapped before matching and such a prefix would otherwise silently never match. 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. --- README.md | 63 ++++- internal/config/config.go | 95 ++++++++ internal/config/config_test.go | 164 ++++++++++++- internal/middleware/ratelimit.go | 161 ++++++++++--- internal/middleware/ratelimit_test.go | 326 ++++++++++++++++++++++++-- 5 files changed, 750 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index d0e5924..d089ea3 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,54 @@ 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 `192.168.1.7` is accepted and treated as a single +host), for example `192.168.1.7, 2001:db8::5`. It decides whose +`X-Forwarded-For` header the rate limiters believe, so it should name +the addresses of your reverse proxies and nothing else. + +`X-Forwarded-For` is 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 header is 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. A set but unparseable value aborts +startup. + +`X-Real-IP` and `True-Client-IP` are **never** read, from any peer. +Reverse proxies append to `X-Forwarded-For` but forward other client +headers verbatim, so a single-valued header is client-controlled even +behind a trusted proxy. + +Within a trusted request, `X-Forwarded-For` is read right to left, +because the rightmost entry is the one the nearest proxy appended and +everything left of it may have been written by the client. The first +hop that is not itself a trusted proxy is taken as the client. A hop +that is not a bare IP address — `ip:port`, a bracketed IPv6 literal, +the token `unknown` — ends the walk and the peer address is used +instead, since past such an entry the chain is not the shape assumed +here. The peer address is likewise used when the header is absent or +every hop in it is a trusted proxy. + +Two operator requirements follow: + +- Your proxy must **append** the peer address to `X-Forwarded-For` + (nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`, + Caddy and AWS ALB by default), and must append a bare address with + no port. +- List proxy hosts **only**. Any address inside `TRUSTED_PROXIES` + chooses its own rate-limit key: its `X-Forwarded-For` is walked, so + it can name a different address on every request to get a fresh + bucket each time, or name another client's address to drain that + client's bucket. Never list a block that also covers clients — a + broad `10.0.0.0/8` on a network where clients live in the same range + makes all three limits, including the unauthenticated webhook + receiver, silently bypassable by every client in the block. Sessions are bounded by two independent clocks, and end at whichever one runs out first: @@ -124,8 +172,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 +851,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..27e875f 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" @@ -45,6 +47,11 @@ const ( // maxPort is the highest valid TCP port number. The lower // bound (at least 1) is enforced by envPositiveInt. maxPort = 65535 + + // mappedV4Offset is the number of leading bits an IPv4-mapped + // IPv6 prefix spends on the ::ffff:0:0/96 wrapper, so a /104 + // covers the same addresses as an IPv4 /8. + mappedV4Offset = 96 ) // ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT @@ -59,6 +66,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 +102,17 @@ 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 X-Forwarded-For, the + // only forwarded header read. 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. + // Members can choose their own rate-limit key, so this must + // name proxy hosts only, never a block that also covers + // clients. + TrustedProxies []netip.Prefix + params *ConfigParams log *slog.Logger } @@ -212,6 +235,71 @@ 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). +// +// Both forms are unmapped, because peer addresses are unmapped +// before they are matched against the list: an IPv4-mapped prefix +// left in that form would silently never match. +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 + } + + if addr := prefix.Addr(); addr.Is4In6() && + prefix.Bits() >= mappedV4Offset { + prefix = netip.PrefixFrom( + addr.Unmap(), prefix.Bits()-mappedV4Offset, + ) + } + + 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 +370,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 +387,7 @@ func loadFromEnv() (*Config, error) { RetentionSweepInterval: retentionSweepInterval, SessionIdleTimeout: sessionIdleTimeout, ReceiverRateLimit: receiverRateLimit, + TrustedProxies: trustedProxies, }, nil } @@ -335,6 +429,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..816d5a2 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -20,6 +20,10 @@ const ( caseUnsetUsesDefault = "unset uses default" caseValidValueParsed = "valid value is parsed" caseUnparseableFails = "unparseable value fails startup" + + // cidrPrivateV4 is the sample trusted-proxy block the + // TRUSTED_PROXIES cases are built from. + cidrPrivateV4 = "10.0.0.0/8" ) func TestEnvironmentConfig(t *testing.T) { @@ -179,9 +183,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 +201,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 +382,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 +410,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 +436,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 +471,116 @@ 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: cidrPrivateV4 + ", 192.168.1.7 ,2001:db8::/32", + expected: []string{ + cidrPrivateV4, "192.168.1.7/32", "2001:db8::/32", + }, + }, + { + name: "host bits are masked off", + set: true, + value: "10.1.2.3/8", + expected: []string{cidrPrivateV4}, + }, + { + // Peer addresses are unmapped before they are + // matched, so an IPv4-mapped prefix kept in that + // form could never match anything. + name: "IPv4-mapped prefix is unmapped", + set: true, + value: "::ffff:10.0.0.0/104", + expected: []string{cidrPrivateV4}, + }, + { + name: caseUnparseableFails, + set: true, + value: cidrPrivateV4 + ",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..6a97bb7 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,120 @@ 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 X-Forwarded-For chain. It is consulted only for requests +// whose direct peer is a trusted proxy. +// +// X-Forwarded-For is the only header read. X-Real-IP and +// True-Client-IP are deliberately ignored: the reverse proxies in +// common use append to X-Forwarded-For and pass any other header the +// client sent through untouched, so believing a single-valued header +// would let a client behind the trusted proxy name its own bucket — +// the very bypass this gating exists to close. +// +// The chain is walked right to left, because the rightmost entry is +// the one the nearest proxy appended and everything to its left may +// have been written by the client. The first hop that is not itself +// a trusted proxy is the client. A hop that cannot be read as a bare +// address ends the walk: past it the chain is not the shape assumed +// here, so the caller falls back to the peer address. +func (m *Middleware) forwardedClientAddr( + r *http.Request, +) (netip.Addr, bool) { + hops := strings.Split( + strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",", + ) + + for _, hop := range slices.Backward(hops) { + hop = strings.TrimSpace(hop) + if hop == "" { + continue + } + + addr, err := netip.ParseAddr(hop) + if err != nil { + return netip.Addr{}, false + } + + 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 +176,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 +185,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 +215,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..69ce76a 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,252 @@ 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" +) + +// assertSharedBucket drives the login limiter from peer with the +// trusted-proxy set proxies, sending one more request than the limit +// allows and varying the headers on each with headers(i). Every +// request must land in the same bucket, so the last one is rejected: +// if any of the varying header values reached the key, the run would +// have minted fresh buckets and nothing would be rejected. +func assertSharedBucket( + t *testing.T, + proxies []netip.Prefix, + peer string, + headers func(i int) map[string]string, + msg string, +) { + t.Helper() + + m := rateLimitMiddleware( + t, &config.Config{TrustedProxies: proxies}, + ) + handler := m.LoginRateLimit()(okHandler()) + + for i := range middleware.LoginRateLimitConst { + w := postWithHeaders(handler, peer, loginPath, headers(i)) + assert.Equal( + t, http.StatusOK, w.Code, + "request %d should pass", i, + ) + } + + w := postWithHeaders( + handler, peer, loginPath, + headers(middleware.LoginRateLimitConst), + ) + assert.Equal(t, http.StatusTooManyRequests, w.Code, msg) +} + +// 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() + + assertSharedBucket( + t, nil, "203.0.113.9:44444", + func(i int) map[string]string { + return map[string]string{ + header: fmt.Sprintf( + "198.51.100.%d", i+1, + ), + } + }, + "a spoofed "+header+" from an untrusted peer "+ + "must not mint a fresh bucket", + ) + }) + } +} + +// TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer is the +// regression test for the bypass hiding inside the trusted case. +// Real reverse proxies (nginx, HAProxy, Caddy, ALB) set only +// X-Forwarded-For and pass every other client header through +// verbatim, so a client behind the configured proxy can send its own +// X-Real-IP or True-Client-IP. Reading either would hand that client +// a fresh bucket per request from inside exactly the deployment +// TRUSTED_PROXIES exists to serve, so neither header is read at all. +func TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer( + t *testing.T, +) { + t.Parallel() + + for _, header := range []string{headerReal, headerTrue} { + t.Run(header, func(t *testing.T) { + t.Parallel() + + assertSharedBucket( + t, trustedProxies("10.0.0.0/8"), + "10.0.0.1:44444", + func(i int) map[string]string { + return map[string]string{ + header: fmt.Sprintf( + "198.51.100.%d", i+1, + ), + } + }, + header+" from a trusted peer must not mint a "+ + "fresh bucket: only X-Forwarded-For is read", + ) + }) + } +} + +// TestRateLimitKey_MalformedRightmostHopFallsBackToPeer covers the +// other end of the chain walk. The rightmost X-Forwarded-For entry +// is the one the trusted proxy appended; if it cannot be read as an +// address the chain is not the shape the walk assumes, and every +// entry to its left may have come from the client. The walk must +// stop and fall back to the peer rather than select one of them. +func TestRateLimitKey_MalformedRightmostHopFallsBackToPeer( + t *testing.T, +) { + t.Parallel() + + // Forms seen in the wild: host:port (Azure Application + // Gateway, IIS ARR), a bracketed IPv6 literal, and the + // RFC 7239 placeholder token. + for _, tail := range []string{ + "198.51.100.7:1234", "[2001:db8::1]", "unknown", + } { + t.Run(tail, func(t *testing.T) { + t.Parallel() + + assertSharedBucket( + t, trustedProxies("10.0.0.0/8"), + "10.0.0.1:44444", + func(i int) map[string]string { + return map[string]string{ + headerXFF: fmt.Sprintf( + "9.9.9.%d, %s", i+1, tail, + ), + } + }, + "an unparseable rightmost hop must fall back "+ + "to the peer address, not select a "+ + "client-controlled entry", + ) + }) + } +} + +// 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() + + assertSharedBucket( + t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444", + func(i int) map[string]string { + return map[string]string{ + headerXFF: fmt.Sprintf( + "9.9.9.%d, 198.51.100.7, 10.0.0.2", i+1, + ), + } + }, + "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", + ) +}