nic-6443 commented on code in PR #13825:
URL: https://github.com/apache/apisix/pull/13825#discussion_r3781623604
##########
apisix/plugins/openid-connect.lua:
##########
@@ -1294,34 +1303,65 @@ 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
+ if err == STATE_MISMATCH_ERR then
+ restart_reason = "state mismatch (replayed or pruned callback)"
+ elseif 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
Review Comment:
This branch restarts the flow without ever validating `state`, which I think
is worth fixing before merge.
`resty.openidc` bails out at `if not args.code or not args.state` in
`openidc_authorization_response()`, so the state check right below it
(`openidc_authorization_state()`) never runs on this path. That means any third
party can trigger the 302 plus the session write with something like `<img
src="https://app/.apisix/redirect?error=temporarily_unavailable";>`, and three
of those burn the whole restart budget — the user's next genuine
`authentication_expired` callback then dead-ends at 500, since the budget only
resets on a successful authentication. mod_auth_openidc matches state *before*
it looks at the `error` parameter, for exactly this reason.
Keycloak does send `state` back on the error redirect
(`buildErrorRedirectUri()` adds it whenever the client sent one, and it is
visible in the log in #13776), and the tests here already pass it, so the check
is cheap — roughly what `openidc_authorization_state()` itself does:
```lua
local authorization_states = session:get("authorization_states") or {}
local authorization_state = authorization_states[uri_args.state]
or (uri_args.state and uri_args.state == session:get("state") and {
original_url = session:get("original_url")
})
```
That also fixes a second thing. Since 1.9.0 each in-flight flow keeps its
own `original_url` inside `authorization_states`, but this path falls back to
the session-level `session:get("original_url")`, i.e. whichever flow started
last. With two tabs mid-login, an error in tab A currently sends the browser to
tab B's target; looking the state up gives you the right URL for free.
The final request in TEST 7 succeeding with `state=deadbeef` is the tell
that nothing is checking it today.
--
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]
