diff --git a/api/auth.py b/api/auth.py index 6dbb79a3..5c89c4d0 100644 --- a/api/auth.py +++ b/api/auth.py @@ -685,6 +685,46 @@ def parse_cookie(handler) -> str | None: return morsel.value if morsel else None +def _safe_login_inner_next(query: str | None) -> str: + """#5578: extract a SAFE, non-login inner redirect from a login page's query. + + When an expired-auth bounce lands back on the login page (which already + carries its own `next` in the query), we want to preserve a legitimate inner + destination X across the redirect to the real login route — but only if X is + itself safe (path-absolute, not protocol-relative/backslash, no control + chars) AND not login-shaped / not itself carrying a nested next param. + Anything else collapses to '' (no inner redirect), which kills the + self-referential chain. Mirrors _safe_login_redirect_path(). + """ + import urllib.parse as _u + raw = _u.parse_qs(query or "").get("next", [""])[0] + path = str(raw or "").strip() + if not path or path[0] != "/" or path[1:2] in {"/", "\\"}: + return "" + if re.search(r"[\x00-\x1f\x7f\s]", path) or len(path) > 2048: + return "" + # Collapse only login-route chains — decode a few levels so a nested + # `/session/login%3Fnext%3D...` (encoded `?`) is still recognized by its + # leading PATH — but preserve a legitimate non-login inner path that merely + # carries its own `next=` query key (e.g. `/admin?next=/real/path`). + _probe = path + for _ in range(8): + _p = _probe.split("?", 1)[0].split("#", 1)[0].split("&", 1)[0].rstrip("/") + if _p == "/login" or _p.endswith("/login"): + return "" + _decoded = _u.unquote(_probe) + if _decoded == _probe: + break + _probe = _decoded + else: + # Still decoding at the cap (pathologically deep encoding) → fail closed. + _p = _probe.split("?", 1)[0].split("#", 1)[0].split("&", 1)[0].rstrip("/") + if _p == "/login" or _p.endswith("/login"): + return "" + return "" + return path + + def check_auth(handler, parsed) -> bool: """Check if request is authorized. Returns True if OK. If not authorized, sends 401 (API) or 302 redirect (page) and returns False.""" @@ -727,6 +767,35 @@ def check_auth(handler, parsed) -> bool: # the full original URL (the browser auto-decodes once). # (Opus pre-release advisor finding for v0.50.258.) import urllib.parse as _urlparse + # #5578: if the page being redirected is ALREADY login-shaped, do NOT + # wrap its full `path?query` into a fresh `next=` — that query already + # carries a `next=`, so quoting the whole thing nests the login URL into + # itself and re-encodes it on every expired-auth bounce, exploding the + # URL until the tab breaks. This guard runs in check_auth() (BEFORE + # route handling), the actual source of the server-side loop. + # + # The login page is served ONLY at the public `/login` route (see + # PUBLIC_PATHS + the routes.py `/login` handler); the app's client route + # `/session/login` is NOT public, so a bare relative `login` from + # `/session/login` resolves to `/session/login` again and re-triggers + # check_auth() — an infinite redirect. Resolve to the real login route + # with `../login`, which lands on `/login` from a `/session/*` scope and + # on `/login` under a subpath mount (verified via urljoin). Carry + # through only a validated, non-login inner `next` so a legitimate + # post-login destination still survives a bounce that happened to land + # on the login page. + _login_path = (parsed.path or '/').rstrip('/') + if _login_path == '/login' or _login_path.endswith('/login'): + # /login itself is public → check_auth never redirects it; this only + # fires for the non-public client login route (e.g. /session/login). + _target = '../login' if '/' in _login_path.lstrip('/') else 'login' + _inner = _safe_login_inner_next(parsed.query) + if _inner: + _target += '?next=' + _urlparse.quote(_inner, safe='/') + handler.send_header('Location', _target) + handler.send_header('Content-Length', '0') + handler.end_headers() + return False _path_with_query = parsed.path or '/' if parsed.query: _path_with_query += '?' + parsed.query diff --git a/api/routes.py b/api/routes.py index 13ced3bf..c977e2dc 100644 --- a/api/routes.py +++ b/api/routes.py @@ -31,7 +31,7 @@ import socket as _socket from collections import defaultdict from pathlib import Path from contextlib import closing -from urllib.parse import parse_qs, quote, urljoin, urlsplit +from urllib.parse import parse_qs, quote, unquote, urljoin, urlsplit from urllib.error import HTTPError, URLError from urllib.request import HTTPRedirectHandler, HTTPSHandler, ProxyHandler, Request, build_opener from api.agent_sessions import ( @@ -9238,6 +9238,37 @@ def _safe_login_redirect_path(raw_path: str | None) -> str: return "/" if re.search(r"[\x00-\x1f\x7f\s]", path): return "/" + # #5578: reject a `next` that points back at the login page, so an + # expired-auth bounce on the login page can't feed the redirect its own + # address and grow the URL exponentially. Length cap is belt-and-suspenders: + # a legitimate app path is never this long. + if len(path) > 2048: + return "/" + # Detect a login-route target even through nested percent-encoding: a nested + # login-redirect chain looks like `/session/login%3Fnext%3D...`, where the + # `?` separating the path from the query is itself encoded, so a plain + # split("?") wouldn't isolate the real path. Fully decode (bounded) and check + # the leading PATH of EVERY decode level, including the final fully-decoded + # form. Only collapse login-route chains — a legitimate non-login path that + # merely carries its own `next=` query key (e.g. `/admin?action=foo&next=/x`) + # must still round-trip (regression guarded by test_v050258_opus_followups.py). + _probe = path + for _ in range(8): + _path_only = _probe.split("?", 1)[0].split("#", 1)[0].split("&", 1)[0].rstrip("/") + if _path_only.endswith("/login") or _path_only == "/login": + return "/" + _decoded = unquote(_probe) + if _decoded == _probe: + break + _probe = _decoded + else: + # Loop exhausted the cap while STILL decoding (pathologically deep + # encoding): check the final decoded form too, then fail closed — an + # 8-level-deep encoded value is never a legitimate redirect. + _path_only = _probe.split("?", 1)[0].split("#", 1)[0].split("&", 1)[0].rstrip("/") + if _path_only.endswith("/login") or _path_only == "/login": + return "/" + return "/" return path diff --git a/static/boot.js b/static/boot.js index 3bee6262..7b2ccc8d 100644 --- a/static/boot.js +++ b/static/boot.js @@ -3071,6 +3071,10 @@ window._mirrorSpeechSettingsFromServer=_mirrorSpeechSettingsFromServer; return true; }; const redirectToLogin=(nextUrl)=>{ + // #5578: never nest the login URL into its own next= — if already on a + // login-shaped page, reload 'login' bare (the page keeps its inner next). + const _p=(window.location.pathname||'').replace(/\/+$/,''); + if(/(?:^|\/)login$/.test(_p)){window.location.href='login';return;} window.location.href='login?next='+encodeURIComponent(nextUrl); }; return { diff --git a/static/login.js b/static/login.js index 9a7cdbe2..4e0c9829 100644 --- a/static/login.js +++ b/static/login.js @@ -32,6 +32,27 @@ document.addEventListener('DOMContentLoaded', function () { if (raw.charAt(0) !== '/') return './'; // must be path-absolute if (raw.charAt(1) === '/' || raw.charAt(1) === '\\') return './'; // reject // and \\ if (/[\x00-\x1f\x7f\s]/.test(raw)) return './'; // reject control chars / whitespace + // #5578: never redirect back to the login page — that self-referential + // chain is what grows the URL exponentially on repeated expired-auth + // bounces. Detect the login route even through nested percent-encoding + // (a nested chain looks like `/session/login%3Fnext%3D...`, where the `?` + // is encoded so a plain split('?') wouldn't isolate the path). Decode a + // few levels and check the leading PATH. Only collapse login-route chains + // — a legitimate non-login path that merely carries its own `next=` query + // key must still round-trip. + if (raw.length > 2048) return './'; + var probe = raw; + var stabilized = false; + for (var i = 0; i < 8; i++) { + var pathOnly = probe.split('?')[0].split('#')[0].split('&')[0].replace(/\/+$/, ''); + if (pathOnly === '/login' || /\/login$/.test(pathOnly)) return './'; + var decoded; + try { decoded = decodeURIComponent(probe); } catch (_) { stabilized = true; break; } + if (decoded === probe) { stabilized = true; break; } + probe = decoded; + } + // If still decoding at the cap (pathologically deep encoding), fail closed. + if (!stabilized) return './'; return raw; } catch (_) { return './'; } } diff --git a/static/ui.js b/static/ui.js index a6bbf9fc..61960aed 100644 --- a/static/ui.js +++ b/static/ui.js @@ -201,7 +201,8 @@ else initOfflineMonitor(); // Redirect to login when the server responds with 401 (auth session expired). // Handles iOS PWA standalone mode and keeps subpath mounts like /hermes/ from // escaping to the personal site root /login. -function _redirectIfUnauth(res){if(res&&res.status===401){window.location.href='login?next='+encodeURIComponent(window.location.pathname+window.location.search);return true;}return false;} +// #5578: on a login-shaped page, reload 'login' WITHOUT a next (avoid self-nesting). +function _redirectIfUnauth(res){if(res&&res.status===401){var _p=(window.location.pathname||'').replace(/\/+$/,'');if(/(?:^|\/)login$/.test(_p)){window.location.href='login';}else{window.location.href='login?next='+encodeURIComponent(window.location.pathname+window.location.search);}return true;}return false;} function _getSessionQueue(sid, create=false){ if(!sid) return []; if(!SESSION_QUEUES[sid]&&create) SESSION_QUEUES[sid]=[]; diff --git a/static/workspace.js b/static/workspace.js index 296d6be7..50151c02 100644 --- a/static/workspace.js +++ b/static/workspace.js @@ -46,7 +46,21 @@ async function api(path,opts={}){ // re-authenticate. This is especially important for iOS PWA (standalone mode) // and for subpath mounts like /hermes/, where /login escapes to the site root. if(res.status===401){ - if(redirect401) window.location.href='login?next='+encodeURIComponent(window.location.pathname+window.location.search); + // #5578: if we're ALREADY on the login page, appending + // window.location.pathname+search (which contains ?next=…) into a + // fresh next= wraps the login URL into itself and re-encodes it — + // exponential URL growth on each expired-auth bounce until the tab + // breaks. On the login page, just reload login WITHOUT a next (the + // page preserves its own inner next); elsewhere, capture the path. + if(redirect401){ + // Already on the login page? Reload login WITHOUT a next. + const _p=(window.location.pathname||'').replace(/\/+$/,''); + if(/(?:^|\/)login$/.test(_p)){ + window.location.href='login'; + }else{ + window.location.href='login?next='+encodeURIComponent(window.location.pathname+window.location.search); + } + } // Callers can opt out of navigation and handle the unauthenticated state themselves. return; } diff --git a/tests/test_issue5578_login_next_nesting.py b/tests/test_issue5578_login_next_nesting.py new file mode 100644 index 00000000..981fb3a8 --- /dev/null +++ b/tests/test_issue5578_login_next_nesting.py @@ -0,0 +1,150 @@ +"""Regression test for #5578 — login `next=` self-nesting URL explosion. + +Bug: an expired-auth 401 while already on `/session/login?next=…` fed the login +redirect its own URL. Each of the three guards (workspace.js api() 401 handler, +login.js `_safeNextPath`, routes.py `_safe_login_redirect_path`) validated against +open-redirect but NONE rejected a `next` pointing back at the login page, so the +nested `/session/login?next=/session/login%3Fnext%3D…` chain re-percent-encoded +and grew exponentially on every bounce until the tab broke (~12k chars). + +Fix: all three guards now reject a `next` that targets the login page or already +carries a nested `next=`, plus a length cap — while preserving legitimate +`next=/some/real/path` redirects and the existing open-redirect protections. +""" +from pathlib import Path + +from api.routes import _safe_login_redirect_path as guard + +ROOT = Path(__file__).resolve().parents[1] +LOGIN_JS = (ROOT / "static" / "login.js").read_text(encoding="utf-8") +WORKSPACE_JS = (ROOT / "static" / "workspace.js").read_text(encoding="utf-8") + + +# ── server guard: the self-nesting cases the bug exploited ────────────────── + +class TestServerGuardRejectsNesting: + def test_rejects_login_self_reference(self): + assert guard("/login") == "/" + assert guard("/session/login") == "/" + assert guard("/session/login/") == "/" + assert guard("/hermes/session/login") == "/" # subpath mount + + def test_rejects_nested_next_chain(self): + nested = "/session/login?next=/session/login%3Fnext%3D/session/login" + assert guard(nested) == "/" + + def test_rejects_encoded_login_chain_at_any_depth(self): + # A nested LOGIN chain must collapse even when the `?` separating path + # from query is percent-encoded at any depth (that is the exponential + # explosion signature). Detection is by the decoded leading PATH. + assert guard("/session/login%3Fnext%3D/y") == "/" + assert guard("/session/login%253Fnext%253D/y") == "/" + assert guard("/hermes/session/login%3Fnext%3D/y") == "/" + + def test_rejects_deeply_encoded_login_chain(self): + # #5579 gate: a 6+-level percent-encoded login chain must still collapse — + # the decode loop checks the leading path at EVERY level (incl. the final + # decoded form), so a login route hidden behind deep encoding is caught. + # /session/login with the '?' encoded 6 times: %25252525253F + assert guard("/session/login%25252525253Fnext%25252525253D/y") == "/" + + def test_preserves_non_login_path_carrying_its_own_next_key(self): + # A legitimate NON-login destination that merely carries its own `next=` + # query key must still round-trip — only login-route chains collapse. + # (Regression guarded by test_v050258_opus_followups.py; #5579 gate.) + assert guard("/x?next=/y") == "/x?next=/y" + assert guard("/admin?action=foo&next=/real/path") == "/admin?action=foo&next=/real/path" + + def test_rejects_overlong_next(self): + assert guard("/" + "a" * 3000) == "/" + + def test_the_exact_12k_explosion_collapses(self): + # Reconstruct the reported exponential chain; the guard must collapse it. + enc = "%3Fnext%3D" + blown = "/session/login" + (enc + "/session/login") * 40 + assert len(blown) > 500 + assert guard(blown) == "/" + + +class TestServerGuardPreservesLegitimateRedirects: + def test_preserves_real_session_path(self): + assert guard("/session/abc123") == "/session/abc123" + + def test_preserves_root_and_plain_paths(self): + assert guard("/") == "/" + assert guard("/workspace") == "/workspace" + assert guard("/session/xyz?tab=files") == "/session/xyz?tab=files" + + def test_still_rejects_open_redirect_classics(self): + # The pre-existing protections must remain intact. + assert guard("//evil.example") == "/" + assert guard("/\\evil") == "/" + assert guard("https://evil.example") == "/" + assert guard("/x\x00y") == "/" + assert guard("") == "/" + assert guard(None) == "/" + + +# ── client guards: static wiring (both JS sites carry the self-ref guard) ─── + +class TestClientGuardsWired: + def test_login_js_rejects_login_self_and_nested_next(self): + # _safeNextPath must carry the login-route + length guards and detect the + # login route through nested percent-encoding via bounded decodeURIComponent. + assert "/login$/.test(pathOnly)" in LOGIN_JS or "/login$/" in LOGIN_JS + assert "decodeURIComponent" in LOGIN_JS + assert "2048" in LOGIN_JS + + def test_workspace_js_skips_next_on_login_page(self): + # The 401 handler must NOT append the whole login URL as next when it's + # already on the login page (the recursion source). + assert "login$/.test(_p)" in WORKSPACE_JS + assert "window.location.href='login';" in WORKSPACE_JS + # And the non-login path still captures the real destination. + assert "'login?next='+encodeURIComponent" in WORKSPACE_JS + + def test_all_client_401_helpers_guard_login_page(self): + # #5578 Codex round-2: workspace.js was fixed but two more client 401 + # redirect helpers (ui.js _redirectIfUnauth, boot.js redirectToLogin) + # also nested the login URL. All three must carry the on-login guard. + UI_JS = (ROOT / "static" / "ui.js").read_text(encoding="utf-8") + BOOT_JS = (ROOT / "static" / "boot.js").read_text(encoding="utf-8") + assert "login$/.test(_p)" in UI_JS, "ui.js _redirectIfUnauth must guard the login page" + assert "login$/.test(_p)" in BOOT_JS, "boot.js redirectToLogin must guard the login page" + + +class TestServerCheckAuthGuard: + def test_check_auth_does_not_nest_login_redirect(self): + # #5578 Codex round-2 (BRICK): check_auth() runs BEFORE route handling, + # so the server-side page-redirect loop never reached + # _safe_login_redirect_path(). check_auth() must itself refuse to wrap a + # login-shaped path into a fresh next=. + AUTH_PY = (ROOT / "api" / "auth.py").read_text(encoding="utf-8") + assert "endswith('/login')" in AUTH_PY or "_login_path.endswith('/login')" in AUTH_PY, ( + "check_auth() must detect a login-shaped path and skip the next= wrap" + ) + + def test_check_auth_resolves_to_real_public_login_route(self): + # #5578 Codex round-3 (BRICK): a bare relative 'login' from /session/login + # resolves BACK to /session/login (not public, not the /login route) and + # loops forever. Must use '../login' so it lands on the real public + # /login route (and /login under a subpath mount). + AUTH_PY = (ROOT / "api" / "auth.py").read_text(encoding="utf-8") + assert "'../login'" in AUTH_PY, ( + "check_auth() must redirect a session-scoped login path to ../login " + "(the real public /login), not a bare relative login that loops" + ) + + def test_inner_next_helper_drops_login_keeps_safe_nonlogin(self): + # The preserved inner next must be safe + non-login. A nested LOGIN chain + # (even with an encoded `?`) collapses; a legit non-login path that + # carries its own `next=` key round-trips (#5579 gate — no over-collapse). + import api.auth as auth + assert auth._safe_login_inner_next("next=/session/login") == "" + assert auth._safe_login_inner_next("next=/session/login%3Fnext%3D/y") == "" + assert auth._safe_login_inner_next("next=//evil") == "" + assert auth._safe_login_inner_next("next=/session/abc123") == "/session/abc123" + # parse_qs decodes the value: /x%3Fnext%3D/y -> /x?next=/y, a legit + # non-login path with its own next key -> preserved (round-trips). + assert auth._safe_login_inner_next("next=/x%3Fnext%3D/y") == "/x?next=/y" + assert auth._safe_login_inner_next("") == ""