From 930d6fbdc9ab3f03ce8e74e8c83f0c81c5469984 Mon Sep 17 00:00:00 2001 From: Nick <40026523+Nick-CHI@users.noreply.github.com> Date: Tue, 8 Sep 2026 19:12:36 +0800 Subject: [PATCH] fix(auth-core): make the fixation revoke fire on the form_post path (#36) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `issue` revokes the session the browser was carrying before minting a new one, and it identified that session by the raw cookie the request presented. On Apple's `form_post` callback the session cookie is `SameSite=Lax` and is simply not sent with a cross-site POST, so `presented_cookie` was always `None` and the revoke could never fire for that provider. #44 documented the gap; this closes it. Option 1 from the issue, which it called probably right: - `Login` gains `presented_session_id`, used when `presented_cookie` is `None`. The cookie stays the stronger evidence and wins when both are present. - `/start` — which IS same-site, so the cookie arrives there — seals the session **id** into the signed flow next to the user id it already sealed, and the callback hands it back. The id, not the value, and that is the point of the design: the value is a bearer credential and a second copy of it inside another cookie widens its exposure for nothing, while an id is useless without the row. Adding the field to `Login` deliberately breaks all thirteen call sites rather than defaulting: whether a login can name the session it replaces is a decision each path should make. The four same-site paths (password login and registration, magic-link consume, passkey login) pass `None` and say why — their cookie arrives. One test, on the path that actually reaches `issue`: two Apple sign-ins, the first session carried into the second `/start`, asserting the old cookie no longer validates and its row is marked revoked rather than merely orphaned. Verified it fails with either half of the chain broken — the core ignoring the id, and `/start` not sealing it. Meta gets the same wiring for symmetry, but its callback is a GET redirect, so its cookie already arrives and the cookie branch wins there; this is a fallback that should not normally fire. ADR 0102 gains an update note. The paragraph describing the gap is left intact: it is an accurate record of the state between #35 and now. 397 tests, clippy -D warnings, fmt, wasm32 workspace build. Closes #36. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01V8K9TBtmnf29dV6uEP5oM3 --- crates/auth-core/src/federated.rs | 4 + crates/auth-core/src/sessions.rs | 64 ++++++++++---- crates/auth-core/tests/authorize.rs | 1 + crates/auth-core/tests/sessions.rs | 5 ++ crates/auth-core/tests/tokens.rs | 1 + crates/auth-magic-link/src/handlers.rs | 3 + crates/auth-meta/src/flow.rs | 11 +++ crates/auth-meta/src/handlers.rs | 13 ++- crates/auth-oidc/src/flow.rs | 11 +++ crates/auth-oidc/src/handlers.rs | 13 ++- crates/auth-oidc/src/session.rs | 5 ++ crates/auth-oidc/tests/apple.rs | 100 ++++++++++++++++++++++ crates/auth-passkeys/src/login.rs | 3 + crates/auth-passkeys/tests/support/kit.rs | 1 + crates/auth-password/src/handlers.rs | 6 ++ docs/adr/0102-sign-in-with-apple.md | 7 ++ 16 files changed, 223 insertions(+), 25 deletions(-) diff --git a/crates/auth-core/src/federated.rs b/crates/auth-core/src/federated.rs index 78a927c..55f906e 100644 --- a/crates/auth-core/src/federated.rs +++ b/crates/auth-core/src/federated.rs @@ -50,6 +50,9 @@ pub struct Caller<'a> { /// The session cookie the request presented, revoked before a new /// session exists (the fixation defence in [`issue`]). pub presented_cookie: Option<&'a str>, + /// The id of that session, for a `form_post` callback where the cookie + /// itself cannot arrive. See [`Login::presented_session_id`]. + pub presented_session_id: Option<&'a str>, pub ip: Option<&'a str>, pub user_agent: Option<&'a str>, } @@ -200,6 +203,7 @@ pub async fn complete( ip: caller.ip, user_agent: caller.user_agent, presented_cookie: caller.presented_cookie, + presented_session_id: caller.presented_session_id, amr, }, ) diff --git a/crates/auth-core/src/sessions.rs b/crates/auth-core/src/sessions.rs index 3c975ba..9167483 100644 --- a/crates/auth-core/src/sessions.rs +++ b/crates/auth-core/src/sessions.rs @@ -175,10 +175,24 @@ pub struct Login<'a> { pub ip: Option<&'a str>, pub user_agent: Option<&'a str>, /// The raw cookie the login request presented, if any: the session it - /// names is revoked before the new one exists (fixation). `None` on a - /// `form_post` callback, where the `SameSite=Lax` cookie does not arrive, - /// so the revoke does not fire there (see [`issue`]; auth #36). + /// names is revoked before the new one exists (fixation). pub presented_cookie: Option<&'a str>, + /// The **id** of the session the browser had, for the paths where the + /// cookie itself cannot arrive. + /// + /// On a `form_post` callback the session cookie is `SameSite=Lax` and + /// does not come with a cross-site POST, so `presented_cookie` is + /// always `None` there and the revoke above could never fire (auth + /// #36). The provider modules seal this id into the signed flow at + /// `/start`, which *is* same-site, and hand it back here. + /// + /// It is the id and not the value on purpose: the value is a bearer + /// credential, and putting a second copy of it inside another cookie + /// widens its exposure for nothing. An id is useless without the row. + /// + /// Ignored when `presented_cookie` is `Some` — that path already + /// identifies the same session, from the stronger evidence. + pub presented_session_id: Option<&'a str>, /// Authentication-method references (RFC 8176) for this login — /// `passkey` methods say `["user","passkey"]`-style values when the /// login-method issues wire them (issues #13-#22). Stored as the @@ -208,19 +222,23 @@ fn audit(action: &str, user_id: &str) { /// Issues a session on a successful login: 32 random bytes in the /// cookie, only the SHA-256 stored, expiry `now + 30 days`. /// -/// **Fixation defence, and its one blind spot.** A session whose raw -/// cookie the request presents ([`Login::presented_cookie`]) is revoked -/// first, so a fixed pre-login value dies here. This does not fire on a -/// `form_post` callback (Apple): the session cookie is `SameSite=Lax` and -/// does not arrive on a cross-site POST, so `presented_cookie` is `None` -/// there. That is not a hole — the cookie is `__Host-` and `HttpOnly`, so -/// it cannot be planted cross-site, which is the attack the revoke exists -/// for; the browser has also just overwritten it with the new value. The -/// only residue is the old server-side row living until it expires. Firing -/// the revoke on every path needs the session **id** sealed into the flow -/// at `/start` and a `presented_session_id` on [`Login`] -/// ([auth #36](https://github.com/Factory-Zero/auth/issues/36)); until then, -/// do not read this as "always revokes". +/// **Fixation defence.** The session the browser was carrying is revoked +/// before the new one exists, so a fixed pre-login value dies here. It is +/// identified either by the raw cookie the request presented +/// ([`Login::presented_cookie`]) or, where that cannot arrive, by the id +/// the caller carried across ([`Login::presented_session_id`]). +/// +/// The second exists because a `form_post` callback is a cross-site POST +/// and the session cookie is `SameSite=Lax`, so it is simply not sent: for +/// Apple and Meta the cookie path can never fire (auth #36). Those modules +/// seal the session id into their signed flow at `/start`, which is +/// same-site, and pass it back here. +/// +/// Revoking is deliberate rather than incidental. The browser overwrites +/// its own cookie either way, so what this buys is the server-side row not +/// outliving the login that replaced it — a session listing that tells the +/// truth, and a revoked row that cannot be resurrected by anyone who +/// captured the old value earlier. /// /// # Errors /// @@ -248,10 +266,18 @@ pub async fn issue( } } - if let Some(presented) = login.presented_cookie - && let Some(old) = + // The cookie is the stronger evidence, so it wins where both are + // present; the id is the fallback for the paths a cookie cannot reach. + let superseded = match login.presented_cookie { + Some(presented) => { store::session_by_token_hash(db, &sha256_raw(presented.as_bytes())).await? - { + } + None => match login.presented_session_id { + Some(id) => store::session_by_id(db, id).await?, + None => None, + }, + }; + if let Some(old) = superseded { store::revoke_session(db, &old.id, &iso(clock.now())).await?; audit("session.revoke", login.user_id); } diff --git a/crates/auth-core/tests/authorize.rs b/crates/auth-core/tests/authorize.rs index 1265370..1615365 100644 --- a/crates/auth-core/tests/authorize.rs +++ b/crates/auth-core/tests/authorize.rs @@ -154,6 +154,7 @@ async fn signed_in(kit: &TestHarness, user_id: &str) -> String { ip: None, user_agent: None, presented_cookie: None, + presented_session_id: None, amr: &[], }, ) diff --git a/crates/auth-core/tests/sessions.rs b/crates/auth-core/tests/sessions.rs index 31bb86f..e5be2ec 100644 --- a/crates/auth-core/tests/sessions.rs +++ b/crates/auth-core/tests/sessions.rs @@ -78,6 +78,7 @@ fn a_session_is_never_issued_to_an_account_that_is_not_active() { ip: None, user_agent: None, presented_cookie: None, + presented_session_id: None, amr: &["passkey"], }, ) @@ -98,6 +99,7 @@ fn a_session_is_never_issued_to_an_account_that_is_not_active() { ip: None, user_agent: None, presented_cookie: None, + presented_session_id: None, amr: &["passkey"], }, ) @@ -127,6 +129,7 @@ async fn login( ip: Some("203.0.113.7"), user_agent: Some("Mozilla/5.0 Macintosh Safari/605.1.15"), presented_cookie: presented, + presented_session_id: None, amr: &["passkey"], }, ) @@ -439,6 +442,7 @@ async fn the_expired_shape_through_the_router_is_the_same_401() { ip: None, user_agent: None, presented_cookie: None, + presented_session_id: None, amr: &[], }, ) @@ -472,6 +476,7 @@ async fn listing_shows_ip_hash_ua_family_and_the_current_flag() { ip: Some("198.51.100.9"), user_agent: Some("Mozilla/5.0 Windows Chrome/120.0 Safari/537.36"), presented_cookie: None, + presented_session_id: None, amr: &[], }, ) diff --git a/crates/auth-core/tests/tokens.rs b/crates/auth-core/tests/tokens.rs index 189056f..45383d4 100644 --- a/crates/auth-core/tests/tokens.rs +++ b/crates/auth-core/tests/tokens.rs @@ -121,6 +121,7 @@ async fn seed_session( ip: Some("203.0.113.7"), user_agent: Some("Mozilla/5.0 Macintosh Safari/605.1.15"), presented_cookie: None, + presented_session_id: None, amr, }, ) diff --git a/crates/auth-magic-link/src/handlers.rs b/crates/auth-magic-link/src/handlers.rs index b6deb67..db0b954 100644 --- a/crates/auth-magic-link/src/handlers.rs +++ b/crates/auth-magic-link/src/handlers.rs @@ -521,6 +521,9 @@ async fn spend( .get(header::USER_AGENT) .and_then(|value| value.to_str().ok()), presented_cookie: presented.as_deref(), + // A top-level GET from the mailed link, so the cookie above + // arrives and names the session itself (auth #36). + presented_session_id: None, amr: &AMR, }, ) diff --git a/crates/auth-meta/src/flow.rs b/crates/auth-meta/src/flow.rs index 0825752..624abc4 100644 --- a/crates/auth-meta/src/flow.rs +++ b/crates/auth-meta/src/flow.rs @@ -48,6 +48,16 @@ pub(crate) struct Flow { /// the answer `/start` saw is the one the linking rules asked about. #[serde(default, skip_serializing_if = "Option::is_none")] pub signed_in_user: Option, + /// The id of the session that person was carrying at `/start`. + /// + /// Carried for the same reason as `signed_in_user` and revoked at the + /// callback: the fixation defence in `auth-core`'s `issue` needs to + /// name the superseded session, and on a `form_post` callback the + /// cookie that would name it never arrives (auth #36). The id, not the + /// value — the value is a bearer credential and does not belong in a + /// second cookie. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub signed_in_session_id: Option, } impl Flow { @@ -135,6 +145,7 @@ mod tests { return_to: "/".to_owned(), expires_at, signed_in_user: None, + signed_in_session_id: None, } } diff --git a/crates/auth-meta/src/handlers.rs b/crates/auth-meta/src/handlers.rs index 3bd1ad5..6156961 100644 --- a/crates/auth-meta/src/handlers.rs +++ b/crates/auth-meta/src/handlers.rs @@ -207,14 +207,19 @@ async fn start( }; // Who is signed in now, read here because `/start` is same-site. - let signed_in_user = match (ctx.ports.db.as_deref(), session_cookie_value(&headers)) { + // Both halves of the answer: who it is, and which session row it is, + // so the callback can revoke that row even though the cookie naming it + // will not arrive there (auth #36). + let signed_in = match (ctx.ports.db.as_deref(), session_cookie_value(&headers)) { (Some(db), Some(value)) => factory0_auth_core::validate(db, clock, &value) .await .ok() - .flatten() - .map(|session| session.user_id), + .flatten(), _ => None, }; + let (signed_in_user, signed_in_session_id) = signed_in.map_or((None, None), |session| { + (Some(session.user_id), Some(session.id)) + }); let challenge = PkceCodeChallenge::from_code_verifier_sha256(&PkceCodeVerifier::new(verifier.clone())); @@ -234,6 +239,7 @@ async fn start( .unwrap_or_else(|| settings.default_return_to.clone()), expires_at: clock.now().unix_timestamp() + flow::TTL_SECS, signed_in_user, + signed_in_session_id, } .seal(signer); @@ -389,6 +395,7 @@ async fn callback( &Caller { current_user: current_user.as_deref(), presented_cookie: presented.as_deref(), + presented_session_id: flow.signed_in_session_id.as_deref(), ip: ip.as_deref(), user_agent, }, diff --git a/crates/auth-oidc/src/flow.rs b/crates/auth-oidc/src/flow.rs index cdbe6b3..4a40fa7 100644 --- a/crates/auth-oidc/src/flow.rs +++ b/crates/auth-oidc/src/flow.rs @@ -55,6 +55,16 @@ pub(crate) struct Flow { /// only carries the answer across, inside a payload we signed. #[serde(default, skip_serializing_if = "Option::is_none")] pub signed_in_user: Option, + /// The id of the session that person was carrying at `/start`. + /// + /// Carried for the same reason as `signed_in_user` and revoked at the + /// callback: the fixation defence in `auth-core`'s `issue` needs to + /// name the superseded session, and on a `form_post` callback the + /// cookie that would name it never arrives (auth #36). The id, not the + /// value — the value is a bearer credential and does not belong in a + /// second cookie. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub signed_in_session_id: Option, } impl Flow { @@ -207,6 +217,7 @@ mod tests { return_to: "/v1/auth-core/authorize?x=1".to_owned(), expires_at, signed_in_user: None, + signed_in_session_id: None, } } diff --git a/crates/auth-oidc/src/handlers.rs b/crates/auth-oidc/src/handlers.rs index e9860a1..d72b476 100644 --- a/crates/auth-oidc/src/handlers.rs +++ b/crates/auth-oidc/src/handlers.rs @@ -239,14 +239,19 @@ async fn start( // arrives here even for a provider whose callback will not carry it. // Sealing the answer into the flow is what makes "add this provider to // my account" work for a `form_post` provider at all. - let signed_in_user = match (ctx.ports.db.as_deref(), session_cookie_value(&headers)) { + // Both halves of the answer: who it is, and which session row it is, + // so the callback can revoke that row even though the cookie naming it + // will not arrive there (auth #36). + let signed_in = match (ctx.ports.db.as_deref(), session_cookie_value(&headers)) { (Some(db), Some(value)) => factory0_auth_core::validate(db, clock, &value) .await .ok() - .flatten() - .map(|session| session.user_id), + .flatten(), _ => None, }; + let (signed_in_user, signed_in_session_id) = signed_in.map_or((None, None), |session| { + (Some(session.user_id), Some(session.id)) + }); let sealed = Flow { provider: provider.slug.to_owned(), @@ -257,6 +262,7 @@ async fn start( .unwrap_or_else(|| settings.default_return_to.clone()), expires_at: clock.now().unix_timestamp() + flow::TTL_SECS, signed_in_user, + signed_in_session_id, } .seal(signer); @@ -443,6 +449,7 @@ async fn complete_callback( &session::Caller { current_user: current_user.as_deref(), presented_cookie: presented.as_deref(), + presented_session_id: flow.signed_in_session_id.as_deref(), ip: ip.as_deref(), user_agent, }, diff --git a/crates/auth-oidc/src/session.rs b/crates/auth-oidc/src/session.rs index 4b32395..617b559 100644 --- a/crates/auth-oidc/src/session.rs +++ b/crates/auth-oidc/src/session.rs @@ -50,6 +50,10 @@ pub(crate) struct Caller<'a> { /// The session cookie the request presented, revoked before a new /// session exists (auth-core's fixation defence). pub presented_cookie: Option<&'a str>, + /// The id of that session, sealed into the flow at `/start`. On a + /// `form_post` callback the cookie above never arrives, so this is the + /// only thing that can name the session being superseded (auth #36). + pub presented_session_id: Option<&'a str>, pub ip: Option<&'a str>, pub user_agent: Option<&'a str>, } @@ -86,6 +90,7 @@ pub(crate) async fn complete( &CoreCaller { current_user: caller.current_user, presented_cookie: caller.presented_cookie, + presented_session_id: caller.presented_session_id, ip: caller.ip, user_agent: caller.user_agent, }, diff --git a/crates/auth-oidc/tests/apple.rs b/crates/auth-oidc/tests/apple.rs index b84c7bc..af89503 100644 --- a/crates/auth-oidc/tests/apple.rs +++ b/crates/auth-oidc/tests/apple.rs @@ -411,6 +411,7 @@ fn signing_in_while_signed_in_adds_a_provider_rather_than_a_second_account() { ip: None, user_agent: None, presented_cookie: None, + presented_session_id: None, amr: &["password"], }, ) @@ -464,6 +465,105 @@ fn signing_in_while_signed_in_adds_a_provider_rather_than_a_second_account() { }); } +#[test] +fn the_form_post_callback_revokes_the_session_it_replaces() { + // The fixation defence in `issue` revokes the session the browser was + // carrying, identifying it by the raw cookie the request presented. + // That cookie is `SameSite=Lax`, so it never arrives on Apple's + // cross-site POST: for this provider the revoke could not fire at all, + // and the superseded row stayed live until it expired (auth #36). + // + // `/start` IS same-site, so the session id is sealed into the signed + // flow there and handed back at the callback. Signing in twice with + // Apple, carrying the first session into the second `/start`, is the + // path that exercises it. + pollster::block_on(async { + let kit = apple_kit(); + + // First sign-in: creates the account and a session. + kit.provider.set_claims(TokenClaims::apple()); + let first = start_at(&kit, APPLE_START, "").await; + let response = post_form_with( + &kit, + APPLE_CALLBACK, + &body("code-1", &first.state), + &[("__Host-fz_oidc", &first.flow_cookie)], + ) + .await; + assert_eq!(response.status, StatusCode::FOUND, "{}", response.text()); + let old_value = session_cookie(&response).expect("the first sign-in set a session"); + + let old = factory0_auth_core::validate(&*kit.db, &*kit.clock, &old_value) + .await + .expect("validate runs") + .expect("the first session is valid to begin with"); + + // Second sign-in. `/start` is same-site, so the session cookie + // arrives here and its id is sealed into the flow. + kit.provider.set_claims(TokenClaims::apple()); + let started = { + let response = get(&kit, APPLE_START, &[("__Host-fz_session", &old_value)]).await; + assert_eq!(response.status, StatusCode::FOUND); + let url = response.location().expect("a Location"); + let parsed = url::Url::parse(&url).expect("a url"); + let state = parsed + .query_pairs() + .find(|(key, _)| key == "state") + .map(|(_, value)| value.to_string()) + .expect("a state"); + let nonce = parsed + .query_pairs() + .find(|(key, _)| key == "nonce") + .map(|(_, value)| value.to_string()) + .expect("a nonce"); + kit.provider.set_nonce(&nonce); + ( + response.cookie("__Host-fz_oidc").expect("a flow cookie"), + state, + ) + }; + + // The callback carries NO session cookie. That is the whole point. + let response = post_form_with( + &kit, + APPLE_CALLBACK, + &body("code-2", &started.1), + &[("__Host-fz_oidc", &started.0)], + ) + .await; + assert_eq!(response.status, StatusCode::FOUND, "{}", response.text()); + let new_value = session_cookie(&response).expect("the second sign-in set a session"); + assert_ne!(new_value, old_value, "the same session value came back"); + + // The cookie the browser arrived with is now worthless. + assert!( + factory0_auth_core::validate(&*kit.db, &*kit.clock, &old_value) + .await + .expect("validate runs") + .is_none(), + "the session the login replaced is still valid: the fixation revoke did not fire" + ); + + // Revoked, not merely deleted or expired, and the new one is live. + let rows = factory0_auth_core::sessions_by_user(&*kit.db, &old.user_id) + .await + .expect("sessions read"); + let superseded = rows + .iter() + .find(|row| row.id == old.id) + .expect("the superseded session row still exists"); + assert!( + superseded.revoked_at.is_some(), + "the superseded session row was not marked revoked" + ); + assert!( + rows.iter() + .any(|row| row.id != old.id && row.revoked_at.is_none()), + "the callback issued no live session" + ); + }); +} + #[test] fn a_relay_address_never_links_to_an_existing_account() { // Apple hands out a per-app alias. Two people can hold relay addresses diff --git a/crates/auth-passkeys/src/login.rs b/crates/auth-passkeys/src/login.rs index eec1d27..5fa3549 100644 --- a/crates/auth-passkeys/src/login.rs +++ b/crates/auth-passkeys/src/login.rs @@ -240,6 +240,9 @@ async fn start_session( // Any cookie the login request carried is revoked before the new // session exists, which is auth-core's fixation defence. presented_cookie: factory0_auth_core::cookie_value(headers).as_deref(), + // A same-origin POST from our own page, so the cookie above + // arrives and names the session itself (auth #36). + presented_session_id: None, amr: &amr, }, ) diff --git a/crates/auth-passkeys/tests/support/kit.rs b/crates/auth-passkeys/tests/support/kit.rs index 14561b5..218a6d3 100644 --- a/crates/auth-passkeys/tests/support/kit.rs +++ b/crates/auth-passkeys/tests/support/kit.rs @@ -158,6 +158,7 @@ impl Kit { ip: None, user_agent: None, presented_cookie: None, + presented_session_id: None, amr: &["test"], }, ) diff --git a/crates/auth-password/src/handlers.rs b/crates/auth-password/src/handlers.rs index 167913b..59e0fdf 100644 --- a/crates/auth-password/src/handlers.rs +++ b/crates/auth-password/src/handlers.rs @@ -406,6 +406,9 @@ async fn login( ip: ip.as_deref(), user_agent, presented_cookie: presented.as_deref(), + // A same-origin POST from our own form, so the cookie above + // arrives and names the session itself (auth #36). + presented_session_id: None, amr: &AMR, }, ) @@ -615,6 +618,9 @@ async fn change( .and_then(|value| value.to_str().ok()), // Already revoked above. presented_cookie: None, + // A same-origin POST from our own form, so the cookie above + // arrives and names the session itself (auth #36). + presented_session_id: None, amr: &AMR, }, ) diff --git a/docs/adr/0102-sign-in-with-apple.md b/docs/adr/0102-sign-in-with-apple.md index d24f125..5f60f48 100644 --- a/docs/adr/0102-sign-in-with-apple.md +++ b/docs/adr/0102-sign-in-with-apple.md @@ -151,6 +151,13 @@ untidy rather than exploitable — the cookie is `__Host-` and `HttpOnly`, so it cannot be planted cross-site, which is the attack the revoke exists for. Filed rather than bodged. +> **Update, 2026-09-08 (issue #36).** Fixed, by the first of the two options +> that issue lists. `/start` now seals the session **id** alongside the user +> id, and `Login` takes a `presented_session_id` the callback fills from the +> flow. The value is still never carried: an id is useless without the row, +> where the value is a bearer credential. The paragraph above describes the +> state between #35 and that fix. + ### 6. Private relay addresses were already handled Apple may return a `@privaterelay.appleid.com` address. It is a per-app