rosa/vernier

Fork 0

web: Rate limiters key on the TCP peer, ignoring RealIpLayer's resolved client IP #91

New issue

Closed

opened 2026-07-04 17:50:24 +00:00 by rosa · 2 comments

rosa commented

2026-07-04 17:50:24 +00:00

Owner

Every route group in crates/web/src/routes.rs builds its governor with the PeerIp extractor, which reads only ConnectInfo<SocketAddr> — the raw TCP peer address. Meanwhile the middleware stack installs RealIpLayer (routes.rs:55), which resolves the forwarded client IP into a real::RealIp request extension that nothing ever reads.

Behind a reverse proxy, every request shares the proxy's peer IP, so each rate limiter collapses into a single global bucket:

The auth limiter's 10 requests/minute becomes a site-wide cap — one password-guessing bot rate-limits everyone's logins.
Conversely, no individual attacker is ever limited below the shared budget.

If the app is instead deployed directly exposed, PeerIp is correct and RealIpLayer is dead weight. Either way the two layers currently disagree about who the client is.

Suggested fix: decide the intended deployment topology, then either

key the governors off the resolved client IP — axum-governor 2.0.3 offers extractor::Extension<T> (which can key off real::RealIp) or SmartIp, or
drop RealIpLayer if the server genuinely terminates client TCP connections itself.

Note RealIpLayer::default() only trusts forwarded headers when the peer is a private IP, which matters when choosing the trust configuration for the proxy.

Every route group in `crates/web/src/routes.rs` builds its governor with the `PeerIp` extractor, which reads only `ConnectInfo<SocketAddr>` — the raw TCP peer address. Meanwhile the middleware stack installs `RealIpLayer` (`routes.rs:55`), which resolves the forwarded client IP into a `real::RealIp` request extension that nothing ever reads. Behind a reverse proxy, every request shares the proxy's peer IP, so each rate limiter collapses into a single global bucket: - The auth limiter's 10 requests/minute becomes a site-wide cap — one password-guessing bot rate-limits everyone's logins. - Conversely, no individual attacker is ever limited below the shared budget. If the app is instead deployed directly exposed, `PeerIp` is correct and `RealIpLayer` is dead weight. Either way the two layers currently disagree about who the client is. **Suggested fix:** decide the intended deployment topology, then either - key the governors off the resolved client IP — axum-governor 2.0.3 offers `extractor::Extension<T>` (which can key off `real::RealIp`) or `SmartIp`, or - drop `RealIpLayer` if the server genuinely terminates client TCP connections itself. Note `RealIpLayer::default()` only trusts forwarded headers when the peer is a private IP, which matters when choosing the trust configuration for the proxy.

rosa added the

needs-triage

label

2026-07-04 17:51:47 +00:00

rosa commented

2026-07-05 17:40:20 +00:00

Author

Owner

This was generated by AI during triage.

Agent Brief

Category: bug
Summary: Rate limiters key on the TCP peer, so behind the Fly.io proxy every request shares one bucket — rekey them on the resolved client IP.

Deployment topology (decided): production runs behind the Fly.io proxy, which terminates client connections and forwards to the app over Fly's private network. The TCP peer the app sees is always fly-proxy, never the client. Recorded in docs/adr/0012-web-server-runs-behind-the-flyio-proxy.md — read it before starting.

Current behavior:
Every governor in the web router is built with a PeerIp extractor, which keys on ConnectInfo<SocketAddr> — the raw TCP peer. A RealIpLayer in the outer middleware stack resolves the forwarded client IP into a real::RealIp request extension, but nothing reads it. Behind the proxy, all requests share the proxy's peer IP, so each per-IP limiter collapses into a single global bucket: the auth limiter's 10/min becomes a site-wide login cap, and no individual attacker is ever isolated below the shared budget.

Desired behavior:
Every governor keys on the resolved client IP carried by RealIpLayer, so limits apply per real client rather than per proxy. Concretely:

Rekey all governor configs (currently one per route group — health, page, auth, app, feed, indieauth, micropub, webmention) off the real::RealIp extension. axum-governor exposes extractor::Extension<T> (key off real::RealIp) and SmartIp; pick whichever cleanly satisfies the fallback rule below. Keep each group's existing quota unchanged.
Keep RealIpLayer installed, and keep it ordered before the governors so the real::RealIp extension is populated when a governor extracts its key. (It already sits in the outer ServiceBuilder, which wraps the per-route governor layers, so the current ordering is correct — verify it stays that way.)
Fallback: a request whose client IP can't be resolved (no/unparseable forwarded header) must degrade to the TCP peer IP, never to a shared or empty key. Confirm the chosen extractor does this; Extension<RealIp> on a missing extension must not fail open.
Trust config: confirm RealIpLayer's forwarded-header trust is correct for Fly. On Fly the app's peer is a private address and the proxy sets the forwarded headers, so RealIpLayer::default()'s "trust forwarded headers only from a private-IP peer" heuristic should match — verify against what the real crate actually reads and what fly-proxy sends (X-Forwarded-For / Fly-Client-IP). Do not widen trust to accept forwarded headers from public peers.

