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