AlinsRan opened a new pull request, #13712: URL: https://github.com/apache/apisix/pull/13712
### Description Fixes #12360 `resty.openidc` stores 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 returns `500`. 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's `original_url` as its third value, right next to the error. The plugin was discarding it with `_`. This makes a `GET` callback with a state mismatch respond `302` to 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: | Case | Before | After | |---|---|---| | GET callback, state mismatch, `original_url` present | 500 | **302 to `original_url`** | | Same but non-GET (`response_mode=form_post`) | 500 | 500 | | Callback with no session at all | 500 | 500 | | Any other error (iss/client_id mismatch, token endpoint failure, …) | 500 | 500 | A replayed callback after a completed login lands in the same branch and now also recovers, since `original_url` is never cleared from the session — a free side benefit. Notes on the two things worth scrutinising: - **Matching the error string verbatim.** `openidc_authorization_response` builds `client_err` without 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 #13649 will bump to — the string, the third return value, and the `original_url` write/clear behaviour are identical in both, so this needs no follow-up when that PR lands. - **No open redirect.** `original_url` is read from the session, and the only place it is written is from `ngx.var.request_uri` — a path on this server. A request cannot inject the `Location` value. 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.t` reuses 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. - TEST 2 — two flows, then the first tab's callback: expects `302` with `Location: /oidc11/page?tab=B` - TEST 3 — same but POST: still 500 - TEST 4 — callback with no session cookie: still 500 Verified discriminating: on master TEST 2 returns `500` with no `Location` and fails; TEST 3 and 4 pass either way, pinning the guard boundaries. Existing `t/plugin/openid-connect*.t` were 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. -- 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]
