Bound the receiver rate limit per client IP across /webhook/* (closes #139)

The receiver limiter keyed buckets on (client IP, request path). The
route pattern /webhook/{uuid} matches any single segment, so a client
that invented a fresh path per request minted a fresh bucket per
request and never refilled one: its aggregate rate against the only
unauthenticated, internet-exposed endpoint was unbounded, and every
one of those requests reached an entrypoint lookup before it 404ed.

Put a second limiter in front of it, keyed on the client IP alone and
covering the whole route at ten times the configured per-entrypoint
limit (1200/min by default). The per-entrypoint limit is unchanged and
still wanted; it just bounds nothing in aggregate on its own. Ten
entrypoints' worth of headroom lets one sender address drive several
entrypoints at full rate while still capping what one address costs
the receiver. The multiplication saturates rather than wrapping, since
nothing bounds RECEIVER_RATE_LIMIT from above and a negative limit
would reject every request.

Move the handler's INFO line for an incoming webhook below the
entrypoint lookup. The UUID is attacker-controlled path text, so
logging it first let a client write an INFO line per invented path; a
miss is already logged at DEBUG and the request is already in the
access log.

README.md

@@ -94,7 +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` |
+| `RECEIVER_RATE_LIMIT`   | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` |
 | `TRUSTED_PROXIES`       | CIDRs whose forwarded headers are trusted | `""` (none) |
 
 #### Trusted proxies
@@ -851,6 +851,20 @@ 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.
 
+A second limit sits in front of that one, keyed on the client IP alone
+and covering the whole route at ten times `RECEIVER_RATE_LIMIT` requests
+per minute (default 1200). The per-entrypoint limit needs it: the route
+pattern matches any single path segment, so a client that invents a
+fresh path per request gets a fresh per-entrypoint bucket every time and
+would otherwise have no aggregate limit at all — while each of those
+requests still costs an entrypoint lookup before it 404s. The aggregate
+limit leaves room for one address to drive several entrypoints at their
+full rate, and it is not configurable separately.
+
+Requests to a `/webhook/` path that names no entrypoint are logged at
+`DEBUG` only, since the path is attacker-controlled; the request itself
+still appears in the access log.
+
 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

internal/handlers/webhook.go

@@ -39,12 +39,6 @@ func (h *Handlers) HandleWebhook() http.HandlerFunc {
 			return
 		}
 
-		h.log.Info("webhook request received",
-			"entrypoint_uuid", entrypointUUID,
-			"method", r.Method,
-			"remote_addr", r.RemoteAddr,
-		)
-
 		entrypoint, ok := h.lookupEntrypoint(
 			w, r, entrypointUUID,
 		)
@@ -52,6 +46,18 @@ func (h *Handlers) HandleWebhook() http.HandlerFunc {
 			return
 		}
 
+		// Logged only once the UUID is known to name a real
+		// entrypoint. The UUID comes straight out of the path on
+		// the one unauthenticated endpoint, so logging it before
+		// the lookup let a client write an INFO line per invented
+		// path; the request itself is already in the access log
+		// and a miss is already logged at DEBUG.
+		h.log.Info("webhook request received",
+			"entrypoint_uuid", entrypointUUID,
+			"method", r.Method,
+			"remote_addr", r.RemoteAddr,
+		)
+
 		if !entrypoint.Active {
 			http.Error(w, "Gone", http.StatusGone)
 

internal/middleware/export_test.go

@@ -41,3 +41,13 @@ const LoginRateLimitConst = loginRateLimit
 // PasswordChangeRateLimitConst exposes the
 // passwordChangeRateLimit constant.
 const PasswordChangeRateLimitConst = passwordChangeRateLimit
+
+// ReceiverAggregateMultiplierConst exposes the
+// receiverAggregateMultiplier constant.
+const ReceiverAggregateMultiplierConst = receiverAggregateMultiplier
+
+// ReceiverAggregateLimitForTest exposes receiverAggregateLimit for
+// testing.
+func ReceiverAggregateLimitForTest(perEntrypoint int) int {
+	return receiverAggregateLimit(perEntrypoint)
+}

internal/middleware/ratelimit.go

@@ -1,6 +1,7 @@
 package middleware
 
 import (
+	"math"
 	"net/http"
 	"net/netip"
 	"slices"
@@ -33,6 +34,14 @@ const (
 	// requests per minute.
 	receiverRateInterval = 1 * time.Minute
 
+	// receiverAggregateMultiplier scales the configured
+	// per-entrypoint receiver limit into the aggregate limit one
+	// client IP may spend across the whole /webhook/* route. Ten
+	// entrypoints' worth lets a single sender address drive several
+	// entrypoints at their full rate, while still capping what one
+	// address costs the unauthenticated receiver.
+	receiverAggregateMultiplier = 10
+
 	// maxForwardedHops bounds how many X-Forwarded-For entries the
 	// chain walk examines. Real chains are one to three hops, but a
 	// client can pad the header up to MaxHeaderBytes, so without a
@@ -242,15 +251,26 @@ func (m *Middleware) postRateLimit(
 	}
 }
 
-// ReceiverRateLimit returns middleware that rate-limits the
+// ReceiverRateLimit returns middleware that rate-limits the public
-// public webhook receiver endpoint per client IP per request
+// webhook receiver endpoint with two limits in series.
-// path (the path contains the entrypoint UUID, so each sender
+//
-// is limited per entrypoint without affecting other senders or
+// The inner limit is per client IP per request path: the path
-// other entrypoints). The limit is Config.ReceiverRateLimit
+// contains the entrypoint UUID, so each sender is limited per
-// requests per minute. Requests over the limit receive a 429.
+// entrypoint without affecting other senders or other entrypoints.
-// Clients are identified by rateLimitKey.
+// It is Config.ReceiverRateLimit requests per minute.
+//
+// That limit alone bounds nothing in aggregate. The route pattern
+// /webhook/{uuid} matches any single segment, so a client that
+// invents a fresh path per request mints a fresh bucket per request
+// and never refills one — and every such request still reaches the
+// handler's entrypoint lookup before it 404s. The outer limit is
+// therefore keyed on the client IP alone, capping what one address
+// can spend across the whole route however it varies the path.
+//
+// Requests over either limit receive a 429. Clients are identified
+// by rateLimitKey.
 func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler {
-	return httprate.Limit(
+	perEntrypoint := httprate.Limit(
 		m.params.Config.ReceiverRateLimit,
 		receiverRateInterval,
 		httprate.WithKeyFuncs(
@@ -262,4 +282,31 @@ func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler {
 			"Too many requests. Please slow down.",
 		)),
 	)
+
+	aggregate := httprate.Limit(
+		receiverAggregateLimit(m.params.Config.ReceiverRateLimit),
+		receiverRateInterval,
+		httprate.WithKeyFuncs(m.rateLimitKey),
+		httprate.WithLimitHandler(m.tooManyRequests(
+			"webhook receiver aggregate rate limit exceeded",
+			"Too many requests. Please slow down.",
+		)),
+	)
+
+	return func(next http.Handler) http.Handler {
+		return aggregate(perEntrypoint(next))
+	}
+}
+
+// receiverAggregateLimit is the per-IP aggregate limit derived from
+// the configured per-entrypoint limit. The operator sets the latter
+// and nothing bounds it from above, so the multiplication is
+// saturated rather than allowed to wrap into a negative limit that
+// would reject every request.
+func receiverAggregateLimit(perEntrypoint int) int {
+	if perEntrypoint > math.MaxInt/receiverAggregateMultiplier {
+		return math.MaxInt
+	}
+
+	return perEntrypoint * receiverAggregateMultiplier
 }

internal/middleware/ratelimit_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"log/slog"
+	"math"
 	"net/http"
 	"net/http/httptest"
 	"net/netip"
@@ -670,6 +671,77 @@ func TestRateLimitKey_LongChainAllocationIsBounded(t *testing.T) {
 	)
 }
 
+// TestReceiverRateLimit_LimitsAggregateAcrossInventedPaths is the
+// regression test for the per-path bucket key. The route pattern
+// matches any single segment, so a client that never reuses a path
+// never reuses a per-entrypoint bucket either, and its aggregate
+// rate against the receiver is whatever it likes — with every
+// request reaching an entrypoint lookup before it 404s. The IP-only
+// aggregate limiter is what bounds that, so this must fail if the
+// aggregate limiter is removed.
+func TestReceiverRateLimit_LimitsAggregateAcrossInventedPaths(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	const (
+		limit = 3
+		ip    = "6.6.6.6:1234"
+	)
+
+	aggregate := limit * middleware.ReceiverAggregateMultiplierConst
+
+	handler := receiverLimitedHandler(t, limit)
+
+	// Every request goes to a path this client has never used, so
+	// none of them shares a per-entrypoint bucket with another.
+	for i := range aggregate {
+		w := receiverPost(
+			handler, ip, fmt.Sprintf("/webhook/invented-%d", i),
+		)
+		assert.Equal(
+			t, http.StatusOK, w.Code,
+			"request %d to a distinct path should pass", i,
+		)
+	}
+
+	w := receiverPost(
+		handler, ip, fmt.Sprintf("/webhook/invented-%d", aggregate),
+	)
+	assert.Equal(
+		t, http.StatusTooManyRequests, w.Code,
+		"a client must not be able to raise its aggregate rate "+
+			"against /webhook/* by varying the path",
+	)
+
+	// The aggregate limit is still per client IP: exhausting one
+	// address must not throttle another.
+	w = receiverPost(handler, "6.6.6.7:1234", "/webhook/invented-0")
+	assert.Equal(
+		t, http.StatusOK, w.Code,
+		"a different client IP must not be affected",
+	)
+}
+
+// TestReceiverAggregateLimit_SaturatesOnOverflow covers the derived
+// aggregate limit for a configured per-entrypoint limit large enough
+// that multiplying it would wrap negative, which httprate would read
+// as a limit that rejects every request.
+func TestReceiverAggregateLimit_SaturatesOnOverflow(t *testing.T) {
+	t.Parallel()
+
+	assert.Equal(
+		t, 1200,
+		middleware.ReceiverAggregateLimitForTest(120),
+		"the default limit scales by the multiplier",
+	)
+	assert.Equal(
+		t, math.MaxInt,
+		middleware.ReceiverAggregateLimitForTest(math.MaxInt),
+		"an overflowing limit saturates instead of wrapping",
+	)
+}
+
 // TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves
 // the receiver limiter uses the same gated key function as the
 // POST limiters.