From b6f81b99e2b0b92cd840504bd9c30d0326afbcff Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 17 Sep 2025 07:19:57 +0000 Subject: [PATCH 01/10] Refactor OIDC state handling to use separate nonce and redirect cookies Co-authored-by: contact --- src/webserver/oidc.rs | 153 +++++++++++++++++++++++++++++++++++------- 1 file changed, 128 insertions(+), 25 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index d54dedb6..c496e9ac 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -40,7 +40,8 @@ type LocalBoxFuture = Pin + 'static>>; const SQLPAGE_AUTH_COOKIE_NAME: &str = "sqlpage_auth"; const SQLPAGE_REDIRECT_URI: &str = "/sqlpage/oidc_callback"; -const SQLPAGE_STATE_COOKIE_NAME: &str = "sqlpage_oidc_state"; +const SQLPAGE_NONCE_COOKIE_NAME: &str = "sqlpage_oidc_nonce"; +const SQLPAGE_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); @@ -375,8 +376,17 @@ async fn handle_oidc_callback(oidc_state: &OidcState, request: ServiceRequest) - match process_oidc_callback(oidc_state, query_string, &request).await { Ok(response) => request.into_response(response), Err(e) => { + // Try to get redirect URL from query params if available let redirect_url = - get_state_from_cookie(&request).map_or_else(|_| "/".into(), |s| s.initial_url); + if let Ok(params) = Query::::from_query(query_string) { + if let Ok(state) = get_state_from_cookies(&request, ¶ms.state) { + state.initial_url + } else { + "/".to_string() + } + } else { + "/".to_string() + }; log::error!("Failed to process OIDC callback. Refreshing oidc provider metadata, then redirecting to {redirect_url}: {e:#}"); oidc_state.refresh_on_error(&request).await; let resp = build_auth_provider_redirect_response(oidc_state, redirect_url).await; @@ -387,10 +397,17 @@ 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 { - let redirect_url = match get_state_from_cookie(&request) { - Ok(state) => state.initial_url, - Err(_) => "/".to_string(), - }; + // Try to get redirect URL from query params if available + let redirect_url = + if let Ok(params) = Query::::from_query(request.query_string()) { + if let Ok(state) = get_state_from_cookies(&request, ¶ms.state) { + state.initial_url + } else { + "/".to_string() + } + } else { + "/".to_string() + }; log::debug!("OIDC callback received for authenticated user. Redirecting to {redirect_url}"); request.into_response(build_redirect_response(redirect_url)) } @@ -425,8 +442,6 @@ async fn process_oidc_callback( ) -> anyhow::Result { let http_client = get_http_client_from_appdata(request)?; - let state = get_state_from_cookie(request).context("Failed to read oidc state cookie")?; - let params = Query::::from_query(query_string) .with_context(|| { format!( @@ -435,10 +450,8 @@ async fn process_oidc_callback( })? .into_inner(); - if state.csrf_token.secret() != params.state.secret() { - log::debug!("CSRF token mismatch: expected {state:?}, got {params:?}"); - return Err(anyhow!("Invalid CSRF token: {}", params.state.secret())); - } + let state = get_state_from_cookies(request, ¶ms.state) + .context("Failed to read oidc state cookies")?; let client = oidc_state.get_client().await; log::debug!("Processing OIDC callback with params: {params:?}. Requesting token..."); @@ -451,6 +464,22 @@ async fn process_oidc_callback( set_auth_cookie(&mut response, &token_response, oidc_state) .await .context("Failed to set auth cookie")?; + + // Clean up the state-specific cookie after successful authentication + let state_cookie_name = format!( + "{}{}", + SQLPAGE_STATE_COOKIE_PREFIX, + state.csrf_token.secret() + ); + let cleanup_cookie = Cookie::build(state_cookie_name, "") + .secure(true) + .http_only(true) + .same_site(actix_web::cookie::SameSite::Lax) + .path("/") + .max_age(actix_web::cookie::time::Duration::seconds(0)) // Expire immediately + .finish(); + response.add_cookie(&cleanup_cookie).unwrap(); + Ok(response) } @@ -511,10 +540,11 @@ async fn build_auth_provider_redirect_response( ) -> HttpResponse { let AuthUrl { url, params } = build_auth_url(oidc_state).await; let state = OidcLoginState::new(initial_url, params); - let state_cookie = create_state_cookie(&state); + let (nonce_cookie, redirect_cookie) = create_state_cookies(&state); HttpResponse::TemporaryRedirect() .append_header(("Location", url.to_string())) - .cookie(state_cookie) + .cookie(nonce_cookie) + .cookie(redirect_cookie) .body("Redirecting...") } @@ -536,9 +566,21 @@ async fn get_authenticated_user_info( let id_token = OidcToken::from_str(&cookie_value) .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; - let state = get_state_from_cookie(request)?; + // Try to get state from cookies if this is a callback request + let state = if request.path() == SQLPAGE_REDIRECT_URI { + if let Ok(params) = Query::::from_query(request.query_string()) { + get_state_from_cookies(request, ¶ms.state).ok() + } else { + None + } + } else { + None + }; + log::debug!("Verifying id token: {id_token:?}"); - let claims = oidc_state.get_token_claims(id_token, Some(&state)).await?; + let claims = oidc_state + .get_token_claims(id_token, state.as_ref()) + .await?; log::debug!("The current user is: {claims:?}"); Ok(Some(claims)) } @@ -788,6 +830,21 @@ struct OidcLoginState { nonce: Nonce, } +#[derive(Debug, Serialize, Deserialize)] +struct OidcNonceState { + /// The source nonce to use for the login process. It must be checked against the hash + /// stored in the ID token. + #[serde(rename = "n")] + nonce: Nonce, +} + +#[derive(Debug, Serialize, Deserialize)] +struct OidcRedirectState { + /// The URL to redirect to after the login process is complete. + #[serde(rename = "u")] + initial_url: String, +} + impl OidcLoginState { fn new(initial_url: String, auth_url: AuthUrlParams) -> Self { Self { @@ -798,22 +855,68 @@ impl OidcLoginState { } } -fn create_state_cookie(login_state: &OidcLoginState) -> Cookie<'_> { - let state_json = serde_json::to_string(login_state).unwrap(); - Cookie::build(SQLPAGE_STATE_COOKIE_NAME, state_json) +fn create_state_cookies(login_state: &OidcLoginState) -> (Cookie<'_>, Cookie<'_>) { + // Create nonce cookie (longer-lived for security) + let nonce_state = OidcNonceState { + nonce: login_state.nonce.clone(), + }; + let nonce_json = serde_json::to_string(&nonce_state).unwrap(); + let nonce_cookie = Cookie::build(SQLPAGE_NONCE_COOKIE_NAME, nonce_json) + .secure(true) + .http_only(true) + .same_site(actix_web::cookie::SameSite::Lax) + .path("/") + .max_age(actix_web::cookie::time::Duration::minutes(10)) // 10 minutes should be enough for login flow + .finish(); + + // Create state-specific redirect cookie (short-lived) + let redirect_state = OidcRedirectState { + initial_url: login_state.initial_url.clone(), + }; + let redirect_json = serde_json::to_string(&redirect_state).unwrap(); + let state_cookie_name = format!( + "{}{}", + SQLPAGE_STATE_COOKIE_PREFIX, + login_state.csrf_token.secret() + ); + let redirect_cookie = Cookie::build(state_cookie_name, redirect_json) .secure(true) .http_only(true) .same_site(actix_web::cookie::SameSite::Lax) .path("/") - .finish() + .max_age(actix_web::cookie::time::Duration::minutes(5)) // Short-lived, only needed during login flow + .finish(); + + (nonce_cookie, redirect_cookie) } -fn get_state_from_cookie(request: &ServiceRequest) -> anyhow::Result { - let state_cookie = request.cookie(SQLPAGE_STATE_COOKIE_NAME).with_context(|| { - format!("No {SQLPAGE_STATE_COOKIE_NAME} cookie found for {SQLPAGE_REDIRECT_URI}") +fn get_state_from_cookies( + request: &ServiceRequest, + csrf_token: &CsrfToken, +) -> anyhow::Result { + // Get nonce from the nonce cookie + let nonce_cookie = request.cookie(SQLPAGE_NONCE_COOKIE_NAME).with_context(|| { + format!("No {SQLPAGE_NONCE_COOKIE_NAME} cookie found for {SQLPAGE_REDIRECT_URI}") })?; - serde_json::from_str(state_cookie.value()) - .with_context(|| format!("Failed to parse OIDC state from cookie: {state_cookie}")) + let nonce_state: OidcNonceState = serde_json::from_str(nonce_cookie.value()) + .with_context(|| format!("Failed to parse OIDC nonce from cookie: {nonce_cookie}"))?; + + // Get redirect URL from the state-specific cookie + let state_cookie_name = format!("{}{}", SQLPAGE_STATE_COOKIE_PREFIX, csrf_token.secret()); + let redirect_cookie = request.cookie(&state_cookie_name).with_context(|| { + format!("No {state_cookie_name} cookie found for {SQLPAGE_REDIRECT_URI}") + })?; + let redirect_state: OidcRedirectState = serde_json::from_str(redirect_cookie.value()) + .with_context(|| { + format!("Failed to parse OIDC redirect state from cookie: {redirect_cookie}") + })?; + + // Reconstruct the full login state + Ok(OidcLoginState { + initial_url: redirect_state.initial_url, + csrf_token: csrf_token.clone(), + nonce: nonce_state.nonce, + }) } /// Given an audience, verify if it is trusted. The `client_id` is always trusted, independently of this function. From 8c4a300e5bd76313a7ba8739caac5ae2ab4d0f21 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 17 Sep 2025 07:27:06 +0000 Subject: [PATCH 02/10] Refactor OIDC state management to use separate nonce and redirect cookies Co-authored-by: contact --- src/webserver/oidc.rs | 182 ++++++++++++++++-------------------------- 1 file changed, 68 insertions(+), 114 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index c496e9ac..43786893 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -210,11 +210,11 @@ impl OidcState { async fn get_token_claims( &self, id_token: OidcToken, - state: Option<&OidcLoginState>, + expected_nonce: Option<&Nonce>, ) -> anyhow::Result { let client = &self.get_client().await; let verifier = self.config.create_id_token_verifier(client); - let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, state); + let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, expected_nonce); let claims: OidcClaims = id_token .into_claims(&verifier, nonce_verifier) .map_err(|e| anyhow::anyhow!("Could not verify the ID token: {}", e))?; @@ -379,11 +379,8 @@ async fn handle_oidc_callback(oidc_state: &OidcState, request: ServiceRequest) - // Try to get redirect URL from query params if available let redirect_url = if let Ok(params) = Query::::from_query(query_string) { - if let Ok(state) = get_state_from_cookies(&request, ¶ms.state) { - state.initial_url - } else { - "/".to_string() - } + get_redirect_url_from_cookie(&request, ¶ms.state) + .unwrap_or_else(|_| "/".to_string()) } else { "/".to_string() }; @@ -398,16 +395,13 @@ 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 = - if let Ok(params) = Query::::from_query(request.query_string()) { - if let Ok(state) = get_state_from_cookies(&request, ¶ms.state) { - state.initial_url - } else { - "/".to_string() - } - } else { - "/".to_string() - }; + let redirect_url = if let Ok(params) = + Query::::from_query(request.query_string()) + { + get_redirect_url_from_cookie(&request, ¶ms.state).unwrap_or_else(|_| "/".to_string()) + } else { + "/".to_string() + }; log::debug!("OIDC callback received for authenticated user. Redirecting to {redirect_url}"); request.into_response(build_redirect_response(redirect_url)) } @@ -450,15 +444,15 @@ async fn process_oidc_callback( })? .into_inner(); - let state = get_state_from_cookies(request, ¶ms.state) - .context("Failed to read oidc state cookies")?; + let redirect_url = get_redirect_url_from_cookie(request, ¶ms.state) + .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 token_response = exchange_code_for_token(&client, http_client, params).await?; + 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(state.initial_url); + let redirect_target = validate_redirect_url(redirect_url); 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, oidc_state) @@ -466,18 +460,7 @@ async fn process_oidc_callback( .context("Failed to set auth cookie")?; // Clean up the state-specific cookie after successful authentication - let state_cookie_name = format!( - "{}{}", - SQLPAGE_STATE_COOKIE_PREFIX, - state.csrf_token.secret() - ); - let cleanup_cookie = Cookie::build(state_cookie_name, "") - .secure(true) - .http_only(true) - .same_site(actix_web::cookie::SameSite::Lax) - .path("/") - .max_age(actix_web::cookie::time::Duration::seconds(0)) // Expire immediately - .finish(); + let cleanup_cookie = create_cleanup_cookie(¶ms.state); response.add_cookie(&cleanup_cookie).unwrap(); Ok(response) @@ -539,8 +522,8 @@ async fn build_auth_provider_redirect_response( initial_url: String, ) -> HttpResponse { let AuthUrl { url, params } = build_auth_url(oidc_state).await; - let state = OidcLoginState::new(initial_url, params); - let (nonce_cookie, redirect_cookie) = create_state_cookies(&state); + let nonce_cookie = create_nonce_cookie(¶ms.nonce); + let redirect_cookie = create_redirect_cookie(¶ms.csrf_token, &initial_url); HttpResponse::TemporaryRedirect() .append_header(("Location", url.to_string())) .cookie(nonce_cookie) @@ -566,10 +549,10 @@ async fn get_authenticated_user_info( let id_token = OidcToken::from_str(&cookie_value) .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; - // Try to get state from cookies if this is a callback request - let state = if request.path() == SQLPAGE_REDIRECT_URI { - if let Ok(params) = Query::::from_query(request.query_string()) { - get_state_from_cookies(request, ¶ms.state).ok() + // Try to get nonce from cookies if this is a callback request + let nonce = if request.path() == SQLPAGE_REDIRECT_URI { + if let Ok(_params) = Query::::from_query(request.query_string()) { + get_nonce_from_cookie(request).ok() } else { None } @@ -579,7 +562,7 @@ async fn get_authenticated_user_info( log::debug!("Verifying id token: {id_token:?}"); let claims = oidc_state - .get_token_claims(id_token, state.as_ref()) + .get_token_claims(id_token, nonce.as_ref()) .await?; log::debug!("The current user is: {claims:?}"); Ok(Some(claims)) @@ -731,7 +714,7 @@ fn make_oidc_client( Ok(client) } -#[derive(Debug, Deserialize)] +#[derive(Debug, Deserialize, Clone)] struct OidcCallbackParams { code: String, state: CsrfToken, @@ -783,13 +766,13 @@ fn hash_nonce(nonce: &Nonce) -> String { fn check_nonce( id_token_nonce: Option<&Nonce>, - login_state: Option<&OidcLoginState>, + expected_nonce: Option<&Nonce>, ) -> Result<(), String> { - let Some(state) = login_state else { - return Ok(()); // No login state, no nonce to check + let Some(expected) = expected_nonce else { + return Ok(()); // No expected nonce, no validation needed }; match id_token_nonce { - Some(id_token_nonce) => nonce_matches(id_token_nonce, &state.nonce), + Some(id_token_nonce) => nonce_matches(id_token_nonce, expected), None => Err("No nonce found in the ID token".to_string()), } } @@ -816,107 +799,78 @@ fn nonce_matches(id_token_nonce: &Nonce, state_nonce: &Nonce) -> Result<(), Stri Ok(()) } -#[derive(Debug, Serialize, Deserialize)] -struct OidcLoginState { - /// The URL to redirect to after the login process is complete. - #[serde(rename = "u")] - initial_url: String, - /// The CSRF token to use for the login process. - #[serde(rename = "c")] - csrf_token: CsrfToken, - /// The source nonce to use for the login process. It must be checked against the hash - /// stored in the ID token. - #[serde(rename = "n")] - nonce: Nonce, -} - #[derive(Debug, Serialize, Deserialize)] struct OidcNonceState { - /// The source nonce to use for the login process. It must be checked against the hash - /// stored in the ID token. #[serde(rename = "n")] nonce: Nonce, } #[derive(Debug, Serialize, Deserialize)] struct OidcRedirectState { - /// The URL to redirect to after the login process is complete. #[serde(rename = "u")] initial_url: String, } -impl OidcLoginState { - fn new(initial_url: String, auth_url: AuthUrlParams) -> Self { - Self { - initial_url, - csrf_token: auth_url.csrf_token, - nonce: auth_url.nonce, - } - } -} - -fn create_state_cookies(login_state: &OidcLoginState) -> (Cookie<'_>, Cookie<'_>) { - // Create nonce cookie (longer-lived for security) +fn create_nonce_cookie(nonce: &Nonce) -> Cookie<'_> { let nonce_state = OidcNonceState { - nonce: login_state.nonce.clone(), + nonce: nonce.clone(), }; let nonce_json = serde_json::to_string(&nonce_state).unwrap(); - let nonce_cookie = Cookie::build(SQLPAGE_NONCE_COOKIE_NAME, nonce_json) + Cookie::build(SQLPAGE_NONCE_COOKIE_NAME, nonce_json) .secure(true) .http_only(true) .same_site(actix_web::cookie::SameSite::Lax) .path("/") - .max_age(actix_web::cookie::time::Duration::minutes(10)) // 10 minutes should be enough for login flow - .finish(); + .max_age(actix_web::cookie::time::Duration::minutes(10)) + .finish() +} - // Create state-specific redirect cookie (short-lived) +fn create_redirect_cookie(csrf_token: &CsrfToken, initial_url: &str) -> Cookie<'static> { let redirect_state = OidcRedirectState { - initial_url: login_state.initial_url.clone(), + initial_url: initial_url.to_string(), }; let redirect_json = serde_json::to_string(&redirect_state).unwrap(); - let state_cookie_name = format!( - "{}{}", - SQLPAGE_STATE_COOKIE_PREFIX, - login_state.csrf_token.secret() - ); - let redirect_cookie = Cookie::build(state_cookie_name, redirect_json) + let cookie_name = format!("{}{}", SQLPAGE_STATE_COOKIE_PREFIX, csrf_token.secret()); + Cookie::build(cookie_name, redirect_json) .secure(true) .http_only(true) .same_site(actix_web::cookie::SameSite::Lax) .path("/") - .max_age(actix_web::cookie::time::Duration::minutes(5)) // Short-lived, only needed during login flow - .finish(); + .max_age(actix_web::cookie::time::Duration::minutes(5)) + .finish() +} - (nonce_cookie, redirect_cookie) +fn create_cleanup_cookie(csrf_token: &CsrfToken) -> Cookie<'_> { + let cookie_name = format!("{}{}", SQLPAGE_STATE_COOKIE_PREFIX, csrf_token.secret()); + Cookie::build(cookie_name, "") + .secure(true) + .http_only(true) + .same_site(actix_web::cookie::SameSite::Lax) + .path("/") + .max_age(actix_web::cookie::time::Duration::seconds(0)) + .finish() +} + +fn get_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"))?; + let nonce_state: OidcNonceState = serde_json::from_str(cookie.value()) + .with_context(|| format!("Failed to parse nonce from cookie: {cookie}"))?; + Ok(nonce_state.nonce) } -fn get_state_from_cookies( +fn get_redirect_url_from_cookie( request: &ServiceRequest, csrf_token: &CsrfToken, -) -> anyhow::Result { - // Get nonce from the nonce cookie - let nonce_cookie = request.cookie(SQLPAGE_NONCE_COOKIE_NAME).with_context(|| { - format!("No {SQLPAGE_NONCE_COOKIE_NAME} cookie found for {SQLPAGE_REDIRECT_URI}") - })?; - let nonce_state: OidcNonceState = serde_json::from_str(nonce_cookie.value()) - .with_context(|| format!("Failed to parse OIDC nonce from cookie: {nonce_cookie}"))?; - - // Get redirect URL from the state-specific cookie - let state_cookie_name = format!("{}{}", SQLPAGE_STATE_COOKIE_PREFIX, csrf_token.secret()); - let redirect_cookie = request.cookie(&state_cookie_name).with_context(|| { - format!("No {state_cookie_name} cookie found for {SQLPAGE_REDIRECT_URI}") - })?; - let redirect_state: OidcRedirectState = serde_json::from_str(redirect_cookie.value()) - .with_context(|| { - format!("Failed to parse OIDC redirect state from cookie: {redirect_cookie}") - })?; - - // Reconstruct the full login state - Ok(OidcLoginState { - initial_url: redirect_state.initial_url, - csrf_token: csrf_token.clone(), - nonce: nonce_state.nonce, - }) +) -> anyhow::Result { + let cookie_name = format!("{}{}", SQLPAGE_STATE_COOKIE_PREFIX, csrf_token.secret()); + let cookie = request + .cookie(&cookie_name) + .with_context(|| format!("No {cookie_name} cookie found"))?; + let redirect_state: OidcRedirectState = serde_json::from_str(cookie.value()) + .with_context(|| format!("Failed to parse redirect state from cookie: {cookie}"))?; + Ok(redirect_state.initial_url) } /// Given an audience, verify if it is trusted. The `client_id` is always trusted, independently of this function. From f080051d858e5743acf5666d309b6abffbc8200f Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 00:10:40 +0200 Subject: [PATCH 03/10] Refactor OIDC redirect handling to use updated cookie names and improve error logging --- src/webserver/oidc.rs | 78 +++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 44 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 43786893..6dafc4d4 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -41,7 +41,7 @@ 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_STATE_COOKIE_PREFIX: &str = "sqlpage_oidc_state_"; +const SQLPAGE_REDIRECT_URL_COOKIE_PREFIX: &str = "sqlpage_oidc_redirect_url_"; const OIDC_CLIENT_MAX_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60); const OIDC_CLIENT_MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5); @@ -367,7 +367,7 @@ async fn handle_unauthenticated_request( log::debug!("Redirecting to OIDC provider"); let initial_url = request.uri().to_string(); - let response = build_auth_provider_redirect_response(oidc_state, initial_url).await; + let response = build_auth_provider_redirect_response(oidc_state, &initial_url).await; MiddlewareResponse::Respond(request.into_response(response)) } @@ -376,17 +376,9 @@ async fn handle_oidc_callback(oidc_state: &OidcState, request: ServiceRequest) - match process_oidc_callback(oidc_state, query_string, &request).await { Ok(response) => request.into_response(response), Err(e) => { - // Try to get redirect URL from query params if available - let redirect_url = - if let Ok(params) = Query::::from_query(query_string) { - get_redirect_url_from_cookie(&request, ¶ms.state) - .unwrap_or_else(|_| "/".to_string()) - } else { - "/".to_string() - }; - log::error!("Failed to process OIDC callback. Refreshing oidc provider metadata, then redirecting to {redirect_url}: {e:#}"); + log::error!("Failed to process OIDC callback. Refreshing oidc provider metadata, then redirecting to home page: {e:#}"); oidc_state.refresh_on_error(&request).await; - let resp = build_auth_provider_redirect_response(oidc_state, redirect_url).await; + let resp = build_auth_provider_redirect_response(oidc_state, "/").await; request.into_response(resp) } } @@ -395,13 +387,17 @@ 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 = if let Ok(params) = - Query::::from_query(request.query_string()) - { - get_redirect_url_from_cookie(&request, ¶ms.state).unwrap_or_else(|_| "/".to_string()) - } else { - "/".to_string() - }; + 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)) } @@ -444,15 +440,15 @@ async fn process_oidc_callback( })? .into_inner(); - let redirect_url = get_redirect_url_from_cookie(request, ¶ms.state) - .context("Failed to read redirect URL from cookie")?; + let 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 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); + 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, oidc_state) @@ -460,8 +456,7 @@ async fn process_oidc_callback( .context("Failed to set auth cookie")?; // Clean up the state-specific cookie after successful authentication - let cleanup_cookie = create_cleanup_cookie(¶ms.state); - response.add_cookie(&cleanup_cookie).unwrap(); + response.add_removal_cookie(&redirect_url_cookie)?; Ok(response) } @@ -519,11 +514,11 @@ async fn set_auth_cookie( async fn build_auth_provider_redirect_response( oidc_state: &OidcState, - initial_url: String, + 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 redirect_cookie = create_redirect_cookie(¶ms.csrf_token, initial_url); HttpResponse::TemporaryRedirect() .append_header(("Location", url.to_string())) .cookie(nonce_cookie) @@ -830,7 +825,11 @@ fn create_redirect_cookie(csrf_token: &CsrfToken, initial_url: &str) -> Cookie<' initial_url: initial_url.to_string(), }; let redirect_json = serde_json::to_string(&redirect_state).unwrap(); - let cookie_name = format!("{}{}", SQLPAGE_STATE_COOKIE_PREFIX, csrf_token.secret()); + let cookie_name = format!( + "{}{}", + SQLPAGE_REDIRECT_URL_COOKIE_PREFIX, + csrf_token.secret() + ); Cookie::build(cookie_name, redirect_json) .secure(true) .http_only(true) @@ -840,17 +839,6 @@ fn create_redirect_cookie(csrf_token: &CsrfToken, initial_url: &str) -> Cookie<' .finish() } -fn create_cleanup_cookie(csrf_token: &CsrfToken) -> Cookie<'_> { - let cookie_name = format!("{}{}", SQLPAGE_STATE_COOKIE_PREFIX, csrf_token.secret()); - Cookie::build(cookie_name, "") - .secure(true) - .http_only(true) - .same_site(actix_web::cookie::SameSite::Lax) - .path("/") - .max_age(actix_web::cookie::time::Duration::seconds(0)) - .finish() -} - fn get_nonce_from_cookie(request: &ServiceRequest) -> anyhow::Result { let cookie = request .cookie(SQLPAGE_NONCE_COOKIE_NAME) @@ -860,17 +848,19 @@ fn get_nonce_from_cookie(request: &ServiceRequest) -> anyhow::Result { Ok(nonce_state.nonce) } -fn get_redirect_url_from_cookie( +fn get_redirect_url_cookie( request: &ServiceRequest, csrf_token: &CsrfToken, -) -> anyhow::Result { - let cookie_name = format!("{}{}", SQLPAGE_STATE_COOKIE_PREFIX, csrf_token.secret()); +) -> Result, anyhow::Error> { + let cookie_name = format!( + "{}{}", + SQLPAGE_REDIRECT_URL_COOKIE_PREFIX, + csrf_token.secret() + ); let cookie = request .cookie(&cookie_name) .with_context(|| format!("No {cookie_name} cookie found"))?; - let redirect_state: OidcRedirectState = serde_json::from_str(cookie.value()) - .with_context(|| format!("Failed to parse redirect state from cookie: {cookie}"))?; - Ok(redirect_state.initial_url) + Ok(cookie) } /// Given an audience, verify if it is trusted. The `client_id` is always trusted, independently of this function. From 98bf6e1c24607faa877fc5d38c01090d2c23b843 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 00:19:53 +0200 Subject: [PATCH 04/10] Update OIDC redirect response to use header constant and simplify cookie creation --- src/webserver/oidc.rs | 36 +++++++----------------------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 6dafc4d4..29a4394d 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -6,6 +6,7 @@ use std::{future::Future, pin::Pin, str::FromStr, sync::Arc}; use crate::webserver::http_client::get_http_client_from_appdata; use crate::{app_config::AppConfig, AppState}; +use actix_web::http::header; use actix_web::{ body::BoxBody, cookie::Cookie, @@ -520,7 +521,7 @@ async fn build_auth_provider_redirect_response( let nonce_cookie = create_nonce_cookie(¶ms.nonce); let redirect_cookie = create_redirect_cookie(¶ms.csrf_token, initial_url); HttpResponse::TemporaryRedirect() - .append_header(("Location", url.to_string())) + .append_header((header::LOCATION, url.to_string())) .cookie(nonce_cookie) .cookie(redirect_cookie) .body("Redirecting...") @@ -794,48 +795,27 @@ fn nonce_matches(id_token_nonce: &Nonce, state_nonce: &Nonce) -> Result<(), Stri Ok(()) } -#[derive(Debug, Serialize, Deserialize)] -struct OidcNonceState { - #[serde(rename = "n")] - nonce: Nonce, -} - -#[derive(Debug, Serialize, Deserialize)] -struct OidcRedirectState { - #[serde(rename = "u")] - initial_url: String, -} - fn create_nonce_cookie(nonce: &Nonce) -> Cookie<'_> { - let nonce_state = OidcNonceState { - nonce: nonce.clone(), - }; - let nonce_json = serde_json::to_string(&nonce_state).unwrap(); - Cookie::build(SQLPAGE_NONCE_COOKIE_NAME, nonce_json) + Cookie::build(SQLPAGE_NONCE_COOKIE_NAME, nonce.secret()) .secure(true) .http_only(true) .same_site(actix_web::cookie::SameSite::Lax) .path("/") - .max_age(actix_web::cookie::time::Duration::minutes(10)) .finish() } -fn create_redirect_cookie(csrf_token: &CsrfToken, initial_url: &str) -> Cookie<'static> { - let redirect_state = OidcRedirectState { - initial_url: initial_url.to_string(), - }; - let redirect_json = serde_json::to_string(&redirect_state).unwrap(); +fn create_redirect_cookie<'a>(csrf_token: &CsrfToken, initial_url: &'a str) -> Cookie<'a> { let cookie_name = format!( "{}{}", SQLPAGE_REDIRECT_URL_COOKIE_PREFIX, csrf_token.secret() ); - Cookie::build(cookie_name, redirect_json) + Cookie::build(cookie_name, initial_url) .secure(true) .http_only(true) .same_site(actix_web::cookie::SameSite::Lax) .path("/") - .max_age(actix_web::cookie::time::Duration::minutes(5)) + .max_age(actix_web::cookie::time::Duration::minutes(10)) .finish() } @@ -843,9 +823,7 @@ fn get_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"))?; - let nonce_state: OidcNonceState = serde_json::from_str(cookie.value()) - .with_context(|| format!("Failed to parse nonce from cookie: {cookie}"))?; - Ok(nonce_state.nonce) + Ok(Nonce::new(cookie.value().to_string())) } fn get_redirect_url_cookie( From 0f5ff8cf63ff1253935d75e360154a7597713927 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 00:34:55 +0200 Subject: [PATCH 05/10] Refactor nonce retrieval in OIDC user info handling to simplify logic and ensure proper token claims processing --- src/webserver/oidc.rs | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 29a4394d..72b21c02 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -546,20 +546,10 @@ async fn get_authenticated_user_info( .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; // Try to get nonce from cookies if this is a callback request - let nonce = if request.path() == SQLPAGE_REDIRECT_URI { - if let Ok(_params) = Query::::from_query(request.query_string()) { - get_nonce_from_cookie(request).ok() - } else { - None - } - } else { - None - }; + let nonce = get_nonce_from_cookie(request)?; log::debug!("Verifying id token: {id_token:?}"); - let claims = oidc_state - .get_token_claims(id_token, nonce.as_ref()) - .await?; + let claims = oidc_state.get_token_claims(id_token, Some(&nonce)).await?; log::debug!("The current user is: {claims:?}"); Ok(Some(claims)) } From f9d477fb39ed75635ad206f980bbeb21ba875cf8 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 00:49:00 +0200 Subject: [PATCH 06/10] Refactor OIDC cookie handling to introduce a constant for cookie expiration and simplify nonce validation logic --- src/webserver/oidc.rs | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 72b21c02..4046c6c1 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -17,7 +17,6 @@ use actix_web::{ }; use anyhow::{anyhow, Context}; use awc::Client; -use chrono::Utc; use openidconnect::core::{ CoreAuthDisplay, CoreAuthPrompt, CoreErrorResponseType, CoreGenderClaim, CoreJsonWebKey, CoreJweContentEncryptionAlgorithm, CoreJwsSigningAlgorithm, CoreRevocableToken, @@ -45,6 +44,8 @@ const SQLPAGE_NONCE_COOKIE_NAME: &str = "sqlpage_oidc_nonce"; const SQLPAGE_REDIRECT_URL_COOKIE_PREFIX: &str = "sqlpage_oidc_redirect_url_"; 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); #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(transparent)] @@ -211,7 +212,7 @@ impl OidcState { async fn get_token_claims( &self, id_token: OidcToken, - expected_nonce: Option<&Nonce>, + expected_nonce: &Nonce, ) -> anyhow::Result { let client = &self.get_client().await; let verifier = self.config.create_id_token_verifier(client); @@ -452,9 +453,7 @@ async fn process_oidc_callback( 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, oidc_state) - .await - .context("Failed to set auth cookie")?; + set_auth_cookie(&mut response, &token_response).context("Failed to set auth cookie")?; // Clean up the state-specific cookie after successful authentication response.add_removal_cookie(&redirect_url_cookie)?; @@ -477,10 +476,9 @@ async fn exchange_code_for_token( Ok(token_response) } -async fn set_auth_cookie( +fn set_auth_cookie( response: &mut HttpResponse, token_response: &OidcTokenResponse, - oidc_state: &OidcState, ) -> anyhow::Result<()> { let access_token = token_response.access_token(); log::trace!("Received access token: {}", access_token.secret()); @@ -488,10 +486,6 @@ async fn set_auth_cookie( .id_token() .context("No ID token found in the token response. You may have specified an oauth2 provider that does not support OIDC.")?; - let claims_res = oidc_state.get_token_claims(id_token.clone(), None).await; - let expiration = claims_res.context("Parsing ID token claims")?.expiration(); - let max_age_seconds = expiration.signed_duration_since(Utc::now()).num_seconds(); - 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; @@ -504,7 +498,7 @@ async fn set_auth_cookie( let cookie = Cookie::build(SQLPAGE_AUTH_COOKIE_NAME, id_token_str) .secure(true) .http_only(true) - .max_age(actix_web::cookie::time::Duration::seconds(max_age_seconds)) + .max_age(AUTH_COOKIE_EXPIRATION) .same_site(actix_web::cookie::SameSite::Lax) .path("/") .finish(); @@ -545,11 +539,9 @@ async fn get_authenticated_user_info( let id_token = OidcToken::from_str(&cookie_value) .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; - // Try to get nonce from cookies if this is a callback request let nonce = get_nonce_from_cookie(request)?; - log::debug!("Verifying id token: {id_token:?}"); - let claims = oidc_state.get_token_claims(id_token, Some(&nonce)).await?; + let claims = oidc_state.get_token_claims(id_token, &nonce).await?; log::debug!("The current user is: {claims:?}"); Ok(Some(claims)) } @@ -750,15 +742,9 @@ fn hash_nonce(nonce: &Nonce) -> String { hash.to_string() } -fn check_nonce( - id_token_nonce: Option<&Nonce>, - expected_nonce: Option<&Nonce>, -) -> Result<(), String> { - let Some(expected) = expected_nonce else { - return Ok(()); // No expected nonce, no validation needed - }; +fn check_nonce(id_token_nonce: Option<&Nonce>, expected_nonce: &Nonce) -> Result<(), String> { match id_token_nonce { - Some(id_token_nonce) => nonce_matches(id_token_nonce, expected), + Some(id_token_nonce) => nonce_matches(id_token_nonce, expected_nonce), None => Err("No nonce found in the ID token".to_string()), } } @@ -790,6 +776,7 @@ fn create_nonce_cookie(nonce: &Nonce) -> Cookie<'_> { .secure(true) .http_only(true) .same_site(actix_web::cookie::SameSite::Lax) + .max_age(AUTH_COOKIE_EXPIRATION) .path("/") .finish() } From 6d6f5034869bdbae7ea62cd4937b4e35b7ce2bfa Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 09:57:42 +0200 Subject: [PATCH 07/10] Refactor OIDC redirect URL cookie retrieval to streamline logic and improve error handling --- src/webserver/oidc.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 4046c6c1..4f0a22e1 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -806,16 +806,12 @@ fn get_nonce_from_cookie(request: &ServiceRequest) -> anyhow::Result { fn get_redirect_url_cookie( request: &ServiceRequest, csrf_token: &CsrfToken, -) -> Result, anyhow::Error> { - let cookie_name = format!( - "{}{}", - SQLPAGE_REDIRECT_URL_COOKIE_PREFIX, - csrf_token.secret() - ); - let cookie = request +) -> anyhow::Result> { + let state = csrf_token.secret(); + let cookie_name = format!("{SQLPAGE_REDIRECT_URL_COOKIE_PREFIX}{state}"); + request .cookie(&cookie_name) - .with_context(|| format!("No {cookie_name} cookie found"))?; - Ok(cookie) + .with_context(|| format!("No {cookie_name} cookie found")) } /// Given an audience, verify if it is trusted. The `client_id` is always trusted, independently of this function. From 95462083a6c8baadbde4f0eeeebc750a3016b2bc Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 09:59:19 +0200 Subject: [PATCH 08/10] Refactor OIDC redirect URL cookie name construction for improved clarity --- src/webserver/oidc.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 4f0a22e1..f5e157aa 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -807,8 +807,7 @@ fn get_redirect_url_cookie( request: &ServiceRequest, csrf_token: &CsrfToken, ) -> anyhow::Result> { - let state = csrf_token.secret(); - let cookie_name = format!("{SQLPAGE_REDIRECT_URL_COOKIE_PREFIX}{state}"); + let cookie_name = SQLPAGE_REDIRECT_URL_COOKIE_PREFIX.to_owned() + csrf_token.secret(); request .cookie(&cookie_name) .with_context(|| format!("No {cookie_name} cookie found")) From 10a895d0a1e3af5544ec6328a17fd2eeeb1435b9 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 11:11:38 +0200 Subject: [PATCH 09/10] fix redirect auth cookie removal Update OIDC redirect URL cookie path to ensure proper handling after authentication --- 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 f5e157aa..174c6026 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -442,7 +442,7 @@ async fn process_oidc_callback( })? .into_inner(); - let redirect_url_cookie = get_redirect_url_cookie(request, ¶ms.state) + 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; @@ -456,6 +456,7 @@ async fn process_oidc_callback( 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("/"); response.add_removal_cookie(&redirect_url_cookie)?; Ok(response) From ef2f0828863f0c4a260039df9dad82038d74a042 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 19 Sep 2025 11:20:10 +0200 Subject: [PATCH 10/10] Refactor OIDC redirect URL cookie name construction for improved efficiency --- src/webserver/oidc.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 174c6026..8a5dc47d 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -783,11 +783,7 @@ fn create_nonce_cookie(nonce: &Nonce) -> Cookie<'_> { } fn create_redirect_cookie<'a>(csrf_token: &CsrfToken, initial_url: &'a str) -> Cookie<'a> { - let cookie_name = format!( - "{}{}", - SQLPAGE_REDIRECT_URL_COOKIE_PREFIX, - csrf_token.secret() - ); + let cookie_name = SQLPAGE_REDIRECT_URL_COOKIE_PREFIX.to_owned() + csrf_token.secret(); Cookie::build(cookie_name, initial_url) .secure(true) .http_only(true)