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

New Issue

2026-08-07T19:11:38+02:00

clawbot commented

2026-08-07 19:11:38 +02:00

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

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.