diff --git a/README.md b/README.md index dd26e9b..d0e5924 100644 --- a/README.md +++ b/README.md @@ -94,6 +94,7 @@ TTY detection, and security headers are always applied. | `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` | | `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` | Sessions are bounded by two independent clocks, and end at whichever one runs out first: @@ -123,7 +124,8 @@ 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. +additionally be a number in the range 1–65535, and +`RECEIVER_RATE_LIMIT` must be at least 1. Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`, @@ -785,17 +787,24 @@ just delayed until the target is healthy again. ### Rate Limiting -Global rate limiting middleware (e.g., per-IP throttling applied at the -router level) **must not** apply to webhook receiver endpoints. Webhook -endpoints receive automated traffic from external services at -unpredictable rates, and blanket rate limits would cause legitimate -deliveries to be dropped. +Global blanket rate limiting middleware (e.g., a per-IP throttle shared +with the web UI) **must not** apply to webhook receiver endpoints. +Webhook endpoints receive automated traffic from external services at +unpredictable rates, and blanket limits shared with other routes would +cause legitimate deliveries to be dropped. -Instead, each webhook has its own individually configurable rate limit, -applied within the webhook handler itself. By default, no rate limit is -applied — webhook endpoints accept traffic as fast as it arrives. Rate -limits can be configured per-webhook when needed (e.g., to protect -against a misbehaving sender). +The receiver instead has its own dedicated abuse limit, scoped to the +`/webhook/{uuid}` route only and keyed per client IP per entrypoint: one +misbehaving sender is throttled without affecting other senders of the +same entrypoint or the same sender's other entrypoints. The limit is +`RECEIVER_RATE_LIMIT` requests per minute (default 120, generous for +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. + +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. ### API Endpoints diff --git a/internal/config/config.go b/internal/config/config.go index 4200b69..d2511b9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -35,6 +35,13 @@ const ( // authenticated activity before it expires. defaultSessionIdleTimeout = 24 * time.Hour + // defaultReceiverRateLimit is the default number of requests + // per minute each client IP may send to a single webhook + // receiver entrypoint. Generous for legitimate webhook + // senders while bounding abuse of the one unauthenticated, + // internet-exposed endpoint. + defaultReceiverRateLimit = 120 + // maxPort is the highest valid TCP port number. The lower // bound (at least 1) is enforced by envPositiveInt. maxPort = 65535 @@ -79,6 +86,10 @@ type Config struct { // which a session expires. Non-positive disables idle expiry. SessionIdleTimeout time.Duration + // ReceiverRateLimit is the number of requests per minute each + // client IP may send to a single webhook receiver entrypoint. + ReceiverRateLimit int + params *ConfigParams log *slog.Logger } @@ -263,6 +274,14 @@ func loadFromEnv() (*Config, error) { return nil, err } + receiverRateLimit, err := envPositiveInt( + "RECEIVER_RATE_LIMIT", + defaultReceiverRateLimit, + ) + if err != nil { + return nil, err + } + return &Config{ DataDir: envString("DATA_DIR"), Debug: debug, @@ -274,6 +293,7 @@ func loadFromEnv() (*Config, error) { SentryDSN: envString("SENTRY_DSN"), RetentionSweepInterval: retentionSweepInterval, SessionIdleTimeout: sessionIdleTimeout, + ReceiverRateLimit: receiverRateLimit, }, nil } @@ -314,6 +334,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { "maintenanceMode", s.MaintenanceMode, "dataDir", s.DataDir, "retentionSweepInterval", s.RetentionSweepInterval.String(), + "receiverRateLimit", s.ReceiverRateLimit, "hasSentryDSN", s.SentryDSN != "", "hasMetricsAuth", s.MetricsUsername != "" && s.MetricsPassword != "", diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 571d16d..31067db 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -14,6 +14,14 @@ import ( "sneak.berlin/go/webhooker/internal/logger" ) +// Shared subtest names for the env-parsing tables below, which all +// exercise the same three cases against different variables. +const ( + caseUnsetUsesDefault = "unset uses default" + caseValidValueParsed = "valid value is parsed" + caseUnparseableFails = "unparseable value fails startup" +) + func TestEnvironmentConfig(t *testing.T) { tests := []struct { name string @@ -130,18 +138,18 @@ func TestRetentionSweepInterval(t *testing.T) { expected time.Duration }{ { - name: "unset uses default", + name: caseUnsetUsesDefault, set: false, expected: time.Hour, }, { - name: "valid value is parsed", + name: caseValidValueParsed, set: true, value: "15m", expected: 15 * time.Minute, }, { - name: "unparseable value fails startup", + name: caseUnparseableFails, set: true, value: "not-a-duration", expectError: true, @@ -172,7 +180,7 @@ func TestRetentionSweepInterval(t *testing.T) { } // expectStartupError asserts that fx refuses to build the app, -// which is what a set-but-unparseable duration must cause. +// which is what a set-but-invalid environment value must cause. func expectStartupError(t *testing.T) { t.Helper() @@ -226,18 +234,18 @@ func TestSessionIdleTimeout(t *testing.T) { expected time.Duration }{ { - name: "unset uses default", + name: caseUnsetUsesDefault, set: false, expected: 24 * time.Hour, }, { - name: "valid value is parsed", + name: caseValidValueParsed, set: true, value: "30m", expected: 30 * time.Minute, }, { - name: "unparseable value fails startup", + name: caseUnparseableFails, set: true, value: "not-a-duration", expectError: true, @@ -336,3 +344,91 @@ func TestDefaultDataDir(t *testing.T) { }) } } + +func TestReceiverRateLimit(t *testing.T) { + tests := []struct { + name string + set bool + value string + expectError bool + expected int + }{ + { + name: caseUnsetUsesDefault, + set: false, + expected: 120, + }, + { + name: caseValidValueParsed, + set: true, + value: "30", + expected: 30, + }, + { + name: caseUnparseableFails, + set: true, + value: "not-a-number", + expectError: true, + }, + { + name: "zero fails startup", + set: true, + value: "0", + expectError: true, + }, + { + name: "negative fails startup", + set: true, + value: "-5", + 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("RECEIVER_RATE_LIMIT", tt.value) + } else { + require.NoError(t, os.Unsetenv( + "RECEIVER_RATE_LIMIT", + )) + } + + if tt.expectError { + expectStartupError(t) + } else { + testReceiverRateLimitSuccess(t, tt.expected) + } + }) + } +} + +func testReceiverRateLimitSuccess( + t *testing.T, + expected int, +) { + 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() + + assert.Equal(t, expected, cfg.ReceiverRateLimit) +} diff --git a/internal/middleware/ratelimit.go b/internal/middleware/ratelimit.go index f3ae469..59aef7c 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -24,6 +24,11 @@ const ( // passwordChangeRateInterval is the time window for the // password change rate limit. passwordChangeRateInterval = 1 * time.Minute + + // receiverRateInterval is the time window for the webhook + // receiver rate limit. The configured limit is expressed in + // requests per minute. + receiverRateInterval = 1 * time.Minute ) // LoginRateLimit returns middleware that enforces per-IP rate @@ -105,3 +110,37 @@ func (m *Middleware) postRateLimit( }) } } + +// ReceiverRateLimit returns middleware that rate-limits the +// public webhook receiver endpoint per client IP per request +// 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. +func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler { + return httprate.Limit( + m.params.Config.ReceiverRateLimit, + receiverRateInterval, + httprate.WithKeyFuncs( + httprate.KeyByRealIP, + 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, + ) + }, + )), + ) +} diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index c0f7209..4664b30 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -2,8 +2,10 @@ package middleware_test import ( "context" + "log/slog" "net/http" "net/http/httptest" + "os" "testing" "github.com/stretchr/testify/assert" @@ -179,3 +181,133 @@ func TestLoginRateLimit_IndependentPerIP(t *testing.T) { "different IP should not be affected", ) } + +// receiverLimitedHandler builds a ReceiverRateLimit-wrapped +// handler with the given per-minute limit. +func receiverLimitedHandler( + t *testing.T, limit int, +) http.Handler { + t.Helper() + + log := slog.New(slog.NewTextHandler( + os.Stderr, + &slog.HandlerOptions{Level: slog.LevelDebug}, + )) + + m := middleware.NewForTest( + log, + &config.Config{ReceiverRateLimit: limit}, + nil, + ) + + return m.ReceiverRateLimit()(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + )) +} + +// receiverPost sends one POST to the handler from the given IP +// and path and returns the recorder. +func receiverPost( + handler http.Handler, ip, path string, +) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPost, path, nil, + ) + req.RemoteAddr = ip + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + return w +} + +func TestReceiverRateLimit_LimitsPerIPAndPath(t *testing.T) { + t.Parallel() + + const limit = 3 + + handler := receiverLimitedHandler(t, limit) + + // The first limit requests from one IP to one entrypoint + // pass. + for i := range limit { + w := receiverPost( + handler, "9.9.9.9:1234", "/webhook/uuid-a", + ) + assert.Equal( + t, http.StatusOK, w.Code, + "request %d should pass", i, + ) + } + + // The next request over the limit is rejected with a 429 + // carrying a Retry-After header. + w := receiverPost( + handler, "9.9.9.9:1234", "/webhook/uuid-a", + ) + assert.Equal(t, http.StatusTooManyRequests, w.Code) + assert.NotEmpty( + t, w.Header().Get("Retry-After"), + "429 must carry a Retry-After header", + ) + + // The same IP is not limited on a different entrypoint. + w = receiverPost( + handler, "9.9.9.9:1234", "/webhook/uuid-b", + ) + assert.Equal( + t, http.StatusOK, w.Code, + "a different entrypoint must not be affected", + ) + + // A different IP is not limited on the same entrypoint. + w = receiverPost( + handler, "8.8.8.8:1234", "/webhook/uuid-a", + ) + assert.Equal( + t, http.StatusOK, w.Code, + "a different client IP must not be affected", + ) +} + +// TestReceiverRateLimit_CountsEveryMethod proves the receiver +// limit counts non-POST requests too: a GET shares the bucket +// with a POST and is itself rejected once over the limit. +func TestReceiverRateLimit_CountsEveryMethod(t *testing.T) { + t.Parallel() + + const ( + limit = 2 + ip = "7.7.7.7:1234" + path = "/webhook/uuid-c" + ) + + handler := receiverLimitedHandler(t, limit) + + get := func() *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, path, nil, + ) + req.RemoteAddr = ip + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + return w + } + + // One POST plus one GET fill the bucket, so the GET must + // have been counted. + assert.Equal( + t, http.StatusOK, receiverPost(handler, ip, path).Code, + ) + assert.Equal(t, http.StatusOK, get().Code) + + assert.Equal( + t, http.StatusTooManyRequests, get().Code, + "a GET over the limit must be rate-limited", + ) +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 75a68b4..b282e12 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -171,7 +171,7 @@ func (s *Server) setupSourceRoutes() { } func (s *Server) setupWebhookRoutes() { - s.router.HandleFunc( + s.router.With(s.mw.ReceiverRateLimit()).HandleFunc( "/webhook/{uuid}", s.h.HandleWebhook(), )