From 91fd2cbb393591690fbf86192452440a499d7cce Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Thu, 3 Sep 2026 14:50:06 -0400 Subject: [PATCH] test(auth): race two processes refreshing one identity The deployment atgc is actually used in, and the one case with nothing behind it: several agents in several worktrees sharing one account and one $HOME, all acting at once. A refresh token is single-use and rotates, so two processes that both read the session, both find it expired and both present the same token get one success and one `invalid_grant` -- which jacquard classifies permanent and deletes the session over, logging the account out from a command that reported success. The mock now rotates refresh tokens and refuses a spent one, so a double spend is detectable rather than invisible. Without the refresh lock the test reproduces the incident exactly: "session refresh failed permanently". Co-Authored-By: Claude Opus 5 (1M context) Change-Id: Iceea5a5108f49b75c0169e073b6f054e73accff4 --- tests/login_layout.rs | 72 +++++++++++++++++++++++++++++++++++++++ tests/support/services.rs | 32 ++++++++++++++--- tests/support/world.rs | 12 +++++++ 3 files changed, 112 insertions(+), 4 deletions(-) diff --git a/tests/login_layout.rs b/tests/login_layout.rs index 7484340..15372cb 100644 --- a/tests/login_layout.rs +++ b/tests/login_layout.rs @@ -581,3 +581,75 @@ fn a_did_web_login_lands_under_its_method() { "the did:web account it just created has no token" ); } + +/// Two processes refreshing one identity do not both spend its refresh token. +/// +/// **The deployment atgc is actually used in**: several agents in several +/// worktrees, sharing one account and one `$HOME`, all acting at once. A +/// refresh token is single-use and rotates, so two processes that both read +/// the session, both find it expired and both present the same token produce +/// one success and one `invalid_grant` — which jacquard classifies permanent +/// and deletes the session over. The account is then logged out, by a command +/// that reported success. +/// +/// This is what the lock is for, and why it is taken *around the read* rather +/// than around the token request: the loser of the race re-reads inside the +/// critical section, finds the token the winner just wrote, and returns +/// without spending anything. +#[test] +fn two_processes_refreshing_one_identity_do_not_double_spend() { + let world = Scenario::new("login-contention"); + let config = world.config_path(""); + std::fs::remove_dir_all(&config).expect("clear the fixture's config"); + std::fs::create_dir_all(&config).expect("an empty config directory"); + + let dir = log_in(&world); + let shard = dir.join("session.json"); + + // Expire the access token. Both processes will find it stale. + let mut store: serde_json::Value = + serde_json::from_slice(&std::fs::read(&shard).unwrap()).unwrap(); + let key = store.as_object().unwrap().keys().next().unwrap().clone(); + store[&key]["ClientSession"]["expires_at"] = serde_json::json!("2020-01-01T00:00:00Z"); + std::fs::write(&shard, serde_json::to_vec_pretty(&store).unwrap()).unwrap(); + + // Both started before either is waited on, so they overlap. + let first = world + .command(&["auth", "token"]) + .env("ATGC_ACCOUNT", ALICE) + .start(); + let second = world + .command(&["auth", "token"]) + .env("ATGC_ACCOUNT", ALICE) + .start(); + let first = first.finish(); + let second = second.finish(); + + assert_eq!( + (first.code, second.code), + (Some(0), Some(0)), + "a concurrent refresh failed\n--- first ---\n{}\n--- second ---\n{}", + first.stderr, + second.stderr + ); + + // The session is still there. A double spend deletes it, so its absence + // is the failure this guards rather than a detail of it. + let after: serde_json::Value = serde_json::from_slice( + &std::fs::read(&shard).expect("the session survived two concurrent refreshes"), + ) + .expect("json"); + assert!( + !after.as_object().expect("an object").is_empty(), + "two concurrent refreshes emptied the session store:\n{after:#}" + ); + + // And exactly one refresh was spent: the loser re-read inside the lock + // and found the winner's token rather than presenting the old one. + let spent = world.with(|w| w.spent_refresh.len()); + assert_eq!( + spent, 1, + "both processes spent a refresh token; the second would have been \ + answered invalid_grant by a real server" + ); +} diff --git a/tests/support/services.rs b/tests/support/services.rs index 0316fdb..6e96e8b 100644 --- a/tests/support/services.rs +++ b/tests/support/services.rs @@ -188,10 +188,33 @@ fn oauth(world: &mut World, req: &Incoming, host_did: &str) -> Option { }), ) } - "oauth/token" => Reply::ok(serde_json::json!({ - "access_token": "access-token-for-alice.test", + "oauth/token" => { + let body = String::from_utf8_lossy(&req.body).into_owned(); + let field = |name: &str| { + body.split('&').find_map(|pair| { + pair.strip_prefix(&format!("{name}=")) + .map(|v| v.replace('+', " ")) + }) + }; + // A refresh token is single-use. Presenting a spent one is what a + // real authorization server answers `invalid_grant` to, and what + // two unserialised refreshes of one session produce. + if field("grant_type").as_deref() == Some("refresh_token") + && let Some(token) = field("refresh_token") + && !world.spent_refresh.insert(token.clone()) + { + return Some(Reply::Error( + hyper::StatusCode::BAD_REQUEST, + "invalid_grant", + format!("refresh token {token} has already been spent"), + )); + } + world.tokens_issued += 1; + let n = world.tokens_issued; + Reply::ok(serde_json::json!({ + "access_token": format!("access-token-{n}"), "token_type": "DPoP", - "refresh_token": "refresh-token-for-alice.test", + "refresh_token": format!("refresh-token-{n}"), "expires_in": 3600, "sub": did, // What was asked for at the pushed request, so the scope @@ -200,7 +223,8 @@ fn oauth(world: &mut World, req: &Incoming, host_did: &str) -> Option { .granted_scope .clone() .unwrap_or_else(|| "atproto transition:generic".to_string()), - })), + })) + } _ => return None, }) } diff --git a/tests/support/world.rs b/tests/support/world.rs index 1620abe..efe2de8 100644 --- a/tests/support/world.rs +++ b/tests/support/world.rs @@ -207,6 +207,16 @@ pub struct World { /// the same mock in a test, and `sub` is what the client keys everything /// it writes by. pub login_as: Option, + /// Refresh tokens already spent. atproto rotates them and they are + /// single-use, so presenting one twice is `invalid_grant` — which + /// jacquard classifies permanent and *deletes the session* over. A mock + /// that reissued the same token could not tell a serialised pair of + /// refreshes from a double spend, which is the whole failure the lock + /// exists to prevent. + pub spent_refresh: BTreeSet, + /// How many tokens this authorization server has issued, so each one is + /// distinguishable. + pub tokens_issued: u32, pub pds_down: BTreeSet, /// Make the PDS fail on one method, for anybody. /// @@ -244,6 +254,8 @@ impl World { pds_base: BTreeMap::new(), granted_scope: None, login_as: None, + spent_refresh: BTreeSet::new(), + tokens_issued: 0, tokens: BTreeMap::new(), journal: Vec::new(), repo_owners: BTreeMap::new(), -- knotmirror 0.0.1