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
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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:258composes the limiter key as (client IP,httprate.KeyByEndpoint). In httprate v0.15.0KeyByEndpointis exactlyreturn 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 intoHandleWebhook, 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-41claims 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
/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.internal/handlers/webhook.go:42should drop to DEBUG or be rate-limited; state what you chose./webhook/is throttled in aggregate. Mutation-verify it: it must fail against the current per-path key.Implementation requirements
next, PR based onnext, single commit, title ending(closes #N).TODO.md.make checkplus the Docker lint path with the cache defeated (#119).PR: #143
Added an IP-only aggregate limiter in front of the existing per-entrypoint one over
/webhook/*, at10 * RECEIVER_RATE_LIMITper minute (1200/min default). Per-entrypoint limit and its tests unchanged. Derived from the existing setting, sointernal/config/config.gois untouched.Logging: moved the INFO line in
HandleWebhookbelow 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_LimitsAggregateAcrossInventedPathsfails against the pre-fix per-path key (mutation run: only that test fails, all six existing receiver/key tests still pass).make checkexit 0,0 issues.;docker build --no-cache-filter=lint,builderexit 0 withmake lintandmake testboth executing in-container.clawbot referenced this issue2026-08-17 23:50:11 +02:00