-
-
Notifications
You must be signed in to change notification settings - Fork 175
Prevent caching OIDC redirects #1344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
| // 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()) | ||
|
Comment on lines
+272
to
+273
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When compiling this new OIDC test, Useful? React with 👍 / 👎. |
||
| .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,13 +336,80 @@ 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<Cookie<'static>> = Vec::new(); | ||
|
|
||
| // 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 | ||
| .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; | ||
| let mut cookies: Vec<Cookie<'static>> = Vec::new(); | ||
|
|
||
| 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 +425,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); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.