ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 12 Packages Projects Releases Wiki Activity

refactor(dashboard-auth): replace PKCE cookie payload with base64url(JSON) codec (#99210)

Browse Source 
The PKCE cookie's payload has now needed three serialization fixes at
the same spot: the original flat 'k=v;k=v' string tripped http.cookies'
\073 quoted form (dropped whole by strict cookie parsers like Go's
net/http — #83832 field case), and #99176 URL-encoded the whole flat
payload to stay inside the RFC 6265 cookie-octet set. The stacked
layers (single-encoded next=, ';' joins, whole-payload encoding, legacy
discriminator) were the recurring defect source.

Kill the bug class instead of patching it again: the payload is a dict
end-to-end and goes on the wire as base64url(JSON) — the urlsafe
alphabet is a strict subset of cookie-octets, and JSON framing means no
segment value can ever collide with a delimiter. parse_pkce_payload
keeps a three-rung compatibility ladder (base64url(JSON) -> oldest flat
form split-as-is -> #99176 unquote-then-split) for in-flight cookies
during a rolling upgrade (10-minute TTL); a new cookie hitting an old
server fails the OAuth state check and the user just retries.

The 'next' segment is stored as its plain validated path — no extra
encoding layer, so the post-login redirect Location is byte-for-byte
the original target.

Refs #99176, #84065.
This commit is contained in:
Ben Barclay
2026-09-01 11:47:38 +10:00
committed by GitHub
parent bcecd675f7
commit 56916841b5
4 changed files with 217 additions and 110 deletions
Download Patch File Download Diff File Expand all files Collapse all files
hermes_cli/dashboard_auth/cookies.py
+94 -42
View File
@@ -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
)
hermes_cli/dashboard_auth/routes.py
+34 -19
View File
@@ -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", "")
tests/hermes_cli/test_dashboard_auth_cookies.py
+88 -48
View File
@@ -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, (
tests/hermes_cli/test_dashboard_auth_prefix.py
+1 -1
View File
@@ -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)