fix(openid-connect): redirect instead of 500 on a stale authorization callback#13712
Open
AlinsRan wants to merge 2 commits into
Open
fix(openid-connect): redirect instead of 500 on a stale authorization callback#13712AlinsRan wants to merge 2 commits into
AlinsRan wants to merge 2 commits into
Conversation
resty.openidc keeps the state of a single login flow in the session, so a second flow started in another tab overwrites it. When the first tab's callback then arrives, its state no longer matches and the plugin turned that into a 500 — a dead end the user can only escape by manually navigating back to a protected page. The library already returns the original URL alongside that error, which is exactly what a caller needs to recover. Use it: on a GET callback with a state mismatch, respond 302 to that URL so a fresh flow starts from there. While the ID provider still holds an SSO session the user sees no interruption at all. Callbacks with other methods, and callbacks carrying no session, keep returning 500. The error string is matched verbatim because the library builds it without interpolation; it is identical in lua-resty-openidc 1.8.0 (the pinned version) and 1.9.0. There is no open-redirect exposure: the URL comes from the session, written from $request_uri, so it is always a path on this server and never client-controlled. Fixes apache#12360
dosubot
Bot
added
size:L
The original URL comes from the user's own request and can carry query string credentials or PII. The mismatch itself is what matters for diagnosis, and the URL is already in the access log.
nic-6443
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #12360
resty.openidcstores the state of a single login flow in the session. Open a protected page in a second tab before the first login completes and the second flow overwrites that state; when the first tab's callback arrives, its state no longer matches and the plugin returns500.That 500 is a dead end — the user has to manually navigate back to a protected page to recover. Meanwhile the library already hands us what is needed to recover automatically:
openidc.authenticate()returns the session'soriginal_urlas its third value, right next to the error. The plugin was discarding it with_.This makes a
GETcallback with a state mismatch respond302to that URL instead. A fresh flow starts from there, and while the IdP still holds an SSO session the user notices nothing. Everything else is unchanged:original_urlpresentoriginal_urlresponse_mode=form_post)A replayed callback after a completed login lands in the same branch and now also recovers, since
original_urlis never cleared from the session — a free side benefit.Notes on the two things worth scrutinising:
openidc_authorization_responsebuildsclient_errwithout interpolation (the version with the state values goes only to the log), so equality is stable. Checked against both lua-resty-openidc 1.8.0 (currently pinned in the rockspec) and 1.9.0, which feat(openid-connect): support PAR and DPoP client options #13649 will bump to — the string, the third return value, and theoriginal_urlwrite/clear behaviour are identical in both, so this needs no follow-up when that PR lands.original_urlis read from the session, and the only place it is written is fromngx.var.request_uri— a path on this server. A request cannot inject theLocationvalue.There is also no unattended redirect loop: a client without cookies never reaches this branch (it hits the "no session state found" error instead and still gets 500), and each 302 starts a normal authorization request, exactly as if the user had retried by hand.
session:close()in the existing error path is left as is — the session still holds the other, in-flight flow's state, so destroying it would kill that flow too.Tests
New
t/plugin/openid-connect11.treuses the Keycloak instance already in CI. It does not need to complete a login: the state check happens before the token endpoint is ever called, so simulating two tabs with a cookie jar is enough.302withLocation: /oidc11/page?tab=BVerified discriminating: on master TEST 2 returns
500with noLocationand fails; TEST 3 and 4 pass either way, pinning the guard boundaries. Existingt/plugin/openid-connect*.twere run against this branch with no new failures.Upstream follow-up
Handling this in the library would be cleaner than matching an error string. I will open an issue on zmartzone/lua-resty-openidc (related to their #482) proposing a structured error or a built-in re-auth option; if that lands, this can switch to it. Users are hitting the 500 today, so the plugin-side fix should not wait on it.