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

Gate forwarded-header trust behind trusted-proxy config in rate limiters #88

New Issue
Closed
opened 2026-08-07 19:11:38 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-07 19:11:38 +02:00
Collaborator
Copy Link

Tracking issue from the PR #87 review (advisory, non-blocking there because it matches the agreed plan and the existing LoginRateLimit pattern).

Problem

Both ReceiverRateLimit and LoginRateLimit (internal/middleware/ratelimit.go) key on httprate.KeyByRealIP, which trusts True-Client-IP, X-Real-IP, and the first X-Forwarded-For entry unconditionally. The first XFF entry is client-controlled even behind an appending reverse proxy, so a deliberate attacker can:

  • bypass the limit entirely by rotating spoofed XFF values (fresh bucket per request), or
  • starve a legitimate sender's bucket by spoofing that sender's IP.

REPO_POLICIES.md requires trusted-proxy-gated forwarded-header handling before 1.0.

Definition of done

  • Forwarded headers are honored only when the direct peer (RemoteAddr) is within a configured trusted-proxy set (env-configured CIDR list; set-but-unparseable value aborts startup, per repo policy). Otherwise the key is RemoteAddr.
  • Applies to BOTH limiters (receiver and login) via one shared key function.
  • Tests: spoofed XFF from an untrusted peer does not create a fresh bucket; honored from a trusted peer; unparseable CIDR config aborts startup.

Minor cleanups to fold in (same files, from the same review)

  • internal/config/config_test.go: the RECEIVER_RATE_LIMIT error-path tests assert only assert.Error; tighten to ErrorContains (variable name in the message) / errors.Is(config.ErrNonPositiveValue).
  • internal/middleware/ratelimit.go: the two 429 limit-handler bodies are near-duplicates; extract a shared helper.
Tracking issue from the PR #87 review (advisory, non-blocking there because it matches the agreed plan and the existing `LoginRateLimit` pattern). ## Problem Both `ReceiverRateLimit` and `LoginRateLimit` (`internal/middleware/ratelimit.go`) key on `httprate.KeyByRealIP`, which trusts `True-Client-IP`, `X-Real-IP`, and the first `X-Forwarded-For` entry unconditionally. The first XFF entry is client-controlled even behind an appending reverse proxy, so a deliberate attacker can: - bypass the limit entirely by rotating spoofed XFF values (fresh bucket per request), or - starve a legitimate sender's bucket by spoofing that sender's IP. `REPO_POLICIES.md` requires trusted-proxy-gated forwarded-header handling before 1.0. ## Definition of done - Forwarded headers are honored only when the direct peer (`RemoteAddr`) is within a configured trusted-proxy set (env-configured CIDR list; set-but-unparseable value aborts startup, per repo policy). Otherwise the key is `RemoteAddr`. - Applies to BOTH limiters (receiver and login) via one shared key function. - Tests: spoofed XFF from an untrusted peer does not create a fresh bucket; honored from a trusted peer; unparseable CIDR config aborts startup. ## Minor cleanups to fold in (same files, from the same review) - `internal/config/config_test.go`: the `RECEIVER_RATE_LIMIT` error-path tests assert only `assert.Error`; tighten to `ErrorContains` (variable name in the message) / `errors.Is(config.ErrNonPositiveValue)`. - `internal/middleware/ratelimit.go`: the two 429 limit-handler bodies are near-duplicates; extract a shared helper.
clawbot commented 2026-08-07 19:12:09 +02:00
Author
Collaborator
Copy Link

Scope note: the PR #83 review found the same unconditional forwarded-header trust in the new PasswordChangeRateLimit (added in commit 8362ce9, shares the postRateLimit helper with LoginRateLimit). This issue covers ALL rate limiters — receiver, login, and password change — via the one shared key function described in the DoD.

Scope note: the PR #83 review found the same unconditional forwarded-header trust in the new `PasswordChangeRateLimit` (added in commit 8362ce9, shares the `postRateLimit` helper with `LoginRateLimit`). This issue covers ALL rate limiters — receiver, login, and password change — via the one shared key function described in the DoD.
clawbot added this to the 1.0.0 milestone 2026-08-11 14:34:08 +02:00
clawbot commented 2026-08-11 15:03:48 +02:00
Author
Collaborator
Copy Link

Implemented in #122.

All three limiters (receiver, login, password change) now share one key function: the connection's own address, unless the direct peer is inside a network listed in the new TRUSTED_PROXIES CIDR list, in which case the forwarded client address is used. The list is empty by default, so nothing is trusted until an operator names their proxy; a set-but-unparseable value aborts startup (config.ErrInvalidCIDR), matching the other parsed variables. Within a trusted request X-Forwarded-For is walked right to left and the first hop that is not itself a trusted proxy wins, so client-prepended entries cannot be selected. Documented in the README.

Both folded-in cleanups are in the same commit: the 429 responder is extracted into one helper, and the RECEIVER_RATE_LIMIT error-path tests now assert the failure names the variable and, for zero/negative, wraps ErrNonPositiveValue.

Verified: the bypass test bites — reverting the key function to plain httprate.KeyByRealIP makes TestRateLimitKey_SpoofedForwardedFromUntrustedPeer fail (expected 429, actual 200) for all three header names, along with the chain-walk and receiver variants; restoring it makes them pass. make check exit 0, and docker build --no-cache-filter=lint,builder ran make lint (0 issues) and make test in-container with no cached package results.

Implemented in https://git.eeqj.de/sneak/webhooker/pulls/122. All three limiters (receiver, login, password change) now share one key function: the connection's own address, unless the direct peer is inside a network listed in the new `TRUSTED_PROXIES` CIDR list, in which case the forwarded client address is used. The list is empty by default, so nothing is trusted until an operator names their proxy; a set-but-unparseable value aborts startup (`config.ErrInvalidCIDR`), matching the other parsed variables. Within a trusted request `X-Forwarded-For` is walked right to left and the first hop that is not itself a trusted proxy wins, so client-prepended entries cannot be selected. Documented in the README. Both folded-in cleanups are in the same commit: the 429 responder is extracted into one helper, and the `RECEIVER_RATE_LIMIT` error-path tests now assert the failure names the variable and, for zero/negative, wraps `ErrNonPositiveValue`. Verified: the bypass test bites — reverting the key function to plain `httprate.KeyByRealIP` makes `TestRateLimitKey_SpoofedForwardedFromUntrustedPeer` fail (expected 429, actual 200) for all three header names, along with the chain-walk and receiver variants; restoring it makes them pass. `make check` exit 0, and `docker build --no-cache-filter=lint,builder` ran `make lint` (0 issues) and `make test` in-container with no cached package results.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#88
Delete Branch "%!s()"

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?