diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e98b22808..1cd3d37f7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ ## [Unreleased] +## [v0.50.258] — 2026-05-01 + +### Fixed +- **Login stability: 30-day session TTL, redirect-back, connectivity probe** — three independent fixes for users on flaky networks (VPN, Tailscale). (1) `SESSION_TTL` extended from 24 hours to 30 days in `api/auth.py` so users no longer get kicked out daily. (2) When a session expires and the user is redirected to `/login`, the server now passes `?next=` so `_safeNextPath()` in `static/login.js` redirects them back after a successful login instead of dumping them on the login screen. (3) Login page now probes `/health` on load (a public endpoint) and distinguishes "session expired / wrong password" from "can't reach server" — when the server is unreachable, shows a clear "Cannot reach server — check your VPN / Tailscale connection." message, disables the form, retries every 3 seconds, and auto-reloads the page once the server becomes reachable again. (`api/auth.py`, `static/login.js`) @bsgdigital — PR #1419 + +### Changed (Opus pre-release advisor) +- **Login redirect URL encoding fix — multi-param queries no longer truncated** — the original PR #1419 implementation built the outer `?next=` parameter via `quote(path, safe='/:@!$&\'()*+,;=')` which kept `?` and `&` literal. Two problems: (a) paths with multi-param queries (e.g. `/api/sessions?limit=50&offset=0`) round-tripped as `/api/sessions?limit=50` because the inner `&` terminated the outer `next` value, (b) attacker-controlled paths with embedded `&next=...` injected a second top-level `next` parameter (browsers parse first-match, Python parse_qs parses last-match — parser-divergence footgun even though `_safeNextPath()` rejects the actual exploit). Fix encodes the entire `path?query` blob with `safe='/'` so `?`, `&`, `=` all percent-encode. The outer `next` then holds exactly one path-with-query string. 6 regression tests in `test_v050258_opus_followups.py` pin the round-trip behavior across simple paths, single-query paths, multi-param queries, and attacker-injection neutralization. (`api/auth.py`, `tests/test_v050258_opus_followups.py`) + + ## [v0.50.257] — 2026-05-01 ### Added diff --git a/api/auth.py b/api/auth.py index fc39776017..17ecbac722 100644 --- a/api/auth.py +++ b/api/auth.py @@ -24,7 +24,7 @@ }) COOKIE_NAME = 'hermes_session' -SESSION_TTL = 86400 # 24 hours +SESSION_TTL = 86400 * 30 # 30 days _SESSIONS_FILE = STATE_DIR / '.sessions.json' @@ -229,7 +229,33 @@ def check_auth(handler, parsed) -> bool: handler.wfile.write(b'{"error":"Authentication required"}') else: handler.send_response(302) - handler.send_header('Location', '/login') + # Pass the original path as ?next= so login.js redirects back after auth. + # SECURITY/CORRECTNESS: the inner `?` and `&` MUST be percent-encoded + # when stuffed into the outer `?next=` parameter, otherwise: + # (a) multi-param query strings get truncated at the first inner `&` + # (e.g. `/api/sessions?limit=50&offset=0` would round-trip as + # just `/api/sessions?limit=50` after the browser parses the + # outer URL — `offset=0` becomes a separate top-level query + # parameter that the login page ignores). + # (b) attacker-controlled paths could inject a second `next=` + # parameter; per RFC 3986 the duplicate behaviour is undefined + # and parsers diverge (Python's parse_qs returns last-match, + # URLSearchParams returns first-match), opening a query-pollution + # footgun even though _safeNextPath() rejects most malicious + # shapes downstream. + # Encoding the entire `path?query` blob with quote(safe='/') turns + # `?` → `%3F` and `&` → `%26`, so the outer parameter holds exactly + # one path-with-query string and `searchParams.get('next')` returns + # the full original URL (the browser auto-decodes once). + # (Opus pre-release advisor finding for v0.50.258.) + import urllib.parse as _urlparse + _path_with_query = parsed.path or '/' + if parsed.query: + _path_with_query += '?' + parsed.query + # safe='/' keeps path separators readable; everything else (including + # `?`, `&`, `=`) gets percent-encoded. + _next = _urlparse.quote(_path_with_query, safe='/') + handler.send_header('Location', '/login?next=' + _next) handler.end_headers() return False diff --git a/static/login.js b/static/login.js index bba29b5020..72c47a5b49 100644 --- a/static/login.js +++ b/static/login.js @@ -66,4 +66,45 @@ document.addEventListener('DOMContentLoaded', function () { doLogin(e); } }); + + // On page load, probe the server so we can distinguish "can't reach server" + // (Tailscale off, wrong network) from "session expired / need to log in". + // Uses /health — a public endpoint, no auth required. + // If unreachable, retries every 3 s and auto-reloads once the server is back. + (function checkConnectivity() { + var retryTimer = null; + + function setFormDisabled(disabled) { + if (input) input.disabled = disabled; + var btn = form.querySelector('button'); + if (btn) btn.disabled = disabled; + } + + function probe() { + fetch('health', { method: 'GET', credentials: 'omit' }) + .then(function (r) { + if (r.ok) { + // Server is reachable — if we were in retry mode, reload so the + // page reflects the correct auth state (expired session, etc.). + if (retryTimer !== null) { + clearTimeout(retryTimer); + retryTimer = null; + window.location.reload(); + } + } else { + showErr(connFailed + ' (server error ' + r.status + ')'); + } + }) + .catch(function () { + showErr('Cannot reach server — check your VPN / Tailscale connection.'); + setFormDisabled(true); + // Keep retrying so the page auto-recovers once the network is back. + if (retryTimer === null) { + retryTimer = setInterval(probe, 3000); + } + }); + } + + probe(); + })(); }); diff --git a/tests/test_v050258_opus_followups.py b/tests/test_v050258_opus_followups.py new file mode 100644 index 0000000000..34d22c0b7e --- /dev/null +++ b/tests/test_v050258_opus_followups.py @@ -0,0 +1,135 @@ +"""Regression tests for v0.50.258 Opus pre-release follow-up. + +PR #1419 introduced server-side `?next=` redirect after session expiry. The +initial implementation built the outer `next` parameter via: + + _next = quote(path, safe='/:@!$&\'()*+,;=') + if query: + _next += '?' + query + location = '/login?next=' + quote(_next, safe='/:@!$&\'()*+,;=?') + +Two problems with this shape: + +1. The inner `?` was kept literal because both `quote()` calls had `?` in + their `safe` set. Combined with `&` also being kept literal, paths with + multi-param queries (`/api/sessions?limit=50&offset=0`) round-tripped as + `/api/sessions?limit=50` — the rest got eaten as a top-level outer query + parameter the login page ignored. + +2. Attacker-controlled paths with embedded `&next=...` could inject a second + top-level `next` parameter. Browsers' URLSearchParams.get() returns the + first-match (safe), Python's parse_qs returns last-match (unsafe). The + downstream `_safeNextPath()` rejects non-`/` prefixes which closed the + actual exploit, but the parser-divergence is a footgun. + +Fix: percent-encode the entire `path?query` blob with `safe='/'`, so `?`, +`&`, `=` all get encoded. The outer `next` then holds exactly one +path-with-query string that the browser auto-decodes once. +""" + +from __future__ import annotations + +import re +import urllib.parse as _urlparse +from pathlib import Path + +REPO = Path(__file__).resolve().parents[1] + + +# ── 1: source-level pin: redirect uses safe='/' encoding ──────────────────── + + +def test_login_redirect_uses_path_only_safe_encoding(): + """The check_auth redirect must encode `?` and `&` so multi-param query + strings round-trip correctly. Negative pattern guards revert to the + original `safe='/:@!$&\'()*+,;=?'` shape.""" + src = (REPO / "api" / "auth.py").read_text(encoding="utf-8") + + redirect_idx = src.find("/login?next=") + assert redirect_idx != -1, "login redirect missing" + block = src[max(0, redirect_idx - 1200) : redirect_idx + 600] + + # Must use safe='/' (path-separators only). + assert "safe='/'" in block, ( + "check_auth must encode `?` and `&` in the next= parameter so multi-param " + "query strings round-trip. safe='/' is the correct encoding shape." + ) + + # Must NOT use the broad safe set that keeps `?` and `&` literal. + assert "safe='/:@!$&\\'()*+,;=?'" not in block, ( + "check_auth must not use the broad safe='/:@!$&\\'()*+,;=?' encoding — " + "that keeps `?` and `&` literal, which truncates multi-param queries " + "and creates a parser-divergence footgun." + ) + + +# ── 2: behavioral round-trip ──────────────────────────────────────────────── + + +def _build_redirect_like_check_auth(path: str, query: str) -> str: + """Mirror api.auth.check_auth's redirect construction so we can assert + the round-trip without spinning up a server.""" + _path_with_query = path or "/" + if query: + _path_with_query += "?" + query + _next = _urlparse.quote(_path_with_query, safe="/") + return "/login?next=" + _next + + +def _browser_searchparams_get_next(location: str) -> str: + """Mirror the browser's URLSearchParams.get('next') behaviour.""" + parsed = _urlparse.urlparse("https://host" + location) + qs = _urlparse.parse_qs(parsed.query, keep_blank_values=True) + values = qs.get("next", []) + return values[0] if values else None + + +def test_redirect_roundtrip_simple_path(): + location = _build_redirect_like_check_auth("/foo/bar", "") + assert _browser_searchparams_get_next(location) == "/foo/bar" + + +def test_redirect_roundtrip_single_query_param(): + location = _build_redirect_like_check_auth("/foo/bar", "baz=qux") + assert _browser_searchparams_get_next(location) == "/foo/bar?baz=qux" + + +def test_redirect_roundtrip_multi_query_params(): + """REGRESSION: pre-fix, this round-tripped to `/api/sessions?limit=50` + (offset got eaten as a top-level outer query).""" + location = _build_redirect_like_check_auth("/api/sessions", "limit=50&offset=0") + got = _browser_searchparams_get_next(location) + assert got == "/api/sessions?limit=50&offset=0", ( + f"multi-param query round-trip broken: got {got!r}, expected the full string" + ) + + +def test_redirect_roundtrip_attacker_controlled_next_injection_neutralized(): + """REGRESSION: pre-fix, an attacker-controlled `&next=https://evil.com` + in the source query injected a second top-level `next` parameter. + Browsers parse first-match (benign), Python parses last-match (the evil + value) — parser-divergence footgun even if downstream guards reject it.""" + location = _build_redirect_like_check_auth( + "/admin", "action=foo&next=https://evil.com" + ) + got = _browser_searchparams_get_next(location) + # The entire string is preserved as the FIRST `next` value. + assert got == "/admin?action=foo&next=https://evil.com" + # And there is exactly ONE top-level `next` parameter. + parsed = _urlparse.urlparse("https://host" + location) + qs = _urlparse.parse_qs(parsed.query, keep_blank_values=True) + assert len(qs.get("next", [])) == 1, ( + f"expected exactly one top-level `next` parameter, got {qs.get('next')}" + ) + # _safeNextPath() in login.js (charAt(0)==='/' and charAt(1)!=='/') would + # accept this as a valid same-origin path. The /admin page receives the + # benign embedded query and the evil URL never becomes a redirect target. + + +def test_redirect_session_ttl_30_days(): + """Pin the SESSION_TTL constant to the 30-day value introduced by #1419.""" + src = (REPO / "api" / "auth.py").read_text(encoding="utf-8") + assert "SESSION_TTL = 86400 * 30" in src, ( + "SESSION_TTL must be 30 days (86400 * 30) per #1419. Reverting to " + "24h would re-introduce the daily-kick-out UX regression." + )