fix(openid-connect): scope session cookie per route to avoid state collision#13671
Closed
shreemaan-abhishek wants to merge 2 commits into
Closed
Conversation
…llision Two openid-connect routes on the same host that share a session.secret both used resty.session's default cookie name "session", so an unauthenticated visit to one route overwrote the pre-login state (state, nonce) stored for the other. Completing the first route's callback then failed with "state ... does not match state restored from session" (HTTP 500). Default an unset session.cookie_name to a per-route name derived from the route id (falling back to client_id), so sibling routes no longer share one cookie. An explicit session.cookie_name is still respected.
dosubot
Bot
added
the
size:L
There was a problem hiding this comment.
Pull request overview
This PR addresses OpenID Connect Authorization Code flow failures caused by session cookie name collisions when multiple openid-connect routes on the same host share the same session.secret. It introduces a derived, stable per-route default session.cookie_name (while still honoring operator-configured session.cookie_name) and adds test coverage plus documentation updates.
Changes:
- Derive a per-route default session cookie name and apply it when
session.cookie_nameis unset. - Add unit-style tests for cookie name derivation / session option building, plus an end-to-end-style isolation test.
- Document the new defaulting behavior and the reason for explicitly setting
session.cookie_namewhen sharing sessions is desired.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
apisix/plugins/openid-connect.lua |
Derives a per-route session cookie name and applies it during openidc.authenticate() session initialization. |
t/plugin/openid-connect10.t |
Adds tests validating cookie name derivation and session.cookie_name defaulting/override behavior. |
t/plugin/openid-connect11.t |
Adds an integration-style test ensuring sibling routes produce distinct session cookies when sharing session.secret. |
docs/en/latest/plugins/openid-connect.md |
Documents the new derived default for session.cookie_name and how to explicitly share a session cookie across routes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+41
to
+45
| local seed = ctx and ctx.route_id or conf.client_id | ||
| if not seed then | ||
| return nil | ||
| end | ||
| return "session_" .. string.sub(ngx.md5(seed), 1, 16) |
Comment on lines
+66
to
+68
| if default_cookie_name and not session_conf.cookie_name then | ||
| session_conf.cookie_name = default_cookie_name | ||
| end |
Cross-route session sharing now requires a shared session.cookie_name since the default cookie name is per-route. Set an explicit shared cookie_name on both routes in the redis session-sharing test so it keeps exercising the shared-session refresh flow.
nic-6443
reviewed
Jul 10, 2026
nic-6443
left a comment
nic-6443
left a comment
Member
There was a problem hiding this comment.
If users want to distinguish login status across multiple routes, they can manually configure different Cookie names. Configuring the same Cookie name among multiple routes is also a reasonable scenario in itself.
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
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.
Description
When two
openid-connectroutes on the same host share asession.secret, they both fall back to lua-resty-session's default cookie name"session". An unauthenticated visit to route A stores the pre-loginstate/noncein that cookie; a subsequent unauthenticated visit to route B (carrying the same cookie) overwrites it. When route A's callback then arrives, the state no longer matches andlua-resty-openidcreturns:surfacing as an HTTP 500 to the user. This is purely a cookie-naming collision, not an auth bypass.
This PR defaults an unset
session.cookie_nameto a stable per-route name derived from the route id (falling back toclient_id), so sibling routes each get their own session cookie and no longer clobber each other's pre-login state.Behavior change / opt-in sharing: cross-route session sharing (e.g. SSO across routes on the same host with redis storage) previously worked implicitly via the shared default cookie name. With per-route defaults, sharing is now explicit: set the same
session.cookie_nameon each route that should share a session. An operator-setsession.cookie_nameis always honored. The redis session-sharing test is updated to set a sharedcookie_nameaccordingly.On upgrade, in-flight sessions using the old default
"session"cookie are not recognized and users re-authenticate once. Config API and behavior contracts are otherwise unchanged.Which issue(s) this PR fixes:
Checklist