From a0f83277950538deac39d28db2db7eb3d4a996b0 Mon Sep 17 00:00:00 2001 From: Ophir LOJKINE Date: Thu, 9 Jul 2026 14:25:46 +0200 Subject: [PATCH 1/2] Document OIDC replayed callback fix --- CHANGELOG.md | 1 + src/webserver/oidc.rs | 21 +++++++++++++++++++++ tests/oidc/mod.rs | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 448e0b80..7cc19c35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## unreleased - **Access logs now go to stdout.** SQLPage now writes the single per-request completion log line to stdout with the target `sqlpage::access`, matching common application-server and container logging conventions. Diagnostic logs, warnings, and internal errors still go to stderr. If your `LOG_LEVEL` or `RUST_LOG` filter is scoped to a specific old target such as `sqlpage::webserver::http=info`, add `sqlpage::access=info` so request-completion logs are still emitted. If your log pipeline only collects stderr, update it to collect stdout too. +- **OIDC login no longer restarts when a browser replays an already-consumed callback.** This fixes a short redirect loop that could appear on cold browser start when parallel protected-page loads raced through the OIDC flow: one callback completed login and removed its temporary `sqlpage_oidc_state_*` cookie, while a replay of the same callback arrived afterward with the new authenticated session but without that temporary state cookie. ## v0.44.1 diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 0723d5c9..18ae1685 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -539,6 +539,18 @@ fn handle_oidc_callback_error( request: ServiceRequest, e: &anyhow::Error, ) -> ServiceResponse { + if is_missing_tmp_login_flow_state_cookie_error(e) + && matches!( + get_authenticated_user_info(oidc_state, &request), + Ok(Some(_)) + ) + { + log::warn!("Ignoring replayed OIDC callback for an already authenticated session: {e:#}"); + let mut resp = build_redirect_response("/".to_string()); + clear_redirect_count_cookie(&mut resp); + return request.into_response(resp); + } + let redirect_count = get_redirect_count(&request); if redirect_count >= MAX_OIDC_REDIRECTS { return handle_max_redirect_count_reached(request, e, redirect_count); @@ -909,6 +921,15 @@ fn build_oidc_error_response(request: &ServiceRequest, e: &anyhow::Error) -> Htt ) } +fn is_missing_tmp_login_flow_state_cookie_error(e: &anyhow::Error) -> bool { + e.chain().any(|cause| { + let msg = cause.to_string(); + msg.starts_with("No ") + && msg.contains(SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX) + && msg.ends_with(" cookie found") + }) +} + /// Returns the claims from the ID token in the `SQLPage` auth cookie. fn get_authenticated_user_info( oidc_state: &OidcState, diff --git a/tests/oidc/mod.rs b/tests/oidc/mod.rs index af32ad22..97dbe1d0 100644 --- a/tests/oidc/mod.rs +++ b/tests/oidc/mod.rs @@ -352,6 +352,43 @@ async fn test_oidc_happy_path() { assert_eq!(final_resp.status(), StatusCode::OK); } +#[actix_web::test] +async fn test_oidc_replayed_callback_after_login_does_not_restart_login() { + let (app, provider) = setup_oidc_test(|_| {}).await; + let mut cookies: Vec> = Vec::new(); + + let resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies); + assert_eq!(resp.status(), StatusCode::SEE_OTHER); + let auth_url = Url::parse(resp.headers().get("location").unwrap().to_str().unwrap()).unwrap(); + + let state = get_query_param(&auth_url, "state"); + let nonce = get_query_param(&auth_url, "nonce"); + let redirect_uri = get_query_param(&auth_url, "redirect_uri"); + provider.store_auth_code("test_auth_code".to_string(), nonce); + + let callback_uri = format!( + "{}?code=test_auth_code&state={}", + Url::parse(&redirect_uri).unwrap().path(), + state + ); + let callback_resp = + request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies); + assert_eq!(callback_resp.status(), StatusCode::SEE_OTHER); + + let replay_resp = + request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies); + assert_eq!(replay_resp.status(), StatusCode::SEE_OTHER); + assert_eq!( + replay_resp + .headers() + .get("location") + .unwrap() + .to_str() + .unwrap(), + "/" + ); +} + async fn assert_oidc_login_fails( provider_mutator: impl FnOnce(&mut ProviderState), state_override: Option, From 7be8522b9fc845af2e073ac1285c71a934b52451 Mon Sep 17 00:00:00 2001 From: Ophir LOJKINE Date: Thu, 9 Jul 2026 14:49:02 +0200 Subject: [PATCH 2/2] Clarify OIDC stale callback reproduction --- CHANGELOG.md | 2 +- tests/oidc/mod.rs | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cc19c35..7f9ba9e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ## unreleased - **Access logs now go to stdout.** SQLPage now writes the single per-request completion log line to stdout with the target `sqlpage::access`, matching common application-server and container logging conventions. Diagnostic logs, warnings, and internal errors still go to stderr. If your `LOG_LEVEL` or `RUST_LOG` filter is scoped to a specific old target such as `sqlpage::webserver::http=info`, add `sqlpage::access=info` so request-completion logs are still emitted. If your log pipeline only collects stderr, update it to collect stdout too. -- **OIDC login no longer restarts when a browser replays an already-consumed callback.** This fixes a short redirect loop that could appear on cold browser start when parallel protected-page loads raced through the OIDC flow: one callback completed login and removed its temporary `sqlpage_oidc_state_*` cookie, while a replay of the same callback arrived afterward with the new authenticated session but without that temporary state cookie. +- **OIDC login no longer restarts on a stale callback after login has already completed.** If an OIDC callback URL is received again after its temporary `sqlpage_oidc_state_*` cookie has been consumed, but the request already has a valid SQLPage session, SQLPage now treats it as a stale callback and redirects home instead of starting a new login flow. ## v0.44.1 diff --git a/tests/oidc/mod.rs b/tests/oidc/mod.rs index 97dbe1d0..b1ee4b85 100644 --- a/tests/oidc/mod.rs +++ b/tests/oidc/mod.rs @@ -375,6 +375,11 @@ async fn test_oidc_replayed_callback_after_login_does_not_restart_login() { request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies); assert_eq!(callback_resp.status(), StatusCode::SEE_OTHER); + // This is the smallest deterministic reproduction of the log pattern from + // the bug report: the first callback completed the login and removed the + // temporary sqlpage_oidc_state_* cookie; the same callback URL is then + // received again while the browser already carries the final authenticated + // session cookies. That stale callback must not start another OIDC flow. let replay_resp = request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies); assert_eq!(replay_resp.status(), StatusCode::SEE_OTHER);