Bound the receiver rate limit per client IP across /webhook/* (closes #139)
All checks were successful
check / check (push) Successful in 3m35s

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.

The aggregate limiter is the outer one, so it counts the requests the
per-entrypoint limiter rejects; a test pins that order by exhausting
one path against the inner limit and then requiring a request to an
unused path to be rejected.

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.

Give the aggregate limiter its own 429 handler that logs at DEBUG and
without the path, rather than the shared one that logs at WARN with
it. Its rejections are one line per request of the very flood it
exists to bound, so the shared handler would have let a client write
its own text into the operator's log at an alerting level once per
request. What this limiter bounds is the database work an invented
path costs; the access log still records every request once at INFO,
and the README now says so instead of claiming DEBUG-only logging.
This commit is contained in:
2026-08-12 10:37:58 +00:00
parent 3941f0b0ff
commit ae74852ea2
5 changed files with 255 additions and 16 deletions

View File

@@ -95,7 +95,7 @@ TTY detection, and security headers are always applied.
| `SENTRY_DSN` | Sentry error reporting DSN | `""` |
| `RETENTION_SWEEP_INTERVAL` | Retention reaper period (Go duration, must be positive) | `1h` |
| `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
@@ -856,6 +856,26 @@ 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.
What that aggregate limit bounds is the database work an invented path
costs, not the number of log lines it produces. The path is
attacker-controlled, so nothing on this route writes it to the log
above `DEBUG`: a path that names no entrypoint is recorded by the
handler at `DEBUG`, and the aggregate limiter logs its rejections at
`DEBUG` and without the path. Every request is still recorded once by
the access log, at `INFO`, with its full URL, whether it was served or
rejected — so a flood of invented paths still writes one `INFO` line
per request.
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
@@ -864,7 +884,14 @@ 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.
per-client limits back. That shared bucket matters more for the
aggregate limit than for the per-entrypoint one: with `TRUSTED_PROXIES`
unset behind the reverse proxy a production deployment is required to
run behind, every request keys on the proxy, so the aggregate limit
becomes a service-wide ceiling of 1200 requests per minute across all
senders and all entrypoints, where the per-entrypoint limit's capacity
still grows with the number of entrypoints. Any deployment with more
than a handful of busy entrypoints must set `TRUSTED_PROXIES`.
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