Copilot commented on code in PR #13671:
URL: https://github.com/apache/apisix/pull/13671#discussion_r3542088592
##########
apisix/plugins/openid-connect.lua:
##########
@@ -48,6 +63,9 @@ local function build_session_opts(session_conf)
"use session.absolute_timeout instead")
end
end
+ if default_cookie_name and not session_conf.cookie_name then
+ session_conf.cookie_name = default_cookie_name
+ end
Review Comment:
`build_session_opts` mutates `session_conf` in-place when defaulting
`cookie_name`. When `openid-connect` is supplied via `plugin_config_id`,
`route_conf.value.plugins` reuses the same plugin config tables (see
`apisix/plugin_config.lua:84-90`), so the first request to hit any route will
permanently set `session.cookie_name` for *all* routes sharing that
plugin_config. That defeats the per-route cookie isolation and can reintroduce
state collisions across sibling routes. Return a cloned opts table instead of
mutating the shared config.
##########
apisix/plugins/openid-connect.lua:
##########
@@ -33,11 +33,26 @@ local ngx_encode_base64 = ngx.encode_base64
local plugin_name = "openid-connect"
--- Session config is passed as-is to resty.session.start(); the only
--- translation is the legacy session.cookie.lifetime alias from the
--- lua-resty-session 3.x schema, which is mapped to absolute_timeout
--- when the latter is unset.
-local function build_session_opts(session_conf)
+-- Derive a stable, per-route session cookie name. resty.session names every
+-- cookie "session" by default, so sibling openid-connect routes on the same
+-- host that share a session.secret overwrite each other's pre-login state.
+-- Keying the cookie name on the route id isolates them.
+local function route_session_cookie_name(conf, ctx)
+ 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)
Review Comment:
`ctx.route_id` can be an integer (route id schema allows `type=integer`),
but `ngx.md5()` expects a string. Passing a numeric route id here can raise a
runtime error and break all OIDC requests for that route. Coerce the seed to
string before hashing (and prefer an explicit `== nil` check).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
