The receiver rate limit is per-URL-path, so an attacker gets unlimited aggregate rate against /webhook/* #139

New Issue

clawbot · 2026-08-12T12:29:51+02:00

clawbot commented

2026-08-12 12:29:51 +02:00

Found by the integration review of #111. Blocks the 1.0.0 tag: this is the milestone's headline internet-facing control and it does not deliver its stated guarantee.

internal/middleware/ratelimit.go:258 composes the limiter key as (client IP, httprate.KeyByEndpoint). In httprate v0.15.0 KeyByEndpoint is exactly return r.URL.Path, nil.

The chi pattern /webhook/{uuid} matches any single segment, so a client mints a fresh 120/min bucket per invented path string. Aggregate rate against the route is therefore unlimited. Every one of those requests is admitted into HandleWebhook, which logs an INFO line containing the attacker-controlled path (internal/handlers/webhook.go:42) and performs a database lookup (:48) before returning 404.

Net effect: a single IP drives unbounded DB queries and unbounded log volume against the only unauthenticated, internet-exposed endpoint — the exact property internal/config/config.go:38-41 claims to provide and that #64 existed to obtain.

Neither unit review could have caught this. #64 landed the per-endpoint key before the trusted-proxy work existed, and #88 rewrote only the IP half of the key.

Definition of done

A client cannot raise its aggregate request rate against /webhook/* by varying the path. The usual shape is an IP-only aggregate limiter over the whole /webhook/* group, kept ALONGSIDE the existing per-entrypoint limiter rather than replacing it — the per-entrypoint limit is still wanted, it is just not sufficient alone.
Unknown-entrypoint requests must not be able to drive unbounded log volume or unbounded database lookups. Consider whether the INFO log at internal/handlers/webhook.go:42 should drop to DEBUG or be rate-limited; state what you chose.
A test that a single IP hitting many DISTINCT invented paths under /webhook/ is throttled in aggregate. Mutation-verify it: it must fail against the current per-path key.
The existing per-entrypoint rate-limit tests must still pass unchanged.

Implementation requirements

Do NOT change IPv6 bucketing — that is #125, awaiting an owner decision on the same file.
Do not restructure the trusted-proxy key function; it has passed three adversarial reviews.
Branch from next, PR based on next, single commit, title ending (closes #N).
Do not modify TODO.md.
Gate on make check plus the Docker lint path with the cache defeated (#119).

Found by the integration review of https://git.eeqj.de/sneak/webhooker/pulls/111. Blocks the 1.0.0 tag: this is the milestone's headline internet-facing control and it does not deliver its stated guarantee. `internal/middleware/ratelimit.go:258` composes the limiter key as (client IP, `httprate.KeyByEndpoint`). In httprate v0.15.0 `KeyByEndpoint` is exactly `return r.URL.Path, nil`. The chi pattern `/webhook/{uuid}` matches any single segment, so a client mints a fresh 120/min bucket per invented path string. Aggregate rate against the route is therefore unlimited. Every one of those requests is admitted into `HandleWebhook`, which logs an INFO line containing the attacker-controlled path (`internal/handlers/webhook.go:42`) and performs a database lookup (`:48`) before returning 404. Net effect: a single IP drives unbounded DB queries and unbounded log volume against the only unauthenticated, internet-exposed endpoint — the exact property `internal/config/config.go:38-41` claims to provide and that https://git.eeqj.de/sneak/webhooker/issues/64 existed to obtain. Neither unit review could have caught this. https://git.eeqj.de/sneak/webhooker/issues/64 landed the per-endpoint key before the trusted-proxy work existed, and https://git.eeqj.de/sneak/webhooker/issues/88 rewrote only the IP half of the key. ## Definition of done - A client cannot raise its aggregate request rate against `/webhook/*` by varying the path. The usual shape is an IP-only aggregate limiter over the whole `/webhook/*` group, kept ALONGSIDE the existing per-entrypoint limiter rather than replacing it — the per-entrypoint limit is still wanted, it is just not sufficient alone. - Unknown-entrypoint requests must not be able to drive unbounded log volume or unbounded database lookups. Consider whether the INFO log at `internal/handlers/webhook.go:42` should drop to DEBUG or be rate-limited; state what you chose. - A test that a single IP hitting many DISTINCT invented paths under `/webhook/` is throttled in aggregate. Mutation-verify it: it must fail against the current per-path key. - The existing per-entrypoint rate-limit tests must still pass unchanged. ## Implementation requirements - Do NOT change IPv6 bucketing — that is https://git.eeqj.de/sneak/webhooker/issues/125, awaiting an owner decision on the same file. - Do not restructure the trusted-proxy key function; it has passed three adversarial reviews. - Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`. - Do not modify `TODO.md`. - Gate on `make check` plus the Docker lint path with the cache defeated (https://git.eeqj.de/sneak/webhooker/issues/119).

clawbot added this to the 1.0.0 milestone 2026-08-12 12:29:51 +02:00

clawbot self-assigned this 2026-08-12 12:29:51 +02:00

clawbot referenced this issue from a commit

2026-08-12 12:38:02 +02:00

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

clawbot referenced a pull request that will close this issue

2026-08-12 12:38:21 +02:00

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

clawbot commented

2026-08-12 12:38:35 +02:00

PR: #143

Added an IP-only aggregate limiter in front of the existing per-entrypoint one over /webhook/*, at 10 * RECEIVER_RATE_LIMIT per minute (1200/min default). Per-entrypoint limit and its tests unchanged. Derived from the existing setting, so internal/config/config.go is untouched.

Logging: moved the INFO line in HandleWebhook below the entrypoint lookup rather than dropping it to DEBUG. Unknown entrypoints now produce only the pre-existing DEBUG line; the request is still in the access log.

Verified: TestReceiverRateLimit_LimitsAggregateAcrossInventedPaths fails against the pre-fix per-path key (mutation run: only that test fails, all six existing receiver/key tests still pass). make check exit 0, 0 issues.; docker build --no-cache-filter=lint,builder exit 0 with make lint and make test both executing in-container.

PR: https://git.eeqj.de/sneak/webhooker/pulls/143 Added an IP-only aggregate limiter in front of the existing per-entrypoint one over `/webhook/*`, at `10 * RECEIVER_RATE_LIMIT` per minute (1200/min default). Per-entrypoint limit and its tests unchanged. Derived from the existing setting, so `internal/config/config.go` is untouched. Logging: moved the INFO line in `HandleWebhook` below the entrypoint lookup rather than dropping it to DEBUG. Unknown entrypoints now produce only the pre-existing DEBUG line; the request is still in the access log. Verified: `TestReceiverRateLimit_LimitsAggregateAcrossInventedPaths` fails against the pre-fix per-path key (mutation run: only that test fails, all six existing receiver/key tests still pass). `make check` exit 0, `0 issues.`; `docker build --no-cache-filter=lint,builder` exit 0 with `make lint` and `make test` both executing in-container.

clawbot referenced this issue

2026-08-12 12:46:36 +02:00

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

clawbot referenced this issue from a commit

2026-08-12 12:55:54 +02:00

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

clawbot referenced this issue

2026-08-12 12:57:46 +02:00

The access log writes one INFO line with the full attacker-controlled URL per request, including rejected ones #146

clawbot referenced this issue from a commit

2026-08-12 13:13:10 +02:00

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