diff --git a/BUGS.md b/BUGS.md index 1cc50ae..5681025 100644 --- a/BUGS.md +++ b/BUGS.md @@ -11,7 +11,8 @@ next to the fix so `git log --all --grep 'B-nn'` finds it later. Already fixed and removed: B-52 (a round past ~50 participants deadlocked the payout — `025754c`), B-53 (a bet could pay into the pool of a round it was left -out of — `64f6229`). +out of — `64f6229`), B-54 (`X-Forwarded-For` was read from the front, so every +IP-keyed control was bypassable — `PLACEHOLDER`). State of the tree at audit time: 264 unit tests, all passing; `tests/integration/` still empty; withdrawal and the RBF bump path still never live-broadcast. @@ -43,20 +44,6 @@ remains the last prerequisite for running unattended. ## High — security -### B-54 — `X-Forwarded-For` is trusted blindly, so every IP-keyed control is bypassable - -`app/api/client_ip.py:14-17`, `Caddyfile`. - -`client_ip()` returns `xff.split(",")[0]`. Caddy's `reverse_proxy` *appends* the -real peer address to an incoming `X-Forwarded-For` rather than replacing it, so -the first element is whatever the client sent. Rotating a fake value per request -defeats `login_ip`, `register_ip` (B-33) and the per-IP SSE subscriber cap -(B-38) outright; only the per-username login bucket still bites. - -Fix: take the *last* element of the header, or force it at the proxy with -`header_up X-Forwarded-For {remote_host}` and keep reading element 0. The proxy-side -fix is the more robust of the two, since it makes the app's assumption true. - ### B-55 — Argon2 hashing runs on the event loop `app/auth/security.py:20-39`, called from `app/auth/routes.py` and diff --git a/CLAUDE.md b/CLAUDE.md index 6492379..d95705c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin ## Project status -All 10 stages of the original build order are code-complete and unit-tested — 275 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. +All 10 stages of the original build order are code-complete and unit-tested — 278 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. Verified on mainnet with real money: registration + address derivation, deposit crediting (1-conf), a real 10 PLM bet (broadcast → confirmed → change credited back), and one full round cycle (close → draw on a real block hash → 70/30 payout with sat math checked against the broadcast tx → confirmation → close → next round auto-opened). **Withdrawal and the RBF bump path have never been exercised against a live broadcast** — unit-tested only. @@ -33,7 +33,7 @@ PYTHONPATH=. python scripts/decrypt_master_key.py # ops recovery: decrypt+pr PYTHONPATH=. python scripts/encrypt_master_key.py # ops bootstrap: import an externally-generated xprv (--overwrite to replace) PYTHONPATH=. python scripts/electrum_smoke_test.py # manual check: connect, handshake, subscribe to headers, print the tip -python -m pytest # all 275 tests +python -m pytest # all 278 tests python -m pytest tests/unit/test_hd.py # one file python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test ``` @@ -55,7 +55,7 @@ docker compose down `SITE_ADDRESS` unset → `localhost`, Caddy issues a self-signed cert from its internal CA (browser warning on first visit is expected; `curl -k`). `SITE_ADDRESS=lottery.example.com docker compose up -d` → real Let's Encrypt cert, automatically renewed (needs DNS pointing here and ports 80+443 reachable). -The `Caddyfile` sends baseline security headers — HSTS, `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, `Referrer-Policy`, and a CSP scoped to `default-src 'self'` plus the Google Fonts `@import` in `style.css`/`admin.css`. `script-src`/`style-src` need `'unsafe-inline'` because both SPAs use inline `onclick` handlers and `style=""` attributes throughout — removing those is a separate, larger refactor, not a header change. `restart: unless-stopped` on `app` means a mid-round crash auto-restarts: `closing` and `paying_out` resume on their own, `drawing` does not (see Known gaps). +The `Caddyfile` sends baseline security headers — HSTS, `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, `Referrer-Policy`, and a CSP scoped to `default-src 'self'` plus the Google Fonts `@import` in `style.css`/`admin.css`. It also overwrites `X-Forwarded-For` with the real peer (`header_up X-Forwarded-For {remote_host}`, B-54) — Caddy otherwise *appends* to whatever the client sent, which made every IP-keyed control (the B-33 throttles, B-38's SSE cap) bypassable; `app/api/client_ip.py` independently reads the *last* hop, so either half closes it. `script-src`/`style-src` need `'unsafe-inline'` because both SPAs use inline `onclick` handlers and `style=""` attributes throughout — removing those is a separate, larger refactor, not a header change. `restart: unless-stopped` on `app` means a mid-round crash auto-restarts: `closing` and `paying_out` resume on their own, `drawing` does not (see Known gaps). ## Tech stack diff --git a/Caddyfile b/Caddyfile index f6db413..25ddc19 100644 --- a/Caddyfile +++ b/Caddyfile @@ -29,5 +29,15 @@ Content-Security-Policy "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; font-src 'self' https://fonts.gstatic.com; img-src 'self'; connect-src 'self'; base-uri 'self'; form-action 'self'; frame-ancestors 'none'; object-src 'none'" } - reverse_proxy app:8123 + # B-54: Caddy *appends* the real peer address to whatever X-Forwarded-For the + # client sent, so without this the header arrives as ", " and every IP-keyed control in the app (the login and + # registration throttles of B-33, the SSE per-IP subscriber cap of B-38) is + # defeated by simply rotating a fake value per request. Overwriting the header + # with the actual peer makes the app's assumption true at the source; it also + # reads the last hop rather than the first (app/api/client_ip.py), so the two + # defences hold independently. + reverse_proxy app:8123 { + header_up X-Forwarded-For {remote_host} + } } diff --git a/app/api/client_ip.py b/app/api/client_ip.py index 0d64756..0c1c8d6 100644 --- a/app/api/client_ip.py +++ b/app/api/client_ip.py @@ -10,8 +10,22 @@ def client_ip(request: Request) -> str: Shared by the login/registration throttles (B-33) and the SSE per-IP subscriber cap (B-38) so the two can't drift into different notions of "the client's IP". + + B-54: the *last* element, not the first. A proxy appends the address it saw + to any X-Forwarded-For the client already sent, so the first element is + attacker-controlled — with the header read from the front, rotating a fake + value per request gave every request a fresh identity and turned all three + IP-keyed controls above into decoration. The last element is the one written + by the hop closest to us, i.e. by our own proxy. Exactly one trusted proxy + sits in front of this app (Caddy, see docker-compose.yml, where `app` is + only `expose`d on the compose network and never published to the host), so + the last element is the real peer. The Caddyfile now also overwrites the + header with `header_up X-Forwarded-For {remote_host}`, which collapses it to + a single value — belt and braces: either fix alone closes B-54. """ forwarded = request.headers.get("x-forwarded-for") if forwarded: - return forwarded.split(",")[0].strip() + hops = [hop.strip() for hop in forwarded.split(",") if hop.strip()] + if hops: + return hops[-1] return request.client.host if request.client else "unknown" diff --git a/tests/unit/test_caddyfile_security_headers.py b/tests/unit/test_caddyfile_security_headers.py index ed12a9a..5774971 100644 --- a/tests/unit/test_caddyfile_security_headers.py +++ b/tests/unit/test_caddyfile_security_headers.py @@ -33,3 +33,12 @@ def test_referrer_policy_is_set(): def test_csp_default_src_is_self(): assert "Content-Security-Policy" in CADDYFILE assert "default-src 'self'" in CADDYFILE + + +def test_forwarded_for_is_overwritten_with_the_real_peer(): # B-54 + """Caddy appends to a client-supplied X-Forwarded-For instead of replacing it, + so without this directive the header's first element is whatever the caller + claimed. app/api/client_ip.py reads the last hop and so holds on its own, but + this is what makes the header itself trustworthy — losing it silently weakens + every IP-keyed control (B-33's throttles, B-38's SSE cap).""" + assert "header_up X-Forwarded-For {remote_host}" in CADDYFILE diff --git a/tests/unit/test_client_ip.py b/tests/unit/test_client_ip.py index acabfa4..29237fc 100644 --- a/tests/unit/test_client_ip.py +++ b/tests/unit/test_client_ip.py @@ -1,7 +1,10 @@ """app.api.client_ip is shared by the login/registration throttles (B-33) and the SSE per-IP subscriber cap (B-38) — both depend on it correctly preferring X-Forwarded-For (Caddy reverse-proxies every request, see Caddyfile) over -request.client.host, which would otherwise be the proxy's own address.""" +request.client.host, which would otherwise be the proxy's own address. + +Which *element* of that header it reads is a security property, not a detail: +B-54 below is the whole reason all three controls hold at all.""" from starlette.requests import Request @@ -23,14 +26,33 @@ def test_client_ip_prefers_x_forwarded_for(): assert client_ip(request) == "5.6.7.8" -def test_client_ip_takes_the_first_hop_of_a_forwarded_chain(): +def test_client_ip_takes_the_last_hop_of_a_forwarded_chain(): # B-54 + """The hop closest to us — the one our own proxy appended. Exactly one trusted + proxy sits in front of the app, so this is the real peer.""" request = _request(forwarded="5.6.7.8, 10.0.0.1, 172.17.0.1") - assert client_ip(request) == "5.6.7.8" + assert client_ip(request) == "172.17.0.1" + + +def test_client_ip_ignores_a_client_supplied_prefix(): # B-54 + """Caddy *appends* to whatever the client sent, so the front of the header is + attacker-controlled. Reading it from the front let anyone mint a fresh identity + per request and walk straight through the login/registration throttles (B-33) + and the SSE per-IP subscriber cap (B-38). Two requests spoofing different + values must still key to the same real IP.""" + first = _request(forwarded="1.1.1.1, 203.0.113.9") + second = _request(forwarded="2.2.2.2, 203.0.113.9") + assert client_ip(first) == client_ip(second) == "203.0.113.9" def test_client_ip_strips_whitespace(): - request = _request(forwarded=" 5.6.7.8 , 10.0.0.1") - assert client_ip(request) == "5.6.7.8" + request = _request(forwarded=" 5.6.7.8 , 10.0.0.1 ") + assert client_ip(request) == "10.0.0.1" + + +def test_client_ip_falls_back_when_the_header_is_empty(): + """An empty or comma-only header used to yield "" — a single shared bucket every + caller lands in, which is its own throttle-evasion trick.""" + assert client_ip(_request(forwarded=" , ", client_host="10.0.0.1")) == "10.0.0.1" def test_client_ip_falls_back_to_request_client_without_the_header():