diff --git a/crates/pdfs-core/src/auth.rs b/crates/pdfs-core/src/auth.rs index 160da38..f9ac0d8 100644 --- a/crates/pdfs-core/src/auth.rs +++ b/crates/pdfs-core/src/auth.rs @@ -22,6 +22,8 @@ use std::collections::BTreeMap; use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64; @@ -702,8 +704,21 @@ static TOKEN_SINKS: parking_lot::Mutex>> = struct TokenSink { session_id: String, state: parking_lot::Mutex, + /// Whether a thread is retrying a write the keyring refused. + retrying: AtomicBool, } +/// How long to wait before each new try at a write the keyring refused; the +/// last delay repeats until it succeeds. A locked or restarting keyring +/// usually comes back within seconds, and one that does not must not be asked +/// in a tight loop. +const KEYRING_RETRY: [Duration; 4] = [ + Duration::from_secs(1), + Duration::from_secs(5), + Duration::from_secs(30), + Duration::from_secs(300), +]; + 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. @@ -745,13 +760,59 @@ impl TokenSink { stored_refresh, unsaved: None, }), + retrying: AtomicBool::new(false), } } - /// Take a new rotation and try to write it. - fn rotated(&self, tokens: Tokens) -> Result<()> { + /// Take a new rotation and try to write it. If the keyring refuses, keep + /// trying in the background: until then the only live refresh token is in + /// memory, and a stop that cannot write it either loses the session + /// (`docs/BUGS.md` B61). + fn rotated(self: &Arc, tokens: Tokens) -> Result<()> { self.state.lock().unsaved = Some(tokens); - self.flush() + let written = self.flush(); + if written.is_err() { + self.retry_in_background(); + } + written + } + + /// Retry the unsaved rotation on [`KEYRING_RETRY`] until it is written or + /// has nowhere left to go. One thread per sink; a newer rotation simply + /// replaces what it is retrying. + fn retry_in_background(self: &Arc) { + if self.retrying.swap(true, Ordering::SeqCst) { + return; + } + let sink = Arc::clone(self); + let spawned = std::thread::Builder::new() + .name("pdfs-keyring-retry".into()) + .spawn(move || { + loop { + let delays = KEYRING_RETRY + .into_iter() + .chain(std::iter::repeat(KEYRING_RETRY[KEYRING_RETRY.len() - 1])); + for delay in delays { + std::thread::sleep(delay); + match sink.flush() { + Ok(()) => break, + Err(e) => tracing::debug!(error = %e, ?delay, "keyring still refuses"), + } + } + sink.retrying.store(false, Ordering::SeqCst); + // A rotation refused after the last try succeeded found + // this thread still running, and started none of its own. + if sink.state.lock().unsaved.is_none() + || sink.retrying.swap(true, Ordering::SeqCst) + { + return; + } + } + }); + if let Err(e) = spawned { + self.retrying.store(false, Ordering::SeqCst); + tracing::error!(error = %e, "could not start retrying the keyring write"); + } } /// Write the unsaved rotation, if there is one. A blob that moved on, or @@ -806,7 +867,11 @@ fn register_refresh_handler(session: &ProtonApiSession, stored_refresh: String) .insert(session_id.to_owned(), Arc::clone(&sink)); session.http().set_on_tokens_refreshed(move |tokens| { if let Err(e) = sink.rotated(tokens) { - tracing::warn!(error = %e, "failed to auto-persist refreshed tokens in keyring") + tracing::error!( + error = %e, + "the keyring refused the refreshed session tokens; retrying. \ + If the daemon stops first, the next start needs `pdfs login`" + ) } }); } @@ -946,27 +1011,32 @@ mod tests { assert_eq!(stored.refresh_token, "r"); } - /// The only test that touches the keyring, through `keyring-core`'s - /// in-memory store. + /// The keyring is one store per process, so the tests that use it, + /// through `keyring-core`'s in-memory store, take turns. + fn mock_keyring() -> parking_lot::MutexGuard<'static, ()> { + static KEYRING: parking_lot::Mutex<()> = parking_lot::Mutex::new(()); + let turn = KEYRING.lock(); + keyring_core::set_default_store(keyring_core::mock::Store::new().unwrap()); + turn + } + + fn stored_with(refresh: &str) -> StoredSession { + serde_json::from_str(&blob("").replace(r#""r""#, &format!("{refresh:?}"))).unwrap() + } + #[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 _turn = mock_keyring(); + write_stored(&stored_with("r")).unwrap(); - let sink = TokenSink::new("s", "r".into()); + let sink = Arc::new(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"); + let mut login = stored_with("fresh"); login.session_id = "new".into(); write_stored(&login).unwrap(); sink.rotated(tokens("a4", "r4")).unwrap(); @@ -975,4 +1045,32 @@ mod tests { assert_eq!(now.session_id, "new"); assert_eq!(now.refresh_token, "fresh"); } + + /// B61: a rotation the keyring refused lived only in memory. + #[test] + fn a_rotation_the_keyring_refused_is_written_once_it_takes_it() { + let _turn = mock_keyring(); + write_stored(&stored_with("r")).unwrap(); + let sink = Arc::new(TokenSink::new("s", "r".into())); + + let entry = keyring_entry().unwrap(); + let cred = entry + .as_any() + .downcast_ref::() + .unwrap(); + cred.set_error(keyring_core::Error::NoStorageAccess( + std::io::Error::other("locked").into(), + )); + assert!(sink.rotated(tokens("a2", "r2")).is_err()); + + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while load().unwrap().refresh_token != "r2" { + assert!( + std::time::Instant::now() < deadline, + "the retry never wrote the rotation" + ); + std::thread::sleep(Duration::from_millis(50)); + } + assert!(sink.state.lock().unsaved.is_none()); + } } diff --git a/docs/BUGS.md b/docs/BUGS.md index be5b103..b26c705 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -73,7 +73,6 @@ original wording. | [B67](#b67--fresh-state-restore-can-silently-select-a-new-device-high-22) | Fresh-state restore can silently select a new device (HIGH-22) | An ambiguous device after a loss of local state is not asked about | | [B66](#b66--local-only-pending-data-is-not-protected-at-shutdown-high-21) | Local-only pending data is not protected at shutdown (HIGH-21) | No shutdown protection and no `pdfs sync flush` | | [B65](#b65--release-artifacts-lack-supply-chain-verification-med-11) | Release artifacts lack supply-chain verification (MED-11) | No SBOM, provenance or checksums | -| [B61](#b61--rotated-single-use-credentials-can-be-lost-high-18) | Rotated single-use credentials can be lost (HIGH-18) | Keyring write failures are only logged | | [B15](#b15--an-empty-but-listed-folder-and-duplicated-folder-uids) | An empty-but-`listed` folder, and duplicated folder uids | Cause not attributed | | [B10](#b10--glib-critical-when-the-launcher-closes-over-an-in-flight-open) | GLib critical when the launcher closes over an in-flight open | | @@ -88,6 +87,7 @@ original wording. | [B68](#b68--profile-backup-cannot-be-written-to-the-device-root) | Profile backup cannot be written to the device root | Backup health is not shown in the app; the restore was not re-run | | [B64](#b64--stable-publishing-lacks-data-safety-and-recovery-gates-crit-12) | Stable publishing lacks data-safety and recovery gates (CRIT-12) | Recovery approval before a release | | [B63](#b63--release-version-identity-is-inconsistent-and-unenforced-high-20) | Release version identity is inconsistent and unenforced (HIGH-20) | Protocol version identity | +| [B61](#b61--rotated-single-use-credentials-can-be-lost-high-18) | Rotated single-use credentials can be lost (HIGH-18) | A crash or SIGKILL before a retried write lands still loses it; the GUI is not told | | [B57](#b57--event-cursor-can-advance-past-unapplied-state-high-14) | Event cursor can advance past unapplied state (HIGH-14) | Database and apply fault tests | | [B56](#b56--database-schema-version-parsing-fails-open-high-13) | Database schema version parsing fails open (HIGH-13) | Malformed schema versions | | [B12](#b12--cold-enumeration-is-slow-per-entry-and-goes-superlinear-past-500) | Cold enumeration is slow per entry, and goes superlinear past ~500 | The attribute-invalidation follow-up is unverified | @@ -4921,7 +4921,8 @@ daemon/tray/FUSE, upgrade, rollback, and uninstall without deleting user state. ### B61 — Rotated single-use credentials can be lost (HIGH-18) -**Status:** Open — 1.0 authentication/recovery blocker +**Status:** Partly fixed. A process that dies before a retried write lands still loses the +rotation, and only the log says so. **Found:** 2026-07-22, 1.0 authentication audit **Where:** `crates/pdfs-core/src/auth.rs`, refresh callback @@ -4933,6 +4934,15 @@ keyring retains an invalid single-use refresh token, so restart loses login. notify the user, and define a safe re-login/shutdown path. Test locked/full/ unavailable keyrings, callback races, repeated rotations, death, and restart. +**Fix (2026-10-09, issue #35).** A rotation the keyring refuses is kept as unsaved and retried +in the background after 1 s, 5 s, 30 s and then every 5 minutes, until it is written or the blob +has moved on (B201). The refusal is an error in the log naming `pdfs login`, and the daemon's stop +writes a rotation still unsaved, logging an error if that fails too. The re-login path is B201's: +an old session never writes into a newer login. Left: a crash or SIGKILL before a retry lands, +and telling the GUI. + +**Test:** `auth::tests::a_rotation_the_keyring_refused_is_written_once_it_takes_it`. + --- ### B60 — Config writes are non-atomic and parse errors are overwritten (HIGH-17)