diff --git a/CHANGELOG.md b/CHANGELOG.md index 10e0380..585f12d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Login: stable CSRF cookie + open favicon route.** `GET /login` no longer + rotates the `tb_csrf` double-submit cookie on every render — a background + request auth-redirected to `/login` (typically the browser's `/favicon.ico` + probe) used to rotate the cookie under the form the user was looking at, so + every submit 403'd. The render now reuses a well-formed existing cookie + (shape-checked so an arbitrary value is never echoed into the form), and + `/favicon.ico` is an open route (308 → `/static/favicon.svg`) so the probe + never bounces through the auth redirect at all. + ### Deprecated ### Removed diff --git a/trading_bot/interfaces/api/app.py b/trading_bot/interfaces/api/app.py index d3c785c..91339bc 100644 --- a/trading_bot/interfaces/api/app.py +++ b/trading_bot/interfaces/api/app.py @@ -687,7 +687,16 @@ async def _generator() -> Any: #: Login attempts per minute per client (brute-force throttle). _LOGIN_RATE_PER_MIN = 10 #: Path prefixes reachable without a session (the login flow + assets). -_OPEN_PREFIXES = ("/login", "/logout", "/static") +#: ``/favicon.ico`` is open on purpose: a browser fetches it in the background on +#: every page — including the login page — and an auth redirect to ``/login`` +#: would re-render the form and (before the stable-cookie fix in ``_login_page``) +#: rotate the CSRF cookie under the form the user is looking at. +_OPEN_PREFIXES = ("/login", "/logout", "/static", "/favicon.ico") + +#: Shape of a CSRF token we minted (``secrets.token_urlsafe(32)`` → 43 urlsafe +#: chars). An existing cookie is only reused when it matches, so an arbitrary +#: value can never be echoed back into the login form. +_CSRF_SHAPE = re.compile(r"^[A-Za-z0-9_-]{43}$") #: Hard cap on live sessions (I-7): once reached, the oldest session is evicted on a #: new login so the in-memory map cannot grow without bound over a long-lived daemon. @@ -1411,10 +1420,16 @@ async def _auth_guard(request: Request, call_next: Any) -> Any: def _login_page(request: Request, *, error: str = "", status: int = 200) -> Any: nxt = _safe_next(request.query_params.get("next")) - # I-13: mint (or reuse) a per-render CSRF token, embed it in the form AND - # set it as a SameSite=strict cookie — the POST must echo both (double - # submit). A fresh token each GET is fine (the browser keeps the latest). - csrf = secrets.token_urlsafe(32) + # I-13: double-submit CSRF — the POST must echo the form field AND the + # cookie, and they must match. REUSE the browser's existing cookie when it + # has the shape we mint: any background request landing on /login (e.g. an + # unauthenticated asset fetch redirected here) re-renders this page, and + # minting a fresh token per render would rotate the cookie *under* the form + # the user is already looking at — every submit would then 403. A stable + # per-browser token is the standard double-submit pattern. + csrf = request.cookies.get(_CSRF_COOKIE, "") + if not _CSRF_SHAPE.match(csrf): + csrf = secrets.token_urlsafe(32) if templates is not None: resp: Any = templates.TemplateResponse( request, @@ -1447,6 +1462,11 @@ def _login_page(request: Request, *, error: str = "", status: int = 200) -> Any: ) return resp + @app.get("/favicon.ico", include_in_schema=False) + async def favicon() -> Any: + """Serve the browser's default icon probe (open route, see _OPEN_PREFIXES).""" + return RedirectResponse("/static/favicon.svg", status_code=308) + @app.get("/login", response_class=HTMLResponse) async def login_form(request: Request) -> Any: return _login_page(request) diff --git a/trading_bot/interfaces/ui/templates/login.html b/trading_bot/interfaces/ui/templates/login.html index fec7f5e..f35fccc 100644 --- a/trading_bot/interfaces/ui/templates/login.html +++ b/trading_bot/interfaces/ui/templates/login.html @@ -4,6 +4,7 @@ trading_bot — sign in + diff --git a/trading_bot/tests/interfaces/test_dashboard.py b/trading_bot/tests/interfaces/test_dashboard.py index 2810856..425560e 100644 --- a/trading_bot/tests/interfaces/test_dashboard.py +++ b/trading_bot/tests/interfaces/test_dashboard.py @@ -248,6 +248,53 @@ def test_auth_login_flow_authenticates() -> None: assert client.get("/api/health").status_code == 200 # session cookie works +def test_login_survives_a_background_login_rerender_stable_csrf() -> None: + """The CSRF cookie is stable across /login renders — the favicon race. + + A browser fetches ``/favicon.ico`` in the background right after loading the + login page. Before the fix that fetch was auth-redirected to ``/login``, + whose render minted a FRESH token and rotated the ``tb_csrf`` cookie under + the form the user was already looking at — so every submit 403'd, forever. + The token embedded in the FIRST render's form must still authenticate after + a second (background) render. + """ + import re as _re + + client, token = _auth_client() + first = client.get("/login") + m = _re.search(r'name="csrf" value="([^"]+)"', first.text) + assert m, "login form must embed the CSRF field" + form_csrf = m.group(1) # what the user's visible form will submit + # A background request re-renders /login (pre-fix: rotated the cookie). + client.get("/login?next=/favicon.ico") + assert client.cookies.get("tb_csrf", "") == form_csrf # cookie is STABLE + ok = client.post( + "/login", + data={"token": token, "next": "/", "csrf": form_csrf}, + follow_redirects=False, + ) + assert ok.status_code == 303 # the form the user sees still works + + +def test_favicon_is_open_and_never_bounces_to_login() -> None: + """``/favicon.ico`` needs no session and never triggers a /login re-render.""" + client, _ = _auth_client() + r = client.get("/favicon.ico", follow_redirects=False) + assert r.status_code == 308 + assert r.headers["location"] == "/static/favicon.svg" + + +def test_login_render_rejects_a_malformed_csrf_cookie() -> None: + """A cookie not matching our minted shape is replaced, never echoed back.""" + client, _ = _auth_client() + # Raw header (not the cookie jar) so the single malformed value is exactly + # what the server sees. + page = client.get("/login", headers={"cookie": "tb_csrf="}) + assert "