diff --git a/CHANGELOG.md b/CHANGELOG.md index 448e0b80..7f9ba9e0 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 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/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..b1ee4b85 100644 --- a/tests/oidc/mod.rs +++ b/tests/oidc/mod.rs @@ -352,6 +352,48 @@ 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); + + // 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); + 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,