Copilot commented on code in PR #13081:
URL: https://github.com/apache/apisix/pull/13081#discussion_r2921893916
##########
apisix/plugins/openid-connect.lua:
##########
@@ -640,16 +640,23 @@ function _M.rewrite(plugin_conf, ctx)
-- '.apisix/redirect' suffix if not configured.
local suffix = "/.apisix/redirect"
local uri = ctx.var.uri
+ local redirect_path
if core.string.has_suffix(uri, suffix) then
-- This is the redirection response from the OIDC provider.
- conf.redirect_uri = uri
+ redirect_path = uri
else
if string.sub(uri, -1, -1) == "/" then
- conf.redirect_uri = string.sub(uri, 1, -2) .. suffix
+ redirect_path = string.sub(uri, 1, -2) .. suffix
else
- conf.redirect_uri = uri .. suffix
+ redirect_path = uri .. suffix
end
end
+ -- Build full redirect_uri using the original Host header (http_host)
+ -- to preserve the client-facing port (e.g., for Docker mappings) and
+ -- prevent lua-resty-openidc from dropping non-standard ports.
+ local scheme = ctx.var.scheme
+ local host = ctx.var.http_host or ctx.var.host
Review Comment:
Host selection uses `ctx.var.http_host` first, which may not represent the
client-facing host when APISIX is behind a (trusted) proxy that sets
`X-Forwarded-Host` but forwards a different `Host`. Previously
lua-resty-openidc would prefer `X-Forwarded-Host`; switching to `http_host` can
change redirect_uris in those deployments. Consider preferring
`X-Forwarded-Host` when it’s present and not just the sanitized `ctx.var.host`
value (to keep port preservation for direct/non-proxy cases), and fall back to
`http_host`/`host` otherwise.
```suggestion
-- Build full redirect_uri using the client-facing host.
-- Prefer X-Forwarded-Host when present (for trusted proxy setups),
-- otherwise fall back to the original Host header (http_host) or
-- the sanitized host value to preserve non-standard ports.
local scheme = ctx.var.scheme
local host = ctx.var.http_x_forwarded_host
if not host or host == "" then
host = ctx.var.http_host or ctx.var.host
end
```
##########
apisix/plugins/openid-connect.lua:
##########
@@ -640,16 +640,23 @@ function _M.rewrite(plugin_conf, ctx)
-- '.apisix/redirect' suffix if not configured.
local suffix = "/.apisix/redirect"
local uri = ctx.var.uri
+ local redirect_path
if core.string.has_suffix(uri, suffix) then
-- This is the redirection response from the OIDC provider.
- conf.redirect_uri = uri
+ redirect_path = uri
else
if string.sub(uri, -1, -1) == "/" then
- conf.redirect_uri = string.sub(uri, 1, -2) .. suffix
+ redirect_path = string.sub(uri, 1, -2) .. suffix
else
- conf.redirect_uri = uri .. suffix
+ redirect_path = uri .. suffix
end
end
+ -- Build full redirect_uri using the original Host header (http_host)
+ -- to preserve the client-facing port (e.g., for Docker mappings) and
+ -- prevent lua-resty-openidc from dropping non-standard ports.
+ local scheme = ctx.var.scheme
Review Comment:
`redirect_uri` is now built with `ctx.var.scheme`, which reflects the
downstream connection scheme to APISIX and does not account for trusted
`X-Forwarded-Proto` (e.g., TLS terminated at a trusted proxy). This can
generate `http://...` redirect_uris when the client-facing scheme is `https`,
breaking OIDC callbacks. Consider deriving the scheme from `X-Forwarded-Proto`
(which `handle_x_forwarded_headers` already sanitizes for untrusted sources),
falling back to `ctx.var.scheme` if absent, and trimming to the first value
when multiple are present.
```suggestion
local scheme = ctx.var.http_x_forwarded_proto
if scheme then
-- When multiple values are present (e.g., "https,http"), use
the first one.
local comma = string.find(scheme, ",", 1, true)
if comma then
scheme = string.sub(scheme, 1, comma - 1)
end
if core.string and core.string.trim then
scheme = core.string.trim(scheme)
end
else
scheme = ctx.var.scheme
end
```
##########
t/plugin/openid-connect3.t:
##########
@@ -112,3 +112,154 @@ true
--- error_code: 302
--- error_log
use http proxy
+
+
+
+=== TEST 3: Set up route with plugin matching URI `/hello` with redirect_uri
use default value.
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "openid-connect": {
+ "client_id":
"kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
+ "client_secret":
"60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa",
+ "discovery":
"http://127.0.0.1:1980/.well-known/openid-configuration";,
+ "ssl_verify": false,
+ "timeout": 10,
+ "scope": "apisix",
+ "unauth_action": "auth",
+ "use_pkce": false,
+ "session": {
+ "secret":
"jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK"
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: The value of redirect_uri should be appended to `.apisix/redirect`
in the original request.
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local httpc = http.new()
+ local uri = "http://127.0.0.1:"; .. ngx.var.server_port .. "/hello"
+ local res, err = httpc:request_uri(uri, {method = "GET"})
+ ngx.status = res.status
+ local location = res.headers['Location']
+ if not location then
+ ngx.say("no Location header")
+ return
+ end
+ -- extract and decode the redirect_uri from the Location header
+ local encoded_redirect = string.match(location,
"redirect_uri=([^&]+)")
+ local redirect_uri = ngx.unescape_uri(encoded_redirect)
+ local expected = uri .. "/.apisix/redirect"
+ if string.find(location, 'https://samples.auth0.com/authorize', 1,
true) and
+ string.find(location, 'scope=apisix', 1, true) and
+ string.find(location,
'client_id=kbyuFDidLLm280LIwVFiazOqjO3ty8KH', 1, true) and
+ string.find(location, 'response_type=code', 1, true) and
+ redirect_uri == expected then
+ ngx.say(true)
+ else
+ ngx.say("redirect_uri mismatch, expected: " .. expected .. ",
got: " .. redirect_uri)
+ end
+ }
+ }
+--- timeout: 10s
+--- response_body
+true
+--- error_code: 302
+
+
+
+=== TEST 5: auto-generated redirect_uri preserves non-default port from Host
header
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local httpc = http.new()
+ local uri = "http://127.0.0.1:"; .. ngx.var.server_port .. "/hello"
+ local res, err = httpc:request_uri(uri, {
+ method = "GET",
+ headers = {
+ ["Host"] = "localhost:8888"
+ }
+ })
+ ngx.status = res.status
+ local location = res.headers['Location']
+ if not location then
+ ngx.say("no Location header")
+ return
+ end
+ local encoded_redirect = string.match(location,
"redirect_uri=([^&]+)")
+ local redirect_uri = ngx.unescape_uri(encoded_redirect)
+ local expected = "http://localhost:8888/hello/.apisix/redirect";
+ if redirect_uri == expected then
+ ngx.say(true)
+ else
+ ngx.say("redirect_uri mismatch, expected: " .. expected .. ",
got: " .. redirect_uri)
+ end
+ }
+ }
+--- timeout: 10s
+--- response_body
+true
+--- error_code: 302
+
+
+
+=== TEST 6: auto-generated redirect_uri omits port for default HTTP port
Review Comment:
TEST 6 description says it covers the “default HTTP port”, but the test
doesn’t ensure the request is actually on port 80; it only sets `Host:
localhost` (no port) while still sending the request to
`127.0.0.1:$server_port`. This is misleading and makes the intent unclear;
consider renaming the test to reflect “Host header without port” (or otherwise
constraining the port if you really want to test default-port behavior).
```suggestion
=== TEST 6: auto-generated redirect_uri omits port when Host header has no
port
```
--
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]
