fix(auth): mirror the PKCE setter's cookie shape in clear_pkce_cookie

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).
This commit is contained in:
Ben Barclay
2026-08-11 10:03:51 +10:00
parent 7c7cd24d00
commit 5e57bf19a2
4 changed files with 120 additions and 16 deletions

View File

@@ -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(

View File

@@ -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

View File

@@ -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

View File

@@ -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