Per-IP unlock rate limiting via X-Real-IP #161

New issue

Open

opened 2026-09-15 21:17:25 +02:00 by lz · 0 comments

lz commented

2026-09-15 21:17:25 +02:00

Owner

Follow-up to #149 / PR #160, which shipped a global-only limiter. This adds the per-IP key that #110 item 1 also asked for, by a route that PR #160 did not consider.

What #160 established, and where its conclusion goes too far

PR #160 concluded that per-IP is mutually exclusive with the worker callbacks. The blocker it found is real — I verified it in @sveltejs/adapter-node@5.5.4 (files/handler.js):

1273: const xff_depth      = parseInt(env('XFF_DEPTH', '1'));
1274: const address_header = env('ADDRESS_HEADER', '').toLowerCase();
...
1371: getClientAddress: () => {
1372:   if (address_header) {
1373:     if (!(address_header in req.headers)) {
1374:       throw new Error(`... ADDRESS_HEADER=${address_header} but is absent from request`)

address_header is read once at module scope — process-global, no per-route form — and getClientAddress() throws when the named header is absent. So enabling it for /unlock would break the four /api/agent/* callbacks, which reach host-mode Nexus directly over the workers bridge carrying no such header, taking the bridge-IP guard (fact #17) with them.

But that proves ADDRESS_HEADER is unusable, not that per-IP is impossible. The mutual exclusivity is a property of the adapter's mechanism, not of the problem.

The route not taken

DEPLOYMENT.md already documents:

proxy_set_header   X-Real-IP   $remote_addr;

proxy_set_header overwrites rather than appends, so via nginx that header is the real TCP peer and is not client-forgeable. The unlock route can read request.headers.get('x-real-ip') directly, scoped to that one route, never touching getClientAddress() and leaving the agent callbacks completely alone.

(Note this is why the same trick does not work for X-Forwarded-For: DEPLOYMENT.md does not set it at all, so under the documented config it is whatever the caller sent.)

Why it is worth doing

It fixes the exact weakness PR #160 names in its own body:

a global counter means a determined attacker can keep the operator out for as long as they keep guessing

Per-IP removes that for the internet-facing case, which is the case that motivated #149.

The honest caveat

Nexus runs network_mode: host on port 3001, so it is also directly reachable on the LAN, bypassing nginx. A LAN client can forge X-Real-IP. That is a weaker threat model — the attacker is already on the LAN — and the global counter still covers it.

So the design is: per-IP when a trustworthy header is present, global always. Not per-IP instead of global.

Work

Read x-real-ip in the unlock route only. Do not set ADDRESS_HEADER, and do not route this through getClientAddress().
Keep the global counter exactly as it is. This is an additional, narrower key, not a replacement.
Bound the per-IP map. This reintroduces the attacker-controlled key that #149 warned about and PR #160 got to sidestep — a single counter has no map to exhaust, a per-IP one does. Cap entries and evict.
Verify against the live nginx config, not just DEPLOYMENT.md. PR #160 explicitly could not inspect a.lck.sh and flagged that its conclusions rest on the documented config. If the live config differs, DEPLOYMENT.md is stale and should be corrected in the same PR.
Decide what happens when the header is absent or unparseable: fall back to global-only for that request, never to a shared fake key.

Tests

PR #160 already ships a guard that forges a different X-Forwarded-For on every request, so a naive left-most per-IP key goes red. Keep it. Add the equivalent for X-Real-IP: prove that a request reaching Nexus directly (no proxy) cannot mint an arbitrary key.

Repo standard: break the thing each test protects and confirm it goes red before counting it.

Follow-up to #149 / PR #160, which shipped a **global-only** limiter. This adds the per-IP key that #110 item 1 also asked for, by a route that PR #160 did not consider. ## What #160 established, and where its conclusion goes too far PR #160 concluded that per-IP is *mutually exclusive* with the worker callbacks. The blocker it found is real — I verified it in `@sveltejs/adapter-node@5.5.4` (`files/handler.js`): ``` 1273: const xff_depth = parseInt(env('XFF_DEPTH', '1')); 1274: const address_header = env('ADDRESS_HEADER', '').toLowerCase(); ... 1371: getClientAddress: () => { 1372: if (address_header) { 1373: if (!(address_header in req.headers)) { 1374: throw new Error(`... ADDRESS_HEADER=${address_header} but is absent from request`) ``` `address_header` is read **once at module scope** — process-global, no per-route form — and `getClientAddress()` **throws** when the named header is absent. So enabling it for `/unlock` would break the four `/api/agent/*` callbacks, which reach host-mode Nexus directly over the workers bridge carrying no such header, taking the bridge-IP guard (fact #17) with them. **But that proves `ADDRESS_HEADER` is unusable, not that per-IP is impossible.** The mutual exclusivity is a property of the adapter's mechanism, not of the problem. ## The route not taken `DEPLOYMENT.md` already documents: ```nginx proxy_set_header X-Real-IP $remote_addr; ``` `proxy_set_header` **overwrites** rather than appends, so via nginx that header is the real TCP peer and is not client-forgeable. The unlock route can read `request.headers.get('x-real-ip')` **directly**, scoped to that one route, never touching `getClientAddress()` and leaving the agent callbacks completely alone. (Note this is why the same trick does *not* work for `X-Forwarded-For`: DEPLOYMENT.md does not set it at all, so under the documented config it is whatever the caller sent.) ## Why it is worth doing It fixes the exact weakness PR #160 names in its own body: > a global counter means a determined attacker can keep the operator out for as long as they keep guessing Per-IP removes that for the internet-facing case, which is the case that motivated #149. ## The honest caveat Nexus runs `network_mode: host` on port 3001, so it is **also** directly reachable on the LAN, bypassing nginx. A LAN client can forge `X-Real-IP`. That is a weaker threat model — the attacker is already on the LAN — and the global counter still covers it. So the design is: **per-IP when a trustworthy header is present, global always.** Not per-IP instead of global. ## Work - Read `x-real-ip` in the unlock route only. Do not set `ADDRESS_HEADER`, and do not route this through `getClientAddress()`. - Keep the global counter exactly as it is. This is an additional, narrower key, not a replacement. - **Bound the per-IP map.** This reintroduces the attacker-controlled key that #149 warned about and PR #160 got to sidestep — a single counter has no map to exhaust, a per-IP one does. Cap entries and evict. - Verify against the **live** nginx config, not just DEPLOYMENT.md. PR #160 explicitly could not inspect `a.lck.sh` and flagged that its conclusions rest on the documented config. If the live config differs, DEPLOYMENT.md is stale and should be corrected in the same PR. - Decide what happens when the header is absent or unparseable: fall back to global-only for that request, never to a shared fake key. ## Tests PR #160 already ships a guard that forges a different `X-Forwarded-For` on every request, so a naive left-most per-IP key goes red. **Keep it.** Add the equivalent for `X-Real-IP`: prove that a request reaching Nexus *directly* (no proxy) cannot mint an arbitrary key. Repo standard: break the thing each test protects and confirm it goes red before counting it.

lz added this to the MCP support (#140) milestone

2026-09-15 21:17:25 +02:00