fix(auth): two Set-Cookie headers were collapsing into one; clear the spent transaction
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=<ms>` and `?signed_out=<ms>`). 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
This commit is contained in:
parent
4a704acc02
commit
43737941cb
|
|
@ -479,3 +479,56 @@ mod tests {
|
||||||
assert!(!u.contains(".one//oauth"), "{u}");
|
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());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -9302,12 +9302,19 @@ async fn oauth_callback(
|
||||||
};
|
};
|
||||||
tracing::info!(sub = %principal.subject, "browser sign-in complete");
|
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::StatusCode::FOUND,
|
||||||
[
|
// AppendHeaders, NOT an array: the array form REPLACES same-name
|
||||||
(axum::http::header::LOCATION, "/ui/".to_string()),
|
// 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, session_cookie),
|
||||||
],
|
(axum::http::header::SET_COOKIE, bs::clear_transaction(secure)),
|
||||||
|
]),
|
||||||
)
|
)
|
||||||
.into_response()
|
.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
|
// Local only: forgets this browser's session. Revoking the Cognitum session
|
||||||
// for every device is an account-level action at auth.cognitum.one.
|
// for every device is an account-level action at auth.cognitum.one.
|
||||||
let secure = request_is_tls(&headers);
|
let secure = request_is_tls(&headers);
|
||||||
|
use wifi_densepose_sensing_server::browser_session as bs;
|
||||||
(
|
(
|
||||||
axum::http::StatusCode::FOUND,
|
axum::http::StatusCode::FOUND,
|
||||||
[
|
axum::response::AppendHeaders([
|
||||||
(axum::http::header::LOCATION, "/ui/".to_string()),
|
// 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::SET_COOKIE,
|
(axum::http::header::LOCATION, format!("/ui/?signed_out={}", now_millis())),
|
||||||
wifi_densepose_sensing_server::browser_session::clear_session(secure),
|
(axum::http::header::SET_COOKIE, bs::clear_session(secure)),
|
||||||
),
|
(axum::http::header::SET_COOKIE, bs::clear_transaction(secure)),
|
||||||
],
|
]),
|
||||||
)
|
)
|
||||||
.into_response()
|
.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.
|
/// `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
|
/// Deliberately ungated and deliberately thin: capability flags and, if a live
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue