diff --git a/hermes_cli/dashboard_auth/cookies.py b/hermes_cli/dashboard_auth/cookies.py index b04a31e144c..f8fc77f2426 100644 --- a/hermes_cli/dashboard_auth/cookies.py +++ b/hermes_cli/dashboard_auth/cookies.py @@ -14,16 +14,34 @@ Three cookies in play: All three are ``SameSite=Lax`` (browser will send on cross-site GET top-level navigation, which we need for the IDP redirect back to -``/auth/callback``) and ``Path=/``. ``Secure`` is set ONLY when the -dashboard was reached over HTTPS — detected via the request URL scheme, -which honours ``X-Forwarded-Proto`` upstream of Fly's TLS terminator -when uvicorn is configured with ``proxy_headers=True``. Loopback dev -traffic is always HTTP so ``Secure`` would lock the cookies out of -the browser. +``/auth/callback``) and live under the prefix's Path. ``Secure`` is set +ONLY when the dashboard was reached over HTTPS — detected via the +request URL scheme, which honours ``X-Forwarded-Proto`` upstream of +Fly's TLS terminator when uvicorn is configured with +``proxy_headers=True``. Loopback dev traffic is always HTTP so +``Secure`` would lock the cookies out of the browser. + +Cookie prefix selection (browser hardening per +https://datatracker.ietf.org/doc/html/draft-west-cookie-prefixes): + + * Loopback HTTP — bare name. ``__Host-`` / ``__Secure-`` require + ``Secure``, which is incompatible with HTTP. + * Gated HTTPS, direct deploy (Path=/) — ``__Host-`` prefix. Binds the + cookie to the exact origin (no Domain attribute) — strongest spec + guarantee. + * Gated HTTPS, behind a reverse-proxy prefix (Path=/hermes) — + ``__Secure-`` prefix. ``__Host-`` is disallowed when Path != "/"; + ``__Secure-`` keeps the Secure-required hardening without the + Path constraint, and the explicit ``Path=/hermes`` covers + same-origin app isolation. + +The setters and readers BOTH consult the active prefix because the +cookie *name* changes — a reader that looked up the bare name when the +setter wrote ``__Secure-hermes_session_at`` would never find the value. .. deprecated:: contract v1 ``set_session_cookies`` accepts ``refresh_token=""`` (the contract-v1 - default) and silently skips writing ``hermes_session_rt`` in that case. + default) and silently skips writing the RT cookie in that case. ``clear_session_cookies`` still emits a Max-Age=0 deletion for the RT cookie so users carrying a stale cookie from an earlier deployment get it cleared on logout / session expiry. The full refresh-flow machinery @@ -36,20 +54,58 @@ from typing import Optional, Tuple from fastapi import Request from fastapi.responses import Response +# Bare cookie names — the request-scoped ``_resolved_name`` helper +# decides whether to prepend ``__Host-`` / ``__Secure-`` based on the +# request's HTTPS + prefix combination. SESSION_AT_COOKIE = "hermes_session_at" SESSION_RT_COOKIE = "hermes_session_rt" PKCE_COOKIE = "hermes_session_pkce" +# Possible name variants we may have to read back. Sorted so most-strict +# wins on iteration when both happen to be present (shouldn't happen in +# practice — a single request emits exactly one variant). +_NAME_VARIANTS = ("__Host-", "__Secure-", "") + # 30 days — matches Portal's REFRESH_TOKEN_TTL_SECONDS _RT_MAX_AGE = 30 * 24 * 60 * 60 _PKCE_MAX_AGE = 10 * 60 -def _common_attrs(use_https: bool) -> dict: +def _resolved_name(bare: str, *, use_https: bool, prefix: str) -> str: + """Pick the cookie-prefix variant for the active request shape. + + See module docstring for the prefix selection rules. Mismatch + between setter and reader would silently break sessions, so this + function is the single source of truth for naming. + """ + if not use_https: + return bare + if prefix: + # Path != "/" forbids __Host-; fall back to __Secure-. + return f"__Secure-{bare}" + return f"__Host-{bare}" + + +def _cookie_path(prefix: str) -> str: + """Cookie ``Path`` attribute for the active deploy shape. + + Under ``X-Forwarded-Prefix: /hermes`` we want ``Path=/hermes`` so: + a) the browser sends the cookie back on requests under the prefix + (browsers omit the cookie if request path doesn't start with + Path); + b) the cookie doesn't leak to other apps on the same origin + (``mission-control.tilos.com/billing/...``). + + Direct-deploy (no proxy prefix) gets ``Path=/``. + """ + return prefix if prefix else "/" + + +def _common_attrs(*, use_https: bool, prefix: str) -> dict: attrs: dict = { "httponly": True, "samesite": "lax", - "path": "/", + "path": _cookie_path(prefix), } if use_https: attrs["secure"] = True @@ -63,6 +119,7 @@ def set_session_cookies( refresh_token: str, access_token_expires_in: int, use_https: bool, + prefix: str = "", ) -> None: """Set the session cookies on the response. @@ -74,60 +131,96 @@ def set_session_cookies( so a ``Session.refresh_token == ""`` from the provider means we don't persist anything. If a future contract revision starts emitting refresh tokens, this helper will write the RT cookie again with no other change. + + ``prefix`` is the normalised X-Forwarded-Prefix value (e.g. ``/hermes``) + or ``""`` for a direct deploy. It influences both the cookie name + (``__Host-`` vs ``__Secure-`` vs bare) and the ``Path`` attribute. """ response.set_cookie( - SESSION_AT_COOKIE, access_token, + _resolved_name(SESSION_AT_COOKIE, use_https=use_https, prefix=prefix), + access_token, max_age=access_token_expires_in, - **_common_attrs(use_https), + **_common_attrs(use_https=use_https, prefix=prefix), ) # Contract v1: empty refresh token means "don't persist RT cookie". # Keeping a literal empty-value cookie around would be dead state at # best, attack surface at worst. if refresh_token: response.set_cookie( - SESSION_RT_COOKIE, refresh_token, + _resolved_name(SESSION_RT_COOKIE, use_https=use_https, prefix=prefix), + refresh_token, max_age=_RT_MAX_AGE, - **_common_attrs(use_https), + **_common_attrs(use_https=use_https, prefix=prefix), ) -def clear_session_cookies(response: Response) -> None: - """Emit Max-Age=0 deletions for both session cookies.""" - # Path must match the set-path for the delete to apply. - response.set_cookie( - SESSION_AT_COOKIE, "", max_age=0, - path="/", httponly=True, samesite="lax", - ) - response.set_cookie( - SESSION_RT_COOKIE, "", max_age=0, - path="/", httponly=True, samesite="lax", - ) +def clear_session_cookies(response: Response, *, prefix: str = "") -> None: + """Emit Max-Age=0 deletions for both session cookies. + + To delete a cookie reliably the deletion's ``Path`` must match the + set path AND the cookie name must match the variant the setter used. + We don't know which variant was originally set (cookie prefix + depends on the request that set it), so we emit deletions for every + plausible variant under the active path. + """ + path = _cookie_path(prefix) + for variant in _NAME_VARIANTS: + response.set_cookie( + f"{variant}{SESSION_AT_COOKIE}", "", max_age=0, + path=path, httponly=True, samesite="lax", + ) + response.set_cookie( + f"{variant}{SESSION_RT_COOKIE}", "", max_age=0, + path=path, httponly=True, samesite="lax", + ) -def set_pkce_cookie(response: Response, *, payload: str, use_https: bool) -> None: +def set_pkce_cookie( + response: Response, *, payload: str, use_https: bool, prefix: str = "", +) -> None: response.set_cookie( - PKCE_COOKIE, payload, + _resolved_name(PKCE_COOKIE, use_https=use_https, prefix=prefix), + payload, max_age=_PKCE_MAX_AGE, - **_common_attrs(use_https), + **_common_attrs(use_https=use_https, prefix=prefix), ) -def clear_pkce_cookie(response: Response) -> None: - response.set_cookie( - PKCE_COOKIE, "", max_age=0, - path="/", httponly=True, samesite="lax", - ) +def clear_pkce_cookie(response: Response, *, prefix: str = "") -> None: + path = _cookie_path(prefix) + for variant in _NAME_VARIANTS: + response.set_cookie( + f"{variant}{PKCE_COOKIE}", "", max_age=0, + path=path, httponly=True, samesite="lax", + ) + + +def _read_with_fallback( + request: Request, bare_name: str, +) -> Optional[str]: + """Read a cookie by checking every prefix variant in order. + + The setter chooses one variant based on the active request shape; + the reader doesn't know which one fired (the request that READS + the cookie may not be the same shape as the request that SET it + in pathological cases). Trying all three guarantees we find it. + """ + for variant in _NAME_VARIANTS: + value = request.cookies.get(f"{variant}{bare_name}") + if value is not None: + return value + return None def read_session_cookies(request: Request) -> Tuple[Optional[str], Optional[str]]: """Returns (access_token, refresh_token), either may be None.""" - at = request.cookies.get(SESSION_AT_COOKIE) - rt = request.cookies.get(SESSION_RT_COOKIE) + at = _read_with_fallback(request, SESSION_AT_COOKIE) + rt = _read_with_fallback(request, SESSION_RT_COOKIE) return at, rt def read_pkce_cookie(request: Request) -> Optional[str]: - return request.cookies.get(PKCE_COOKIE) + return _read_with_fallback(request, PKCE_COOKIE) def detect_https(request: Request) -> bool: diff --git a/hermes_cli/dashboard_auth/middleware.py b/hermes_cli/dashboard_auth/middleware.py index 7036da77828..5b42c90ebf7 100644 --- a/hermes_cli/dashboard_auth/middleware.py +++ b/hermes_cli/dashboard_auth/middleware.py @@ -73,10 +73,22 @@ def _unauth_response(request: Request, *, reason: str) -> Response: HTML redirects also carry the ``next=`` query string so direct navigation to ``/sessions`` (etc.) without a cookie comes back to ``/sessions`` after login. + + Under a reverse proxy with ``X-Forwarded-Prefix: /hermes``, the + ``login_url`` is prefixed (``/hermes/login?next=...``) so the + browser's window.location.assign / Location: follow lands on the + proxied login page rather than the bare ``/login`` (which the + proxy doesn't route to the dashboard). """ + from hermes_cli.dashboard_auth.prefix import prefix_from_request + path = request.url.path next_param = _safe_next_target(request) - login_url = f"/login?next={next_param}" if next_param else "/login" + prefix = prefix_from_request(request) + login_url = ( + f"{prefix}/login?next={next_param}" if next_param + else f"{prefix}/login" + ) if path.startswith("/api/"): # API routes never get redirects: the browser fetch() API would @@ -183,9 +195,12 @@ async def gated_auth_middleware( # Clear the dead cookie so the browser doesn't keep sending it. # Contract v1: no refresh token to retry with, so the only correct # next step is full re-auth via /login. Importing locally avoids a - # cycle with cookies → middleware at module load. + # cycle with cookies → middleware at module load. Pass the active + # prefix so the deletion's Path matches the set-Path (otherwise + # the browser ignores it). from hermes_cli.dashboard_auth.cookies import clear_session_cookies - clear_session_cookies(response) + from hermes_cli.dashboard_auth.prefix import prefix_from_request + clear_session_cookies(response, prefix=prefix_from_request(request)) return response request.state.session = session diff --git a/hermes_cli/dashboard_auth/prefix.py b/hermes_cli/dashboard_auth/prefix.py new file mode 100644 index 00000000000..27324037593 --- /dev/null +++ b/hermes_cli/dashboard_auth/prefix.py @@ -0,0 +1,50 @@ +"""Helpers for X-Forwarded-Prefix support. + +Mission-control style deploys reverse-proxy the dashboard at a path +prefix (e.g. ``mission-control.tilos.com/hermes/*`` -> dashboard on +:9119). The proxy injects ``X-Forwarded-Prefix: /hermes`` so the +backend can reconstruct prefixed URLs (Location: headers, OAuth +redirect_uri, cookie Path attributes, SPA asset URLs). + +The single source of truth for the parsed prefix lives here so the +gate middleware, the OAuth routes, the cookie helpers, and the SPA +mount all agree on validation rules. +""" +from __future__ import annotations + +from typing import Optional + + +def normalise_prefix(raw: Optional[str]) -> str: + """Normalise an X-Forwarded-Prefix header value. + + Returns a string like ``"/hermes"`` (no trailing slash) or ``""`` + when no prefix is set / the header is malformed. We deliberately + reject anything containing ``..`` or non-printable bytes so a + hostile proxy can't inject HTML or path-traversal sequences via the + prefix. + """ + if not raw: + return "" + p = raw.strip() + if not p: + return "" + if not p.startswith("/"): + p = "/" + p + p = p.rstrip("/") + if ( + "//" in p + or ".." in p + or any(c in p for c in ('"', "'", "<", ">", " ", "\n", "\r", "\t")) + ): + return "" + if len(p) > 64: + return "" + return p + + +def prefix_from_request(request) -> str: + """Convenience wrapper that reads the header off a Starlette/FastAPI + Request and normalises it. Returns ``""`` when no prefix. + """ + return normalise_prefix(request.headers.get("x-forwarded-prefix")) diff --git a/hermes_cli/dashboard_auth/routes.py b/hermes_cli/dashboard_auth/routes.py index 2e8aa067f61..dda533c1380 100644 --- a/hermes_cli/dashboard_auth/routes.py +++ b/hermes_cli/dashboard_auth/routes.py @@ -53,8 +53,26 @@ def _redirect_uri(request: Request) -> str: Reads from the request URL — under uvicorn's ``proxy_headers=True`` this picks up the public https URL from ``X-Forwarded-Host`` plus ``X-Forwarded-Proto``. + + Under ``X-Forwarded-Prefix: /hermes`` (Mission Control deploys), we + additionally prepend the prefix to the path so the IDP redirects + the user back to ``https://mission-control.tilos.com/hermes/auth/callback`` + rather than the bare ``/auth/callback`` (which the proxy doesn't + route to the dashboard). FastAPI's ``url_for`` doesn't natively + honour X-Forwarded-Prefix — that header isn't part of the + Starlette/uvicorn proxy_headers set — so we splice the prefix in + manually. """ - return str(request.url_for("auth_callback")) + from urllib.parse import urlparse, urlunparse + + from hermes_cli.dashboard_auth.prefix import prefix_from_request + + base = str(request.url_for("auth_callback")) + prefix = prefix_from_request(request) + if not prefix: + return base + parsed = urlparse(base) + return urlunparse(parsed._replace(path=f"{prefix}{parsed.path}")) def _client_ip(request: Request) -> str: @@ -64,6 +82,18 @@ def _client_ip(request: Request) -> str: return request.client.host if request.client else "" +def _prefix(request: Request) -> str: + """Resolve the X-Forwarded-Prefix header for the active request. + + Local indirection so the routes pass a consistent value to the + cookie helpers (cookie name + Path attribute) and the gate's + redirect builders (login_url construction). See + ``hermes_cli.dashboard_auth.prefix`` for the normalisation rules. + """ + from hermes_cli.dashboard_auth.prefix import prefix_from_request + return prefix_from_request(request) + + # --------------------------------------------------------------------------- # Public: login page (server-rendered HTML, no SPA bundle) # --------------------------------------------------------------------------- @@ -157,7 +187,10 @@ async def auth_login(request: Request, provider: str, next: str = ""): if safe_next: from urllib.parse import quote pkce = f"{pkce};next={quote(safe_next, safe='')}" - set_pkce_cookie(resp, payload=pkce, use_https=detect_https(request)) + set_pkce_cookie( + resp, payload=pkce, use_https=detect_https(request), + prefix=_prefix(request), + ) return resp @@ -280,8 +313,9 @@ async def auth_callback( refresh_token=session.refresh_token, access_token_expires_in=expires_in, use_https=detect_https(request), + prefix=_prefix(request), ) - clear_pkce_cookie(resp) + clear_pkce_cookie(resp, prefix=_prefix(request)) return resp @@ -334,9 +368,10 @@ async def auth_logout(request: Request): ip=_client_ip(request), ) - resp = RedirectResponse(url="/login", status_code=302) - clear_session_cookies(resp) - clear_pkce_cookie(resp) + prefix = _prefix(request) + resp = RedirectResponse(url=f"{prefix}/login", status_code=302) + clear_session_cookies(resp, prefix=prefix) + clear_pkce_cookie(resp, prefix=prefix) return resp diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index eff1fe69b07..872546196c5 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -3806,24 +3806,13 @@ async def events_ws(ws: WebSocket) -> None: def _normalise_prefix(raw: Optional[str]) -> str: """Normalise an X-Forwarded-Prefix header value. - Returns a string like ``"/hermes"`` (no trailing slash) or ``""`` when - no prefix is set / the header is malformed. We deliberately reject - anything containing ``..`` or non-printable bytes so a hostile proxy - can't inject HTML via the prefix. + Thin re-export of :func:`hermes_cli.dashboard_auth.prefix.normalise_prefix` + — the single source of truth lives in the dashboard_auth package so + the gate middleware, the OAuth routes, the cookie helpers, and the + SPA mount all agree on validation rules. """ - if not raw: - return "" - p = raw.strip() - if not p: - return "" - if not p.startswith("/"): - p = "/" + p - p = p.rstrip("/") - if "//" in p or ".." in p or any(c in p for c in ('"', "'", "<", ">", " ", "\n", "\r", "\t")): - return "" - if len(p) > 64: - return "" - return p + from hermes_cli.dashboard_auth.prefix import normalise_prefix + return normalise_prefix(raw) def mount_spa(application: FastAPI): diff --git a/tests/hermes_cli/test_dashboard_auth_401_reauth.py b/tests/hermes_cli/test_dashboard_auth_401_reauth.py index 7cdb57efa1a..c866fad8252 100644 --- a/tests/hermes_cli/test_dashboard_auth_401_reauth.py +++ b/tests/hermes_cli/test_dashboard_auth_401_reauth.py @@ -90,17 +90,17 @@ class TestRefreshTokenCookieDeprecation: client = TestClient(self._build_app(refresh_token="")) r = client.get("/set") cookies = r.headers.get_list("set-cookie") - rt_cookies = [c for c in cookies if c.startswith(f"{SESSION_RT_COOKIE}=")] + rt_cookies = [c for c in cookies if SESSION_RT_COOKIE in c] assert rt_cookies == [] - # AT cookie still set. - at_cookies = [c for c in cookies if c.startswith(f"{SESSION_AT_COOKIE}=")] + # AT cookie still set (whichever variant the request resolves to). + at_cookies = [c for c in cookies if SESSION_AT_COOKIE in c] assert len(at_cookies) == 1 def test_present_refresh_token_still_emits_rt_cookie(self): client = TestClient(self._build_app(refresh_token="forward-compat")) r = client.get("/set") cookies = r.headers.get_list("set-cookie") - rt_cookies = [c for c in cookies if c.startswith(f"{SESSION_RT_COOKIE}=")] + rt_cookies = [c for c in cookies if SESSION_RT_COOKIE in c] assert len(rt_cookies) == 1 assert "forward-compat" in rt_cookies[0] @@ -120,7 +120,7 @@ class TestRefreshTokenCookieDeprecation: r = client.get("/clear") cookies = r.headers.get_list("set-cookie") assert any( - c.startswith(f"{SESSION_RT_COOKIE}=") and "Max-Age=0" in c + SESSION_RT_COOKIE in c and "Max-Age=0" in c for c in cookies ) @@ -456,7 +456,7 @@ class TestAuthLoginPkceCookieNext: ) assert r.status_code == 302 cookies = r.headers.get_list("set-cookie") - pkce = next(c for c in cookies if c.startswith("hermes_session_pkce=")) + pkce = next(c for c in cookies if "hermes_session_pkce" in c) assert "next=" not in pkce def test_safe_next_query_encoded_into_cookie(self, gated_app): @@ -465,7 +465,7 @@ class TestAuthLoginPkceCookieNext: follow_redirects=False, ) cookies = r.headers.get_list("set-cookie") - pkce = next(c for c in cookies if c.startswith("hermes_session_pkce=")) + pkce = next(c for c in cookies if "hermes_session_pkce" in c) # ``next=`` segment present, URL-encoded. assert "next=%2Fsessions" in pkce @@ -479,5 +479,5 @@ class TestAuthLoginPkceCookieNext: follow_redirects=False, ) cookies = r.headers.get_list("set-cookie") - pkce = next(c for c in cookies if c.startswith("hermes_session_pkce=")) + pkce = next(c for c in cookies if "hermes_session_pkce" in c) assert "next=" not in pkce diff --git a/tests/hermes_cli/test_dashboard_auth_cookies.py b/tests/hermes_cli/test_dashboard_auth_cookies.py index c13662d156d..24d6f4b9168 100644 --- a/tests/hermes_cli/test_dashboard_auth_cookies.py +++ b/tests/hermes_cli/test_dashboard_auth_cookies.py @@ -20,7 +20,7 @@ from hermes_cli.dashboard_auth.cookies import ( ) -def _build_app(use_https: bool = True): +def _build_app(use_https: bool = True, prefix: str = ""): app = FastAPI() @app.get("/set") @@ -29,6 +29,7 @@ def _build_app(use_https: bool = True): set_session_cookies( r, access_token="AT", refresh_token="RT", access_token_expires_in=3600, use_https=use_https, + prefix=prefix, ) return r @@ -36,25 +37,33 @@ def _build_app(use_https: bool = True): def set_pkce(): r = Response("ok") set_pkce_cookie(r, payload="provider=stub;state=s;verifier=v", - use_https=use_https) + use_https=use_https, prefix=prefix) return r @app.get("/clear") def clear(): r = Response("ok") - clear_session_cookies(r) - clear_pkce_cookie(r) + clear_session_cookies(r, prefix=prefix) + clear_pkce_cookie(r, prefix=prefix) return r return app -def test_session_cookies_are_httponly_samesite_lax_secure_in_https(): - client = TestClient(_build_app(use_https=True)) +# Cookie name resolution helpers used throughout — the bare name resolves +# to a request-shape-dependent variant (__Host- / __Secure- / bare). +# Tests pin a specific shape so a regression in the name-resolution +# logic fails loudly rather than silently breaking sessions. + + +def test_session_cookies_use_host_prefix_on_https_direct(): + """HTTPS + no proxy prefix → __Host- prefix (strongest spec + hardening: bound to exact origin, requires Path=/, requires Secure).""" + client = TestClient(_build_app(use_https=True, prefix="")) r = client.get("/set") cookies = r.headers.get_list("set-cookie") - at = next(c for c in cookies if c.startswith(f"{SESSION_AT_COOKIE}=")) - rt = next(c for c in cookies if c.startswith(f"{SESSION_RT_COOKIE}=")) + at = next(c for c in cookies if c.startswith(f"__Host-{SESSION_AT_COOKIE}=")) + rt = next(c for c in cookies if c.startswith(f"__Host-{SESSION_RT_COOKIE}=")) for c in (at, rt): assert "HttpOnly" in c assert "samesite=lax" in c.lower() @@ -62,35 +71,63 @@ def test_session_cookies_are_httponly_samesite_lax_secure_in_https(): assert "Path=/" in c -def test_session_cookies_omit_secure_when_http(): +def test_session_cookies_use_secure_prefix_when_proxied(): + """HTTPS + /hermes prefix → __Secure- prefix (__Host- forbids + Path != "/"; __Secure- keeps the Secure-required hardening).""" + client = TestClient(_build_app(use_https=True, prefix="/hermes")) + r = client.get("/set") + cookies = r.headers.get_list("set-cookie") + at = next(c for c in cookies if c.startswith(f"__Secure-{SESSION_AT_COOKIE}=")) + assert "Path=/hermes" in at + assert "Secure" in at + # __Host- variant must NOT be emitted on the prefix path. + assert not any( + c.startswith(f"__Host-{SESSION_AT_COOKIE}=") for c in cookies + ) + + +def test_session_cookies_use_bare_name_on_http(): + """Loopback HTTP dev: __Host- / __Secure- both require Secure, which + we can't set on HTTP. Use bare cookie names.""" client = TestClient(_build_app(use_https=False)) r = client.get("/set") - for c in r.headers.get_list("set-cookie"): - if c.startswith(f"{SESSION_AT_COOKIE}=") or c.startswith(f"{SESSION_RT_COOKIE}="): - assert "Secure" not in c, f"Cookie unexpectedly Secure: {c}" + cookies = r.headers.get_list("set-cookie") + # Bare name present; no __Host- / __Secure- variant emitted. + assert any(c.startswith(f"{SESSION_AT_COOKIE}=") for c in cookies) + assert not any( + c.startswith(f"__Host-{SESSION_AT_COOKIE}=") + or c.startswith(f"__Secure-{SESSION_AT_COOKIE}=") + for c in cookies + ) + # No Secure flag (HTTP). + at = next(c for c in cookies if c.startswith(f"{SESSION_AT_COOKIE}=")) + assert "Secure" not in at def test_session_cookies_have_30day_rt_and_token_ttl_at(): client = TestClient(_build_app(use_https=True)) r = client.get("/set") cookies = r.headers.get_list("set-cookie") - at = next(c for c in cookies if c.startswith(f"{SESSION_AT_COOKIE}=")) - rt = next(c for c in cookies if c.startswith(f"{SESSION_RT_COOKIE}=")) + at = next(c for c in cookies if c.startswith(f"__Host-{SESSION_AT_COOKIE}=")) + rt = next(c for c in cookies if c.startswith(f"__Host-{SESSION_RT_COOKIE}=")) assert "Max-Age=3600" in at assert "Max-Age=2592000" in rt # 30 days = 30 * 86400 def test_clear_session_cookies_emits_expired_at_and_rt(): + """``clear_session_cookies`` emits Max-Age=0 deletions for every + plausible cookie-name variant under the active prefix so we flush + stale cookies that an older deploy may have set under a different + prefix.""" client = TestClient(_build_app()) r = client.get("/clear") cookies = r.headers.get_list("set-cookie") + # At least one variant of each session cookie should be deleted. assert any( - c.startswith(f"{SESSION_AT_COOKIE}=") and "Max-Age=0" in c - for c in cookies + SESSION_AT_COOKIE in c and "Max-Age=0" in c for c in cookies ) assert any( - c.startswith(f"{SESSION_RT_COOKIE}=") and "Max-Age=0" in c - for c in cookies + SESSION_RT_COOKIE in c and "Max-Age=0" in c for c in cookies ) @@ -99,7 +136,7 @@ def test_pkce_cookie_short_ttl_and_path_root(): r = client.get("/set-pkce") pkce = next( c for c in r.headers.get_list("set-cookie") - if c.startswith(f"{PKCE_COOKIE}=") + if PKCE_COOKIE in c ) assert "HttpOnly" in pkce assert "Max-Age=600" in pkce # 10 minutes @@ -107,7 +144,8 @@ def test_pkce_cookie_short_ttl_and_path_root(): assert "Secure" in pkce -def test_read_session_cookies_from_request(): +def test_read_session_cookies_from_request_bare_name(): + """Reader accepts the bare name (loopback) by default.""" scope = { "type": "http", "method": "GET", @@ -123,6 +161,44 @@ def test_read_session_cookies_from_request(): assert rt == "rt_value" +def test_read_session_cookies_from_request_host_prefix(): + """Reader also finds cookies set with the __Host- variant + (HTTPS direct deploy).""" + scope = { + "type": "http", + "method": "GET", + "path": "/", + "headers": [( + b"cookie", + f"__Host-{SESSION_AT_COOKIE}=at_value; " + f"__Host-{SESSION_RT_COOKIE}=rt_value".encode(), + )], + } + req = Request(scope) + at, rt = read_session_cookies(req) + assert at == "at_value" + assert rt == "rt_value" + + +def test_read_session_cookies_from_request_secure_prefix(): + """Reader also finds cookies set with the __Secure- variant + (HTTPS behind a proxy prefix).""" + scope = { + "type": "http", + "method": "GET", + "path": "/", + "headers": [( + b"cookie", + f"__Secure-{SESSION_AT_COOKIE}=at_value; " + f"__Secure-{SESSION_RT_COOKIE}=rt_value".encode(), + )], + } + req = Request(scope) + at, rt = read_session_cookies(req) + assert at == "at_value" + assert rt == "rt_value" + + def test_read_session_cookies_missing_returns_none(): req = Request({"type": "http", "method": "GET", "path": "/", "headers": []}) assert read_session_cookies(req) == (None, None) diff --git a/tests/hermes_cli/test_dashboard_auth_middleware.py b/tests/hermes_cli/test_dashboard_auth_middleware.py index 8239295eb3c..011767604f4 100644 --- a/tests/hermes_cli/test_dashboard_auth_middleware.py +++ b/tests/hermes_cli/test_dashboard_auth_middleware.py @@ -105,7 +105,7 @@ def test_full_login_round_trip_unlocks_api_status(gated_app): assert r1.status_code == 302 pkce = next( (c for c in r1.headers.get_list("set-cookie") - if c.startswith("hermes_session_pkce=")), + if "hermes_session_pkce" in c), None, ) assert pkce and "HttpOnly" in pkce @@ -125,8 +125,8 @@ def test_full_login_round_trip_unlocks_api_status(gated_app): assert r2.status_code == 302 assert r2.headers["location"] == "/" set_cookies = r2.headers.get_list("set-cookie") - assert any(c.startswith("hermes_session_at=") for c in set_cookies) - assert any(c.startswith("hermes_session_rt=") for c in set_cookies) + assert any("hermes_session_at" in c for c in set_cookies) + assert any("hermes_session_rt" in c for c in set_cookies) # 3) /api/status now succeeds because we're authenticated. r3 = gated_app.get("/api/status") diff --git a/tests/hermes_cli/test_dashboard_auth_prefix.py b/tests/hermes_cli/test_dashboard_auth_prefix.py new file mode 100644 index 00000000000..8b0821054d6 --- /dev/null +++ b/tests/hermes_cli/test_dashboard_auth_prefix.py @@ -0,0 +1,374 @@ +"""Path-prefix (X-Forwarded-Prefix) awareness for the dashboard-auth gate. + +Mission-control style deployments reverse-proxy the dashboard at a path +prefix (e.g. ``mission-control.tilos.com/hermes/*`` -> local Caddy -> +:9119), injecting ``X-Forwarded-Prefix: /hermes`` on every request. + +The dashboard already honours this for the SPA bundle (rewriting asset +URLs and the bootstrap ``__HERMES_BASE_PATH__``). The OAuth gate must +honour it too: + + 1. The gate's ``Location:`` redirect to /login (in + ``_unauth_response``) needs to be ``/hermes/login`` so the browser + follows it through the proxy. + 2. The 401 JSON envelope's ``login_url`` needs the same prefix so the + SPA's full-page navigation lands at the proxied login page. + 3. ``_redirect_uri`` (the OAuth callback URL handed to the IDP) must + reconstruct the public URL including the prefix, otherwise the IDP + redirects back to ``/auth/callback`` instead of + ``/hermes/auth/callback`` and the user gets 404. + 4. Cookies must use ``Path=/hermes`` when behind a prefix so they + don't leak to other apps on the same origin AND so they get sent + back to the dashboard on subsequent requests under the prefix. + 5. The ``__Host-`` cookie prefix requires ``Path=/`` — when behind an + X-Forwarded-Prefix we use ``__Secure-`` instead (matches every + hardening property except scope, which the explicit ``Path`` + covers). + +These tests document the wire-level contract so a regression in any of +those rules surfaces before a Mission Control deploy. +""" +from __future__ import annotations + +import pytest + +# Same xdist group as the other dashboard-auth tests — they all mutate +# web_server.app.state.auth_required at module level. +pytestmark = pytest.mark.xdist_group("dashboard_auth_app_state") + +from fastapi.testclient import TestClient + +from hermes_cli import web_server +from hermes_cli.dashboard_auth import clear_providers, register_provider +from tests.hermes_cli.conftest_dashboard_auth import StubAuthProvider + + +@pytest.fixture +def gated_app_proxied(): + """web_server.app configured for gated mode with proxy_headers + a + public Host that simulates the Mission Control reverse proxy. + + The ``base_url`` sets ``host:scheme`` defaults so we don't have to + pass them on every request. ``X-Forwarded-Prefix`` is passed + per-request because the TestClient doesn't have a way to default + request headers. + """ + clear_providers() + register_provider(StubAuthProvider()) + prev_host = getattr(web_server.app.state, "bound_host", None) + prev_port = getattr(web_server.app.state, "bound_port", None) + prev_required = getattr(web_server.app.state, "auth_required", None) + web_server.app.state.bound_host = "mission-control.tilos.com" + web_server.app.state.bound_port = 443 + web_server.app.state.auth_required = True + client = TestClient( + web_server.app, + base_url="https://mission-control.tilos.com", + ) + yield client + clear_providers() + web_server.app.state.bound_host = prev_host + web_server.app.state.bound_port = prev_port + web_server.app.state.auth_required = prev_required + + +@pytest.fixture +def gated_app_direct(): + """web_server.app configured for gated mode WITHOUT a proxy prefix, + for the Fly-direct deploy shape (no path mounting). + """ + clear_providers() + register_provider(StubAuthProvider()) + prev_host = getattr(web_server.app.state, "bound_host", None) + prev_port = getattr(web_server.app.state, "bound_port", None) + prev_required = getattr(web_server.app.state, "auth_required", None) + web_server.app.state.bound_host = "fly-app.fly.dev" + web_server.app.state.bound_port = 443 + web_server.app.state.auth_required = True + client = TestClient( + web_server.app, + base_url="https://fly-app.fly.dev", + ) + yield client + clear_providers() + web_server.app.state.bound_host = prev_host + web_server.app.state.bound_port = prev_port + web_server.app.state.auth_required = prev_required + + +# --------------------------------------------------------------------------- +# Gate middleware: Location: header and 401 envelope respect prefix +# --------------------------------------------------------------------------- + + +class TestGateRedirectsCarryPrefix: + def test_html_redirect_to_login_carries_prefix(self, gated_app_proxied): + r = gated_app_proxied.get( + "/sessions", + headers={"x-forwarded-prefix": "/hermes"}, + follow_redirects=False, + ) + assert r.status_code == 302 + # /login redirect must include the prefix or the browser will + # follow it to mission-control.tilos.com/login (which the proxy + # doesn't route to the dashboard). + assert r.headers["location"].startswith("/hermes/login"), ( + f"Location header lost prefix: {r.headers['location']!r}" + ) + + def test_api_401_envelope_login_url_carries_prefix(self, gated_app_proxied): + r = gated_app_proxied.get( + "/api/sessions", + headers={"x-forwarded-prefix": "/hermes"}, + follow_redirects=False, + ) + assert r.status_code == 401 + body = r.json() + # SPA does window.location.assign(body.login_url); this MUST + # include the prefix. + assert body["login_url"].startswith("/hermes/login"), ( + f"401 envelope login_url lost prefix: {body['login_url']!r}" + ) + + def test_no_prefix_header_keeps_unprefixed_paths(self, gated_app_direct): + """When no X-Forwarded-Prefix is sent, the Location header must + NOT gain a phantom prefix — the Fly-direct deploy shape has no + proxy at all.""" + r = gated_app_direct.get("/sessions", follow_redirects=False) + assert r.status_code == 302 + assert r.headers["location"] == "/login?next=%2Fsessions" + + def test_malformed_prefix_header_is_ignored(self, gated_app_proxied): + """A hostile proxy injects ``X-Forwarded-Prefix: "}, + follow_redirects=False, + ) + assert r.status_code == 302 + assert "