fix: keep orphaned OIDC PKCE cookies off every request - #9644
Open
zhaohuabing wants to merge 2 commits into
Open
fix: keep orphaned OIDC PKCE cookies off every request#9644zhaohuabing wants to merge 2 commits into
zhaohuabing wants to merge 2 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 880f352dc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
zhaohuabing
force-pushed
the
fix-oidc-orphaned-pkce-cookies
branch
from
August 3, 2026 04:39
880f352 to
d7af18e
Compare
zhaohuabing
marked this pull request as draft
August 3, 2026 04:41
zhaohuabing
marked this pull request as ready for review
August 3, 2026 06:51
zhaohuabing
changed the title
zhaohuabing
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7af18e285
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
zhaohuabing
force-pushed
the
fix-oidc-orphaned-pkce-cookies
branch
from
August 3, 2026 07:06
d7af18e to
c1a0c0a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9644 +/- ##
==========================================
+ Coverage 76.07% 76.09% +0.01%
==========================================
Files 260 260
Lines 43446 43452 +6
==========================================
+ Hits 33051 33063 +12
+ Misses 8191 8187 -4
+ Partials 2204 2202 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zhaohuabing
force-pushed
the
fix-oidc-orphaned-pkce-cookies
branch
from
August 3, 2026 07:33
e794274 to
11f82ee
Compare
Member
Author
|
/retest |
Envoy mints a nonce (CSRF) and a PKCE code verifier cookie for every authorization flow it starts, keyed by a random per-flow id so that several logins can be in flight at once. On a successful callback it deletes only the pair belonging to the flow that completed, which is deliberate - sweeping every flow would strip the cookies of the other tabs still mid-login. A flow that is started and abandoned therefore leaves its pair behind until it expires, and at the default path "/" those orphans are sent on every request. Combined with a provider that issues large id and access tokens they overflow the inbound request header limit, at which point the callback itself fails and the browser loops back through the flow, minting yet another pair. Envoy only needs the value of these two cookies when it validates the callback, so scope them to the OIDC redirect path. Orphans then stay off ordinary application requests instead of counting against the header limit on each one. This bounds the problem rather than eliminating it - they are still sent to the callback endpoint, and they still expire on their own within csrfTokenTTL. Also name the code verifier cookie CodeVerifier-<suffix>. It was the only one of the seven OAuth2 cookies left at Envoy's default name, so SecurityPolicies sharing a cookie domain all wrote one shared cookie, and logging out of one policy deleted the in-flight flow cookies of the others. The redirect path is only applied when it satisfies the pattern Envoy enforces on a cookie path, which is stricter than a URL path - "," and ";" for example are legal RFC 3986 sub-delims but are rejected. An unrepresentable path falls back to leaving the cookie path unset, which Envoy defaults to "/", rather than failing xDS validation and dropping the route. Fixes envoyproxy#9632 Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
The e2e cookie tracker replayed every stored cookie on every request, keyed by name alone. That made it blind to the cookie path, so the scoping this change relies on could regress without any test noticing, and cookies with the same name at two different paths overwrote each other. Track cookies by name and path, honour the RFC 6265 path-match rules when deciding what to send, and drop cookies the server expires. It is deliberately still not a net/http/cookiejar: the OAuth2 filter marks every cookie "secure" and these tests run over plain HTTP, so a spec-compliant jar would store the cookies and then never send them back. With that in place, assert that an authorization flow which has been started but not completed - the state an abandoned flow leaves behind - keeps its nonce and code verifier cookies off ordinary application requests. Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
zhaohuabing
force-pushed
the
fix-oidc-orphaned-pkce-cookies
branch
from
August 4, 2026 01:08
d2f6b3f to
81b460b
Compare
Member
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9632
Problem
Envoy mints a nonce (CSRF) and a PKCE code verifier cookie for every authorization flow it starts, keyed by a random per-flow id so several logins can be in flight at once. On a successful callback it deletes only the pair belonging to the flow that completed — deliberately so, since sweeping every flow would strip the cookies of the other tabs still mid-login.
A flow that is started and abandoned therefore leaves its pair behind until it expires, and at the default path
/those orphans are sent on every request. Combined with a provider that issues large id/access tokens they overflow the inbound request header limit, at which point the callback itself fails and the browser loops back through the flow, minting yet another pair.Changes
Scope the nonce and code verifier cookies to the OIDC redirect path. Envoy only needs their value when validating the callback, so orphans stop counting against the header limit on every ordinary request. The path is applied only when it satisfies the pattern Envoy enforces on a cookie path — stricter than a URL path, e.g.
,and;are legal RFC 3986 sub-delims but are rejected. An unrepresentable path falls back to leaving the cookie path unset, which Envoy defaults to/, rather than failing xDS validation and dropping the route.Name the code verifier cookie
CodeVerifier-<suffix>. It was the only one of the seven OAuth2 cookies left at Envoy's default name, so SecurityPolicies sharing a cookie domain all wrote one shared cookie, and logging out of one policy deleted the in-flight flow cookies of the others.Notes
This bounds the problem rather than eliminating it: orphans are still sent to the callback endpoint until they expire on their own (
csrfTokenTTL, 10 minutes by default), and logout can no longer purge them early since the browser no longer sends them to the signout path. Sweeping all flow cookies on a successful callback is not a viable alternative — it would strip the nonce and verifier of every other in-flight login, which is exactly what the per-flow suffix in envoyproxy/envoy#42079 was added to prevent.Behavior change on upgrade: a browser already holding flow cookies keeps them at the old
path=/until they expire on their original lifetime. The old code verifier cookie name is also no longer read, so a login in progress across the rollout may need to be retried once.No API change, so this is cherry-pickable to release branches. A follow-up PR adds
codeVerifierTTLto bound the code verifier cookie's lifetime the waycsrfTokenTTLalready bounds the nonce.