From f32284a39fe2c485c39a16fb3cf547cc7956b596 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 16:51:52 +0000 Subject: [PATCH] Rate-limit the public webhook receiver endpoint (closes #64) The public receiver /webhook/{uuid} had no rate limiting: anyone who learns an entrypoint UUID can flood it, inflating the per-webhook database and the delivery queue. Add a dedicated limit scoped to the receiver route, keyed per client IP per request path (the path contains the entrypoint UUID), so one misbehaving sender is throttled without affecting other senders of the same entrypoint or other entrypoints. Requests over the limit get a 429; httprate adds the Retry-After header per RFC 6585. IP extraction honours X-Forwarded-For, X-Real-IP, and True-Client-IP for reverse-proxy deployments. The limit is RECEIVER_RATE_LIMIT requests per minute, default 120. A set-but-unparseable or non-positive value aborts startup via the new envPositiveInt strict parser rather than silently falling back to the default. envInt's other callers are unchanged; converting them is tracked in #80. Also update the README env table and Rate Limiting design section, and sync TODO.md. --- README.md | 28 ++++--- TODO.md | 31 ++++---- internal/config/config.go | 98 ++++++++++++++++++++---- 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, 356 insertions(+), 41 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..f5f8aaf 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 @@ -128,27 +175,35 @@ func envDuration( return d, nil } +// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to +// dev, and rejects unrecognised values. +func resolveEnvironment() (string, error) { + environment := os.Getenv("WEBHOOKER_ENVIRONMENT") + if environment == "" { + environment = EnvironmentDev + } + + if environment != EnvironmentDev && + environment != EnvironmentProd { + return "", fmt.Errorf( + "%w: WEBHOOKER_ENVIRONMENT must be '%s' or '%s', got '%s'", + ErrInvalidEnvironment, + EnvironmentDev, EnvironmentProd, environment, + ) + } + + return environment, nil +} + // New creates a Config by reading environment variables. // //nolint:revive // lc parameter is required by fx even if unused. func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { log := params.Logger.Get() - // Determine environment from WEBHOOKER_ENVIRONMENT env var, - // default to dev - environment := os.Getenv("WEBHOOKER_ENVIRONMENT") - if environment == "" { - environment = EnvironmentDev - } - - // Validate environment - if environment != EnvironmentDev && - environment != EnvironmentProd { - return nil, fmt.Errorf( - "%w: WEBHOOKER_ENVIRONMENT must be '%s' or '%s', got '%s'", - ErrInvalidEnvironment, - EnvironmentDev, EnvironmentProd, environment, - ) + environment, err := resolveEnvironment() + if err != nil { + return nil, err } // Parse the retention sweep interval; a set-but-unparseable value @@ -162,6 +217,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 +239,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 +264,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(), ) -- 2.49.1