Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
lovasoa marked this conversation as resolved.

## v0.44.1

Expand Down
4 changes: 4 additions & 0 deletions src/webserver/oidc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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...")
}

Expand Down
82 changes: 82 additions & 0 deletions tests/oidc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Comment on lines +272 to +273

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Call into_iter before filtering header values

When compiling this new OIDC test, HeaderMap::get_all(...) returns a GetAll collection rather than an Iterator, so chaining .filter_map(...) here fails with E0599. Add .iter() or .into_iter() before filter_map; otherwise the OIDC test target and CI cannot build.

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;
Expand Down Expand Up @@ -325,13 +336,79 @@ 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();

// 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;
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");
Expand All @@ -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);
Expand Down