mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
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.
This commit is contained in:
1 parent
51d114a8da
commit
6363125fec
3 files changed
+260
-33
No files matched your search
@@ -2261,9 +2261,14 @@ fn mount_once(mountpoint: Option<PathBuf>) -> Result<pdfs_fuse::MountOutcome> {
|
||||
},
|
||||
);
|
||||
|
||||
// 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).
|
||||
|
||||
+219
-30
@@ -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<BTreeMap<String, Arc<TokenSink>>> =
|
||||
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<SinkState>,
|
||||
}
|
||||
|
||||
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<Tokens>,
|
||||
}
|
||||
|
||||
/// 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::<StoredSession>(
|
||||
&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");
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user