Key interfaces:

GovernorConfigBuilder usage in the web router — swap .with_extractor(PeerIp::default()) for a resolved-client-IP extractor across every route group; quotas and .expect_connect_info() (still needed for the peer-IP fallback) otherwise unchanged.
RealIpLayer / real::RealIp — the extension the governors now consume; keep the layer, verify its trust config and header sources against Fly.
axum_governor::extractor — Extension<T> and SmartIp are the candidate extractors.

Acceptance criteria:

With a spoofable/absent forwarded header the limiter falls back to the TCP peer IP (no shared or unlimited bucket).
Given two requests with distinct resolved client IPs behind one proxy peer, the governors bucket them independently (a test that drives the router with Fly-Client-IP/X-Forwarded-For set and asserts one client's 429 doesn't limit the other).
Given many requests from one resolved client IP, that client is limited at its group's quota regardless of the peer address.
real::RealIp is actually consumed by the limiters (no dead RealIpLayer).
Every route group keeps its existing quota; only the key changes.
RealIpLayer still runs before the governors.
format, clippy, and ci mise tasks pass.

Out of scope:

Changing any quota value or the set of route groups.
Supporting reverse proxies other than Fly, or making the trust config runtime-configurable.
Removing the real dependency (it's now load-bearing).

> *This was generated by AI during triage.* ## Agent Brief **Category:** bug **Summary:** Rate limiters key on the TCP peer, so behind the Fly.io proxy every request shares one bucket — rekey them on the resolved client IP. **Deployment topology (decided):** production runs behind the **Fly.io proxy**, which terminates client connections and forwards to the app over Fly's private network. The TCP peer the app sees is always `fly-proxy`, never the client. Recorded in `docs/adr/0012-web-server-runs-behind-the-flyio-proxy.md` — read it before starting. **Current behavior:** Every governor in the web router is built with a `PeerIp` extractor, which keys on `ConnectInfo<SocketAddr>` — the raw TCP peer. A `RealIpLayer` in the outer middleware stack resolves the forwarded client IP into a `real::RealIp` request extension, but nothing reads it. Behind the proxy, all requests share the proxy's peer IP, so each per-IP limiter collapses into a single global bucket: the auth limiter's 10/min becomes a site-wide login cap, and no individual attacker is ever isolated below the shared budget. **Desired behavior:** Every governor keys on the **resolved client IP** carried by `RealIpLayer`, so limits apply per real client rather than per proxy. Concretely: - Rekey all governor configs (currently one per route group — health, page, auth, app, feed, indieauth, micropub, webmention) off the `real::RealIp` extension. axum-governor exposes `extractor::Extension<T>` (key off `real::RealIp`) and `SmartIp`; pick whichever cleanly satisfies the fallback rule below. Keep each group's existing quota unchanged. - Keep `RealIpLayer` installed, and keep it ordered **before** the governors so the `real::RealIp` extension is populated when a governor extracts its key. (It already sits in the outer `ServiceBuilder`, which wraps the per-route governor layers, so the current ordering is correct — verify it stays that way.) - **Fallback:** a request whose client IP can't be resolved (no/unparseable forwarded header) must degrade to the TCP peer IP, never to a shared or empty key. Confirm the chosen extractor does this; `Extension<RealIp>` on a missing extension must not fail open. - **Trust config:** confirm `RealIpLayer`'s forwarded-header trust is correct for Fly. On Fly the app's peer is a private address and the proxy sets the forwarded headers, so `RealIpLayer::default()`'s "trust forwarded headers only from a private-IP peer" heuristic should match — verify against what the `real` crate actually reads and what `fly-proxy` sends (`X-Forwarded-For` / `Fly-Client-IP`). Do not widen trust to accept forwarded headers from public peers. **Key interfaces:** - `GovernorConfigBuilder` usage in the web router — swap `.with_extractor(PeerIp::default())` for a resolved-client-IP extractor across every route group; quotas and `.expect_connect_info()` (still needed for the peer-IP fallback) otherwise unchanged. - `RealIpLayer` / `real::RealIp` — the extension the governors now consume; keep the layer, verify its trust config and header sources against Fly. - `axum_governor::extractor` — `Extension<T>` and `SmartIp` are the candidate extractors. **Acceptance criteria:** - [ ] With a spoofable/absent forwarded header the limiter falls back to the TCP peer IP (no shared or unlimited bucket). - [ ] Given two requests with distinct resolved client IPs behind one proxy peer, the governors bucket them independently (a test that drives the router with `Fly-Client-IP`/`X-Forwarded-For` set and asserts one client's 429 doesn't limit the other). - [ ] Given many requests from one resolved client IP, that client is limited at its group's quota regardless of the peer address. - [ ] `real::RealIp` is actually consumed by the limiters (no dead `RealIpLayer`). - [ ] Every route group keeps its existing quota; only the key changes. - [ ] `RealIpLayer` still runs before the governors. - [ ] `format`, `clippy`, and `ci` mise tasks pass. **Out of scope:** - Changing any quota value or the set of route groups. - Supporting reverse proxies other than Fly, or making the trust config runtime-configurable. - Removing the `real` dependency (it's now load-bearing).

rosa added

ready-for-agent

bug

and removed

needs-triage

labels

2026-07-05 17:40:21 +00:00

rosa referenced this issue from a commit

2026-07-05 17:41:45 +00:00

docs(adr): Record that the web server runs behind the Fly.io proxy

rosa referenced this issue from a commit

2026-07-05 18:02:43 +00:00

fix(web): Key rate limiters on the resolved client IP behind the proxy

rosa referenced this issue from a pull request that will close it,

2026-07-05 18:03:27 +00:00

fix(web): Key rate limiters on the resolved client IP behind the proxy #106

rosa commented

2026-07-05 18:07:55 +00:00

Author

Owner

This was generated by AI during triage.

Correction to the agent brief

Implemented in #106. The mechanism differs from the brief above after reading the two crates' source:

Not Extension<real::RealIp>. RealIpLayer::default() honors a raw X-Real-IP and the leftmost X-Forwarded-For entry with no peer-trust gating — both client-controllable. Fly appends the real client to the right of X-Forwarded-For and doesn't strip a client-supplied X-Real-IP, so keying on that extension would let any client forge a fresh rate-limit bucket per request (an unlimited-bucket bypass). The brief's note that RealIpLayer "only trusts forwarded headers from a private peer" was wrong about this crate.
Used axum-governor's SmartIp instead, configured with the private-network CIDRs as trusted proxies. It reads X-Forwarded-For only when the TCP peer is trusted and takes the rightmost untrusted entry — the client Fly appends — which a client can't spoof past. Missing header → peer-IP fallback; forwarding header from an untrusted peer → 400 Bad Request (fail closed). The brief said 500; axum-governor maps UntrustedProxy to 400.
RealIpLayer and the real crate are removed (not kept/consumed). Added ipnet for the CIDR type — already in the build via axum-governor.

Trust boundary and the full rationale are recorded in docs/adr/0012-web-server-runs-behind-the-flyio-proxy.md.

> *This was generated by AI during triage.* ## Correction to the agent brief Implemented in #106. The mechanism differs from the brief above after reading the two crates' source: - **Not `Extension<real::RealIp>`.** `RealIpLayer::default()` honors a raw `X-Real-IP` and the **leftmost** `X-Forwarded-For` entry with **no peer-trust gating** — both client-controllable. Fly appends the real client to the *right* of `X-Forwarded-For` and doesn't strip a client-supplied `X-Real-IP`, so keying on that extension would let any client forge a fresh rate-limit bucket per request (an unlimited-bucket bypass). The brief's note that `RealIpLayer` "only trusts forwarded headers from a private peer" was wrong about this crate. - **Used axum-governor's `SmartIp` instead**, configured with the private-network CIDRs as trusted proxies. It reads `X-Forwarded-For` only when the TCP peer is trusted and takes the **rightmost untrusted** entry — the client Fly appends — which a client can't spoof past. Missing header → peer-IP fallback; forwarding header from an untrusted peer → **400 Bad Request** (fail closed). The brief said 500; axum-governor maps `UntrustedProxy` to 400. - **`RealIpLayer` and the `real` crate are removed** (not kept/consumed). Added `ipnet` for the CIDR type — already in the build via axum-governor. Trust boundary and the full rationale are recorded in `docs/adr/0012-web-server-runs-behind-the-flyio-proxy.md`.

rosa closed this issue

2026-07-05 18:09:10 +00:00