From 09b6c2ba8d652ff210e75331ea43842aef39a440 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 10 Jul 2026 08:54:03 +0200 Subject: [PATCH 1/2] Prevent caching of OIDC redirects --- CHANGELOG.md | 1 + src/webserver/oidc.rs | 4 +++ tests/oidc/mod.rs | 82 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 448e0b80..d25915e2 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 redirects are no longer cacheable.** Authorization redirects contain one-time state and post-login redirects set session cookies. SQLPage now sends `Cache-Control: no-store` for these responses, preventing a browser or intermediary from replaying an expired authorization redirect. ## v0.44.1 diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 0723d5c9..9173aff1 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -870,6 +870,9 @@ fn build_auth_provider_redirect_response( .finish(); let mut response = HttpResponse::SeeOther(); response.append_header((header::LOCATION, url.to_string())); + // The location contains a one-time CSRF state. A cached redirect would + // replay it after its state cookie has been consumed. + response.append_header((header::CACHE_CONTROL, "no-store")); if let Ok(cookies) = request.cookies() { for mut cookie in get_tmp_login_flow_state_cookies_to_evict(&cookies).cloned() { cookie.make_removal(); @@ -884,6 +887,7 @@ fn build_auth_provider_redirect_response( fn build_redirect_response(target_url: String) -> HttpResponse { HttpResponse::SeeOther() .append_header(("Location", target_url)) + .append_header((header::CACHE_CONTROL, "no-store")) .body("Redirecting...") } diff --git a/tests/oidc/mod.rs b/tests/oidc/mod.rs index af32ad22..9e88dbcd 100644 --- a/tests/oidc/mod.rs +++ b/tests/oidc/mod.rs @@ -264,6 +264,17 @@ fn get_query_param(url: &Url, name: &str) -> String { .to_string() } +fn permits_storage_after_proxy_adds_freshness(headers: &header::HeaderMap) -> bool { + // The supplied HAR has `Cache-Control: max-age=86400` on the OIDC 303s, + // despite SQLPage not setting it. This models a proxy adding that directive: + // `no-store` still wins when both directives are present. + !headers + .get_all(header::CACHE_CONTROL) + .filter_map(|value| value.to_str().ok()) + .flat_map(|value| value.split(',')) + .any(|directive| directive.trim().eq_ignore_ascii_case("no-store")) +} + macro_rules! request_with_cookies { ($app:expr, $req:expr, $cookies:expr) => {{ let mut req = $req; @@ -325,6 +336,67 @@ async fn setup_oidc_test( (app, provider) } +#[actix_web::test] +async fn test_oidc_cached_authorization_redirect_cannot_replay_consumed_state() { + let (app, provider) = setup_oidc_test(|_| {}).await; + let mut cookies: Vec> = Vec::new(); + + // Chrome's private cache can replay a cached 303 without reapplying its + // Set-Cookie headers. This is the sequence captured in #1341's HAR. + let initial_response = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies); + let initial_auth_url = Url::parse( + initial_response + .headers() + .get(header::LOCATION) + .unwrap() + .to_str() + .unwrap(), + ) + .unwrap(); + let cached_authorization_url = + permits_storage_after_proxy_adds_freshness(initial_response.headers()) + .then_some(initial_auth_url.clone()); + + let state = get_query_param(&initial_auth_url, "state"); + let nonce = get_query_param(&initial_auth_url, "nonce"); + let redirect_uri = get_query_param(&initial_auth_url, "redirect_uri"); + let callback_path = Url::parse(&redirect_uri).unwrap().path().to_owned(); + provider.store_auth_code("first-code".to_string(), nonce.clone()); + let callback_uri = format!("{callback_path}?code=first-code&state={state}"); + let callback_response = + request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies); + assert_eq!(callback_response.status(), StatusCode::SEE_OTHER); + + if let Some(stale_authorization_url) = cached_authorization_url { + // A fresh Entra code is returned for the authorization URL cached by + // Chrome, but its state is the state that the successful callback just + // consumed. Before this fix, SQLPage restarted OIDC here, creating the + // observed redirect loop. + let stale_state = get_query_param(&stale_authorization_url, "state"); + provider.store_auth_code("stale-code".to_string(), nonce); + let stale_callback_uri = format!("{callback_path}?code=stale-code&state={stale_state}"); + let stale_callback_response = request_with_cookies!( + app, + test::TestRequest::get().uri(&stale_callback_uri), + cookies + ); + panic!( + "a cacheable authorization redirect replays a consumed state; stale callback redirected to {}", + stale_callback_response + .headers() + .get(header::LOCATION) + .unwrap() + .to_str() + .unwrap() + ); + } + + // With `no-store`, Chrome re-requests `/` after login and sends its new + // auth cookie instead of following the original authorization redirect. + let final_response = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies); + assert_eq!(final_response.status(), StatusCode::OK); +} + #[actix_web::test] async fn test_oidc_happy_path() { let (app, provider) = setup_oidc_test(|_| {}).await; @@ -332,6 +404,11 @@ async fn test_oidc_happy_path() { let resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies); assert_eq!(resp.status(), StatusCode::SEE_OTHER); + assert_eq!( + resp.headers().get(header::CACHE_CONTROL).unwrap(), + "no-store", + "the authorization redirect contains a one-time state and must not be cached" + ); let auth_url = Url::parse(resp.headers().get("location").unwrap().to_str().unwrap()).unwrap(); let state = get_query_param(&auth_url, "state"); @@ -347,6 +424,11 @@ async fn test_oidc_happy_path() { let callback_resp = request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies); assert_eq!(callback_resp.status(), StatusCode::SEE_OTHER); + assert_eq!( + callback_resp.headers().get(header::CACHE_CONTROL).unwrap(), + "no-store", + "the post-login redirect must not re-enter a cached authorization redirect" + ); let final_resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies); assert_eq!(final_resp.status(), StatusCode::OK); From 65f2d5cefced3a5ba9dc35ba01e38b3be3e991bb Mon Sep 17 00:00:00 2001 From: Ophir LOJKINE Date: Fri, 10 Jul 2026 11:33:15 +0200 Subject: [PATCH 2/2] Clarify OIDC cache replay test comments --- tests/oidc/mod.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/oidc/mod.rs b/tests/oidc/mod.rs index 9e88dbcd..7e605353 100644 --- a/tests/oidc/mod.rs +++ b/tests/oidc/mod.rs @@ -265,9 +265,9 @@ fn get_query_param(url: &Url, name: &str) -> String { } fn permits_storage_after_proxy_adds_freshness(headers: &header::HeaderMap) -> bool { - // The supplied HAR has `Cache-Control: max-age=86400` on the OIDC 303s, - // despite SQLPage not setting it. This models a proxy adding that directive: - // `no-store` still wins when both directives are present. + // Reproduces https://github.com/sqlpage/SQLPage/issues/1341, where an + // intermediary added `Cache-Control: max-age=86400` to OIDC 303 responses. + // `no-store` must still prevent browser storage when both directives exist. !headers .get_all(header::CACHE_CONTROL) .filter_map(|value| value.to_str().ok()) @@ -341,8 +341,9 @@ async fn test_oidc_cached_authorization_redirect_cannot_replay_consumed_state() let (app, provider) = setup_oidc_test(|_| {}).await; let mut cookies: Vec> = Vec::new(); - // Chrome's private cache can replay a cached 303 without reapplying its - // Set-Cookie headers. This is the sequence captured in #1341's HAR. + // Reproduces https://github.com/sqlpage/SQLPage/issues/1341: Chrome's + // private cache can replay a cached 303 without reapplying Set-Cookie + // headers, causing the browser to retry an already-consumed OIDC state. let initial_response = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies); let initial_auth_url = Url::parse( initial_response