From f2278d0b328bf48369dd7af1c28e783100b8c723 Mon Sep 17 00:00:00 2001 From: ArthurBernard Date: Sun, 5 Jul 2026 10:47:28 +0200 Subject: [PATCH 1/2] fix: stable CSRF cookie across /login re-renders + open favicon route WIP preserved from the working tree (was uncommitted on develop): reuse the browser's existing well-formed tb_csrf cookie instead of minting a fresh one per render (a background /login re-render rotated the cookie under the form), open /favicon.ico so the icon probe never bounces through /login, and cover both with tests. --- trading_bot/interfaces/api/app.py | 30 ++++++++++-- .../interfaces/ui/templates/login.html | 1 + .../tests/interfaces/test_dashboard.py | 47 +++++++++++++++++++ 3 files changed, 73 insertions(+), 5 deletions(-) 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 "