From 5e57bf19a2d82b87f957332fcdeda655091ee6b6 Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Tue, 11 Aug 2026 10:03:51 +1000 Subject: [PATCH] fix(auth): mirror the PKCE setter's cookie shape in clear_pkce_cookie MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SameSite=None change left clear_pkce_cookie() deleting every name variant with `SameSite=None; Secure` unconditionally. Over loopback HTTP the setter emits a bare-name cookie with `SameSite=Lax` and no `Secure` — a Secure deletion on a plain-HTTP origin can be ignored by the browser, leaving a stale PKCE cookie behind after callback/logout. Thread use_https from the request into clear_pkce_cookie() and derive both the set and clear attribute shapes from one helper (_pkce_attrs) so they cannot drift apart again. The __Host-/__Secure- variants keep `Secure; SameSite=None` regardless of origin — those names require Secure to be valid at all and only ever exist on HTTPS origins. Adds contract tests pinning the full Set-Cookie shape on both origins for set and clear, and updates the dashboard cookie documentation (which still claimed all cookies are SameSite=Lax). --- hermes_cli/dashboard_auth/cookies.py | 53 +++++++++++--- hermes_cli/dashboard_auth/routes.py | 6 +- .../hermes_cli/test_dashboard_auth_cookies.py | 73 ++++++++++++++++++- .../docs/user-guide/features/web-dashboard.md | 4 +- 4 files changed, 120 insertions(+), 16 deletions(-) diff --git a/hermes_cli/dashboard_auth/cookies.py b/hermes_cli/dashboard_auth/cookies.py index 72c64fe1e4..f212a99250 100644 --- a/hermes_cli/dashboard_auth/cookies.py +++ b/hermes_cli/dashboard_auth/cookies.py @@ -237,6 +237,19 @@ def clear_session_cookies(response: Response, *, prefix: str = "") -> None: ) +def _pkce_attrs(*, use_https: bool, prefix: str) -> dict: + """Cookie attributes for the PKCE cookie's set AND clear paths. + + Single source of truth so a deletion always matches the shape the + setter emitted for the same origin — a shape mismatch means the + browser silently keeps the stale cookie. + """ + attrs = _common_attrs(use_https=use_https, prefix=prefix) + if use_https: + attrs["samesite"] = "none" + return attrs + + def set_pkce_cookie( response: Response, *, payload: str, use_https: bool, prefix: str = "", ) -> None: @@ -249,25 +262,45 @@ def set_pkce_cookie( # sidesteps the bug — these cookies are explicitly designed for cross-site # delivery and Chromium processes them reliably during redirects. # Loopback HTTP degrades to Lax (SameSite=None requires Secure). - attrs = _common_attrs(use_https=use_https, prefix=prefix) - if use_https: - attrs["samesite"] = "none" response.set_cookie( _resolved_name(PKCE_COOKIE, use_https=use_https, prefix=prefix), payload, max_age=_PKCE_MAX_AGE, - **attrs, + **_pkce_attrs(use_https=use_https, prefix=prefix), ) -def clear_pkce_cookie(response: Response, *, prefix: str = "") -> None: +def clear_pkce_cookie( + response: Response, *, use_https: bool, prefix: str = "", +) -> None: + """Emit Max-Age=0 deletions for every plausible PKCE cookie variant. + + A deletion is only honoured when its shape is acceptable to the + browser on the current origin: a ``Secure`` deletion can be dropped + on a plain-HTTP origin, while the ``__Host-``/``__Secure-`` name + variants REQUIRE ``Secure`` to be valid at all. So the bare-name + deletion mirrors the setter's shape for the active origin (Lax + without ``Secure`` over HTTP; ``SameSite=None; Secure`` over HTTPS, + matching :func:`set_pkce_cookie`), and the prefixed variants — which + can only ever have been set on an HTTPS origin — always carry + ``Secure; SameSite=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="none", - secure=True, - ) + if variant: + # __Host-/__Secure- cookies only exist over HTTPS, and a + # deletion for a prefixed name must itself be Secure or the + # browser rejects the header outright. + response.set_cookie( + f"{variant}{PKCE_COOKIE}", "", max_age=0, + path=path, httponly=True, samesite="none", + secure=True, + ) + else: + response.set_cookie( + f"{variant}{PKCE_COOKIE}", "", max_age=0, + **_pkce_attrs(use_https=use_https, prefix=prefix), + ) def _read_with_fallback( diff --git a/hermes_cli/dashboard_auth/routes.py b/hermes_cli/dashboard_auth/routes.py index 0c142963bc..c9086179ac 100644 --- a/hermes_cli/dashboard_auth/routes.py +++ b/hermes_cli/dashboard_auth/routes.py @@ -530,7 +530,7 @@ async def auth_callback( # Clear the PKCE cookie (its job is done) but set NO session cookies: # the desktop is not a browser session, it redeems the code for a # bearer token it stores itself. - clear_pkce_cookie(resp, prefix=_prefix(request)) + clear_pkce_cookie(resp, use_https=detect_https(request), prefix=_prefix(request)) clear_sso_attempt_cookie(resp, prefix=_prefix(request)) return resp @@ -551,7 +551,7 @@ async def auth_callback( prefix=_prefix(request), provider=session.provider, ) - clear_pkce_cookie(resp, prefix=_prefix(request)) + clear_pkce_cookie(resp, use_https=detect_https(request), prefix=_prefix(request)) # Clear the one-shot auto-SSO loop-guard marker now that login succeeded, # so it never lingers to suppress a future silent attempt after logout. clear_sso_attempt_cookie(resp, prefix=_prefix(request)) @@ -766,7 +766,7 @@ async def auth_logout(request: Request): prefix = _prefix(request) resp = RedirectResponse(url=f"{prefix}/login", status_code=302) clear_session_cookies(resp, prefix=prefix) - clear_pkce_cookie(resp, prefix=prefix) + clear_pkce_cookie(resp, use_https=detect_https(request), prefix=prefix) return resp diff --git a/tests/hermes_cli/test_dashboard_auth_cookies.py b/tests/hermes_cli/test_dashboard_auth_cookies.py index 2cd48f4c4d..04f91e7b25 100644 --- a/tests/hermes_cli/test_dashboard_auth_cookies.py +++ b/tests/hermes_cli/test_dashboard_auth_cookies.py @@ -45,7 +45,7 @@ def _build_app(use_https: bool = True, prefix: str = ""): def clear(): r = Response("ok") clear_session_cookies(r, prefix=prefix) - clear_pkce_cookie(r, prefix=prefix) + clear_pkce_cookie(r, use_https=use_https, prefix=prefix) return r return app @@ -135,3 +135,74 @@ def test_read_session_cookies_from_request_secure_prefix(): + +# --------------------------------------------------------------------------- +# PKCE cookie set/clear contract — the OAuth round trip crosses sites, so the +# attribute shape (SameSite / Secure) is load-bearing, not cosmetic. These +# tests pin the full Set-Cookie header shape for both origins so a regression +# in either direction (cookie dropped by Chromium mid-redirect, or a stale +# cookie surviving a clear) fails loudly. +# --------------------------------------------------------------------------- + + +def test_pkce_cookie_https_is_samesite_none_secure(): + """HTTPS: the PKCE cookie must be SameSite=None + Secure. + + The cookie is set on the /auth/login 302 and must survive the + cross-site redirect chain through the IDP back to /auth/callback. + Chromium intermittently drops SameSite=Lax cookies set on a 302 in a + cross-site chain (crbug 40508226); SameSite=None is the fix. + """ + client = TestClient(_build_app(use_https=True, prefix="")) + r = client.get("/set-pkce") + cookies = r.headers.get_list("set-cookie") + pkce = next(c for c in cookies if c.startswith(f"__Host-{PKCE_COOKIE}=")) + assert "samesite=none" in pkce.lower() + assert "Secure" in pkce + assert "HttpOnly" in pkce + + +def test_pkce_cookie_http_stays_lax_without_secure(): + """Loopback HTTP dev: SameSite=None requires Secure, which HTTP can't + carry — so the setter degrades to bare-name Lax without Secure.""" + client = TestClient(_build_app(use_https=False, prefix="")) + r = client.get("/set-pkce") + cookies = r.headers.get_list("set-cookie") + pkce = next(c for c in cookies if c.startswith(f"{PKCE_COOKIE}=")) + assert "samesite=lax" in pkce.lower() + assert "Secure" not in pkce + + +def test_clear_pkce_cookie_https_matches_set_shape(): + """HTTPS clear: every name variant is deleted with SameSite=None + + Secure — matching the HTTPS setter so the browser honours the + deletion for whichever variant was actually set.""" + client = TestClient(_build_app(use_https=True, prefix="")) + cookies = client.get("/clear").headers.get_list("set-cookie") + for name in (f"__Host-{PKCE_COOKIE}", f"__Secure-{PKCE_COOKIE}", PKCE_COOKIE): + deletion = next(c for c in cookies if c.startswith(f'{name}="')) + assert "Max-Age=0" in deletion + assert "samesite=none" in deletion.lower() + assert "Secure" in deletion + + +def test_clear_pkce_cookie_http_bare_deletion_is_insecure_lax(): + """HTTP clear: the bare-name deletion must mirror the HTTP setter's + shape (Lax, no Secure). A Secure deletion can be ignored by browsers + on a plain-HTTP origin, leaving a stale PKCE cookie behind. The + __Host-/__Secure- variants require Secure to be valid at all, so + those deletions keep it regardless of origin.""" + client = TestClient(_build_app(use_https=False, prefix="")) + cookies = client.get("/clear").headers.get_list("set-cookie") + bare = next( + c for c in cookies + if c.startswith(f'{PKCE_COOKIE}="') + and not c.startswith("__") + ) + assert "Max-Age=0" in bare + assert "samesite=lax" in bare.lower() + assert "Secure" not in bare + for name in (f"__Host-{PKCE_COOKIE}", f"__Secure-{PKCE_COOKIE}"): + deletion = next(c for c in cookies if c.startswith(f'{name}="')) + assert "Max-Age=0" in deletion + assert "Secure" in deletion diff --git a/website/docs/user-guide/features/web-dashboard.md b/website/docs/user-guide/features/web-dashboard.md index 30c55a9ae6..67c413d547 100644 --- a/website/docs/user-guide/features/web-dashboard.md +++ b/website/docs/user-guide/features/web-dashboard.md @@ -926,10 +926,10 @@ Access tokens have a 15-minute TTL. **There is no refresh token in contract v1** | Name | Lifetime | Notes | |------|----------|-------| | `hermes_session_at` | Token TTL (15 min) | HttpOnly, SameSite=Lax, Secure-when-HTTPS | -| `hermes_session_pkce` | 10 min | HttpOnly; holds the PKCE verifier + provider hint during the round trip | +| `hermes_session_pkce` | 10 min | HttpOnly; holds the PKCE verifier + provider hint during the round trip. SameSite=None + Secure over HTTPS (must survive the cross-site IDP redirect chain — Chromium drops SameSite=Lax cookies set on a 302 in a cross-site chain); SameSite=Lax on loopback HTTP | | `hermes_session_rt` | unused in v1 | Reserved for forward-compat; not written when `refresh_token` is empty | -All three are `Path=/` and `SameSite=Lax`. The `Secure` flag is set when the dashboard is reached over HTTPS (detected via the request URL scheme — honours `X-Forwarded-Proto` from an upstream TLS terminator under `proxy_headers=True`). +All three are `Path=/`. The session cookies are `SameSite=Lax`; the PKCE cookie is `SameSite=None` when set over HTTPS (see table). The `Secure` flag is set when the dashboard is reached over HTTPS (detected via the request URL scheme — honours `X-Forwarded-Proto` from an upstream TLS terminator under `proxy_headers=True`). ### Logout