From 8cf9d0525aa79ac9338e08872d37a52df20b928e Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 18:32:00 +0000 Subject: [PATCH] feat: add receiver rate limiting (refs #64) --- README.md | 28 ++++--- TODO.md | 31 ++++---- internal/config/config.go | 60 +++++++++++++++ internal/config/config_test.go | 106 ++++++++++++++++++++++++++ internal/middleware/ratelimit.go | 39 ++++++++++ internal/middleware/ratelimit_test.go | 93 ++++++++++++++++++++++ internal/server/routes.go | 2 +- 7 files changed, 333 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 4621dd8..bcc9cd0 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,7 @@ TTY detection, and security headers are always applied. | `METRICS_USERNAME` | Basic auth username for `/metrics` | `""` | | `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` | | `SENTRY_DSN` | Sentry error reporting DSN | `""` | +| `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint | `120` | On first startup, webhooker automatically generates a cryptographically secure session encryption key and stores it in the database. This key @@ -676,17 +677,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/TODO.md b/TODO.md index be74524..16bbccb 100644 --- a/TODO.md +++ b/TODO.md @@ -10,24 +10,27 @@ # Status -pre-1.0. No git tags exist. main (afe88c6) is a working webhook proxy +pre-1.0. No git tags exist. main (81413c5) is a working webhook proxy with auth, CSRF/SSRF protections, login rate limiting, Slack target, -policy compliance (#6), and pinned lint tooling (#55). Note: TODO.md was -deliberately deleted from this repo in f9a9569 (2026-03-01, #6); its -content was folded into the README TODO section, which this draft -reconstructs as of 2026-07-06. +policy compliance (#6), pinned lint tooling (#55), a per-webhook event +retention reaper (#63), and delivery targets behind a Target interface +(#77). Work is tracked as Gitea issues (the authoritative TODO); this +file is a summary. Note: TODO.md was deliberately deleted from this +repo in f9a9569 (2026-03-01, #6); its content was folded into the +README TODO section, which this draft reconstructs as of 2026-07-06. # Next Step -Implement automatic event retention cleanup based on retention_days: a -periodic maintenance job that deletes Events, Deliveries, and -DeliveryResults older than the parent webhook's retention_days from each -per-webhook event database. The field exists on the Webhook model and -the README promises the behavior, but nothing enforces it, so event -databases currently grow without bound. +Manual event redelivery from the web UI (replay is a core promised +capability in the README rationale). # Completed Steps +- 2026-08-07 Rate-limit the public webhook receiver per client IP per + entrypoint, env-configurable with fail-loud parsing (#64) +- 2026-08-07 Per-webhook event retention reaper (#63); NoCache + middleware for authenticated pages (#61); Target interface refactor + (#77) - 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints, Makefile shims, README Entrypoints section - 2026-03-25 pin golangci-lint Docker image for linting (#55) @@ -51,12 +54,10 @@ databases currently grow without bound. # Future Steps -- Manual event redelivery from the web UI (replay is a core promised - capability in the README rationale) - Delivery status and retry management UI - Per-webhook rate limiting in the receiver handler (per-webhook config - plus handler enforcement; global limits must not apply to receiver - endpoints) + plus handler enforcement, layered on the env-level receiver limit + from #64; global limits must not apply to receiver endpoints) - Webhook signature verification for GitHub and Stripe HMAC formats - API key authentication for programmatic access (APIKey model exists; Bearer token middleware does not) diff --git a/internal/config/config.go b/internal/config/config.go index 414a3bb..e540828 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -31,12 +31,23 @@ const ( // defaultRetentionSweepInterval is how often the retention // reaper deletes events older than each webhook's RetentionDays. defaultRetentionSweepInterval = 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 ) // ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT // contains an unrecognised value. var ErrInvalidEnvironment = errors.New("invalid environment") +// ErrNonPositiveValue is returned when an environment variable that +// requires a positive integer is set to zero or a negative number. +var ErrNonPositiveValue = errors.New("value must be positive") + //nolint:revive // ConfigParams is a standard fx naming convention. type ConfigParams struct { fx.In @@ -60,6 +71,10 @@ type Config struct { // RetentionSweepInterval is how often the retention reaper runs. RetentionSweepInterval 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 } @@ -104,6 +119,38 @@ func envInt(key string, defaultValue int) int { return defaultValue } +// envPositiveInt returns the value of the named environment variable +// parsed as a positive integer. Returns defaultValue if not set. If +// the variable is set but cannot be parsed, or parses to less than +// one, it returns a wrapped error naming the key and the bad value, +// so startup fails loudly rather than silently falling back to the +// default. +func envPositiveInt( + key string, + defaultValue int, +) (int, error) { + v := os.Getenv(key) + if v == "" { + return defaultValue, nil + } + + i, err := strconv.Atoi(v) + if err != nil { + return 0, fmt.Errorf( + "invalid integer for %s: %q: %w", key, v, err, + ) + } + + if i < 1 { + return 0, fmt.Errorf( + "%w: %s must be at least 1, got %q", + ErrNonPositiveValue, key, v, + ) + } + + return i, nil +} + // envDuration returns the value of the named environment variable // parsed as a Go duration (e.g. "1h", "30m"). Returns defaultValue if // not set. If the variable is set but cannot be parsed, it returns a @@ -162,6 +209,17 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { return nil, err } + // Parse the receiver rate limit; a set-but-unparseable or + // non-positive value is a hard error so fx aborts startup + // rather than silently using the default. + receiverRateLimit, err := envPositiveInt( + "RECEIVER_RATE_LIMIT", + defaultReceiverRateLimit, + ) + if err != nil { + return nil, err + } + // Load configuration values from environment variables s := &Config{ DataDir: envString("DATA_DIR"), @@ -173,6 +231,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { Port: envInt("PORT", defaultPort), SentryDSN: envString("SENTRY_DSN"), RetentionSweepInterval: retentionSweepInterval, + ReceiverRateLimit: receiverRateLimit, log: log, params: ¶ms, } @@ -197,6 +256,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 7e3c2c8..147576b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -258,3 +258,109 @@ func TestDefaultDataDir(t *testing.T) { }) } } + +func TestReceiverRateLimit(t *testing.T) { + tests := []struct { + name string + set bool + value string + expectError bool + expected int + }{ + { + name: "unset uses default", + set: false, + expected: 120, + }, + { + name: "valid value is parsed", + set: true, + value: "30", + expected: 30, + }, + { + name: "unparseable value fails startup", + 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 { + testReceiverRateLimitError(t) + } else { + testReceiverRateLimitSuccess(t, tt.expected) + } + }) + } +} + +func testReceiverRateLimitError(t *testing.T) { + t.Helper() + + var cfg *config.Config + + app := fx.New( + fx.NopLogger, + fx.Provide( + globals.New, + logger.New, + config.New, + ), + fx.Populate(&cfg), + ) + + assert.Error(t, app.Err()) +} + +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 5076be7..da7ecd5 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -14,6 +14,11 @@ const ( // loginRateInterval is the time window for the rate limit. loginRateInterval = 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 @@ -62,3 +67,37 @@ func (m *Middleware) LoginRateLimit() func(http.Handler) http.Handler { }) } } + +// 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 731903a..08a9f3c 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" @@ -145,3 +147,94 @@ 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", + ) +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 1b6bedc..29efbfe 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -159,7 +159,7 @@ func (s *Server) setupSourceRoutes() { } func (s *Server) setupWebhookRoutes() { - s.router.HandleFunc( + s.router.With(s.mw.ReceiverRateLimit()).HandleFunc( "/webhook/{uuid}", s.h.HandleWebhook(), )