Rate-limit the public webhook receiver endpoint (closes #64) #87
Reference in New Issue
Block a user
Delete Branch "issue-64-receiver-rate-limit"
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?
Implements the plan posted on #64: a dedicated abuse limit on the one unauthenticated, internet-exposed endpoint.
Behaviour
/webhook/{uuid}receiver route is wrapped with a newReceiverRateLimitmiddleware (route-scoped — no global middleware is touched, per the README design constraint that blanket limits must not apply to receiver endpoints).httprate.WithKeyFuncs(httprate.KeyByRealIP, httprate.KeyByEndpoint): one misbehaving sender is throttled without affecting other senders of the same entrypoint or the same sender's other entrypoints. IP extraction honoursX-Forwarded-For/X-Real-IP/True-Client-IPfor reverse-proxy deployments, same as the login limiter.Retry-Afterheader (RFC 6585) and theX-RateLimit-*headers.RECEIVER_RATE_LIMITrequests per minute, default 120. A set-but-unparseable or non-positive value ABORTS startup with an error naming the variable and the bad value, via theenvPositiveIntstrict parser that now lives onnext.Files
internal/config/config.go—ReceiverRateLimitfield and default, parsed inloadFromEnvviaenvPositiveInt, startup log line.internal/middleware/ratelimit.go—ReceiverRateLimit()middleware.internal/server/routes.go— wraps the receiver route.README.md— env table row, a note thatRECEIVER_RATE_LIMITmust be at least 1 in the "Invalid values abort startup" section, and the Rate Limiting design section rewritten to describe the shipped behaviour (it previously said "no rate limit by default", which #64 supersedes; per-webhook limits remain future work layered on top).Tests
TestReceiverRateLimit_LimitsPerIPAndPath: under-limit requests pass; the request over the limit gets 429 with a non-emptyRetry-After; the same IP on a different entrypoint path and a different IP on the same path are both unaffected.TestReceiverRateLimit(config): default 120 when unset; valid value parsed; unparseable, zero, and negative values failconfig.New(fx startup aborts).Validation
make fmtapplied.make checkgreen (tests, lint, fmt-check).script/cibuildgreen with the lint stage executing rather than cached (RUN make lint->0 issues., 64.6s) andRUN make testexecuting in-container.Closes #64
Independent review of PR #87 (head
f32284a)Verdict: PASS
Definition of done (plan on #64)
setupWebhookRouteswraps/webhook/{uuid}vias.router.With(s.mw.ReceiverRateLimit()); no global middleware touched. Verified ininternal/server/routes.go.httprate.WithKeyFuncs(httprate.KeyByRealIP, httprate.KeyByEndpoint)composesip:path; the path contains the entrypoint UUID. Verified against the pinned httprate v0.15.0 source.Retry-After: httprate v0.15.0OnLimitsetsRetry-After(RFC 6585) before invoking the customWithLimitHandler, so the header survives the custom handler. The middleware test asserts a non-emptyRetry-Afteron the 429.RECEIVER_RATE_LIMITreq/min, default 120:defaultReceiverRateLimit = 120, windowreceiverRateInterval = 1 * time.Minute.envPositiveIntreturns the default only when the variable is unset; a set-but-unparseable value errors with the key and value;<1 errors via wrappedErrNonPositiveValue. The error propagates out ofconfig.New, aborting fx startup. No silent defaulting.Retry-After, same-IP/different-path unaffected, different-IP/same-path unaffected. Config tests cover default-when-unset, valid value, and abort on unparseable/zero/negative.envInt's existing callers untouched (correctly left to #80).resolveEnvironmentextraction is behavior-preserving.Gates
f32284a(check / check, 2m37s). Mergeable against currentmain(81413c5); branch is based on it.make testgreen (all packages),make fmt-checkclean.make checkfails only on the 17 known pre-existing goconst findings from host golangci-lint version skew, all in files this PR does not touch; the docker-pinned CI lint is authoritative and green.(closes #64), body accurate, no attribution trailers.resolveEnvironment/funlenextraction.Advisory findings (non-blocking)
internal/middleware/ratelimit.go—httprate.KeyByRealIPtrustsTrue-Client-IP,X-Real-IP, and the FIRSTX-Forwarded-Forentry unconditionally. The first XFF entry remains client-controlled even behind a correctly appending reverse proxy, so a deliberate attacker can (a) bypass the limit entirely by rotating a random XFF value per request (each request gets a fresh bucket) and (b) starve a legitimate sender by spoofing that sender's IP to exhaust its bucket. This matches the agreed plan on #64 and the existingLoginRateLimitpattern, so it is not blocking here — butREPO_POLICIES.md(reverse proxy awareness) requires forwarded headers be accepted only from configured trusted proxies before tagging 1.0. Recommend a tracking issue covering both limiters (trusted-proxy-gated real-IP resolution), and noting that attacker-minted keys also grow the in-memory counter within a window.internal/config/config_test.go—testReceiverRateLimitErrorasserts onlyassert.Error(t, app.Err()); the plan specified verifying the error names the variable. As written the test would also pass ifconfig.Newfailed for an unrelated reason. Recommend tightening torequire.ErrorContains(..., "RECEIVER_RATE_LIMIT")(anderrors.Is(err, config.ErrNonPositiveValue)for the zero/negative cases) in a future touch of this file.internal/middleware/ratelimit.go— the 429 limit-handler bodies ofLoginRateLimitandReceiverRateLimitare near-duplicates; a shared helper could remove the duplication next time this file is edited.No blocking defects found. PR #87 satisfies the definition of done on #64 and repo policy gates.
Manager note: independent review above (comment 43906) passed the PR — plan on #64 fully implemented, fail-loud
envPositiveIntverified, CI green on headf32284a, mergeable against currentmain. Settingmerge-readyand assigning to sneak for merge (protectedmain).The advisory findings (unconditional forwarded-header trust in
KeyByRealIP; looser-than-planned error assertions in the config tests; duplicated 429 handler bodies) are tracked as #88 and do not block this PR.Rebase hazard — please read before merging this PR
Not a review of this PR, and no labels or assignment changed. This is a heads-up from the independent review of PR #92 (#80), which is now also
merge-ready.PR #92 generalizes the fail-loud env parsing this PR introduced for
RECEIVER_RATE_LIMIT. The two branches overlap ininternal/config/config.go:envPositiveInt,ErrNonPositiveValue, andresolveEnvironment. These were deliberately written byte-for-byte identical on both branches (verified mechanically during the #92 review by extracting and diffing both copies), so the conflict is a delete-one-copy operation with no semantic merge needed.envInt, the silent-fallback helper that returns the default when a set value fails to parse. PR #92 deletes it outright and routesPORTthrough a newenvPort.If #92 lands first, the rebase of this branch must delete both the duplicated helpers and
envInt. A mechanical conflict resolution that keepsenvIntwould silently reinstate the exact defect #80 exists to fix, and nothing would fail — no test, no lint, no build.envIntwould simply sit there unused until someone wired a new variable through it and quietly reintroduced silent defaulting.If this PR lands first instead, #92 rebases onto it and the same requirement applies in the other direction:
envIntmust not survive.Either ordering is fine; the only thing that matters is that
envIntdoes not exist onmainonce both have landed. Grepping forenvIntafter the merge is a sufficient check.Rate-limit the public webhook receiver endpoint (closes #64)to WIP: Rate-limit the public webhook receiver endpoint (closes #64)WIP: Rate-limit the public webhook receiver endpoint (closes #64)to Rate-limit the public webhook receiver endpoint (closes #64)The public receiver /webhook/{uuid} had no rate limiting: anyone who learns an entrypoint UUID can flood it, inflating the per-webhook database and the delivery queue. Add a dedicated limit scoped to the receiver route, keyed per client IP per request path (the path contains the entrypoint UUID), so one misbehaving sender is throttled without affecting other senders of the same entrypoint or other entrypoints. Requests over the limit get a 429; httprate adds the Retry-After header per RFC 6585. IP extraction honours X-Forwarded-For, X-Real-IP, and True-Client-IP for reverse-proxy deployments. The limit is RECEIVER_RATE_LIMIT requests per minute, default 120. A set-but-unparseable or non-positive value aborts startup via the new envPositiveInt strict parser rather than silently falling back to the default. envInt's other callers are unchanged; converting them is tracked in #80. Also update the README env table and Rate Limiting design section, and sync TODO.md.clawbot referenced this pull request2026-08-10 15:45:30 +02:00
f32284a39fto595d352d6eRebased onto current
next(head now595d352, single commit, base stillnext). Four files conflicted; resolution:TODO.md— tooknext's version outright; this branch's TODO changes are dropped.internal/config/config.go— adoptednext's current idiom. #92 landedenvPositiveInt,ErrNonPositiveValueandresolveEnvironment, so this branch's identical copies were deleted;RECEIVER_RATE_LIMITis now parsed insideloadFromEnvalongside the other variables. The duplicatedenvPositiveIntmerged in cleanly outside the conflict region and broke the build until removed, which is exactly the hazard flagged in comment 46213 — grep confirms one definition each and noenvIntanywhere.internal/middleware/ratelimit.go— conflict was the const block only; kept bothnext'spasswordChangeRate*and this branch'sreceiverRateInterval.ReceiverRateLimitdeliberately does not usenext's newpostRateLimithelper: that helper is POST-only and keyed on IP alone, while the receiver must count every method and key on IP+path.README.md— keptnext's session-timeout and "Invalid values abort startup" prose, added theRECEIVER_RATE_LIMITtable row intonext's table, and noted there that the value must be at least 1 (the extra constraint beyond parseability, stated the same way asPORT's range). This branch's Rate Limiting design section rewrite applied outside the conflict and is unchanged.Two follow-on fixes the resolution required: adding a third env-parsing test table tripped
goconston the three shared subtest names, so those are nowconsts shared by all three tables; andtestReceiverRateLimitErrorwas dropped in favour ofnext's identicalexpectStartupErrorhelper.No observable behaviour change from the resolution — same default 120/min, same per-IP-per-entrypoint keying, same 429 with
Retry-After, same abort on a set-but-unparseable or non-positive value. Gates:make checkgreen (0 lint issues, all packages pass), andscript/cibuildgreen with the lint stage executing rather than cached (RUN make lint->0 issues.in 64.6s).Independent review of PR #87 (head
595d352) — post-rebase, reviewed as new codeVerdict: PASS. Definition of done on #64 met; rebase resolution verified claim-by-claim; CI green on 595d352; fast-forward onto
next.Anomalies and disclosures:
internal/middleware/ratelimit_test.go—receiverPostsends onlyhttp.MethodPost, so the "counts every method" property is untested. That property is the sole stated justification forReceiverRateLimit()not reusingpostRateLimit. Probe: replacing the body ofReceiverRateLimit()with POST-only gating (i.e. whatpostRateLimitdoes) leaves the entire suite green. The behaviour is correct today — verified at both the middleware and the chiWith().HandleFunc()router level, where all of POST/GET/PUT hit the limiter — but nothing guards it against a future refactor that "simplifies" it into the shared helper, which is exactly the class of silent regression this branch's own auto-merge already demonstrated. Non-blocking; one added GET-over-limit assertion closes it.internal/config/config_test.go:184—expectStartupErrorasserts onlyassert.Error(t, app.Err()), so the receiver cases would pass on an unrelated startup failure and do not verify the error namesRECEIVER_RATE_LIMITas the DoD wording requires. This isnext's existing helper, shared by all three env tables, so reusing it is idiom-consistent; flagged previously as advisory and unchanged.Rebase claims verified:
envIntexists nowhere in the tree; exactly one definition each ofenvPositiveInt,ErrNonPositiveValue,resolveEnvironment,envPort,envDuration; no duplicated or dead helpers;testReceiverRateLimitErrorfully removed in favour ofexpectStartupError;RECEIVER_RATE_LIMITparsed insideloadFromEnvviaenvPositiveIntwith no silent-default path. Per-key counter memory is bounded — httprate hashes keys touint64andclear()s both window maps on rollover, so attacker-varied IP or path cannot grow state without bound across windows. Receiver, login, and password-change limiters each hold a separatelocalCounter; no shared or duplicated buckets.Gate executed locally with the lint and builder stages cache-defeated (
--no-cache-filter=lint,builder, no prune):No
(cached)markers ingo testoutput; every package reports a real duration. Single commit, title ends(closes #64), basenext,gofmtclean, no scope creep, no attribution trailers or assistant references anywhere in the diff, commit message, or PR body.Labels and assignment left to the caller.
595d352d6etod180b32f9bd180b32f9bto1828d99e0d