Bound the receiver rate limit per client IP across /webhook/* (closes #139) #143
Reference in New Issue
Block a user
Delete Branch "issue-139-aggregate-receiver-ratelimit"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #139.
What changed
ReceiverRateLimitnow chains two limiters instead of one.RECEIVER_RATE_LIMIT/min./webhook/*route.The inner limiter keys on the path, and
/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 — unbounded aggregate rate against the only unauthenticated endpoint, with every request reaching an entrypoint lookup before it 404ed. The outer limiter is what bounds that. The per-entrypoint limit is untouched and still does its job (one sender cannot starve another sender or another entrypoint).Limit chosen
Aggregate =
10 * RECEIVER_RATE_LIMITper minute (1200/min = 20/s by default): enough headroom for one sender address to drive ten entrypoints at their full per-entrypoint rate, while capping what a single address can cost the receiver. Derived from the existing setting rather than a new one, so nointernal/config/config.gochange. The multiplication saturates atmath.MaxInt— nothing boundsRECEIVER_RATE_LIMITfrom above and a wrapped negative limit would reject every request.Logging
One code change, aimed at the log-volume clause of the DoD, plus the INFO move in the handler.
HandleWebhookmoved below the entrypoint lookup. The UUID is attacker-controlled path text; logging it before the lookup let a client write one INFO line per invented path. After the move an unknown entrypoint produces only the pre-existing DEBUG line (internal/handlers/webhook.go:128).tooManyRequestshandler. It has its own,floodTooManyRequests(internal/middleware/ratelimit.go:202-209), which logs at DEBUG and withoutr.URL.Path. Its rejections are one line per request of the very flood it exists to bound, so the shared handler (WARN, with the path) would have let a client write its own text into the operator's log at an alerting level, once per request. DEBUG is off by default (internal/logger/logger.go:40).Why DEBUG-and-drop-the-path rather than periodic emission: DEBUG is off by default, so a flood costs zero lines in production, and dropping the path means turning DEBUG on to diagnose one does not reinstate attacker-chosen text; per-key periodic emission would need a keyed map of last-logged times on an unauthenticated endpoint, which is a new unbounded-growth surface for the same attacker.
What this route still writes above DEBUG. Two sources, and the README now says so:
tooManyRequests, which logs at WARN with"path", r.URL.Path(internal/middleware/ratelimit.go:181). It sits behind the aggregate limiter but in front of the handler, so it fires on paths that name no entrypoint. A client hammering one invented path is servedRECEIVER_RATE_LIMITrequests and has the rest of its aggregate budget rejected there — up to 1080 WARN lines/min per client IP at the defaults, each carrying a path the client chose. That WARN is pre-existing and was uncapped before this PR; the aggregate limit is what now bounds it. Dropping the path from the per-entrypoint rejection log too would be a change to the untouched half of the limiter and is not made here.internal/middleware/middleware.go:121) writes one INFO line per request with"url", r.URL.String()— the full attacker-controlled URL including query. It is a router-levelUse, so it runs before the route limiter and records the 429s too. It is bounded by neither limit and is out of scope here (#146).So: the aggregate limit bounds the database work an invented path costs and caps WARN volume; it does not eliminate either, and it does not bound access-log volume at all.
Mutation evidence
Two tests, one for the bypass and one for the chaining order. No existing test was modified.
TestReceiverRateLimit_LimitsAggregateAcrossInventedPathssends10 * limitrequests from one IP, each to a path it has never used, then one more. With the aggregate limiter removed (returning onlyperEntrypoint) it is the only failure in the suite:TestReceiverRateLimit_RejectedRequestsCountTowardAggregatepins the order. It spends the whole aggregate budget on one path — the first 3 served, the other 27 rejected by the inner limiter — then requires a request to a path the client has never used to be rejected, which only the aggregate limiter can do. Mutatingaggregate(perEntrypoint(next))toperEntrypoint(aggregate(next)), it is the only failure in the suite:All 13 other
TestReceiverRateLimit_*/TestRateLimitKey_*/TestReceiverAggregateLimit_*tests passed under that mutation. Mutations were made in a throwaway checkout and reverted; nothing mutated is committed.Gate evidence
Head
20c0d13, rebased ontonext(be57609).make check: exit 0,0 issues., 438--- PASS, 0--- FAIL, 0(cached).docker build --no-cache-filter=lint,builder --progress=plain .: exit 0, both stages really executed —#16 [lint 8/8] RUN make lintDONE 86.2s →#16 81.42 0 issues.#28 [builder 8/10] RUN make testDONE with 438--- PASS, 0--- FAIL, 0(cached)Rework since
b5b3e1ainternal/middleware/ratelimit.go: addedfloodTooManyRequests(DEBUG, no path) and wired the aggregate limiter to it instead of the sharedtooManyRequests. The shared handler is unchanged and still used by the receiver per-entrypoint, login and password-change limiters.internal/middleware/ratelimit_test.go: addedTestReceiverRateLimit_RejectedRequestsCountTowardAggregateto pin the chaining order (sibling test; the existing aggregate test is untouched).README.md: rewrote the logging paragraph in the rate-limiting section. Added a sentence noting that withTRUSTED_PROXIESunset behind the mandated reverse proxy the aggregate limit is a service-wide 1200/min ceiling across all senders and entrypoints — unlike the per-entrypoint limit, whose capacity grows with entrypoint count — and that such deployments must setTRUSTED_PROXIES.Second rework pass (
ae74852→20c0d13), text onlyAddressing #143 (comment). No Go statement changed;
git diff ae74852 HEAD -- '*.go'is comment lines only.README.md:869-882: replaced the false "nothing on this route writes it to the log aboveDEBUG" claim. The paragraph now states that the handler miss and the aggregate limiter's own rejections are at DEBUG (off by default), that the per-entrypoint limiter still logs every rejection at WARN with the attacker-controlled path — bounded by the aggregate ceiling to under ten timesRECEIVER_RATE_LIMITper minute per client IP, 1080 at the defaults, where before it there was no bound — and that the access log is bounded by neither, recording every request once at INFO with its full URL. Checked against the whole section for contradictions, including theTRUSTED_PROXIESservice-wide-ceiling paragraph, which is unchanged.internal/middleware/ratelimit.go:174: the comment no longer claimstooManyRequestsis "shared by every limiter"; it names the login, password-change and per-entrypoint receiver limiters and points atfloodTooManyRequestsfor the aggregate one. Comment only; the function body is untouched.3941f0bontobe57609(#138). No conflict; that PR touchedREADME.mdoutside this section. Because the rebase pulled inDockerfile,.dockerignoreand CI-workflow changes, the containerized build was re-run, not justmake check.10 * RECEIVER_RATE_LIMITmultiplier and 1200/min default, no new config variable, the limiter ordering,receiverAggregateLimit,floodTooManyRequests, and every test.Scope
IPv6 bucketing and the trusted-proxy key function (
rateLimitKey,forwardedClientAddr,normalizeAddr) are untouched.TODO.mdandinternal/config/config.gountouched.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.FAIL —
needs-rework.The security fix itself is correct: chaining order is right (
aggregateoutermost atinternal/middleware/ratelimit.go:297, so it counts requests the inner limiter rejects), no method/header/query bypass, saturation verified at the boundary, per-entrypoint behaviour byte-identical, mutation reproduced independently. Findings are on the log-volume half of the definition of done.1.
internal/middleware/ratelimit.go:290-293— the log-volume clause of the DoD is not met, and the change makes the flood's log output more severe, not less.The new aggregate limiter is wired to the shared
tooManyRequestshandler, which atinternal/middleware/ratelimit.go:181doesm.log.Warn(logMessage, "path", r.URL.Path)— one line per rejected request, carrying the attacker-controlled path. Before this change a path-varying flood tripped no limiter at all, so it produced zero WARN lines; after it, every request past 1200/min produces one. The PR's own gate output shows it:level=WARN msg="webhook receiver aggregate rate limit exceeded" path=/webhook/invented-30.Meanwhile the global access log at
internal/middleware/middleware.go:121logs at INFO with"url", r.URL.String()(full attacker-controlled URL including query). It is a router-levelUse, so it runs before the route-level limiter and records the 429s too.Net for a flooding client: two log lines per request before the fix (access INFO + handler INFO), two after (access INFO + limiter WARN) — same volume, second line now at a level that trips alerting. The DoD bullet in #139 reads "Unknown-entrypoint requests must not be able to drive unbounded log volume or unbounded database lookups". Database lookups are now bounded (good, and that is the important half). Log volume is not. The change removes attacker-controlled path text from
internal/handlers/webhook.goat INFO and routes the same traffic into a line logging the same text at WARN.Acceptable: give the aggregate limiter a rejection log that omits
r.URL.Pathand/or sits at DEBUG, and say in the README that the aggregate limiter bounds database work rather than log lines. Changing the sharedtooManyRequestsfor the login/password limiters is not being asked for — those are low-volume endpoints; this one is not.2.
README.md:864— factually wrong as written."Requests to a
/webhook/path that names no entrypoint are logged atDEBUGonly" is false: the access log records them at INFO with the full URL (internal/middleware/middleware.go:121), and once over the aggregate limit each also produces a WARN (internal/middleware/ratelimit.go:181). The trailing "the request itself still appears in the access log" contradicts "DEBUGonly" two clauses earlier. Acceptable: state that the handler no longer logs the UUID at INFO for a miss, and that the request is still recorded once by the access log at INFO.3.
internal/middleware/ratelimit.go:297— chaining order is not pinned by any test (non-blocking).I mutated
aggregate(perEntrypoint(next))toperEntrypoint(aggregate(next))and the whole suite still passed. The shipped order is the stricter and correct one, butTestReceiverRateLimit_LimitsAggregateAcrossInventedPathsnever trips the inner limiter (one request per path against a limit of 3), so nothing catches a later reversal. Acceptable: exhaust one path against the inner limit first, then assert those rejected requests still counted toward the aggregate.Operational note on 1200/min (judgement, not a defect). Per real client IP the number is defensible — 20/s from one sender address, ten entrypoints' worth of headroom. The exposure is the deployment shape
REPO_POLICIES.md:328mandates: behind a reverse proxy withTRUSTED_PROXIESunset, both limits key on the proxy, and this is the first limit that is service-wide rather than per-entrypoint. A deployment with more than ten busy entrypoints could previously serve 10 x 120/min and up, and is now capped at 1200/min in total across all senders. The existing README paragraph covers shared bucketing generally ("the safe direction to be wrong in") but that framing is about security, not availability, and it predates a global cap. Worth one sentence in the rate-limiting section; not a reason to change the multiplier.Gate evidence (independent, own clone, head
b5b3e1a):make checkexit 0,0 issues., 433--- PASS, 0(cached).docker build --no-cache-filter=lint,builder --progress=plain .exit 0 —#20 [lint 7/8] RUN make fmt-checkDONE 0.9s,#21 [lint 8/8] RUN make lintDONE 83.5s with0 issues.,#28 [builder 8/10] RUN make testDONE 70.3s with both new tests passing in-container and 0(cached). Mutation reproduced: returning onlyperEntrypointfails exactlyTestReceiverRateLimit_LimitsAggregateAcrossInventedPathsand nothing else. Repo CI green onb5b3e1a(3m51s). Fast-forwards ontonext(543005c) — no rebase needed. Single commit, title ends(closes #139), basenext,TODO.mdandinternal/config/config.gountouched,ratelimit_test.godiff vsnextis +72/-0 as claimed, no attribution trailers, terminology clean.Disclosure: mutation testing required temporarily editing
internal/middleware/ratelimit.goin my own throwaway clone; reverted, nothing committed or pushed.b5b3e1a926toae74852ea2Reworked at
ae74852, rebased ontonext(3941f0b). Detail is in the PR body; point-by-point against #143 (comment):1. The aggregate limiter now uses its own 429 handler,
floodTooManyRequests, logging at DEBUG with nopathattribute; the sharedtooManyRequestsis unchanged and still serves the per-entrypoint, login and password-change limiters. DEBUG is off by default, so a flood costs zero lines in production, and dropping the path means enabling DEBUG to diagnose one does not reinstate attacker-chosen text. Periodic emission was rejected: a keyed map of last-logged times on the unauthenticated receiver is a new growth surface for the same attacker. In-container evidence:level=DEBUG msg="webhook receiver aggregate rate limit exceeded", nopath=.On the access log: agreed,
internal/middleware/middleware.go:121is now the dominant remaining source — one INFO line per request with the full URL, including the 429s. Not fixed here. Bounding it means suppressing or sampling the global access log for every route in the service, which is outside this issue. Stated explicitly in the PR body under Logging so it can be filed separately.2.
README.mdparagraph rewritten: the handler records a miss at DEBUG, the aggregate limiter logs rejections at DEBUG without the path, and every request is still recorded once by the access log at INFO with its full URL — so the aggregate limit bounds database work, not log lines. Added to the rate-limiting section: withTRUSTED_PROXIESunset behind the mandated reverse proxy the aggregate limit is a service-wide 1200/min ceiling across all senders and entrypoints, where the per-entrypoint limit's capacity grows with entrypoint count; such deployments must setTRUSTED_PROXIES.3. Added
TestReceiverRateLimit_RejectedRequestsCountTowardAggregateas a sibling; the existing test is untouched. It spends the whole aggregate budget on one path (3 served, 27 rejected by the inner limiter) then requires a request to an unused path to be rejected. UnderperEntrypoint(aggregate(next))it is the only failure in the suite: expected 429, actual 200.Multiplier, default, ordering, saturating helper and all pre-existing tests unchanged. Gate:
make checkexit 0,0 issues., 438--- PASS;docker build --no-cache-filter=lint,builder --progress=plain .exit 0 with#18 [lint 8/8] RUN make lint61.4s /0 issues.and#30 [builder 8/10] RUN make test81.3s, 0(cached).FAIL —
needs-rework. Findings 1 and 3 from #143 (comment) are fixed and independently verified; finding 2 is not.Verified fixed.
floodTooManyRequests(internal/middleware/ratelimit.go:202-209) logs at DEBUG with no path and is wired only to the aggregate limiter (:315); the sharedtooManyRequests(WARN + path) still serves the receiver per-entrypoint limiter (:305) and, viapostRateLimit(:256), the login and password-change limiters — no observability loss on the auth endpoints. DEBUG is off by default (internal/logger/logger.go:40). Order-pinning reproduced independently: mutating:322toperEntrypoint(aggregate(next))fails exactlyTestReceiverRateLimit_RejectedRequestsCountTowardAggregate(ratelimit_test.go:770, expected 429, actual 200) and nothing else — 437 PASS / 1 FAIL, 0(cached). No existing test modified (ratelimit_test.go+124/-0).RETENTION_SWEEP_INTERVALenv-table row appears exactly once (README.md:96).1.
README.md:871-872— the replacement paragraph is false, in the same way the version it replaces was."The path is attacker-controlled, so nothing on this route writes it to the log above
DEBUG" is wrong twice:tooManyRequests, which doesm.log.Warn(logMessage, "path", r.URL.Path)atinternal/middleware/ratelimit.go:181. It sits behind the aggregate limiter but in front of the handler (internal/server/routes.go:174), so it fires on paths that name no entrypoint. A client hammering ONE invented path getsRECEIVER_RATE_LIMITrequests served and everything up to the aggregate ceiling rejected — up to 1080 WARN lines per minute per IP, each carrying a path of the client's choosing. Observed in my own runs at the shipped, unmutated order, both on host and in-container: 30 xlevel=WARN msg="webhook receiver rate limit exceeded" path=/webhook/exhausted.DEBUGonly" … "still appears in the access log") that finding 2 rejected.Why it matters: this is an operational claim about alerting. An operator reading it concludes that no attacker-chosen text can reach a WARN-level line from the receiver route. It can, at roughly 1080/min per IP. The underlying WARN is pre-existing and this PR bounds it (it was uncapped before), so the code is an improvement and is not the defect — the newly written sentence asserting otherwise is.
Acceptable: state that per-entrypoint rejections are still logged at WARN with the path, bounded by the aggregate limit to about 10x
RECEIVER_RATE_LIMITlines/min per client; or drop the path from the receiver per-entrypoint rejection log as well. Either way the "nothing aboveDEBUG" claim has to go.2.
internal/middleware/ratelimit.go:174— comment made stale by this change (minor). "tooManyRequests returns the 429 handler shared by every limiter" is no longer true now that the aggregate limiter has its own. Acceptable: name the limiters it actually serves.Everything else checked and clean: the rest of that README paragraph is accurate (handler miss at DEBUG,
internal/handlers/webhook.go:128; aggregate rejection DEBUG without path; access log once per request at INFO); theTRUSTED_PROXIES-unset service-wide-ceiling sentence (README.md:887-894) matchesrateLimitKeybehaviour;Retry-Afteris still set on both limiters (httprate sets it before the custom handler); single commit, title ends(closes #139), basenext, no attribution trailers,TODO.mdandinternal/config/config.gountouched, fast-forwards ontonext(3941f0b), naming and terminology clean.Gate (own clone, head
ae74852):make checkexit 0 —0 issues., 438--- PASS, 0--- FAIL, 0(cached).docker build --no-cache-filter=lint,builder --progress=plain .exit 0 —#21 [lint 8/8] RUN make lintDONE 75.2s with#21 72.61 0 issues.;#33 [builder 8/10] RUN make testDONE 78.0s with 438--- PASS, 0--- FAIL, 0(cached). Repo CI green onae74852.The access log at
internal/middleware/middleware.go:121is out of scope per #146; I do not find it materially worse than that issue describes.Disclosure: the order-pinning mutation required editing
internal/middleware/ratelimit.goin a separate throwaway copy of my own clone; nothing was committed or pushed, and the reviewed checkout was never modified.ae74852ea2to20c0d13a25PASS at
20c0d13—git diff ae74852 20c0d13 -- '*.go'is one comment hunk and no Go statement; every clause ofREADME.md:869-882verified against source (handler miss DEBUGwebhook.go:128,floodTooManyRequestsDEBUG-no-pathratelimit.go:204-211, default level INFOlogger.go:40, per-entrypoint WARN+pathratelimit.go:183wired at:307, WARN ceiling 9 xRECEIVER_RATE_LIMIT= 1080/min < the 1200 aggregate, access log router-levelUseatroutes.go:35ahead ofroutes.go:174so it records 429s once at INFO with the full URL),ratelimit.go:174-178comment correct (tooManyRequestsused at:258/:307only, httprate setsRetry-Afteratlimiter.go:100before the limit handler), no contradicting logging claim elsewhere in the README and theTRUSTED_PROXIESparagraph still consistent, mergeable ontonext(0e397b3), CI green, single commit titled(closes #139),TODO.mduntouched. Disclosure: the containerized gate was not re-run — the Go tree is byte-identical to the tree two prior reviews gated — and themake checkrun here (438 PASS / 0 FAIL / 0(cached)/0 issues.) lints on the host, so its lint result is not offered as evidence.