fix(dashboard-auth): url-encode pkce cookie value to survive rfc 6265 split - #84065
Kailigithub wants to merge 1 commit into
Conversation
… split
The PKCE cookie value is a flat 'provider=...;state=...;verifier=...;next=...'
string. RFC 6265 treats the unquoted ';' as a cookie-value terminator, so
the browser stores only the first segment ('provider=...') and the OIDC
callback sees a partial payload — failing with 'Missing PKCE state cookie'
or 'Unknown provider in cookie'.
The previous behavior wrapped the value in double quotes and used '\073'
to escape the embedded ';', which works for Python's http.cookies parser
but breaks for browsers that follow RFC 6265 strictly. The portable fix
is to URL-encode the whole payload (safe='') before it hits the wire,
turning ';' into '%3B' with nothing left for the browser's cookie-value
parser to misinterpret. The callback reader URL-decodes the value
back to the original shape before the ';' split.
- cookies.set_pkce_cookie: wrap payload in urllib.parse.quote(safe='')
- routes.callback: URL-decode via urllib.parse.unquote before split
- test_dashboard_auth_cookies: 3 regression tests pinning the wire shape
and the round-trip via the existing end-to-end callback flow
Closes NousResearch#83832
|
Empirical data point from a real browser — the I did the test neither the issue nor this PR had done: drove a real browser (headless Chrome, persistent profile) against the exact wire bytes Test setup: local HTTP server serving the precise Starlette output: Step 1 — browser stores the cookie, then a fresh same-origin request: The full payload came back verbatim — no truncation at the first Step 2 — that exact header through the real read path ( So the claimed failure mode — "the browser stores only the first segment ( That said, the PR is still worth having — as hardening, with an honest framing: URL-encoding the payload removes reliance on the Python-specific octal escape (which other cookie parsers in the wild — proxies, CDNs, non-Python gateways — may not understand even if mainstream browsers do), and the decode is a documented no-op for pre-existing values. That's a legitimate interop-hardening rationale. What doesn't hold is "breaks OIDC login" as a bug claim on current main — the repro should be updated to a browser+proxy combo that actually mangles (For reference: my original claim on #83832 was withdrawn for the same reason — the premise didn't reproduce. This browser test closes the loop on that withdrawal.) |
fix(dashboard-auth): url-encode pkce cookie value to survive rfc 6265 split — good interop hardening, well documented. Observations:
|
… hops stop dropping it (#99176) The PKCE payload is a flat 'provider=...;state=...;verifier=...;next=...' 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 parsers decode it — the browser round trip is fine. But '"' and '\' are outside the plain cookie-octet set, and non-Python hops that re-serialize the Cookie header reject the value and drop the cookie entirely: Go's net/http (Traefik middleware, Authentik outposts, other gateways) refuses any cookie value containing a backslash. The OIDC callback then 400s with "Missing PKCE state cookie" even though the browser sent the cookie. Field reproduction: support thread "Still unable to use Authentik for signin with traefik" — devtools showed the browser sending the intact quoted \073 cookie on /auth/callback while Hermes logged missing_pkce_cookie behind a Traefik+Authentik chain. Fix: URL-encode the whole payload in set_pkce_cookie (quote(payload, safe='') — ';' becomes '%3B') so the wire value contains only cookie-octets and no parser in the chain has anything to reject, and decode through a single shared inverse, cookies.parse_pkce_payload(), in BOTH readers: the OAuth /auth/callback and the native password-login path (routes.login_submit), whose broker/provider binding check would otherwise parse zero segments from the newly-encoded value and silently disable itself. Regression coverage: the wire-shape test pins the full cookie-octet set (the '"'/'\' assertions are the ones a Go-parser hop fails pre-fix), the round-trip tests drive the real /auth/login → /auth/callback path, and the next= test pins the exact post-login redirect byte shape. Native-flow broker assertions updated to decode through parse_pkce_payload instead of substring-matching the raw wire value. Salvaged from #84065 (rebased onto current main, which gained the SameSite=None PKCE attrs and the RFC 8252 native password flow since the PR branched): kept main's _pkce_attrs cookie shape, extended the fix to the login_submit reader the original PR predated, and reframed the rationale — browsers do NOT truncate at the first ';' (there is no literal ';' on the wire in the quoted form); the failing hop is a strict middlebox cookie parser. Closes #83832 Co-authored-by: Kailigithub <12250313+Kailigithub@users.noreply.github.com>
|
Salvaged and merged as #99176 (authorship preserved via cherry-pick — the fix commit there is yours, thanks @Kailigithub). What changed in the salvage, for the record:
Closing as superseded by #99176. #83832 was closed by the merge. |
… hops stop dropping it (NousResearch#99176) The PKCE payload is a flat 'provider=...;state=...;verifier=...;next=...' 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 parsers decode it — the browser round trip is fine. But '"' and '\' are outside the plain cookie-octet set, and non-Python hops that re-serialize the Cookie header reject the value and drop the cookie entirely: Go's net/http (Traefik middleware, Authentik outposts, other gateways) refuses any cookie value containing a backslash. The OIDC callback then 400s with "Missing PKCE state cookie" even though the browser sent the cookie. Field reproduction: support thread "Still unable to use Authentik for signin with traefik" — devtools showed the browser sending the intact quoted \073 cookie on /auth/callback while Hermes logged missing_pkce_cookie behind a Traefik+Authentik chain. Fix: URL-encode the whole payload in set_pkce_cookie (quote(payload, safe='') — ';' becomes '%3B') so the wire value contains only cookie-octets and no parser in the chain has anything to reject, and decode through a single shared inverse, cookies.parse_pkce_payload(), in BOTH readers: the OAuth /auth/callback and the native password-login path (routes.login_submit), whose broker/provider binding check would otherwise parse zero segments from the newly-encoded value and silently disable itself. Regression coverage: the wire-shape test pins the full cookie-octet set (the '"'/'\' assertions are the ones a Go-parser hop fails pre-fix), the round-trip tests drive the real /auth/login → /auth/callback path, and the next= test pins the exact post-login redirect byte shape. Native-flow broker assertions updated to decode through parse_pkce_payload instead of substring-matching the raw wire value. Salvaged from NousResearch#84065 (rebased onto current main, which gained the SameSite=None PKCE attrs and the RFC 8252 native password flow since the PR branched): kept main's _pkce_attrs cookie shape, extended the fix to the login_submit reader the original PR predated, and reframed the rationale — browsers do NOT truncate at the first ';' (there is no literal ';' on the wire in the quoted form); the failing hop is a strict middlebox cookie parser. Closes NousResearch#83832 Co-authored-by: Kailigithub <12250313+Kailigithub@users.noreply.github.com>
…JSON) codec (#99210) 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.
* chore: sync fork with upstream through a9c783f * fix(dashboard-auth): url-encode the PKCE cookie value so strict proxy hops stop dropping it (NousResearch#99176) The PKCE payload is a flat 'provider=...;state=...;verifier=...;next=...' 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 parsers decode it — the browser round trip is fine. But '"' and '\' are outside the plain cookie-octet set, and non-Python hops that re-serialize the Cookie header reject the value and drop the cookie entirely: Go's net/http (Traefik middleware, Authentik outposts, other gateways) refuses any cookie value containing a backslash. The OIDC callback then 400s with "Missing PKCE state cookie" even though the browser sent the cookie. Field reproduction: support thread "Still unable to use Authentik for signin with traefik" — devtools showed the browser sending the intact quoted \073 cookie on /auth/callback while Hermes logged missing_pkce_cookie behind a Traefik+Authentik chain. Fix: URL-encode the whole payload in set_pkce_cookie (quote(payload, safe='') — ';' becomes '%3B') so the wire value contains only cookie-octets and no parser in the chain has anything to reject, and decode through a single shared inverse, cookies.parse_pkce_payload(), in BOTH readers: the OAuth /auth/callback and the native password-login path (routes.login_submit), whose broker/provider binding check would otherwise parse zero segments from the newly-encoded value and silently disable itself. Regression coverage: the wire-shape test pins the full cookie-octet set (the '"'/'\' assertions are the ones a Go-parser hop fails pre-fix), the round-trip tests drive the real /auth/login → /auth/callback path, and the next= test pins the exact post-login redirect byte shape. Native-flow broker assertions updated to decode through parse_pkce_payload instead of substring-matching the raw wire value. Salvaged from NousResearch#84065 (rebased onto current main, which gained the SameSite=None PKCE attrs and the RFC 8252 native password flow since the PR branched): kept main's _pkce_attrs cookie shape, extended the fix to the login_submit reader the original PR predated, and reframed the rationale — browsers do NOT truncate at the first ';' (there is no literal ';' on the wire in the quoted form); the failing hop is a strict middlebox cookie parser. Closes NousResearch#83832 Co-authored-by: Kailigithub <12250313+Kailigithub@users.noreply.github.com> * fix(telegram): bound polling drain with wall-clock deadline _drain_polling_connections still bounded its shutdown()/initialize() with asyncio.wait_for (NousResearch#66377), while its sibling the general-pool drain moved to _await_with_thread_deadline (NousResearch#98094). httpcore's pool close runs under AsyncShieldCancellation, so a cancellation-resistant close keeps wait_for pending forever even after its timeout fires — the tracked _polling_error_task wedges and every escalation gate behind it stalls. Use the same wall-clock deadline helper (cancel + abandon, no cancel-await) on both polling-drain awaits, and add a regression test whose close swallows cancellation — the shape the existing cancellable-hang test cannot catch. * test: settle generation verifier in shielded-close drain test * refactor(telegram): share exception-graph walk across classifiers _looks_like_connect_timeout and _looks_like_pool_timeout carried two copies of the same 15-line DFS skeleton (seen-set, stack, __cause__/ __context__ descent) differing only in the one-line match predicate — follow-up to the NousResearch#98094 review. Extract _iter_exception_graph() and collapse both classifiers onto it. Behavior is byte-identical (subprocess parity vs origin/main on real PTB error fixtures: 6/6 identical), and the two classifiers gain direct unit tests for the first time, including the cycle/diamond chain shapes the inline copies had no coverage for. * test: stabilize Telegram deadline assertion on Windows * fix: address hosted room review findings * fix: close hosted room publication races * fix(relay): resolve fresh-final unfurl decision per chat, not per primary identity (NousResearch#99206) The stream consumer called prefers_fresh_final_streaming(text, metadata=...) only, and no metadata producer stamps a platform key — so RelayAdapter's hook always fell back to the PRIMARY descriptor's platform (the scalar-vs-per-chat capability seam, third occurrence). Two failure directions on multiplexed relays with platforms.relay.extra.slack.unfurl_links/media: true (NousResearch#97957): - Slack primary fronting Telegram/Discord: every link-bearing streamed final on the non-Slack chats finalized as a fresh send with no delete op advertised -> the answer delivered TWICE (orphaned preview). - Non-Slack primary fronting Slack: the hook returned False, leaving the force-on unfurl feature dark on exactly the chats it shipped for. Pass chat_id=self.chat_id from the consumer; the relay hook already accepted it and resolves via _platform_by_chat + the per-platform negotiated descriptor. Graduated TypeError fallback keeps the single-platform hook signatures (Telegram, base class) and legacy test doubles working unchanged. Both regression tests verified RED against the unfixed consumer, GREEN with the fix; single-platform relays are unaffected (NousResearch#97957's own 30 tests unchanged-green). * fix(bot-mode): preserve UTF-8 local DM delivery on Windows * fix(hosted-rooms): close remaining lifecycle races * fix(bot-mode): use subprocess env factory for peer delivery * test: accept asynchronous stop settlement * test: replace fixed waits with lifecycle conditions * fix(groups): close hosted room review races * fix: close hosted room authority review gaps * fix: close hosted room retry review gaps * fix(groups): refresh demotion stop fence on retry * fix(groups): defer deleted profiles before admission * fix(tui): recover long hosted room ids for prompt fence * test(groups): assert fresh stop per demotion attempt * fix(groups): fence profiles deleted after admission * fix(groups): close hosted room review races * fix(groups): account authority loss storage * fix(groups): keep cross-process stops pending * fix(telegram): quarantine abandoned polling shutdown * test(groups): align demotion fixture with process owner * fix(hosted-rooms): harden stop ownership and terminal recovery * fix(groups): close hosted-room concurrency invariants * fix(groups): close terminal recovery races * fix(groups): close remaining hosted room review gaps * fix(groups): preserve concurrent disband and legacy approvals --------- Co-authored-by: Ben Barclay <ben@nousresearch.com> Co-authored-by: Kailigithub <12250313+Kailigithub@users.noreply.github.com> Co-authored-by: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
… hops stop dropping it (NousResearch#99176) The PKCE payload is a flat 'provider=...;state=...;verifier=...;next=...' 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 parsers decode it — the browser round trip is fine. But '"' and '\' are outside the plain cookie-octet set, and non-Python hops that re-serialize the Cookie header reject the value and drop the cookie entirely: Go's net/http (Traefik middleware, Authentik outposts, other gateways) refuses any cookie value containing a backslash. The OIDC callback then 400s with "Missing PKCE state cookie" even though the browser sent the cookie. Field reproduction: support thread "Still unable to use Authentik for signin with traefik" — devtools showed the browser sending the intact quoted \073 cookie on /auth/callback while Hermes logged missing_pkce_cookie behind a Traefik+Authentik chain. Fix: URL-encode the whole payload in set_pkce_cookie (quote(payload, safe='') — ';' becomes '%3B') so the wire value contains only cookie-octets and no parser in the chain has anything to reject, and decode through a single shared inverse, cookies.parse_pkce_payload(), in BOTH readers: the OAuth /auth/callback and the native password-login path (routes.login_submit), whose broker/provider binding check would otherwise parse zero segments from the newly-encoded value and silently disable itself. Regression coverage: the wire-shape test pins the full cookie-octet set (the '"'/'\' assertions are the ones a Go-parser hop fails pre-fix), the round-trip tests drive the real /auth/login → /auth/callback path, and the next= test pins the exact post-login redirect byte shape. Native-flow broker assertions updated to decode through parse_pkce_payload instead of substring-matching the raw wire value. Salvaged from NousResearch#84065 (rebased onto current main, which gained the SameSite=None PKCE attrs and the RFC 8252 native password flow since the PR branched): kept main's _pkce_attrs cookie shape, extended the fix to the login_submit reader the original PR predated, and reframed the rationale — browsers do NOT truncate at the first ';' (there is no literal ';' on the wire in the quoted form); the failing hop is a strict middlebox cookie parser. Closes NousResearch#83832 Co-authored-by: Kailigithub <12250313+Kailigithub@users.noreply.github.com>
… hops stop dropping it (NousResearch#99176) The PKCE payload is a flat 'provider=...;state=...;verifier=...;next=...' 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 parsers decode it — the browser round trip is fine. But '"' and '\' are outside the plain cookie-octet set, and non-Python hops that re-serialize the Cookie header reject the value and drop the cookie entirely: Go's net/http (Traefik middleware, Authentik outposts, other gateways) refuses any cookie value containing a backslash. The OIDC callback then 400s with "Missing PKCE state cookie" even though the browser sent the cookie. Field reproduction: support thread "Still unable to use Authentik for signin with traefik" — devtools showed the browser sending the intact quoted \073 cookie on /auth/callback while Hermes logged missing_pkce_cookie behind a Traefik+Authentik chain. Fix: URL-encode the whole payload in set_pkce_cookie (quote(payload, safe='') — ';' becomes '%3B') so the wire value contains only cookie-octets and no parser in the chain has anything to reject, and decode through a single shared inverse, cookies.parse_pkce_payload(), in BOTH readers: the OAuth /auth/callback and the native password-login path (routes.login_submit), whose broker/provider binding check would otherwise parse zero segments from the newly-encoded value and silently disable itself. Regression coverage: the wire-shape test pins the full cookie-octet set (the '"'/'\' assertions are the ones a Go-parser hop fails pre-fix), the round-trip tests drive the real /auth/login → /auth/callback path, and the next= test pins the exact post-login redirect byte shape. Native-flow broker assertions updated to decode through parse_pkce_payload instead of substring-matching the raw wire value. Salvaged from NousResearch#84065 (rebased onto current main, which gained the SameSite=None PKCE attrs and the RFC 8252 native password flow since the PR branched): kept main's _pkce_attrs cookie shape, extended the fix to the login_submit reader the original PR predated, and reframed the rationale — browsers do NOT truncate at the first ';' (there is no literal ';' on the wire in the quoted form); the failing hop is a strict middlebox cookie parser. Closes NousResearch#83832 Co-authored-by: Kailigithub <12250313+Kailigithub@users.noreply.github.com>
…JSON) codec (NousResearch#99210) 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 — NousResearch#83832 field case), and NousResearch#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 -> NousResearch#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 NousResearch#99176, NousResearch#84065.
Problem
The PKCE cookie value is a flat
provider=...;state=...;verifier=...;next=...string. RFC 6265 treats the unquoted;as a cookie-value terminator, so the browser stores only the first segment (provider=...) and the OIDC callback sees a partial payload — failing with "Missing PKCE state cookie" or "Unknown provider in cookie".The previous behavior wrapped the value in double quotes and used the Python http.cookies backslash-octal escape
\073to encode the embedded;. That works for Python's parser but breaks for browsers that follow RFC 6265 strictly (per section 4.1.1, the unescape rule consumes a backslash only when followed by a terminator —\0is not one, so the backslash itself is preserved in the value). The portable fix is to URL-encode the whole payload before it hits the wire.Fix
cookies.set_pkce_cookie: wrap the payload inurllib.parse.quote(safe="")—;becomes%3B,=becomes%3D, and there is nothing left in the cookie value for the browser's cookie-value parser to misinterpret.routes.callback: URL-decode viaurllib.parse.unquotebefore the;split, so the parsed segments match the original payload byte-for-byte. The decode is a no-op for any pre-existing cookie value that pre-dates this change (no%means nothing to decode).Regression coverage
Three new tests in
test_dashboard_auth_cookies.py:test_set_pkce_cookie_url_encodes_payload_to_avoid_rfc6265_split— pins the wire-level shape: no unquoted;in the Set-Cookie value, and the URL-encoding round-trips back to the original payload.test_pkce_cookie_round_trip_preserves_all_segments— drives the existing end-to-end callback path through the prefix test app and verifies that the callback returns 302 (success), not 400 "Missing PKCE state cookie".test_pkce_callback_works_when_next_query_includes_encoded_path— exercises thenext=round-trip path with a real relative URL (/sessions?view=recent&project=foo) to make sure the post-login redirect carries the target.Proof gate
test_set_pkce_cookie_url_encodes_payload_to_avoid_rfc6265_splitwas verified to fail against the pre-fix source (the Set-Cookie value comes back as"provider=stub\073state=s\073verifier=v"— the broken RFC 6265 quoted-string form) and pass post-fix. The other two tests verify the full callback path is intact.Risk / blast radius
Low. The setter change is a pure transformation of the cookie value (URL-encoding); the reader change is a pure inverse transformation. No new dependencies, no API change, no new helper functions. The pre-existing 4 PKCE-related tests in
test_dashboard_auth_cookies.pyand the 13 prefix-cookie tests intest_dashboard_auth_prefix.pycontinue to pass (20 / 20 in the suite).Closes
Closes #83832