This is an automated email from the ASF dual-hosted git repository.
nic-6443 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/apisix.git
The following commit(s) were added to refs/heads/master by this push:
new 5dd7b9faaf fix(openid-connect): handle temporarily_unavailable error
redirects from the ID provider (#13825)
5dd7b9faaf is described below
commit 5dd7b9faaf8cf8ea486337e9deabb2e1a6df32c4
Author: Mohammad Izzraff Janius
<[email protected]>
AuthorDate: Fri Aug 14 18:59:26 2026 +0900
fix(openid-connect): handle temporarily_unavailable error redirects from
the ID provider (#13825)
---
apisix/plugins/openid-connect.lua | 108 ++++++++--
t/plugin/openid-connect12.t | 439 ++++++++++++++++++++++++++++++++++++++
2 files changed, 529 insertions(+), 18 deletions(-)
diff --git a/apisix/plugins/openid-connect.lua
b/apisix/plugins/openid-connect.lua
index 054cc6f1b1..bc1842a13d 100644
--- a/apisix/plugins/openid-connect.lua
+++ b/apisix/plugins/openid-connect.lua
@@ -41,6 +41,15 @@ local plugin_name = "openid-connect"
local STATE_MISMATCH_ERR =
"state from argument does not match state restored from session"
+-- prefix of the resty.openidc error returned when the redirect_uri is
+-- requested without an authorization response, e.g. an OAuth2 error
+-- redirect (RFC 6749 section 4.1.2.1) instead of a code
+local UNHANDLED_REDIRECT_URI_ERR = "unhandled request to the redirect_uri"
+
+-- max consecutive restarts of the authentication flow from failed
+-- authorization callbacks
+local MAX_AUTH_FLOW_RESTARTS = 3
+
-- Session config is passed as-is to resty.session.start(); the only
-- translation is the legacy session.cookie.lifetime alias from the
@@ -1294,34 +1303,88 @@ function _M.rewrite(plugin_conf, ctx)
build_session_opts(conf.session))
if err then
- if session then
- session:close()
- end
if err == "unauthorized request" then
+ if session then
+ session:close()
+ end
if conf.unauth_action == "pass" then
return nil
end
return 401
end
- -- Stale authorization callback: the session holds no authorization
- -- state for the state in the callback, e.g. an already completed
- -- callback was replayed, or the state was pruned after too many
- -- concurrent flows. (Concurrent logins in several tabs are handled
- -- by resty.openidc itself since 1.9.0, which keeps one
- -- authorization state per in-flight flow.) The client is a browser
- -- mid-navigation, so instead of a dead-end 500, send it back to
the
- -- original URL that resty.openidc returns alongside the error: a
- -- fresh flow starts from there and completes without any user
- -- interaction while the ID provider still holds an SSO session.
- if err == STATE_MISMATCH_ERR and target_url
+ -- Recoverable authorization-callback failures: a stale state
+ -- (replayed or pruned callback), or the ID provider redirecting
+ -- back with error=temporarily_unavailable, e.g. Keycloak after
+ -- its login session expired. The client is a browser
+ -- mid-navigation, so restart the authentication flow by sending
+ -- it back to the original URL instead of dead-ending with a 500.
+ -- Other OAuth2 error codes (access_denied, login_required, ...)
+ -- reflect a deliberate outcome and are not retried.
+ local restart_reason
+ local restart_url = target_url
+ if err == STATE_MISMATCH_ERR then
+ -- state already matched by resty.openidc; no in-flight flow
+ -- for it, so its session-level original_url is all we have
+ restart_reason = "state mismatch (replayed or pruned callback)"
+ elseif session and core.string.has_prefix(err,
UNHANDLED_REDIRECT_URI_ERR) then
+ local uri_args = ngx.req.get_uri_args()
+ -- resty.openidc bails on this path before validating state, so
+ -- match it here as it would: rejects a forged callback, and
+ -- recovers the original_url of the flow it belongs to (each
+ -- in-flight flow keeps its own since 1.9.0)
+ local authorization_state
+ if uri_args.error == "temporarily_unavailable" and
uri_args.state then
+ local states = session:get("authorization_states")
+ authorization_state = states and states[uri_args.state]
+ if not authorization_state
+ and uri_args.state == session:get("state") then
+ authorization_state = {
+ original_url = session:get("original_url")
+ }
+ end
+ end
+ if authorization_state then
+ restart_reason = "authorization callback reported a " ..
+ "temporarily unavailable identity provider" ..
+ (type(uri_args.error_description) == "string" and
+ (" (" .. uri_args.error_description .. ")") or "")
+ restart_url = authorization_state.original_url or
target_url
+ end
+ end
+
+ if restart_reason and restart_url and session
and ngx.req.get_method() == "GET" then
- core.log.warn("OIDC state mismatch (replayed or pruned ",
- "callback), restarting the authentication flow")
- core.response.set_header("Location", target_url)
- return 302
+ -- bound the redirect loop in case the failure is not
+ -- transient; the counter is reset once a request
+ -- authenticates
+ local restarts = session:get("auth_flow_restarts") or 0
+ if restarts < MAX_AUTH_FLOW_RESTARTS then
+ session:set("auth_flow_restarts", restarts + 1)
+ local ok, save_err = session:save()
+ if not ok then
+ session:close()
+ core.log.error("OIDC authentication failed: ", err,
+ " (could not persist the restart ",
+ "counter: ", save_err, ")")
+ return 500
+ end
+ session:close()
+ core.log.warn("OIDC ", restart_reason,
+ ", restarting the authentication flow")
+ core.response.set_header("Location", restart_url)
+ return 302
+ end
+ session:close()
+ core.log.error("OIDC authentication failed: ", err,
+ " (giving up after ", restarts,
+ " restarts of the authentication flow)")
+ return 500
end
+ if session then
+ session:close()
+ end
core.log.error("OIDC authentication failed: ", err)
return 500
end
@@ -1365,6 +1428,15 @@ function _M.rewrite(plugin_conf, ctx)
if enc_id_token and conf.set_raw_id_token_header then
core.request.set_header(ctx, "X-Raw-ID-Token", enc_id_token)
end
+
+ -- a successful authentication resets the restart budget
+ if session:get("auth_flow_restarts") then
+ session:set("auth_flow_restarts", nil)
+ local ok, save_err = session:save()
+ if not ok then
+ core.log.error("failed to save session: ", save_err)
+ end
+ end
end
end
if session then
diff --git a/t/plugin/openid-connect12.t b/t/plugin/openid-connect12.t
new file mode 100644
index 0000000000..2293cb48b0
--- /dev/null
+++ b/t/plugin/openid-connect12.t
@@ -0,0 +1,439 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+use t::APISIX 'no_plan';
+
+log_level('debug');
+repeat_each(1);
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!defined $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ # every block here drives resty.openidc into an error path on purpose,
+ # which logs at [error]; assert on the specific message instead
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "no such assertion");
+ }
+});
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: create a route protected by openid-connect
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/oidc12',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/oidc12/*",
+ "plugins": {
+ "openid-connect": {
+ "client_id": "apisix",
+ "client_secret": "secret",
+ "discovery":
"http://127.0.0.1:8080/realms/basic/.well-known/openid-configuration";,
+ "redirect_uri":
"http://127.0.0.1:1984/oidc12/callback";,
+ "ssl_verify": false,
+ "session": {
+ "secret": "6S8IO+A+6KJsdazbjNyG7g=="
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 2: a callback reporting a temporarily unavailable IDP restarts the
flow
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+
+ local function cookie_of(res)
+ local c = res.headers["Set-Cookie"]
+ if type(c) == "table" then
+ c = table.concat(c, "; ")
+ end
+ return c and c:match("^([^;]+)")
+ end
+
+ -- start a login flow and keep the session cookie
+ local res_a = http.new():request_uri(base .. "/oidc12/page")
+ local state = res_a.headers["Location"]:match("state=([^&]+)")
+ local jar = cookie_of(res_a)
+
+ -- the ID provider redirects back with an error instead of a code,
+ -- e.g. the user's login session expired at the IDP
+ local res_b = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+ "&error_description=authentication_expired&state=" ..
state, {
+ headers = {Cookie = jar}
+ })
+ ngx.say(res_b.status, " ", tostring(res_b.headers["Location"]))
+ }
+ }
+--- response_body
+302 /oidc12/page
+--- error_log
+restarting the authentication flow
+
+
+
+=== TEST 3: a callback with a different IDP error still fails with 500
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+
+ local function cookie_of(res)
+ local c = res.headers["Set-Cookie"]
+ if type(c) == "table" then
+ c = table.concat(c, "; ")
+ end
+ return c and c:match("^([^;]+)")
+ end
+
+ local res_a = http.new():request_uri(base .. "/oidc12/page")
+ local state = res_a.headers["Location"]:match("state=([^&]+)")
+ local jar = cookie_of(res_a)
+
+ -- access_denied reflects a deliberate outcome, not a transient
+ -- failure, so it must not be silently retried
+ local res_b = http.new():request_uri(
+ base .. "/oidc12/callback?error=access_denied" ..
+ "&error_description=user+denied+access&state=" .. state, {
+ headers = {Cookie = jar}
+ })
+ ngx.say(res_b.status)
+ }
+ }
+--- response_body
+500
+
+
+
+=== TEST 4: a non-GET callback reporting a temporarily unavailable IDP still
fails with 500
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+
+ local function cookie_of(res)
+ local c = res.headers["Set-Cookie"]
+ if type(c) == "table" then
+ c = table.concat(c, "; ")
+ end
+ return c and c:match("^([^;]+)")
+ end
+
+ local res_a = http.new():request_uri(base .. "/oidc12/page")
+ local state = res_a.headers["Location"]:match("state=([^&]+)")
+ local jar = cookie_of(res_a)
+
+ local res_b = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+ "&error_description=authentication_expired&state=" ..
state, {
+ method = "POST",
+ body = "",
+ headers = {Cookie = jar}
+ })
+ ngx.say(res_b.status)
+ }
+ }
+--- response_body
+500
+
+
+
+=== TEST 5: a callback reporting a temporarily unavailable IDP without a
session cookie still fails with 500
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+ local res = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+ "&error_description=authentication_expired&state=deadbeef")
+ ngx.say(res.status)
+ }
+ }
+--- response_body
+500
+
+
+
+=== TEST 6: a callback error that keeps recurring stops being retried after 3
restarts
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+
+ local function cookie_of(res)
+ local c = res.headers["Set-Cookie"]
+ if type(c) == "table" then
+ c = table.concat(c, "; ")
+ end
+ return c and c:match("^([^;]+)")
+ end
+
+ local res_a = http.new():request_uri(base .. "/oidc12/page")
+ local state = res_a.headers["Location"]:match("state=([^&]+)")
+ local jar = cookie_of(res_a)
+
+ -- the browser keeps following the restart redirect into the same
+ -- failure; each response carries the session cookie updated with
+ -- the restart count
+ local statuses = {}
+ for i = 1, 5 do
+ local res = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+ "&error_description=authentication_expired&state=" ..
state, {
+ headers = {Cookie = jar}
+ })
+ statuses[i] = res.status
+ jar = cookie_of(res) or jar
+ end
+ ngx.say(table.concat(statuses, " "))
+ }
+ }
+--- response_body
+302 302 302 500 500
+
+
+
+=== TEST 7: a successful authentication resets the restart budget
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local concatenate_cookies =
require("lib.keycloak").concatenate_cookies
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+
+ local function cookie_of(res)
+ local c = res.headers["Set-Cookie"]
+ if type(c) == "table" then
+ c = table.concat(c, "; ")
+ end
+ return c and c:match("^([^;]+)")
+ end
+
+ -- exhaust the restart budget on failing callbacks
+ local res_a = http.new():request_uri(base .. "/oidc12/page")
+ local state = res_a.headers["Location"]:match("state=([^&]+)")
+ local jar = cookie_of(res_a)
+
+ local statuses = {}
+ for i = 1, 4 do
+ local res = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+ "&error_description=authentication_expired&state=" ..
state, {
+ headers = {Cookie = jar}
+ })
+ statuses[i] = res.status
+ jar = cookie_of(res) or jar
+ end
+
+ -- complete a login in the same browser session: fresh flow,
+ -- Keycloak login form, then the code callback
+ local res_b = http.new():request_uri(base .. "/oidc12/page", {
+ headers = {Cookie = jar}
+ })
+ jar = cookie_of(res_b) or jar
+
+ local httpc = http.new()
+ local res_c = httpc:request_uri(res_b.headers["Location"])
+ local action, params = res_c.body:match('.*action="(.*)%?(.*)"
method="post">')
+ params = params:gsub("&", "&")
+ local kc_cookies = concatenate_cookies(res_c.headers["Set-Cookie"])
+
+ local res_d = httpc:request_uri(action .. "?" .. params, {
+ method = "POST",
+ body = "username=jack&password=jack",
+ headers = {
+ ["Content-Type"] = "application/x-www-form-urlencoded",
+ Cookie = kc_cookies
+ }
+ })
+
+ local res_e = http.new():request_uri(res_d.headers["Location"], {
+ headers = {Cookie = jar}
+ })
+ statuses[5] = res_e.status
+ jar = cookie_of(res_e) or jar
+
+ -- the authenticated request resets the budget; it passes the
+ -- plugin and reaches the mock upstream, which has no
+ -- /oidc12/page route, hence its 404
+ local res_f = http.new():request_uri(base .. "/oidc12/page", {
+ headers = {Cookie = jar}
+ })
+ statuses[6] = res_f.status
+ jar = cookie_of(res_f) or jar
+
+ -- ... so a later transient callback error is retried again. The
+ -- first flow's state is still in-flight (only the completed flow's
+ -- state was consumed), so the callback passes the state check.
+ local res_g = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+ "&error_description=authentication_expired&state=" ..
state, {
+ headers = {Cookie = jar}
+ })
+ statuses[7] = res_g.status
+ ngx.say(table.concat(statuses, " "))
+ }
+ }
+--- response_body
+302 302 302 500 302 404 302
+
+
+
+=== TEST 8: a temporarily unavailable callback whose state was never issued is
not retried
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+
+ local function cookie_of(res)
+ local c = res.headers["Set-Cookie"]
+ if type(c) == "table" then
+ c = table.concat(c, "; ")
+ end
+ return c and c:match("^([^;]+)")
+ end
+
+ -- start a login flow so the session holds a valid in-flight state
+ local res_a = http.new():request_uri(base .. "/oidc12/page")
+ local state = res_a.headers["Location"]:match("state=([^&]+)")
+ local jar = cookie_of(res_a)
+
+ -- a forged callback (e.g. a cross-site request) carries a state
the
+ -- session never issued, or none at all: it must not restart the
+ -- flow, so it cannot spend the restart budget of the real flow
+ local statuses = {}
+ local res_b = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+
"&error_description=authentication_expired&state=deadbeef", {
+ headers = {Cookie = jar}
+ })
+ statuses[1] = res_b.status
+
+ local res_c = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+ "&error_description=authentication_expired", {
+ headers = {Cookie = jar}
+ })
+ statuses[2] = res_c.status
+
+ -- the real flow's state is still accepted and still has its full
+ -- budget, so it restarts
+ local res_d = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+ "&error_description=authentication_expired&state=" ..
state, {
+ headers = {Cookie = jar}
+ })
+ statuses[3] = res_d.status
+ ngx.say(table.concat(statuses, " "))
+ }
+ }
+--- response_body
+500 500 302
+
+
+
+=== TEST 9: a restart is not issued when the incremented counter cannot be
persisted
+--- config
+ location /t {
+ content_by_lua_block {
+ -- force session:save() to fail for any request carrying the
+ -- forcesavefail argument, so the callback below exercises the
+ -- persistence-failure path while the flow still starts normally
+ -- (rawset: the module and session tables are read-only)
+ local r_session = require("resty.session")
+ if not rawget(r_session, "_save_patched") then
+ local orig_start = r_session.start
+ rawset(r_session, "start", function(...)
+ local self, err = orig_start(...)
+ if self and ngx.var.arg_forcesavefail then
+ rawset(self, "save", function()
+ return nil, "forced save failure"
+ end)
+ end
+ return self, err
+ end)
+ rawset(r_session, "_save_patched", true)
+ end
+
+ local http = require "resty.http"
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+
+ local function cookie_of(res)
+ local c = res.headers["Set-Cookie"]
+ if type(c) == "table" then
+ c = table.concat(c, "; ")
+ end
+ return c and c:match("^([^;]+)")
+ end
+
+ -- start a login flow so the session holds a valid in-flight state
+ local res_a = http.new():request_uri(base .. "/oidc12/page")
+ local state = res_a.headers["Location"]:match("state=([^&]+)")
+ local jar = cookie_of(res_a)
+
+ -- the callback would restart, but the incremented counter cannot
be
+ -- persisted; without it the cap cannot bound the loop, so the
plugin
+ -- must fail closed instead of redirecting
+ local res_b = http.new():request_uri(
+ base .. "/oidc12/callback?error=temporarily_unavailable" ..
+
"&error_description=authentication_expired&forcesavefail=1" ..
+ "&state=" .. state, {
+ headers = {Cookie = jar}
+ })
+ ngx.say(res_b.status)
+ }
+ }
+--- response_body
+500
+--- error_log
+could not persist the restart counter
