diff --git a/hermes_cli/dashboard_auth/cookies.py b/hermes_cli/dashboard_auth/cookies.py index 5c49df2421..0c26d3ac0a 100644 --- a/hermes_cli/dashboard_auth/cookies.py +++ b/hermes_cli/dashboard_auth/cookies.py @@ -66,8 +66,12 @@ Refresh-token handling: """ from __future__ import annotations +import base64 +import binascii +import json +import re from typing import Literal, Optional, Tuple -from urllib.parse import quote, unquote +from urllib.parse import unquote from fastapi import Request from fastapi.responses import Response @@ -295,8 +299,35 @@ def _pkce_attrs(*, use_https: bool, prefix: str) -> dict: return attrs +def encode_pkce_payload(parts: dict[str, str]) -> str: + """Serialise PKCE segments to the wire value: ``base64url(JSON)``. + + The urlsafe base64 alphabet (``A-Za-z0-9-_``, padding stripped) is a + strict subset of the RFC 6265 cookie-octet set — no ``;`` (attribute + terminator), no ``"`` and no ``\\`` (the chars that make Python's + http.cookies emit the quoted ``\\073`` form, which strict cookie-aware + proxy hops such as Go's net/http reject outright). The ``=`` padding + is stripped because http.cookies treats ``=`` as outside its legal + unquoted set and would re-wrap the value in the quoted form this + codec exists to avoid; the parser restores the padding. JSON carries + the segments, so no delimiter can ever collide with segment values — + the delimiter/quoting bug class this codec replaces (see + :func:`parse_pkce_payload` for the two legacy formats it superseded). + """ + raw = json.dumps(parts, separators=(",", ":"), sort_keys=True) + return ( + base64.urlsafe_b64encode(raw.encode("utf-8")) + .decode("ascii") + .rstrip("=") + ) + + def set_pkce_cookie( - response: Response, *, payload: str, use_https: bool, prefix: str = "", + response: Response, + *, + payload: dict[str, str], + use_https: bool, + prefix: str = "", ) -> None: # SameSite=None when HTTPS: the PKCE cookie is set on the /auth/login # 302 response (redirecting to the IDP) and must survive the cross-site @@ -308,29 +339,17 @@ def set_pkce_cookie( # delivery and Chromium processes them reliably during redirects. # Loopback HTTP degrades to Lax (SameSite=None requires Secure). # - # Value encoding: the PKCE payload is a flat ``key=value;key=value`` - # string (``provider=…;state=…;verifier=…;next=…``). A raw ``;`` is a - # cookie-attribute terminator, so Python's http.cookies wraps the value - # in double quotes and escapes each ``;`` as the backslash-octal - # ``\073``. Mainstream browsers store and echo that quoted form - # verbatim, and Python parsers decode it — but the quoted form is NOT - # made of plain RFC 6265 cookie-octets (``"`` and ``\`` are outside the - # set), and stricter cookie-aware hops that parse and re-emit the - # Cookie header drop the cookie entirely (verified for Go's net/http, - # which rejects any cookie value containing a backslash — the parser - # underlying much Go-based proxy/IDP middleware). The callback then - # fails with "Missing PKCE state cookie" even though the browser sent - # it (field case: a Traefik + Authentik + VPN chain, support thread - # "Still unable to use Authentik for signin with traefik" — devtools - # showed the browser sending the intact quoted cookie while Hermes - # logged missing_pkce_cookie). URL-encoding the whole payload - # (``;`` → ``%3B``) - # keeps the wire value inside the unquoted cookie-octet set so every - # RFC 6265 parser passes it through untouched. The readers in - # routes.py decode via parse_pkce_payload() before the ``;`` split. + # Value encoding: ``payload`` is the segment dict + # (``{"provider": …, "state": …, "verifier": …, "next": …}``) and goes + # on the wire as base64url(JSON) via encode_pkce_payload() — plain + # RFC 6265 cookie-octets end to end, so every cookie-aware hop + # (browsers, Go net/http proxies, Python parsers) passes the value + # through untouched. Readers decode via parse_pkce_payload(), which + # also keeps a compatibility ladder for cookies minted by the two + # earlier wire formats during a rolling upgrade. response.set_cookie( _resolved_name(PKCE_COOKIE, use_https=use_https, prefix=prefix), - quote(payload, safe=""), + encode_pkce_payload(payload), max_age=_PKCE_MAX_AGE, **_pkce_attrs(use_https=use_https, prefix=prefix), ) @@ -391,34 +410,67 @@ def read_pkce_cookie(request: Request) -> Optional[str]: return _read_with_fallback(request, PKCE_COOKIE) +# base64url wire values are exactly the urlsafe alphabet (padding is +# stripped by the encoder; the decoder restores it). Used as a cheap +# pre-filter before attempting the JSON decode so legacy wire forms +# (which always contain ``%`` or ``;``) never even reach the base64 +# decoder. +_B64URL_RE = re.compile(r"^[A-Za-z0-9_-]+={0,2}$") + + def parse_pkce_payload(raw: str) -> dict[str, str]: """Decode + parse a PKCE cookie value into its segment dict. - Single inverse of :func:`set_pkce_cookie`'s encoding: URL-decode the - wire value back to the flat ``provider=...;state=...;verifier=...`` - shape, then split on ``;``. EVERY reader of the PKCE cookie must go - through this helper — a reader that splits the raw wire value sees - the fully URL-encoded string (even ``=`` is ``%3D``), parses zero - segments, and silently disables whatever check it was feeding - (provider dispatch, CSRF state, native-flow broker binding). + Single inverse of :func:`set_pkce_cookie` / + :func:`encode_pkce_payload`. EVERY reader of the PKCE cookie must go + through this helper — a reader that interprets the raw wire value + itself parses zero segments and silently disables whatever check it + was feeding (provider dispatch, CSRF state, native-flow broker + binding). - Mixed-version compatibility (10-minute PKCE TTL during a rolling - upgrade): a cookie minted by a pre-encoding server arrives here — - after starlette's cookie-header unquoting — as the flat form with - RAW ``;`` between segments, and its ``next`` segment still carries - its own single URL-encoding (``next=%2F...``). The new encoded form - can never contain a raw ``;`` (it is ``%3B``). So a raw ``;`` is an - exact old-format discriminator: split it as-is WITHOUT the payload - decode, exactly like the old reader did, so an old ``next`` value - containing ``%3B`` is not decoded into a bogus delimiter. (The - reverse direction — a new encoded cookie hitting an old server — - fails the state check and the user retries; nothing to do here.) + Compatibility ladder — the PKCE cookie has a 10-minute TTL and is + opaque + server-set, so during a rolling upgrade a cookie minted by + one server version can arrive at another. Three formats, tried in + order; each rung is unambiguous: + + 1. **base64url(JSON)** (current): the wire value is pure urlsafe + base64 that decodes to a JSON object. Legacy forms can never + match — they always contain ``%`` (URL-encoded, #99176) or a raw + ``;`` (oldest flat form), both outside the base64url alphabet. + 2. **Oldest flat form** (pre-#99176): raw ``;`` between segments + (``provider=…;state=…;verifier=…``). Split as-is WITHOUT + unquoting the payload — the ``next`` segment carries its own + single URL-encoding, and unquoting here would turn a ``%3B`` + inside it into a bogus delimiter and truncate the post-login + target. Neither newer format can contain a raw ``;``. + 3. **URL-encoded flat form** (#99176): the whole flat payload passed + through ``quote(payload, safe="")`` — no raw ``;`` possible + (it is ``%3B``); unquote once, then split. + + Rollout directions: OLD cookie → NEW server is handled here (rungs + 2 and 3 parse both legacy forms correctly). NEW cookie → OLD server + (a rollback, or a mixed fleet routing the callback to a not-yet- + upgraded instance) fails the OAuth state check — the old reader + can't find a ``state`` segment in the base64url blob — and the user + simply retries login against the now-consistent fleet; no data loss, + nothing minted. """ + if _B64URL_RE.match(raw): + try: + padded = raw + "=" * (-len(raw) % 4) + decoded = json.loads( + base64.urlsafe_b64decode(padded.encode("ascii")) + ) + except (binascii.Error, ValueError, UnicodeDecodeError): + decoded = None + if isinstance(decoded, dict): + return {str(k): str(v) for k, v in decoded.items()} if ";" in raw: - # Old-format (pre-encoding) cookie: already flat, split as-is. + # Oldest flat form: already flat, split as-is (no unquote). return dict( seg.split("=", 1) for seg in raw.split(";") if "=" in seg ) + # #99176 URL-encoded flat form: unquote once, then split. return dict( seg.split("=", 1) for seg in unquote(raw).split(";") if "=" in seg ) diff --git a/hermes_cli/dashboard_auth/routes.py b/hermes_cli/dashboard_auth/routes.py index 57acfda1be..77f9c8ee25 100644 --- a/hermes_cli/dashboard_auth/routes.py +++ b/hermes_cli/dashboard_auth/routes.py @@ -113,6 +113,23 @@ def _client_ip(request: Request) -> str: return request.client.host if request.client else "" +def _provider_pkce_segments(cookie_payload: dict[str, str]) -> dict[str, str]: + """Segment dict from a provider's ``LoginStart.cookie_payload``. + + Providers serialise their PKCE material as the flat + ``state=…;verifier=…`` string documented on + :class:`~hermes_cli.dashboard_auth.base.LoginStart` (values are + ``token_urlsafe`` — never containing ``;`` or ``=``). This is the ONE + place that flat provider string is parsed; from here on the payload + is a dict all the way to :func:`set_pkce_cookie`'s base64url(JSON) + wire encoding. + """ + flat = cookie_payload.get("hermes_session_pkce", "") + return dict( + seg.split("=", 1) for seg in flat.split(";") if "=" in seg + ) + + def _prefix(request: Request) -> str: """Resolve the X-Forwarded-Prefix header for the active request. @@ -225,20 +242,21 @@ async def auth_login(request: Request, provider: str, next: str = ""): resp = RedirectResponse(url=ls.redirect_url, status_code=302) # Pack the provider name into the PKCE cookie so the callback can # find it without a separate cookie. Provider may or may not have - # already included a ``provider=`` segment. - pkce = ls.cookie_payload.get("hermes_session_pkce", "") - if "provider=" not in pkce: - pkce = f"provider={provider};{pkce}" if pkce else f"provider={provider}" + # already included a ``provider`` segment. + pkce = _provider_pkce_segments(ls.cookie_payload) + pkce.setdefault("provider", provider) # Carry ``next=`` through the round trip in the PKCE cookie. Real # IDPs only echo back ``code`` + ``state`` on the callback URL, so # query-string transport would lose the value — the cookie is the # only server-controlled channel that survives. Validate before we # store it so an attacker who reaches /auth/login directly with - # ``next=//evil.example`` can't poison the cookie. + # ``next=//evil.example`` can't poison the cookie. Stored as the + # validator's decoded path VERBATIM — JSON has no delimiter to + # collide with, so no extra encoding layer (the callback validator's + # single unquote stays symmetric with the query-string decode). safe_next = _validate_post_login_target(next) if safe_next: - from urllib.parse import quote - pkce = f"{pkce};next={quote(safe_next, safe='')}" + pkce["next"] = safe_next set_pkce_cookie( resp, payload=pkce, use_https=detect_https(request), prefix=_prefix(request), @@ -391,7 +409,7 @@ async def auth_native_authorize( ) set_pkce_cookie( resp, - payload=f"provider={p.name};broker={broker_state}", + payload={"provider": p.name, "broker": broker_state}, use_https=detect_https(request), prefix=_prefix(request), ) @@ -413,10 +431,9 @@ async def auth_native_authorize( # cookie so the callback can (a) dispatch to the right provider and (b) # find the pending native authorization. The desktop's challenge/state # never touch this cookie — only our opaque broker_state does. - pkce = ls.cookie_payload.get("hermes_session_pkce", "") - if "provider=" not in pkce: - pkce = f"provider={p.name};{pkce}" if pkce else f"provider={p.name}" - pkce = f"{pkce};broker={broker_state}" + pkce = _provider_pkce_segments(ls.cookie_payload) + pkce.setdefault("provider", p.name) + pkce["broker"] = broker_state set_pkce_cookie( resp, payload=pkce, use_https=detect_https(request), prefix=_prefix(request), @@ -444,13 +461,11 @@ async def auth_callback( detail="Missing PKCE state cookie", ) - # Parse ``provider=...;state=...;verifier=...;next=...`` — the - # ``next`` segment is optional (only present when /auth/login was - # given a next= query). All keys live in the same flat namespace; - # ``next`` carries a URL-encoded path so it never contains ``;``. - # parse_pkce_payload URL-decodes the wire value (the setter encodes - # the whole payload so no raw ``;``/``"``/``\`` reaches the wire) - # before the ``;`` split. + # Parse the segment dict (``provider`` / ``state`` / ``verifier`` / + # ``next`` — ``next`` only present when /auth/login was given a + # next= query). parse_pkce_payload decodes the base64url(JSON) wire + # value, with a compatibility ladder for the two legacy flat formats + # (see its docstring). parts = parse_pkce_payload(pkce_raw) provider_name = parts.get("provider", "") expected_state = parts.get("state", "") diff --git a/tests/hermes_cli/test_dashboard_auth_cookies.py b/tests/hermes_cli/test_dashboard_auth_cookies.py index 4a5a0eea46..6feeb68c23 100644 --- a/tests/hermes_cli/test_dashboard_auth_cookies.py +++ b/tests/hermes_cli/test_dashboard_auth_cookies.py @@ -37,8 +37,11 @@ def _build_app(use_https: bool = True, prefix: str = ""): @app.get("/set-pkce") def set_pkce(): r = Response("ok") - set_pkce_cookie(r, payload="provider=stub;state=s;verifier=v", - use_https=use_https, prefix=prefix) + set_pkce_cookie( + r, + payload={"provider": "stub", "state": "s", "verifier": "v"}, + use_https=use_https, prefix=prefix, + ) return r @app.get("/clear") @@ -134,37 +137,40 @@ def test_read_session_cookies_from_request_secure_prefix(): # --------------------------------------------------------------------------- -# PKCE cookie: regression for #83832 (field case: Traefik+Authentik chain) +# PKCE cookie codec: base64url(JSON) wire format # --------------------------------------------------------------------------- # -# The PKCE payload is a flat ``key=value;key=value`` string. A raw ``;`` -# is a cookie-attribute terminator, so Python's http.cookies emits the -# value in RFC 6265 quoted form with each ``;`` escaped as the -# backslash-octal ``\073``. Mainstream browsers echo that form back -# verbatim and Python decodes it — but ``"`` and ``\`` are outside the -# plain cookie-octet set, and cookie-aware proxy hops that parse and -# re-emit the Cookie header (verified for Go's net/http, common in -# Go-based proxy/IDP middleware) reject the value and drop the cookie -# entirely. The callback -# then 400s with "Missing PKCE state cookie" even though the browser -# sent the cookie. The fix URL-encodes the payload in the setter so the -# wire value contains only cookie-octets, and every reader decodes via -# cookies.parse_pkce_payload(). These tests pin the wire shape and the -# round trip. +# History (three serialization fixes at this exact spot): the payload was +# originally a flat ``key=value;key=value`` string. A raw ``;`` is a +# cookie-attribute terminator, so Python's http.cookies emitted the value +# in RFC 6265 quoted form with each ``;`` escaped as ``\073`` — a form +# strict cookie-aware proxy hops (verified for Go's net/http) reject, +# dropping the cookie entirely (#83832, Traefik+Authentik field case). +# #99176 URL-encoded the whole flat payload to stay inside the +# cookie-octet set. The current codec removes the delimiter problem at +# the root: the payload is a dict, serialised as base64url(JSON) — the +# urlsafe alphabet is a strict subset of the cookie-octets, and JSON +# means no segment value can ever collide with a delimiter. Readers keep +# a compatibility ladder for both legacy wire forms (10-minute TTL, +# rolling upgrades). These tests pin the wire shape, the round trip, and +# every ladder rung. -def test_set_pkce_cookie_url_encodes_payload_to_avoid_rfc6265_split(): +def test_set_pkce_cookie_wire_value_is_cookie_octet_base64url_json(): """The wire-level cookie value must contain only plain RFC 6265 cookie-octets: no raw ``;`` (attribute terminator), no ``"`` and no ``\\`` (the http.cookies quoted form that strict cookie-aware proxy parsers — verified for Go's net/http — reject, dropping the whole - cookie). + cookie). With base64url(JSON) the value is drawn from the urlsafe + base64 alphabet, a strict subset of the cookie-octet set. - Regression for #83832 / the Traefik+Authentik support case: the - callback failed with "Missing PKCE state cookie" because a proxy - hop dropped the quoted ``\\073`` form. + Regression lineage: #83832 / the Traefik+Authentik support case — + the callback failed with "Missing PKCE state cookie" because a + proxy hop dropped the quoted ``\\073`` form. """ - from urllib.parse import unquote + import base64 + import json + client = TestClient(_build_app(use_https=True, prefix="")) r = client.get("/set-pkce") pkce_set = next( @@ -174,9 +180,7 @@ def test_set_pkce_cookie_url_encodes_payload_to_avoid_rfc6265_split(): # Take just the cookie name=value pair, ignore the attributes. pkce_value = pkce_set.split(";", 1)[0] wire = pkce_value.split("=", 1)[1] - # No unquoted literal ``;`` in the value (attribute terminator). The - # payload ``provider=stub;state=s;verifier=v`` is encoded as - # ``provider%3Dstub%3Bstate%3Ds%3Bverifier%3Dv``. + # No unquoted literal ``;`` in the value (attribute terminator). assert ";" not in wire, ( f"unquoted ; leaked into the cookie value: {pkce_value!r}" ) @@ -194,22 +198,54 @@ def test_set_pkce_cookie_url_encodes_payload_to_avoid_rfc6265_split(): assert all(ch in cookie_octets for ch in wire), ( f"non-cookie-octet chars in the wire value: {wire!r}" ) - # Round-trip the URL-encoding back to the original payload. - decoded = unquote(wire) - assert decoded == "provider=stub;state=s;verifier=v", ( - f"URL-encoded payload didn't round-trip to the original: " - f"got {decoded!r}" + # And tighter than cookie-octets: pure urlsafe base64 (padding is + # stripped by the encoder — ``=`` is outside http.cookies' legal + # unquoted set and would trigger the quoted form). + b64url = ( + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" + "0123456789-_" + ) + assert all(ch in b64url for ch in wire), ( + f"non-base64url chars in the wire value: {wire!r}" + ) + # Round-trip the codec back to the original segment dict. + decoded = json.loads( + base64.urlsafe_b64decode(wire + "=" * (-len(wire) % 4)) + ) + assert decoded == {"provider": "stub", "state": "s", "verifier": "v"}, ( + f"base64url(JSON) payload didn't round-trip: got {decoded!r}" ) +def test_encode_parse_pkce_payload_round_trips_hostile_values(): + """The codec must round-trip segment values containing every char + that broke the two previous formats — ``;`` ``=`` ``"`` ``\\`` ``%`` + — byte-for-byte. This is the bug class the JSON codec kills: with + delimiter-based formats, these bytes collide with the framing. + """ + from hermes_cli.dashboard_auth.cookies import ( + encode_pkce_payload, + parse_pkce_payload, + ) + + payload = { + "provider": "stub", + "state": 's;t="a\\te"', + "verifier": "v=1%3B;x", + "next": "/sessions?x=a;b&project=foo%25", + } + assert parse_pkce_payload(encode_pkce_payload(payload)) == payload + + def test_parse_pkce_payload_old_format_cookie_survives_rolling_upgrade(): - """Mixed-version window (10-minute PKCE TTL): a cookie minted by a - pre-encoding server arrives at the new reader — after starlette's - cookie-header unquoting — as the FLAT form with raw ``;`` between - segments and a single-encoded ``next``. The reader must split it - as-is, NOT payload-decode it first: decoding early would turn an - old ``next`` value containing ``%3B`` into a bogus delimiter and - truncate the post-login target. + """Compat ladder rung 2 — oldest flat form (pre-#99176). Mixed-version + window (10-minute PKCE TTL): a cookie minted by a pre-encoding server + arrives at the new reader — after starlette's cookie-header + unquoting — as the FLAT form with raw ``;`` between segments and a + single-encoded ``next``. The reader must split it as-is, NOT + payload-decode it first: decoding early would turn an old ``next`` + value containing ``%3B`` into a bogus delimiter and truncate the + post-login target. """ from hermes_cli.dashboard_auth.cookies import parse_pkce_payload @@ -228,9 +264,13 @@ def test_parse_pkce_payload_old_format_cookie_survives_rolling_upgrade(): }, f"old-format cookie mis-parsed: {parts!r}" -def test_parse_pkce_payload_new_format_round_trips_setter_encoding(): - """The new encoded wire form (no raw ``;`` possible — it is %3B) - decodes back to the exact original payload segments.""" +def test_parse_pkce_payload_99176_url_encoded_format_survives_upgrade(): + """Compat ladder rung 3 — the #99176 URL-encoded flat form + (``quote(payload, safe='')`` over the whole flat string; no raw + ``;`` possible — it is %3B). A cookie minted by a #99176-era server + during the 10-minute mixed-version window must decode to the exact + original segments: unquote once, then split. + """ from urllib.parse import quote from hermes_cli.dashboard_auth.cookies import parse_pkce_payload @@ -244,7 +284,7 @@ def test_parse_pkce_payload_new_format_round_trips_setter_encoding(): "state": "s123", "verifier": "v456", "next": "%2Fsessions", - }, f"new-format wire value mis-parsed: {parts!r}" + }, f"#99176-format wire value mis-parsed: {parts!r}" def test_pkce_cookie_round_trip_preserves_all_segments(): @@ -261,7 +301,7 @@ def test_pkce_cookie_round_trip_preserves_all_segments(): from conftest_dashboard_auth import StubAuthProvider # type: ignore from hermes_cli import web_server from hermes_cli.dashboard_auth import clear_providers, register_provider - from urllib.parse import unquote + from hermes_cli.dashboard_auth.cookies import parse_pkce_payload clear_providers() register_provider(StubAuthProvider()) @@ -289,14 +329,14 @@ def test_pkce_cookie_round_trip_preserves_all_segments(): # Pull just the name=value portion so we can echo it back as # a Cookie header. pkce_kv = pkce_set.split(";", 1)[0] - # Confirm the setter URL-encoded the value. + # Decode through the real reader inverse: base64url(JSON). encoded_value = pkce_kv.split("=", 1)[1] - decoded_value = unquote(encoded_value) + parts = parse_pkce_payload(encoded_value) # The login handler packs provider, state, and verifier # into the payload. All three must survive intact. - assert "provider=stub" in decoded_value - assert "state=" in decoded_value - assert "verifier=" in decoded_value + assert parts.get("provider") == "stub" + assert parts.get("state") + assert parts.get("verifier") # And the encoded wire value must NOT have a literal, unquoted ``;`` # between segments. assert ";" not in encoded_value, ( diff --git a/tests/hermes_cli/test_dashboard_auth_prefix.py b/tests/hermes_cli/test_dashboard_auth_prefix.py index f56858ec8c..e18e58de26 100644 --- a/tests/hermes_cli/test_dashboard_auth_prefix.py +++ b/tests/hermes_cli/test_dashboard_auth_prefix.py @@ -451,7 +451,7 @@ class TestCookiePathRespectsPrefix: @app.get("/set") def _set(): r = Response("ok") - set_pkce_cookie(r, payload="x", use_https=False) + set_pkce_cookie(r, payload={"provider": "x"}, use_https=False) return r client = TestClient(app)