From 0045e43aef4f877d5341edf2213222d1d8905ce5 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 11:58:30 +0200 Subject: [PATCH 1/6] Refactor OIDC request handling to streamline callback processing and remove redundant code. Simplify the handling of authenticated user info and improve error logging for OIDC callback parameters. --- src/webserver/oidc.rs | 67 +++++++++++-------------------------------- 1 file changed, 17 insertions(+), 50 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 8a5dc47d..097b14bc 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -329,15 +329,17 @@ enum MiddlewareResponse { async fn handle_request(oidc_state: &OidcState, request: ServiceRequest) -> MiddlewareResponse { log::trace!("Started OIDC middleware request handling"); oidc_state.refresh_if_expired(&request).await; + + if request.path() == SQLPAGE_REDIRECT_URI { + let response = handle_oidc_callback(oidc_state, request).await; + return MiddlewareResponse::Respond(response); + } + match get_authenticated_user_info(oidc_state, &request).await { Ok(Some(claims)) => { - if request.path() == SQLPAGE_REDIRECT_URI { - MiddlewareResponse::Respond(handle_authenticated_oidc_callback(request)) - } else { - log::trace!("Storing authenticated user info in request extensions: {claims:?}"); - request.extensions_mut().insert(claims); - MiddlewareResponse::Forward(request) - } + log::trace!("Storing authenticated user info in request extensions: {claims:?}"); + request.extensions_mut().insert(claims); + MiddlewareResponse::Forward(request) } Ok(None) => { log::trace!("No authenticated user found"); @@ -356,11 +358,6 @@ async fn handle_unauthenticated_request( request: ServiceRequest, ) -> MiddlewareResponse { log::debug!("Handling unauthenticated request to {}", request.path()); - if request.path() == SQLPAGE_REDIRECT_URI { - log::debug!("The request is the OIDC callback"); - let response = handle_oidc_callback(oidc_state, request).await; - return MiddlewareResponse::Respond(response); - } if oidc_state.config.is_public_path(request.path()) { return MiddlewareResponse::Forward(request); @@ -374,8 +371,7 @@ async fn handle_unauthenticated_request( } async fn handle_oidc_callback(oidc_state: &OidcState, request: ServiceRequest) -> ServiceResponse { - let query_string = request.query_string(); - match process_oidc_callback(oidc_state, query_string, &request).await { + match process_oidc_callback(oidc_state, &request).await { Ok(response) => request.into_response(response), Err(e) => { log::error!("Failed to process OIDC callback. Refreshing oidc provider metadata, then redirecting to home page: {e:#}"); @@ -386,24 +382,6 @@ async fn handle_oidc_callback(oidc_state: &OidcState, request: ServiceRequest) - } } -/// When an user has already authenticated (potentially in another tab), we ignore the callback and redirect to the initial URL. -fn handle_authenticated_oidc_callback(request: ServiceRequest) -> ServiceResponse { - // Try to get redirect URL from query params if available - let redirect_url = Query::::from_query(request.query_string()) - .with_context(|| "Failed to parse OIDC callback parameters in authenticated callback") - .and_then(|params| get_redirect_url_cookie(&request, ¶ms.state)) - .map_or_else( - |e| { - log::warn!("No redirect URL cookie: {e:#}"); - "/".to_string() - }, - |cookie| cookie.value().to_string(), - ); - - log::debug!("OIDC callback received for authenticated user. Redirecting to {redirect_url}"); - request.into_response(build_redirect_response(redirect_url)) -} - impl Service for OidcService where S: Service, Error = Error> + 'static, @@ -429,36 +407,25 @@ where async fn process_oidc_callback( oidc_state: &OidcState, - query_string: &str, request: &ServiceRequest, ) -> anyhow::Result { - let http_client = get_http_client_from_appdata(request)?; - - let params = Query::::from_query(query_string) - .with_context(|| { - format!( - "{SQLPAGE_REDIRECT_URI}: failed to parse OIDC callback parameters from {query_string}" - ) - })? + let params = Query::::from_query(request.query_string()) + .with_context(|| format!("{SQLPAGE_REDIRECT_URI}: invalid url parameters"))? .into_inner(); - - let mut redirect_url_cookie = get_redirect_url_cookie(request, ¶ms.state) - .with_context(|| "Failed to read redirect URL from cookie")?; - - let client = oidc_state.get_client().await; log::debug!("Processing OIDC callback with params: {params:?}. Requesting token..."); + let mut redirect_url_cookie = get_redirect_url_cookie(request, ¶ms.state)?; + let client = oidc_state.get_client().await; + let http_client = get_http_client_from_appdata(request)?; let token_response = exchange_code_for_token(&client, http_client, params.clone()).await?; log::debug!("Received token response: {token_response:?}"); let redirect_target = validate_redirect_url(redirect_url_cookie.value().to_string()); + log::info!("Redirecting to {redirect_target} after a successful login"); let mut response = build_redirect_response(redirect_target); set_auth_cookie(&mut response, &token_response).context("Failed to set auth cookie")?; - - // Clean up the state-specific cookie after successful authentication - redirect_url_cookie.set_path("/"); + redirect_url_cookie.set_path("/"); // Required to clean up the cookie response.add_removal_cookie(&redirect_url_cookie)?; - Ok(response) } From ad8246185657d67c9ce8084ecef5c5c4434af596 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 14:39:28 +0200 Subject: [PATCH 2/6] Refactor OIDC callback processing to use ID token directly, enhancing cookie management and nonce handling. Update logging for successful logins and streamline token claims retrieval. --- src/webserver/oidc.rs | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 097b14bc..3700d59d 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -416,14 +416,21 @@ async fn process_oidc_callback( let mut redirect_url_cookie = get_redirect_url_cookie(request, ¶ms.state)?; let client = oidc_state.get_client().await; let http_client = get_http_client_from_appdata(request)?; - let token_response = exchange_code_for_token(&client, http_client, params.clone()).await?; - log::debug!("Received token response: {token_response:?}"); - + let id_token = exchange_code_for_token(&client, http_client, params.clone()).await?; + log::debug!("Received token response: {id_token:?}"); + let nonce = get_nonce_from_cookie(request)?; let redirect_target = validate_redirect_url(redirect_url_cookie.value().to_string()); log::info!("Redirecting to {redirect_target} after a successful login"); let mut response = build_redirect_response(redirect_target); - set_auth_cookie(&mut response, &token_response).context("Failed to set auth cookie")?; + set_auth_cookie(&mut response, &id_token).context("Failed to set auth cookie")?; + let claims = oidc_state + .get_token_claims(id_token, &nonce) + .await + .context("The identity provider returned an invalid ID token")?; + log::debug!("{} successfully logged in", claims.subject().as_str()); + let nonce_cookie = create_nonce_cookie(&nonce); + response.add_cookie(&nonce_cookie)?; redirect_url_cookie.set_path("/"); // Required to clean up the cookie response.add_removal_cookie(&redirect_url_cookie)?; Ok(response) @@ -433,7 +440,7 @@ async fn exchange_code_for_token( oidc_client: &OidcClient, http_client: &awc::Client, oidc_callback_params: OidcCallbackParams, -) -> anyhow::Result { +) -> anyhow::Result { let token_response = oidc_client .exchange_code(openidconnect::AuthorizationCode::new( oidc_callback_params.code, @@ -441,19 +448,15 @@ async fn exchange_code_for_token( .request_async(&AwcHttpClient::from_client(http_client)) .await .context("Failed to exchange code for token")?; - Ok(token_response) -} - -fn set_auth_cookie( - response: &mut HttpResponse, - token_response: &OidcTokenResponse, -) -> anyhow::Result<()> { let access_token = token_response.access_token(); log::trace!("Received access token: {}", access_token.secret()); let id_token = token_response .id_token() .context("No ID token found in the token response. You may have specified an oauth2 provider that does not support OIDC.")?; + Ok(id_token.clone()) +} +fn set_auth_cookie(response: &mut HttpResponse, id_token: &OidcToken) -> anyhow::Result<()> { let id_token_str = id_token.to_string(); log::trace!("Setting auth cookie: {SQLPAGE_AUTH_COOKIE_NAME}=\"{id_token_str}\""); let id_token_size_kb = id_token_str.len() / 1024; From a8a9c2aff0a8262a3d35e73c8615451c46929a17 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 15:21:14 +0200 Subject: [PATCH 3/6] Refactor OIDC state management to use a temporary login flow state cookie, enhancing nonce and redirect handling. Update cookie creation and retrieval methods for improved clarity and maintainability. fixes https://github.com/sqlpage/SQLPage/issues/1014 --- src/webserver/oidc.rs | 69 ++++++++++++++++++++++++++++--------------- 1 file changed, 46 insertions(+), 23 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 3700d59d..82292b33 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -41,11 +41,13 @@ type LocalBoxFuture = Pin + 'static>>; const SQLPAGE_AUTH_COOKIE_NAME: &str = "sqlpage_auth"; const SQLPAGE_REDIRECT_URI: &str = "/sqlpage/oidc_callback"; const SQLPAGE_NONCE_COOKIE_NAME: &str = "sqlpage_oidc_nonce"; -const SQLPAGE_REDIRECT_URL_COOKIE_PREFIX: &str = "sqlpage_oidc_redirect_url_"; +const SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX: &str = "sqlpage_oidc_state_"; const OIDC_CLIENT_MAX_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60); const OIDC_CLIENT_MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5); const AUTH_COOKIE_EXPIRATION: awc::cookie::time::Duration = actix_web::cookie::time::Duration::days(7); +const LOGIN_FLOW_STATE_COOKIE_EXPIRATION: awc::cookie::time::Duration = + actix_web::cookie::time::Duration::minutes(10); #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(transparent)] @@ -413,26 +415,29 @@ async fn process_oidc_callback( .with_context(|| format!("{SQLPAGE_REDIRECT_URI}: invalid url parameters"))? .into_inner(); log::debug!("Processing OIDC callback with params: {params:?}. Requesting token..."); - let mut redirect_url_cookie = get_redirect_url_cookie(request, ¶ms.state)?; + let mut tmp_login_flow_state_cookie = get_tmp_login_flow_state_cookie(request, ¶ms.state)?; let client = oidc_state.get_client().await; let http_client = get_http_client_from_appdata(request)?; let id_token = exchange_code_for_token(&client, http_client, params.clone()).await?; log::debug!("Received token response: {id_token:?}"); - let nonce = get_nonce_from_cookie(request)?; - let redirect_target = validate_redirect_url(redirect_url_cookie.value().to_string()); + let LoginFlowState { + nonce, + redirect_target, + } = parse_login_flow_state(&tmp_login_flow_state_cookie)?; + let redirect_target = validate_redirect_url(redirect_target.to_string()); log::info!("Redirecting to {redirect_target} after a successful login"); let mut response = build_redirect_response(redirect_target); - set_auth_cookie(&mut response, &id_token).context("Failed to set auth cookie")?; + set_auth_cookie(&mut response, &id_token); let claims = oidc_state .get_token_claims(id_token, &nonce) .await .context("The identity provider returned an invalid ID token")?; log::debug!("{} successfully logged in", claims.subject().as_str()); - let nonce_cookie = create_nonce_cookie(&nonce); + let nonce_cookie = create_final_nonce_cookie(&nonce); response.add_cookie(&nonce_cookie)?; - redirect_url_cookie.set_path("/"); // Required to clean up the cookie - response.add_removal_cookie(&redirect_url_cookie)?; + tmp_login_flow_state_cookie.set_path("/"); // Required to clean up the cookie + response.add_removal_cookie(&tmp_login_flow_state_cookie)?; Ok(response) } @@ -456,7 +461,7 @@ async fn exchange_code_for_token( Ok(id_token.clone()) } -fn set_auth_cookie(response: &mut HttpResponse, id_token: &OidcToken) -> anyhow::Result<()> { +fn set_auth_cookie(response: &mut HttpResponse, id_token: &OidcToken) { let id_token_str = id_token.to_string(); log::trace!("Setting auth cookie: {SQLPAGE_AUTH_COOKIE_NAME}=\"{id_token_str}\""); let id_token_size_kb = id_token_str.len() / 1024; @@ -475,7 +480,6 @@ fn set_auth_cookie(response: &mut HttpResponse, id_token: &OidcToken) -> anyhow: .finish(); response.add_cookie(&cookie).unwrap(); - Ok(()) } async fn build_auth_provider_redirect_response( @@ -483,12 +487,10 @@ async fn build_auth_provider_redirect_response( initial_url: &str, ) -> HttpResponse { let AuthUrl { url, params } = build_auth_url(oidc_state).await; - let nonce_cookie = create_nonce_cookie(¶ms.nonce); - let redirect_cookie = create_redirect_cookie(¶ms.csrf_token, initial_url); + let tmp_login_flow_state_cookie = create_tmp_login_flow_state_cookie(¶ms, initial_url); HttpResponse::TemporaryRedirect() .append_header((header::LOCATION, url.to_string())) - .cookie(nonce_cookie) - .cookie(redirect_cookie) + .cookie(tmp_login_flow_state_cookie) .body("Redirecting...") } @@ -510,7 +512,7 @@ async fn get_authenticated_user_info( let id_token = OidcToken::from_str(&cookie_value) .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; - let nonce = get_nonce_from_cookie(request)?; + let nonce = get_final_nonce_from_cookie(request)?; log::debug!("Verifying id token: {id_token:?}"); let claims = oidc_state.get_token_claims(id_token, &nonce).await?; log::debug!("The current user is: {claims:?}"); @@ -742,7 +744,7 @@ fn nonce_matches(id_token_nonce: &Nonce, state_nonce: &Nonce) -> Result<(), Stri Ok(()) } -fn create_nonce_cookie(nonce: &Nonce) -> Cookie<'_> { +fn create_final_nonce_cookie(nonce: &Nonce) -> Cookie<'_> { Cookie::build(SQLPAGE_NONCE_COOKIE_NAME, nonce.secret()) .secure(true) .http_only(true) @@ -752,34 +754,55 @@ fn create_nonce_cookie(nonce: &Nonce) -> Cookie<'_> { .finish() } -fn create_redirect_cookie<'a>(csrf_token: &CsrfToken, initial_url: &'a str) -> Cookie<'a> { - let cookie_name = SQLPAGE_REDIRECT_URL_COOKIE_PREFIX.to_owned() + csrf_token.secret(); - Cookie::build(cookie_name, initial_url) +fn create_tmp_login_flow_state_cookie<'a>( + params: &'a AuthUrlParams, + initial_url: &'a str, +) -> Cookie<'a> { + let csrf_token = ¶ms.csrf_token; + let cookie_name = SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX.to_owned() + csrf_token.secret(); + let cookie_value = serde_json::to_string(&LoginFlowState { + nonce: params.nonce.clone(), + redirect_target: initial_url, + }) + .expect("login flow state is always serializable"); + Cookie::build(cookie_name, cookie_value) .secure(true) .http_only(true) .same_site(actix_web::cookie::SameSite::Lax) .path("/") - .max_age(actix_web::cookie::time::Duration::minutes(10)) + .max_age(LOGIN_FLOW_STATE_COOKIE_EXPIRATION) .finish() } -fn get_nonce_from_cookie(request: &ServiceRequest) -> anyhow::Result { +fn get_final_nonce_from_cookie(request: &ServiceRequest) -> anyhow::Result { let cookie = request .cookie(SQLPAGE_NONCE_COOKIE_NAME) .with_context(|| format!("No {SQLPAGE_NONCE_COOKIE_NAME} cookie found"))?; Ok(Nonce::new(cookie.value().to_string())) } -fn get_redirect_url_cookie( +fn get_tmp_login_flow_state_cookie( request: &ServiceRequest, csrf_token: &CsrfToken, ) -> anyhow::Result> { - let cookie_name = SQLPAGE_REDIRECT_URL_COOKIE_PREFIX.to_owned() + csrf_token.secret(); + let cookie_name = SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX.to_owned() + csrf_token.secret(); request .cookie(&cookie_name) .with_context(|| format!("No {cookie_name} cookie found")) } +#[derive(Debug, Serialize, Deserialize, Clone)] +struct LoginFlowState<'a> { + #[serde(rename = "n")] + nonce: Nonce, + #[serde(rename = "r")] + redirect_target: &'a str, +} + +fn parse_login_flow_state<'a>(cookie: &'a Cookie<'_>) -> anyhow::Result> { + Ok(serde_json::from_str(cookie.value())?) +} + /// Given an audience, verify if it is trusted. The `client_id` is always trusted, independently of this function. #[derive(Clone, Debug)] pub struct AudienceVerifier(Option>); From aa1d31b4df9c821d7c40b029142926d4ed3b9108 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 15:23:09 +0200 Subject: [PATCH 4/6] Enhance error handling in login flow state parsing by adding context to JSON deserialization failure. --- src/webserver/oidc.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 82292b33..2dd82015 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -800,7 +800,8 @@ struct LoginFlowState<'a> { } fn parse_login_flow_state<'a>(cookie: &'a Cookie<'_>) -> anyhow::Result> { - Ok(serde_json::from_str(cookie.value())?) + Ok(serde_json::from_str(cookie.value()) + .with_context(|| format!("Invalid login flow state cookie: {}", cookie.value()))?) } /// Given an audience, verify if it is trusted. The `client_id` is always trusted, independently of this function. From c4a8cff31045d63cd60fb380b4ddd756c0c37168 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 15:28:33 +0200 Subject: [PATCH 5/6] Fix infinite redirect loop in OIDC login flows when multiple tabs initiate login simultaneously, particularly after inactivity in mobile browsers. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70268cbe..59febcc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - Fixed handling of NULL values in `sqlpage.link`. They were encoded as the string `'null'` instead of being omitted from the link's parameters. - Enable submenu autoclosing (on click) in the shell. This is not ideal, but this prevents a bug introduced in v0.36.0 where the page would scroll back to the top when clicking anywhere on the page after navigating from a submenu. The next version will fix this properly. See https://github.com/sqlpage/SQLPage/issues/1011 - Adopt the new nice visual errors introduced in v0.37.1 for "403 Forbidden" and "429 Too Many Requests" errors. + - Fix a bug in oidc login flows. When two tabs in the same browser initiated a login at the same time, an infinite redirect loop could be triggered. This mainly occured when restoring open tabs after a period of inactivity, often in mobile browsers. ## v0.37.0 - We now cryptographically sign the Windows app during releases, which proves the file hasn’t been tampered with. Once the production certificate is active, Windows will show a "verified publisher" and should stop showing screens saying "This app might harm your device", "Windows protected your PC" or "Are you sure you want to run this application ?". From 655b473dba210a841f1a5d1d61040d4587bbd0f5 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 15:29:26 +0200 Subject: [PATCH 6/6] clippy --- src/webserver/oidc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 2dd82015..23607a91 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -800,8 +800,8 @@ struct LoginFlowState<'a> { } fn parse_login_flow_state<'a>(cookie: &'a Cookie<'_>) -> anyhow::Result> { - Ok(serde_json::from_str(cookie.value()) - .with_context(|| format!("Invalid login flow state cookie: {}", cookie.value()))?) + serde_json::from_str(cookie.value()) + .with_context(|| format!("Invalid login flow state cookie: {}", cookie.value())) } /// Given an audience, verify if it is trusted. The `client_id` is always trusted, independently of this function.