fix(openid-connect): validate callback state before restarting the flow

resty.openidc reports an unhandled request to the redirect_uri before it
validates the callback state, so match the state against the session's
in-flight flows here. A callback whose state was never issued no longer
restarts the flow, and the restart redirects to the original URL of the
flow the state belongs to rather than the session-level fallback.

Author: janiussyafiq

apisix/plugins/openid-connect.lua

@@ -1322,19 +1322,38 @@ function _M.rewrite(plugin_conf, ctx)
             -- 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 core.string.has_prefix(err, UNHANDLED_REDIRECT_URI_ERR) then
+            elseif session and core.string.has_prefix(err, UNHANDLED_REDIRECT_URI_ERR) then
                 local uri_args = ngx.req.get_uri_args()
-                if uri_args.error == "temporarily_unavailable" then
+                -- 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 target_url and session
+            if restart_reason and restart_url and session
                and ngx.req.get_method() == "GET" then
                 -- bound the redirect loop in case the failure is not
                 -- transient; the counter is reset once a request
@@ -1349,7 +1368,7 @@ function _M.rewrite(plugin_conf, ctx)
                     session:close()
                     core.log.warn("OIDC ", restart_reason,
                                   ", restarting the authentication flow")
-                    core.response.set_header("Location", target_url)
+                    core.response.set_header("Location", restart_url)
                     return 302
                 end
                 session:close()

t/plugin/openid-connect12.t

@@ -311,10 +311,12 @@ restarting the authentication flow
             statuses[6] = res_f.status
             jar = cookie_of(res_f) or jar
 
-            -- ... so a later transient callback error is retried again
+            -- ... 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=deadbeef", {
+                    "&error_description=authentication_expired&state=" .. state, {
                     headers = {Cookie = jar}
                 })
             statuses[7] = res_g.status
@@ -323,3 +325,57 @@ restarting the authentication flow
     }
 --- 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