sneak/webhooker
1
0
Fork 0
Code Issues 13 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Rate-limit the public webhook receiver endpoint (closes #64) #87

Open
clawbot wants to merge 1 commits from issue-64-receiver-rate-limit into main
pull from: issue-64-receiver-rate-limit
merge into: sneak:main
Conversation 2 Commits 1 Files Changed 7 +356 -41
clawbot commented 2026-08-07 18:52:09 +02:00
Collaborator
Copy Link

Implements the plan posted on #64: a dedicated abuse limit on the one unauthenticated, internet-exposed endpoint.

Behaviour

  • The /webhook/{uuid} receiver route is wrapped with a new ReceiverRateLimit middleware (route-scoped — no global middleware is touched, per the README design constraint that blanket limits must not apply to receiver endpoints).
  • The limit is keyed per client IP per request path via 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 honours X-Forwarded-For / X-Real-IP / True-Client-IP for reverse-proxy deployments, same as the login limiter.
  • Requests over the limit receive HTTP 429 with a clear message; httprate sets the Retry-After header (RFC 6585) and the X-RateLimit-* headers.
  • The limit is RECEIVER_RATE_LIMIT requests 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 a new strict envPositiveInt parser (the envDuration fail-loud pattern). envInt's existing callers are unchanged — converting them is #80's scope.
  • config.New's environment validation was extracted into resolveEnvironment to keep the function within the funlen limit after the new parse block.

Files

  • internal/config/config.go — ReceiverRateLimit field, envPositiveInt, ErrNonPositiveValue, resolveEnvironment extraction, startup log line.
  • internal/middleware/ratelimit.go — ReceiverRateLimit() middleware.
  • internal/server/routes.go — wraps the receiver route.
  • README.md — env table row; 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).
  • TODO.md — synced (stale Next Step was #63, already merged).

Tests

  • TestReceiverRateLimit_LimitsPerIPAndPath: under-limit requests pass; the request over the limit gets 429 with a non-empty Retry-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 fail config.New (fx startup aborts).

Validation

make fmt applied; make test (all 8 packages) and make fmt-check green locally. make lint on the host shows only the 17 pre-existing goconst findings in files this PR does not touch (host golangci-lint is newer than the pinned CI image); none in this diff. CI (docker build . via script/cibuild) on the PR head is the authoritative gate.

Closes #64

Implements the plan posted on #64: a dedicated abuse limit on the one unauthenticated, internet-exposed endpoint. ## Behaviour - The `/webhook/{uuid}` receiver route is wrapped with a new `ReceiverRateLimit` middleware (route-scoped — no global middleware is touched, per the README design constraint that blanket limits must not apply to receiver endpoints). - The limit is keyed per client IP per request path via `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 honours `X-Forwarded-For` / `X-Real-IP` / `True-Client-IP` for reverse-proxy deployments, same as the login limiter. - Requests over the limit receive HTTP 429 with a clear message; httprate sets the `Retry-After` header (RFC 6585) and the `X-RateLimit-*` headers. - The limit is `RECEIVER_RATE_LIMIT` requests 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 a new strict `envPositiveInt` parser (the `envDuration` fail-loud pattern). `envInt`'s existing callers are unchanged — converting them is #80's scope. - `config.New`'s environment validation was extracted into `resolveEnvironment` to keep the function within the `funlen` limit after the new parse block. ## Files - `internal/config/config.go` — `ReceiverRateLimit` field, `envPositiveInt`, `ErrNonPositiveValue`, `resolveEnvironment` extraction, startup log line. - `internal/middleware/ratelimit.go` — `ReceiverRateLimit()` middleware. - `internal/server/routes.go` — wraps the receiver route. - `README.md` — env table row; 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). - `TODO.md` — synced (stale Next Step was #63, already merged). ## Tests - `TestReceiverRateLimit_LimitsPerIPAndPath`: under-limit requests pass; the request over the limit gets 429 with a non-empty `Retry-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 fail `config.New` (fx startup aborts). ## Validation `make fmt` applied; `make test` (all 8 packages) and `make fmt-check` green locally. `make lint` on the host shows only the 17 pre-existing `goconst` findings in files this PR does not touch (host golangci-lint is newer than the pinned CI image); none in this diff. CI (`docker build .` via `script/cibuild`) on the PR head is the authoritative gate. Closes #64
clawbot added 1 commit 2026-08-07 18:52:09 +02:00
Rate-limit the public webhook receiver endpoint (closes #64)
All checks were successful
check / check (push) Successful in 2m37s
Details
f32284a39f 
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 added the needs-review label 2026-08-07 18:52:13 +02:00
clawbot self-assigned this 2026-08-07 18:52:13 +02:00
clawbot commented 2026-08-07 19:10:40 +02:00
Author
Collaborator
Copy Link

Independent review of PR #87 (head f32284a)

Verdict: PASS

Definition of done (plan on #64)

  • Route-scoped limit only: setupWebhookRoutes wraps /webhook/{uuid} via s.router.With(s.mw.ReceiverRateLimit()); no global middleware touched. Verified in internal/server/routes.go.
  • Keyed per client IP per path: httprate.WithKeyFuncs(httprate.KeyByRealIP, httprate.KeyByEndpoint) composes ip:path; the path contains the entrypoint UUID. Verified against the pinned httprate v0.15.0 source.
  • 429 + Retry-After: httprate v0.15.0 OnLimit sets Retry-After (RFC 6585) before invoking the custom WithLimitHandler, so the header survives the custom handler. The middleware test asserts a non-empty Retry-After on the 429.
  • RECEIVER_RATE_LIMIT req/min, default 120: defaultReceiverRateLimit = 120, window receiverRateInterval = 1 * time.Minute.
  • Fail-loud config: envPositiveInt returns the default only when the variable is unset; a set-but-unparseable value errors with the key and value; < 1 errors via wrapped ErrNonPositiveValue. The error propagates out of config.New, aborting fx startup. No silent defaulting.
  • Tests: middleware test covers under-limit pass, over-limit 429 with 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). resolveEnvironment extraction is behavior-preserving.

Gates

  • CI green on head f32284a (check / check, 2m37s). Mergeable against current main (81413c5); branch is based on it.
  • Locally on the head: make test green (all packages), make fmt-check clean. make check fails 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.
  • Commit message: single commit, title ends (closes #64), body accurate, no attribution trailers.
  • README env table row and Rate Limiting section match shipped behavior; TODO.md sync is accurate. No scope creep beyond the declared resolveEnvironment/funlen extraction.

Advisory findings (non-blocking)

  1. internal/middleware/ratelimit.go — httprate.KeyByRealIP trusts True-Client-IP, X-Real-IP, and the FIRST X-Forwarded-For entry 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 existing LoginRateLimit pattern, so it is not blocking here — but REPO_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.
  2. internal/config/config_test.go — testReceiverRateLimitError asserts only assert.Error(t, app.Err()); the plan specified verifying the error names the variable. As written the test would also pass if config.New failed for an unrelated reason. Recommend tightening to require.ErrorContains(..., "RECEIVER_RATE_LIMIT") (and errors.Is(err, config.ErrNonPositiveValue) for the zero/negative cases) in a future touch of this file.
  3. internal/middleware/ratelimit.go — the 429 limit-handler bodies of LoginRateLimit and ReceiverRateLimit are 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.

## Independent review of PR #87 (head f32284a) **Verdict: PASS** ### Definition of done (plan on #64) - Route-scoped limit only: `setupWebhookRoutes` wraps `/webhook/{uuid}` via `s.router.With(s.mw.ReceiverRateLimit())`; no global middleware touched. Verified in `internal/server/routes.go`. - Keyed per client IP per path: `httprate.WithKeyFuncs(httprate.KeyByRealIP, httprate.KeyByEndpoint)` composes `ip:path`; the path contains the entrypoint UUID. Verified against the pinned httprate v0.15.0 source. - 429 + `Retry-After`: httprate v0.15.0 `OnLimit` sets `Retry-After` (RFC 6585) before invoking the custom `WithLimitHandler`, so the header survives the custom handler. The middleware test asserts a non-empty `Retry-After` on the 429. - `RECEIVER_RATE_LIMIT` req/min, default 120: `defaultReceiverRateLimit = 120`, window `receiverRateInterval = 1 * time.Minute`. - Fail-loud config: `envPositiveInt` returns the default only when the variable is unset; a set-but-unparseable value errors with the key and value; `<` 1 errors via wrapped `ErrNonPositiveValue`. The error propagates out of `config.New`, aborting fx startup. No silent defaulting. - Tests: middleware test covers under-limit pass, over-limit 429 with `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). `resolveEnvironment` extraction is behavior-preserving. ### Gates - CI green on head f32284a (`check / check`, 2m37s). Mergeable against current `main` (81413c5); branch is based on it. - Locally on the head: `make test` green (all packages), `make fmt-check` clean. `make check` fails 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. - Commit message: single commit, title ends `(closes #64)`, body accurate, no attribution trailers. - README env table row and Rate Limiting section match shipped behavior; TODO.md sync is accurate. No scope creep beyond the declared `resolveEnvironment`/`funlen` extraction. ### Advisory findings (non-blocking) 1. `internal/middleware/ratelimit.go` — `httprate.KeyByRealIP` trusts `True-Client-IP`, `X-Real-IP`, and the FIRST `X-Forwarded-For` entry 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 existing `LoginRateLimit` pattern, so it is not blocking here — but `REPO_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. 2. `internal/config/config_test.go` — `testReceiverRateLimitError` asserts only `assert.Error(t, app.Err())`; the plan specified verifying the error names the variable. As written the test would also pass if `config.New` failed for an unrelated reason. Recommend tightening to `require.ErrorContains(..., "RECEIVER_RATE_LIMIT")` (and `errors.Is(err, config.ErrNonPositiveValue)` for the zero/negative cases) in a future touch of this file. 3. `internal/middleware/ratelimit.go` — the 429 limit-handler bodies of `LoginRateLimit` and `ReceiverRateLimit` are 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.
clawbot removed their assignment 2026-08-07 19:11:29 +02:00
sneak was assigned by clawbot 2026-08-07 19:11:29 +02:00
clawbot added merge-ready and removed needs-review labels 2026-08-07 19:11:29 +02:00
clawbot commented 2026-08-07 19:12:19 +02:00
Author
Collaborator
Copy Link

Manager note: independent review above (comment 43906) passed the PR — plan on #64 fully implemented, fail-loud envPositiveInt verified, CI green on head f32284a, mergeable against current main. Setting merge-ready and assigning to sneak for merge (protected main).

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.

Manager note: independent review above (comment 43906) passed the PR — plan on #64 fully implemented, fail-loud `envPositiveInt` verified, CI green on head f32284a, mergeable against current `main`. Setting `merge-ready` and assigning to sneak for merge (protected `main`). 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.
All checks were successful
check / check (push) Successful in 2m37s
Details
This pull request has changes conflicting with the target branch.
  • TODO.md
  • internal/middleware/ratelimit.go
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue-64-receiver-rate-limit:issue-64-receiver-rate-limit
git checkout issue-64-receiver-rate-limit
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#87
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?