From 6363125fec5da377ad50c9768bc47fc74dc4933f Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Fri, 9 Oct 2026 11:08:40 +0200 Subject: [PATCH] fix(core,cli): write rotated tokens only into their own session's blob Proton refresh tokens are single-use. store_tokens replaced the tokens of whatever blob was stored, so after `pdfs login` restarted the daemon, the old daemon's shutdown persist wrote its spent tokens into the new session's blob, and the new daemon failed with InvalidRefreshToken (issue #35). `pdfs unlock` and the legacy migration had the same shape. Each session now writes through a TokenSink that only touches a blob with its session id that still holds the refresh token it last left there. Shutdown only flushes a rotation the keyring refused. unlock and migrate_legacy re-read the blob after verifying. docs/BUGS.md B201. --- crates/pdfs-cli/src/main.rs | 11 +- crates/pdfs-core/src/auth.rs | 249 ++++++++++++++++++++++++++++++----- docs/BUGS.md | 33 +++++ 3 files changed, 260 insertions(+), 33 deletions(-) diff --git a/crates/pdfs-cli/src/main.rs b/crates/pdfs-cli/src/main.rs index 7f8d9a9..42873b4 100644 --- a/crates/pdfs-cli/src/main.rs +++ b/crates/pdfs-cli/src/main.rs @@ -2261,9 +2261,14 @@ fn mount_once(mountpoint: Option) -> Result { }, ); - // Clean unmount check (as a best-effort final backup). - if let Err(e) = rt.block_on(auth::persist(&session)) { - tracing::debug!(error = %e, "no new tokens to persist on shutdown"); + // Last chance for a rotation the keyring refused when it happened. Only + // that: by now the keyring may hold a newer login, the one this stop was + // restarted for, and the tokens of this session would overwrite it. + if let Err(e) = auth::persist(&session) { + tracing::error!( + error = %e, + "could not save the refreshed session tokens; the next start may need `pdfs login`" + ); } // Dropping the runtime waits for every blocking task, however long; a call // still stuck on Drive would hold the stop until systemd kills it (B165). diff --git a/crates/pdfs-core/src/auth.rs b/crates/pdfs-core/src/auth.rs index a03b66e..160da38 100644 --- a/crates/pdfs-core/src/auth.rs +++ b/crates/pdfs-core/src/auth.rs @@ -21,6 +21,7 @@ //! such separation to protect and are stored, as they always were. use std::collections::BTreeMap; +use std::sync::Arc; use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64; @@ -31,6 +32,7 @@ use proton_sdk::api::{HumanVerification, HumanVerificationCredential}; use proton_sdk::cache::EncryptedCacheRepository; use proton_sdk::config::ProtonClientConfiguration; use proton_sdk::error::ProtonError; +use proton_sdk::http::Tokens; use proton_sdk::session::{PasswordMode, ProtonApiSession, ResumeParameters}; use proton_sdk::telemetry::TracingTelemetry; use serde::{Deserialize, Serialize}; @@ -307,8 +309,8 @@ pub async fn login_verified( } }; - register_refresh_handler(&session); let tokens = session.current_tokens().await; + register_refresh_handler(&session, tokens.refresh_token.clone()); let stored = StoredSession { session_id: session.session_id().as_str().to_owned(), username: session.username().to_owned(), @@ -492,10 +494,12 @@ pub async fn unlock(mailbox: &str, remember: bool) -> Result<()> { } let mailbox = Zeroizing::new(mailbox.to_owned()); let session = ProtonApiSession::resume(client_config(), stored.to_params())?; - register_refresh_handler(&session); + register_refresh_handler(&session, stored.refresh_token.clone()); let passphrases = derive_verified(&session, mailbox.as_bytes(), stored.key_salts.clone()).await?; - keep_passphrases(stored, &passphrases, remember) + // Read again: verifying may have rotated the tokens, and the copy read + // above would put the spent refresh token back. + keep_passphrases(load()?, &passphrases, remember) } /// Drop the passphrases held in kernel memory, so the account is locked again @@ -552,7 +556,7 @@ pub fn logout() -> Result<()> { pub async fn resume_client() -> Result<(ProtonDriveClient, ProtonApiSession)> { let stored = load()?; let session = ProtonApiSession::resume(client_config(), stored.to_params())?; - register_refresh_handler(&session); + register_refresh_handler(&session, stored.refresh_token.clone()); let passphrases = match find_passphrases(&stored)? { Some((passphrases, _)) => passphrases, @@ -597,11 +601,14 @@ async fn migrate_legacy( stored.key_salts.clone() }; - let mut migrated = stored.clone(); + let verified = derive_verified(session, legacy.as_bytes(), key_salts.clone()).await; + // Read again: the calls above may have rotated the tokens, and `stored` + // would put the spent refresh token back. + let mut migrated = load()?; migrated.mailbox_password = String::new(); - migrated.key_salts = key_salts.clone(); + migrated.key_salts = key_salts; - match derive_verified(session, legacy.as_bytes(), key_salts).await { + match verified { Ok(passphrases) => { migrated.key_passphrases = Some(encode(&passphrases)); write_stored(&migrated)?; @@ -659,37 +666,147 @@ fn tune(client: ProtonDriveClient, passphrases: &KeyPassphrases) -> ProtonDriveC } } -/// Write the session's current tokens back to the keyring. +/// Write any rotation of the session's tokens that has not reached the keyring +/// yet. A best-effort last chance before the process exits. /// -/// Everything else in the stored blob — salts, passphrases — is preserved, so -/// this never undoes an [`unlock`] or [`lock`] made since the session started. -/// Call whenever the session may have rotated its tokens so a later -/// [`resume_client`] presents a live refresh token. -pub async fn persist(session: &ProtonApiSession) -> Result<()> { - let tokens = session.current_tokens().await; - store_tokens(tokens.access_token, tokens.refresh_token) +/// Rotations are written as they happen (see [`register_refresh_handler`]), so +/// this usually has nothing to do. It never writes the session's current +/// tokens for their own sake: the blob may by now belong to a newer login, or +/// hold a newer rotation, and writing them would put a spent refresh token +/// over a live one. +pub fn persist(session: &ProtonApiSession) -> Result<()> { + let sink = TOKEN_SINKS + .lock() + .get(session.session_id().as_str()) + .cloned(); + match sink { + Some(sink) => sink.flush(), + None => Ok(()), + } } -fn store_tokens(access_token: String, refresh_token: String) -> Result<()> { - let mut stored = load()?; - stored.access_token = access_token; - stored.refresh_token = refresh_token; - write_stored(&stored) +/// Where each live session in this process writes its rotated tokens, by +/// session id, for [`persist`] to find. +static TOKEN_SINKS: parking_lot::Mutex>> = + parking_lot::Mutex::new(BTreeMap::new()); + +/// One session's rotated tokens on their way to the keyring. +/// +/// Proton refresh tokens are single-use, so a rotation has to reach the +/// keyring, and it must reach only the blob it belongs to. That is the blob of +/// this session id that still holds the refresh token this session last knew +/// it to hold. Anything else means the blob moved on without this session: a +/// new login, or another session on the same login (`pdfs unlock`) that +/// rotated first. Writing into it would replace a live refresh token with a +/// spent one, and the next start fails with `InvalidRefreshToken` (issue #35). +struct TokenSink { + session_id: String, + state: parking_lot::Mutex, +} + +struct SinkState { + /// The refresh token the blob holds for this session, as far as this + /// session knows: the one it started from, or the last one it wrote. + stored_refresh: String, + /// The latest rotation, until the keyring has taken it. + unsaved: Option, +} + +/// What became of a rotation offered to a stored blob. +#[derive(Debug, PartialEq, Eq)] +enum Rotation { + /// The blob is this session's and took the tokens. + Taken, + /// The blob moved on without this session; it is left alone. + MovedOn, +} + +/// Put `tokens` into `stored` if it is still this session's blob: the same +/// session id, holding the refresh token this session last left in it. +fn take_rotation( + stored: &mut StoredSession, + session_id: &str, + stored_refresh: &str, + tokens: &Tokens, +) -> Rotation { + if stored.session_id != session_id || stored.refresh_token != stored_refresh { + return Rotation::MovedOn; + } + stored.access_token = tokens.access_token.clone(); + stored.refresh_token = tokens.refresh_token.clone(); + Rotation::Taken +} + +impl TokenSink { + fn new(session_id: &str, stored_refresh: String) -> Self { + Self { + session_id: session_id.to_owned(), + state: parking_lot::Mutex::new(SinkState { + stored_refresh, + unsaved: None, + }), + } + } + + /// Take a new rotation and try to write it. + fn rotated(&self, tokens: Tokens) -> Result<()> { + self.state.lock().unsaved = Some(tokens); + self.flush() + } + + /// Write the unsaved rotation, if there is one. A blob that moved on, or + /// is gone with a logout, drops it: there is nothing it may still go into. + fn flush(&self) -> Result<()> { + let mut state = self.state.lock(); + let Some(tokens) = state.unsaved.clone() else { + return Ok(()); + }; + let mut stored = match load() { + Ok(stored) => stored, + Err(Error::NotLoggedIn) => { + state.unsaved = None; + return Ok(()); + } + Err(e) => return Err(e), + }; + match take_rotation( + &mut stored, + &self.session_id, + &state.stored_refresh, + &tokens, + ) { + Rotation::Taken => { + write_stored(&stored)?; + state.stored_refresh = tokens.refresh_token; + tracing::info!("successfully auto-persisted refreshed tokens in keyring"); + } + Rotation::MovedOn => { + tracing::info!( + "the stored session has moved on; not writing this session's tokens over it" + ); + } + } + state.unsaved = None; + Ok(()) + } } /// Persist rotated tokens the moment the session obtains them. /// -/// The handler re-reads the stored blob instead of capturing one, so it cannot -/// write back a stale copy over a later unlock or lock — and cannot resurrect a -/// session that was logged out while the daemon still ran. -fn register_refresh_handler(session: &ProtonApiSession) { +/// `stored_refresh` is the refresh token the keyring holds for this session +/// now. The handler re-reads the stored blob instead of capturing one, so it +/// cannot write back a stale copy over a later unlock or lock, and it writes +/// only into a blob that is still this session's (see [`TokenSink`]), so it +/// cannot resurrect a logged-out session or overwrite a newer login. +fn register_refresh_handler(session: &ProtonApiSession, stored_refresh: String) { + let session_id = session.session_id().as_str(); + let sink = Arc::new(TokenSink::new(session_id, stored_refresh)); + TOKEN_SINKS + .lock() + .insert(session_id.to_owned(), Arc::clone(&sink)); session.http().set_on_tokens_refreshed(move |tokens| { - match store_tokens(tokens.access_token, tokens.refresh_token) { - Ok(()) => tracing::info!("successfully auto-persisted refreshed tokens in keyring"), - Err(Error::NotLoggedIn) => {} - Err(e) => { - tracing::warn!(error = %e, "failed to auto-persist refreshed tokens in keyring") - } + if let Err(e) = sink.rotated(tokens) { + tracing::warn!(error = %e, "failed to auto-persist refreshed tokens in keyring") } }); } @@ -786,4 +903,76 @@ mod tests { assert_eq!(origin, KeyState::Stored); assert_eq!(found, passphrases()); } + + fn tokens(access: &str, refresh: &str) -> Tokens { + Tokens { + access_token: access.into(), + refresh_token: refresh.into(), + } + } + + #[test] + fn own_session_tokens_are_written() { + let mut stored: StoredSession = serde_json::from_str(&blob("")).unwrap(); + assert_eq!( + take_rotation(&mut stored, "s", "r", &tokens("a2", "r2")), + Rotation::Taken + ); + assert_eq!(stored.access_token, "a2"); + assert_eq!(stored.refresh_token, "r2"); + } + + /// Issue #35: `pdfs login` stores a new session and restarts the daemon, + /// and the old daemon's stop wrote its spent tokens into the new blob. + #[test] + fn tokens_of_another_session_are_not_written() { + let mut stored: StoredSession = serde_json::from_str(&blob("")).unwrap(); + assert_eq!( + take_rotation(&mut stored, "old", "r", &tokens("a2", "r2")), + Rotation::MovedOn + ); + assert_eq!(stored.refresh_token, "r"); + } + + /// `pdfs unlock` resumes a second session on the same login. Once it has + /// rotated, the daemon's copy of the refresh token is spent. + #[test] + fn tokens_are_not_written_over_a_newer_rotation_of_the_same_session() { + let mut stored: StoredSession = serde_json::from_str(&blob("")).unwrap(); + assert_eq!( + take_rotation(&mut stored, "s", "spent", &tokens("a2", "r2")), + Rotation::MovedOn + ); + assert_eq!(stored.refresh_token, "r"); + } + + /// The only test that touches the keyring, through `keyring-core`'s + /// in-memory store. + #[test] + fn a_sink_keeps_writing_its_own_rotations_and_stops_at_a_new_login() { + keyring_core::set_default_store(keyring_core::mock::Store::new().unwrap()); + let stored = |refresh: &str| { + serde_json::from_str::( + &blob("").replace(r#""r""#, &format!("{refresh:?}")), + ) + .unwrap() + }; + write_stored(&stored("r")).unwrap(); + + let sink = TokenSink::new("s", "r".into()); + sink.rotated(tokens("a2", "r2")).unwrap(); + sink.rotated(tokens("a3", "r3")).unwrap(); + assert_eq!(load().unwrap().refresh_token, "r3"); + + // A new login replaces the blob; the old session's next rotation, and + // its stop, leave it alone. + let mut login = stored("fresh"); + login.session_id = "new".into(); + write_stored(&login).unwrap(); + sink.rotated(tokens("a4", "r4")).unwrap(); + sink.flush().unwrap(); + let now = load().unwrap(); + assert_eq!(now.session_id, "new"); + assert_eq!(now.refresh_token, "fresh"); + } } diff --git a/docs/BUGS.md b/docs/BUGS.md index 4451403..f34ef2a 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -101,6 +101,7 @@ a live account since. | Entry | Title | |---|---| +| [B201](#b201--after-signing-in-again-the-old-daemon-puts-its-spent-tokens-back) | After signing in again, the old daemon puts its spent tokens back | | [B200](#b200--offline-with-changes-queued-the-daemon-keeps-several-cores-busy) | Offline with changes queued, the daemon keeps several cores busy | | [B198](#b198--switching-to-online-only-deletes-local-files-that-never-reached-drive) | Switching to online-only deletes local files that never reached Drive | | [B197](#b197--switching-to-online-only-trashes-synced-files-that-became-ignored) | Switching to online-only trashes synced files that became ignored | @@ -303,6 +304,38 @@ a live account since. Newest first. +### B201 — After signing in again, the old daemon puts its spent tokens back + +**Status:** Fixed, not verified live +**Found:** 2026-10-09, reading the code for issue #35, where every call failed with +`InvalidRefreshToken` and signing in again did not obviously help. +**Where:** `crates/pdfs-core/src/auth.rs`, `persist`, `register_refresh_handler`, `unlock` and +`migrate_legacy`; `crates/pdfs-cli/src/main.rs`, `cmd_daemon` + +**Repro:** With the daemon running, run `pdfs login`. It stores the new session and restarts the +unit. The old daemon's stop runs `auth::persist`, which wrote its own access and refresh tokens +into the blob, now the new session's. The new daemon starts with a refresh token of another +session, and the first refresh fails with `InvalidRefreshToken`. `pdfs unlock` had the same +shape: its session could rotate the tokens, and the restarted daemon's stop put the spent refresh +token back. `unlock` and `migrate_legacy` could do it themselves, writing a copy of the blob read +before verifying the mailbox password, which may have rotated the tokens. + +**Cause.** Proton refresh tokens are single-use. `store_tokens` loaded whatever blob was stored +and replaced its tokens, without checking that the blob was still this session's. + +**Fix.** Each session writes its rotations through a `TokenSink`. It writes only into a blob with +its session id that still holds the refresh token the session last left there; a blob that moved +on, to a new login or to another session's rotation, is left alone. The stop no longer writes the +session's tokens, only a rotation the keyring refused when it happened. `unlock` and +`migrate_legacy` read the blob again after verifying. A failed final write is an error in the log, +not a debug line claiming there was nothing to save. + +**Test:** `auth::tests::tokens_of_another_session_are_not_written`, +`auth::tests::tokens_are_not_written_over_a_newer_rotation_of_the_same_session`, +`auth::tests::a_sink_keeps_writing_its_own_rotations_and_stops_at_a_new_login`. + +--- + ### B200 — Offline with changes queued, the daemon keeps several cores busy **Status:** Fixed, not verified live