Skip to content

Commit b26d81d

Browse files
benbarclayteknium1
authored andcommitted
feat(dashboard-auth): honour X-Forwarded-Prefix + __Host-/__Secure- cookies
Mission-control style deploys reverse-proxy the dashboard at a path prefix (e.g. mission-control.tilos.com/hermes/* -> :9119) and inject X-Forwarded-Prefix: /hermes on every request. The SPA mount already honoured this for asset URLs and the bootstrap __HERMES_BASE_PATH__, but the OAuth gate didn't: 1. The gate's Location: header to /login and the 401 envelope's login_url were built bare ("/login?next=..."). Under a /hermes prefix the browser follows that to mission-control.tilos.com/login which the proxy doesn't route to the dashboard. 2. _redirect_uri (the OAuth callback URL handed to the IDP) used request.url_for() which doesn't honour X-Forwarded-Prefix (Starlette/uvicorn only proxy_headers Host + Proto + For). The IDP redirects back to /auth/callback instead of /hermes/auth/ callback → 404 in the user's browser. 3. Cookies were set with Path=/ which leaks them to other apps on the same origin and won't be sent back on requests under the prefix in the first place. Fix threads the normalised prefix through every boundary: * New hermes_cli/dashboard_auth/prefix.py — single source of truth for X-Forwarded-Prefix parsing. web_server._normalise_prefix becomes a re-export so the SPA mount, the gate, and the cookies helper all agree. * middleware._unauth_response builds login_url = f"{prefix}/login". * routes._redirect_uri splices the prefix into the path component of the IDP-bound URL (with full validation of the header). * cookies.{set,clear}_{session,pkce}_cookie now take prefix="". Path attribute switches to /hermes when set; cookie name switches name variant (see below). Every caller passes the request's normalised prefix. Cookie hardening (Teknium's lesser-note #1 in the PR review): adopt the __Host- / __Secure- cookie name prefixes per draft-west-cookie- prefixes. The variant is selected from (use_https, prefix): * Loopback HTTP → bare "hermes_session_at" (both prefixes require Secure, incompatible with HTTP). * HTTPS, direct deploy (Path=/) → "__Host-hermes_session_at". Strongest spec: bound to exact origin, no Domain attribute, Secure required. * HTTPS, behind a proxy prefix (Path=/hermes) → "__Secure-hermes_session_at". __Host- forbids Path != "/"; the explicit Path=/hermes covers same-origin app isolation. Setter and reader BOTH consult the prefix because the cookie *name* changes — a reader that looked up the bare name when the setter wrote __Secure- would never find the value. The reader falls back across all three variants so a request whose shape changed mid-session (e.g. post-deploy from no-prefix to /hermes) still picks up the existing cookie until it expires. Test coverage: - tests/hermes_cli/test_dashboard_auth_prefix.py — new file. 11 tests pinning: • Location: /hermes/login on the gate's HTML redirect • 401 envelope login_url carries the prefix • Malformed X-Forwarded-Prefix is ignored (header-injection defence; the script-tag value is normalised to empty string) • _redirect_uri splices /hermes into the path (the property that prevents the IDP-returns-to-404 failure) • PKCE cookie uses Path=/hermes + __Secure- when proxied • Session cookies use __Host- when direct, __Secure- when proxied, bare on loopback HTTP • End-to-end round trip with hand-managed PKCE cookie carriage (TestClient can't simulate a Path=/hermes cookie automatically) - tests/hermes_cli/test_dashboard_auth_cookies.py — rewritten to pin each (use_https, prefix) shape produces its expected cookie name, plus reader-side coverage that __Host- and __Secure- variants are both recognised. - Existing tests across middleware / 401-reauth / etc. updated to match the new cookie names (substring contains instead of startswith). Mutation-tested: reverting _unauth_response to build the bare "/login" URL trips exactly the two tests that pin the prefix carriage, confirming the suite discriminates the regression.
1 parent 034ad95 commit b26d81d

9 files changed

Lines changed: 724 additions & 92 deletions

File tree

hermes_cli/dashboard_auth/cookies.py

Lines changed: 128 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,34 @@
1414
1515
All three are ``SameSite=Lax`` (browser will send on cross-site GET
1616
top-level navigation, which we need for the IDP redirect back to
17-
``/auth/callback``) and ``Path=/``. ``Secure`` is set ONLY when the
18-
dashboard was reached over HTTPS — detected via the request URL scheme,
19-
which honours ``X-Forwarded-Proto`` upstream of Fly's TLS terminator
20-
when uvicorn is configured with ``proxy_headers=True``. Loopback dev
21-
traffic is always HTTP so ``Secure`` would lock the cookies out of
22-
the browser.
17+
``/auth/callback``) and live under the prefix's Path. ``Secure`` is set
18+
ONLY when the dashboard was reached over HTTPS — detected via the
19+
request URL scheme, which honours ``X-Forwarded-Proto`` upstream of
20+
Fly's TLS terminator when uvicorn is configured with
21+
``proxy_headers=True``. Loopback dev traffic is always HTTP so
22+
``Secure`` would lock the cookies out of the browser.
23+
24+
Cookie prefix selection (browser hardening per
25+
https://datatracker.ietf.org/doc/html/draft-west-cookie-prefixes):
26+
27+
* Loopback HTTP — bare name. ``__Host-`` / ``__Secure-`` require
28+
``Secure``, which is incompatible with HTTP.
29+
* Gated HTTPS, direct deploy (Path=/) — ``__Host-`` prefix. Binds the
30+
cookie to the exact origin (no Domain attribute) — strongest spec
31+
guarantee.
32+
* Gated HTTPS, behind a reverse-proxy prefix (Path=/hermes) —
33+
``__Secure-`` prefix. ``__Host-`` is disallowed when Path != "/";
34+
``__Secure-`` keeps the Secure-required hardening without the
35+
Path constraint, and the explicit ``Path=/hermes`` covers
36+
same-origin app isolation.
37+
38+
The setters and readers BOTH consult the active prefix because the
39+
cookie *name* changes — a reader that looked up the bare name when the
40+
setter wrote ``__Secure-hermes_session_at`` would never find the value.
2341
2442
.. deprecated:: contract v1
2543
``set_session_cookies`` accepts ``refresh_token=""`` (the contract-v1
26-
default) and silently skips writing ``hermes_session_rt`` in that case.
44+
default) and silently skips writing the RT cookie in that case.
2745
``clear_session_cookies`` still emits a Max-Age=0 deletion for the RT
2846
cookie so users carrying a stale cookie from an earlier deployment get
2947
it cleared on logout / session expiry. The full refresh-flow machinery
@@ -36,20 +54,58 @@
3654
from fastapi import Request
3755
from fastapi.responses import Response
3856

57+
# Bare cookie names — the request-scoped ``_resolved_name`` helper
58+
# decides whether to prepend ``__Host-`` / ``__Secure-`` based on the
59+
# request's HTTPS + prefix combination.
3960
SESSION_AT_COOKIE = "hermes_session_at"
4061
SESSION_RT_COOKIE = "hermes_session_rt"
4162
PKCE_COOKIE = "hermes_session_pkce"
4263

64+
# Possible name variants we may have to read back. Sorted so most-strict
65+
# wins on iteration when both happen to be present (shouldn't happen in
66+
# practice — a single request emits exactly one variant).
67+
_NAME_VARIANTS = ("__Host-", "__Secure-", "")
68+
4369
# 30 days — matches Portal's REFRESH_TOKEN_TTL_SECONDS
4470
_RT_MAX_AGE = 30 * 24 * 60 * 60
4571
_PKCE_MAX_AGE = 10 * 60
4672

4773

48-
def _common_attrs(use_https: bool) -> dict:
74+
def _resolved_name(bare: str, *, use_https: bool, prefix: str) -> str:
75+
"""Pick the cookie-prefix variant for the active request shape.
76+
77+
See module docstring for the prefix selection rules. Mismatch
78+
between setter and reader would silently break sessions, so this
79+
function is the single source of truth for naming.
80+
"""
81+
if not use_https:
82+
return bare
83+
if prefix:
84+
# Path != "/" forbids __Host-; fall back to __Secure-.
85+
return f"__Secure-{bare}"
86+
return f"__Host-{bare}"
87+
88+
89+
def _cookie_path(prefix: str) -> str:
90+
"""Cookie ``Path`` attribute for the active deploy shape.
91+
92+
Under ``X-Forwarded-Prefix: /hermes`` we want ``Path=/hermes`` so:
93+
a) the browser sends the cookie back on requests under the prefix
94+
(browsers omit the cookie if request path doesn't start with
95+
Path);
96+
b) the cookie doesn't leak to other apps on the same origin
97+
(``mission-control.tilos.com/billing/...``).
98+
99+
Direct-deploy (no proxy prefix) gets ``Path=/``.
100+
"""
101+
return prefix if prefix else "/"
102+
103+
104+
def _common_attrs(*, use_https: bool, prefix: str) -> dict:
49105
attrs: dict = {
50106
"httponly": True,
51107
"samesite": "lax",
52-
"path": "/",
108+
"path": _cookie_path(prefix),
53109
}
54110
if use_https:
55111
attrs["secure"] = True
@@ -63,6 +119,7 @@ def set_session_cookies(
63119
refresh_token: str,
64120
access_token_expires_in: int,
65121
use_https: bool,
122+
prefix: str = "",
66123
) -> None:
67124
"""Set the session cookies on the response.
68125
@@ -74,60 +131,96 @@ def set_session_cookies(
74131
so a ``Session.refresh_token == ""`` from the provider means we don't
75132
persist anything. If a future contract revision starts emitting refresh
76133
tokens, this helper will write the RT cookie again with no other change.
134+
135+
``prefix`` is the normalised X-Forwarded-Prefix value (e.g. ``/hermes``)
136+
or ``""`` for a direct deploy. It influences both the cookie name
137+
(``__Host-`` vs ``__Secure-`` vs bare) and the ``Path`` attribute.
77138
"""
78139
response.set_cookie(
79-
SESSION_AT_COOKIE, access_token,
140+
_resolved_name(SESSION_AT_COOKIE, use_https=use_https, prefix=prefix),
141+
access_token,
80142
max_age=access_token_expires_in,
81-
**_common_attrs(use_https),
143+
**_common_attrs(use_https=use_https, prefix=prefix),
82144
)
83145
# Contract v1: empty refresh token means "don't persist RT cookie".
84146
# Keeping a literal empty-value cookie around would be dead state at
85147
# best, attack surface at worst.
86148
if refresh_token:
87149
response.set_cookie(
88-
SESSION_RT_COOKIE, refresh_token,
150+
_resolved_name(SESSION_RT_COOKIE, use_https=use_https, prefix=prefix),
151+
refresh_token,
89152
max_age=_RT_MAX_AGE,
90-
**_common_attrs(use_https),
153+
**_common_attrs(use_https=use_https, prefix=prefix),
91154
)
92155

93156

94-
def clear_session_cookies(response: Response) -> None:
95-
"""Emit Max-Age=0 deletions for both session cookies."""
96-
# Path must match the set-path for the delete to apply.
97-
response.set_cookie(
98-
SESSION_AT_COOKIE, "", max_age=0,
99-
path="/", httponly=True, samesite="lax",
100-
)
101-
response.set_cookie(
102-
SESSION_RT_COOKIE, "", max_age=0,
103-
path="/", httponly=True, samesite="lax",
104-
)
157+
def clear_session_cookies(response: Response, *, prefix: str = "") -> None:
158+
"""Emit Max-Age=0 deletions for both session cookies.
159+
160+
To delete a cookie reliably the deletion's ``Path`` must match the
161+
set path AND the cookie name must match the variant the setter used.
162+
We don't know which variant was originally set (cookie prefix
163+
depends on the request that set it), so we emit deletions for every
164+
plausible variant under the active path.
165+
"""
166+
path = _cookie_path(prefix)
167+
for variant in _NAME_VARIANTS:
168+
response.set_cookie(
169+
f"{variant}{SESSION_AT_COOKIE}", "", max_age=0,
170+
path=path, httponly=True, samesite="lax",
171+
)
172+
response.set_cookie(
173+
f"{variant}{SESSION_RT_COOKIE}", "", max_age=0,
174+
path=path, httponly=True, samesite="lax",
175+
)
105176

106177

107-
def set_pkce_cookie(response: Response, *, payload: str, use_https: bool) -> None:
178+
def set_pkce_cookie(
179+
response: Response, *, payload: str, use_https: bool, prefix: str = "",
180+
) -> None:
108181
response.set_cookie(
109-
PKCE_COOKIE, payload,
182+
_resolved_name(PKCE_COOKIE, use_https=use_https, prefix=prefix),
183+
payload,
110184
max_age=_PKCE_MAX_AGE,
111-
**_common_attrs(use_https),
185+
**_common_attrs(use_https=use_https, prefix=prefix),
112186
)
113187

114188

115-
def clear_pkce_cookie(response: Response) -> None:
116-
response.set_cookie(
117-
PKCE_COOKIE, "", max_age=0,
118-
path="/", httponly=True, samesite="lax",
119-
)
189+
def clear_pkce_cookie(response: Response, *, prefix: str = "") -> None:
190+
path = _cookie_path(prefix)
191+
for variant in _NAME_VARIANTS:
192+
response.set_cookie(
193+
f"{variant}{PKCE_COOKIE}", "", max_age=0,
194+
path=path, httponly=True, samesite="lax",
195+
)
196+
197+
198+
def _read_with_fallback(
199+
request: Request, bare_name: str,
200+
) -> Optional[str]:
201+
"""Read a cookie by checking every prefix variant in order.
202+
203+
The setter chooses one variant based on the active request shape;
204+
the reader doesn't know which one fired (the request that READS
205+
the cookie may not be the same shape as the request that SET it
206+
in pathological cases). Trying all three guarantees we find it.
207+
"""
208+
for variant in _NAME_VARIANTS:
209+
value = request.cookies.get(f"{variant}{bare_name}")
210+
if value is not None:
211+
return value
212+
return None
120213

121214

122215
def read_session_cookies(request: Request) -> Tuple[Optional[str], Optional[str]]:
123216
"""Returns (access_token, refresh_token), either may be None."""
124-
at = request.cookies.get(SESSION_AT_COOKIE)
125-
rt = request.cookies.get(SESSION_RT_COOKIE)
217+
at = _read_with_fallback(request, SESSION_AT_COOKIE)
218+
rt = _read_with_fallback(request, SESSION_RT_COOKIE)
126219
return at, rt
127220

128221

129222
def read_pkce_cookie(request: Request) -> Optional[str]:
130-
return request.cookies.get(PKCE_COOKIE)
223+
return _read_with_fallback(request, PKCE_COOKIE)
131224

132225

133226
def detect_https(request: Request) -> bool:

hermes_cli/dashboard_auth/middleware.py

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,22 @@ def _unauth_response(request: Request, *, reason: str) -> Response:
7373
HTML redirects also carry the ``next=`` query string so direct
7474
navigation to ``/sessions`` (etc.) without a cookie comes back to
7575
``/sessions`` after login.
76+
77+
Under a reverse proxy with ``X-Forwarded-Prefix: /hermes``, the
78+
``login_url`` is prefixed (``/hermes/login?next=...``) so the
79+
browser's window.location.assign / Location: follow lands on the
80+
proxied login page rather than the bare ``/login`` (which the
81+
proxy doesn't route to the dashboard).
7682
"""
83+
from hermes_cli.dashboard_auth.prefix import prefix_from_request
84+
7785
path = request.url.path
7886
next_param = _safe_next_target(request)
79-
login_url = f"/login?next={next_param}" if next_param else "/login"
87+
prefix = prefix_from_request(request)
88+
login_url = (
89+
f"{prefix}/login?next={next_param}" if next_param
90+
else f"{prefix}/login"
91+
)
8092

8193
if path.startswith("/api/"):
8294
# API routes never get redirects: the browser fetch() API would
@@ -183,9 +195,12 @@ async def gated_auth_middleware(
183195
# Clear the dead cookie so the browser doesn't keep sending it.
184196
# Contract v1: no refresh token to retry with, so the only correct
185197
# next step is full re-auth via /login. Importing locally avoids a
186-
# cycle with cookies → middleware at module load.
198+
# cycle with cookies → middleware at module load. Pass the active
199+
# prefix so the deletion's Path matches the set-Path (otherwise
200+
# the browser ignores it).
187201
from hermes_cli.dashboard_auth.cookies import clear_session_cookies
188-
clear_session_cookies(response)
202+
from hermes_cli.dashboard_auth.prefix import prefix_from_request
203+
clear_session_cookies(response, prefix=prefix_from_request(request))
189204
return response
190205

191206
request.state.session = session
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
"""Helpers for X-Forwarded-Prefix support.
2+
3+
Mission-control style deploys reverse-proxy the dashboard at a path
4+
prefix (e.g. ``mission-control.tilos.com/hermes/*`` -> dashboard on
5+
:9119). The proxy injects ``X-Forwarded-Prefix: /hermes`` so the
6+
backend can reconstruct prefixed URLs (Location: headers, OAuth
7+
redirect_uri, cookie Path attributes, SPA asset URLs).
8+
9+
The single source of truth for the parsed prefix lives here so the
10+
gate middleware, the OAuth routes, the cookie helpers, and the SPA
11+
mount all agree on validation rules.
12+
"""
13+
from __future__ import annotations
14+
15+
from typing import Optional
16+
17+
18+
def normalise_prefix(raw: Optional[str]) -> str:
19+
"""Normalise an X-Forwarded-Prefix header value.
20+
21+
Returns a string like ``"/hermes"`` (no trailing slash) or ``""``
22+
when no prefix is set / the header is malformed. We deliberately
23+
reject anything containing ``..`` or non-printable bytes so a
24+
hostile proxy can't inject HTML or path-traversal sequences via the
25+
prefix.
26+
"""
27+
if not raw:
28+
return ""
29+
p = raw.strip()
30+
if not p:
31+
return ""
32+
if not p.startswith("/"):
33+
p = "/" + p
34+
p = p.rstrip("/")
35+
if (
36+
"//" in p
37+
or ".." in p
38+
or any(c in p for c in ('"', "'", "<", ">", " ", "\n", "\r", "\t"))
39+
):
40+
return ""
41+
if len(p) > 64:
42+
return ""
43+
return p
44+
45+
46+
def prefix_from_request(request) -> str:
47+
"""Convenience wrapper that reads the header off a Starlette/FastAPI
48+
Request and normalises it. Returns ``""`` when no prefix.
49+
"""
50+
return normalise_prefix(request.headers.get("x-forwarded-prefix"))

0 commit comments

Comments
 (0)