From 43737941cb448c33bb766431f65d1f1f28315952 Mon Sep 17 00:00:00 2001 From: Dragan Spiridonov Date: Thu, 23 Jul 2026 10:46:43 +0200 Subject: [PATCH] fix(auth): two Set-Cookie headers were collapsing into one; clear the spent transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs, one of which I introduced while fixing the other and caught only by checking the actual response. 1. THE SPENT TRANSACTION COOKIE WAS NEVER CLEARED ON SUCCESS. `clear_transaction` was only used on error paths, so after a successful sign-in `ruview_oauth_txn` lingered for its full 10-minute TTL and every subsequent request carried a dead cookie. Visible in the logs as `names=ruview_oauth_txn,ruview_session`. Now cleared alongside issuing the session, and on logout too. 2. THE FIX FOR (1) BROKE SIGN-IN, AND THE TEST CAUGHT IT. Axum's array-of-tuples response form REPLACES same-name headers rather than appending. Adding a second `Set-Cookie` silently overwrote the first, so the logout response emitted only the transaction clear and — had this shipped — the CALLBACK would have emitted only the transaction clear too, dropping the session cookie and making a successful OAuth round-trip a no-op. Caught by looking at the real response headers rather than trusting the change: `curl -D-` showed one Set-Cookie where there should have been two. Now uses `axum::response::AppendHeaders`. Both cookies verified present. Two tests pin this: one DOCUMENTS the footgun (the array form collapses two Set-Cookie headers into one) and one asserts AppendHeaders emits both. The first exists so the next person to reach for the tidier-looking array form finds out here instead of in production. Also adds a cache-busting query to both redirect targets (`/ui/?signed_in=` and `?signed_out=`). Landing on the same URL let the browser restore the page from the back/forward cache with a stale panel, which is why signing in appeared not to work until a hard refresh — the session was established correctly every time, the page simply was not re-fetched. Tests: 549 sensing-server lib. Co-Authored-By: Ruflo & AQE --- .../src/browser_session.rs | 53 +++++++++++++++++++ .../wifi-densepose-sensing-server/src/main.rs | 35 ++++++++---- 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs index d2663668..4146834f 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs @@ -479,3 +479,56 @@ mod tests { assert!(!u.contains(".one//oauth"), "{u}"); } } + +/// Regression guard for a response-shape mistake that silently broke sign-in. +#[cfg(test)] +mod response_shape_tests { + use axum::response::IntoResponse; + + /// Axum's array-of-tuples form REPLACES same-name headers. Two `Set-Cookie` + /// entries collapse to one — which, on the sign-in callback, dropped the + /// session cookie and made a successful OAuth round-trip a no-op. Only the + /// last cookie survived. + #[test] + fn an_array_of_headers_silently_drops_a_duplicate_set_cookie() { + let resp = ( + axum::http::StatusCode::FOUND, + [ + (axum::http::header::SET_COOKIE, "a=1".to_string()), + (axum::http::header::SET_COOKIE, "b=2".to_string()), + ], + ) + .into_response(); + assert_eq!( + resp.headers() + .get_all(axum::http::header::SET_COOKIE) + .iter() + .count(), + 1, + "documenting the footgun: the array form replaces, it does not append" + ); + } + + /// `AppendHeaders` is what actually emits both. + #[test] + fn append_headers_emits_every_set_cookie() { + let resp = ( + axum::http::StatusCode::FOUND, + axum::response::AppendHeaders([ + (axum::http::header::LOCATION, "/ui/".to_string()), + (axum::http::header::SET_COOKIE, "a=1".to_string()), + (axum::http::header::SET_COOKIE, "b=2".to_string()), + ]), + ) + .into_response(); + let cookies: Vec<_> = resp + .headers() + .get_all(axum::http::header::SET_COOKIE) + .iter() + .filter_map(|v| v.to_str().ok()) + .collect(); + assert_eq!(cookies.len(), 2, "both cookies must reach the browser"); + assert!(cookies.contains(&"a=1") && cookies.contains(&"b=2")); + assert!(resp.headers().get(axum::http::header::LOCATION).is_some()); + } +} diff --git a/v2/crates/wifi-densepose-sensing-server/src/main.rs b/v2/crates/wifi-densepose-sensing-server/src/main.rs index 147dd721..7766b82e 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/main.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/main.rs @@ -9302,12 +9302,19 @@ async fn oauth_callback( }; tracing::info!(sub = %principal.subject, "browser sign-in complete"); + // Clear the spent transaction as well as issuing the session. A consumed + // OAuth transaction has no further use, and leaving it to age out for ten + // minutes means every subsequent request carries a dead cookie. ( axum::http::StatusCode::FOUND, - [ - (axum::http::header::LOCATION, "/ui/".to_string()), + // AppendHeaders, NOT an array: the array form REPLACES same-name + // headers, so a second Set-Cookie silently overwrites the first — which + // would drop the session cookie and make sign-in a no-op. + axum::response::AppendHeaders([ + (axum::http::header::LOCATION, format!("/ui/?signed_in={}", now_millis())), (axum::http::header::SET_COOKIE, session_cookie), - ], + (axum::http::header::SET_COOKIE, bs::clear_transaction(secure)), + ]), ) .into_response() } @@ -9317,19 +9324,27 @@ async fn oauth_logout(headers: axum::http::HeaderMap) -> axum::response::Respons // Local only: forgets this browser's session. Revoking the Cognitum session // for every device is an account-level action at auth.cognitum.one. let secure = request_is_tls(&headers); + use wifi_densepose_sensing_server::browser_session as bs; ( axum::http::StatusCode::FOUND, - [ - (axum::http::header::LOCATION, "/ui/".to_string()), - ( - axum::http::header::SET_COOKIE, - wifi_densepose_sensing_server::browser_session::clear_session(secure), - ), - ], + axum::response::AppendHeaders([ + // Cache-busting query so the landing page is re-fetched rather than + // restored from the back/forward cache with a stale panel. + (axum::http::header::LOCATION, format!("/ui/?signed_out={}", now_millis())), + (axum::http::header::SET_COOKIE, bs::clear_session(secure)), + (axum::http::header::SET_COOKIE, bs::clear_transaction(secure)), + ]), ) .into_response() } +fn now_millis() -> u128 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis()) + .unwrap_or(0) +} + /// `GET /oauth/status` — what a signed-out browser needs to render the right UI. /// /// Deliberately ungated and deliberately thin: capability flags and, if a live