From ba5b6a8bf2706b0c0c9a90d0f64db7c84ae012e3 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Sat, 14 Nov 2026 15:13:35 +0100 Subject: [PATCH] fix(core): retain unsaved session tokens Retry token persistence when the keyring reports a missing session. oo7 can hide locked entries, so discarding the pending rotation leaves a spent refresh token in the saved session. Refs #78 --- crates/pdfs-core/src/auth.rs | 55 ++++++++++++++---- docs/BUGS.md | 29 ++++++++++ docs/ISSUE-78.md | 91 ++++++++++++++++++++++++++++++ docs/repro/oo7-0.7.0-issue78.patch | 53 +++++++++++++++++ scripts/publish-wiki.sh | 2 +- 5 files changed, 219 insertions(+), 11 deletions(-) create mode 100644 docs/ISSUE-78.md create mode 100644 docs/repro/oo7-0.7.0-issue78.patch diff --git a/crates/pdfs-core/src/auth.rs b/crates/pdfs-core/src/auth.rs index 5acb3a0..603b9d2 100644 --- a/crates/pdfs-core/src/auth.rs +++ b/crates/pdfs-core/src/auth.rs @@ -996,21 +996,15 @@ impl TokenSink { } } - /// 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. + /// Write the unsaved rotation, if there is one. A blob that moved on drops + /// it. A missing blob is retried: some Secret Service providers also report + /// locked or temporarily hidden items as absent. Never recreate it here. 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.settle(); - return Ok(()); - } - Err(e) => return Err(e), - }; + let mut stored = load()?; match take_rotation( &mut stored, &self.session_id, @@ -1306,6 +1300,47 @@ mod tests { } } + /// oo7 0.7.0 reports a locked collection's items as absent (upstream #614). + /// Treating that as logout loses the only live tokens (B284, issue #78). + #[test] + fn a_temporarily_missing_session_does_not_discard_a_rotation() { + let _turn = mock_keyring(); + write_stored(&stored_with("r")).unwrap(); + let sink = Arc::new(TokenSink::new("s", "r".into())); + let entry = keyring_entry().unwrap(); + entry + .as_any() + .downcast_ref::() + .unwrap() + .set_error(keyring_core::Error::NoEntry); + + assert!(sink.rotated(tokens("a2", "r2")).is_err()); + assert!(sink.state.lock().unsaved.is_some()); + assert_eq!(load().unwrap().refresh_token, "r"); + + sink.flush().unwrap(); + assert_eq!(load().unwrap().refresh_token, "r2"); + assert!(sink.state.lock().unsaved.is_none()); + } + + #[test] + fn retrying_a_missing_session_never_recreates_it_or_overwrites_a_new_login() { + let _turn = mock_keyring(); + let sink = TokenSink::new("s", "r".into()); + sink.state.lock().unsaved = Some(tokens("a2", "r2")); + + assert!(matches!(sink.flush(), Err(Error::NotLoggedIn))); + assert!(matches!(load(), Err(Error::NotLoggedIn))); + assert!(sink.state.lock().unsaved.is_some()); + + let mut login = stored_with("fresh"); + login.session_id = "new".into(); + write_stored(&login).unwrap(); + sink.flush().unwrap(); + assert!(sink.state.lock().unsaved.is_none()); + assert_eq!(load().unwrap().refresh_token, "fresh"); + } + #[test] fn an_unsaved_rotation_goes_only_into_the_blob_it_was_refused_for() { let unsaved = UnsavedRotation { diff --git a/docs/BUGS.md b/docs/BUGS.md index c500d28..194be75 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -109,6 +109,7 @@ a live account since. | Entry | Title | |---|---| +| [B284](#b284--a-keyring-entry-reported-missing-discards-refreshed-session-tokens) | A keyring entry reported missing discards refreshed session tokens | | [B283](#b283--videos-have-no-thumbnail-outside-photos-and-a-ts-recording-looks-like-source-code) | Videos have no thumbnail outside Photos, and a `.ts` recording looks like source code | | [B282](#b282--a-file-kept-offline-is-downloaded-again-after-a-change-made-elsewhere) | A file kept offline is downloaded again after a change made elsewhere | | [B281](#b281--a-file-in-a-synced-folder-cannot-be-kept-offline) | A file in a synced folder cannot be kept offline | @@ -386,6 +387,34 @@ a live account since. Newest first. +### B284 — A keyring entry reported missing discards refreshed session tokens + +**Status:** Fixed (unverified) +**Found:** 2026-11-14, investigating [issue #78](https://github.com/narrrl/proton-drive-linux/issues/78) +after the reporter's oo7 0.7.0 retest ruled out the original persistence fix as a sufficient +explanation. See [the investigation and isolated reproducer](ISSUE-78.md). +**Where:** `crates/pdfs-core/src/auth.rs`, `TokenSink::flush` + +**Repro:** Refresh a session while the keyring lookup reports `NoEntry` for its existing blob. +oo7 0.7.0 does this for a locked collection (upstream #614), reproduced with fake tokens on a +private D-Bus connection. pdfs dropped the pending rotation and returned success, leaving the +spent refresh token in the saved blob without triggering B61's retry or kernel recovery paths. +The running session can still work, but restarting it exposes the stale saved credentials. + +**Fix:** Propagate the missing-session error so the rotation stays pending and is retried. +Persistence still requires the matching existing blob; a retry never recreates a deleted +session or overwrites a new login. This does not make a locked keyring writable, and pending +tokens still need to reach it before power-off to survive a cold boot. + +**Verified:** The two regression tests +`auth::tests::a_temporarily_missing_session_does_not_discard_a_rotation` and +`auth::tests::retrying_a_missing_session_never_recreates_it_or_overwrites_a_new_login` +failed before the fix and pass afterward. The oo7 fixture confirms its locked lookup behavior +and repeated binary `SetSecret` persistence. The reporter's complete Fedora cold-boot sequence +has not been reproduced; issue #78's precise trigger remains unconfirmed. + +--- + ### B283 — Videos have no thumbnail outside Photos, and a `.ts` recording looks like source code **Status:** Fixed, not verified live diff --git a/docs/ISSUE-78.md b/docs/ISSUE-78.md new file mode 100644 index 0000000..8d00607 --- /dev/null +++ b/docs/ISSUE-78.md @@ -0,0 +1,91 @@ +# Issue #78: session loss on Fedora 45 + +Investigation on 2026-11-14 of [issue #78](https://github.com/narrrl/proton-drive-linux/issues/78), +using its report and comments through `gh`, pdfs 3.7.0/current source, and the oo7 0.7.0 tag. + +## What is established + +The reporter still saw the problem with oo7 0.7.0 on November 7. The original +[SetSecret persistence fix](https://github.com/linux-credentials/oo7/commit/7fb11dd111c4be8fec01996e3caa31c44fa962b3) +is present in that release. In an isolated test, three successive binary `SetSecret` updates +survived reopening the keyring file. Repeating the original persistence explanation is therefore +insufficient. + +A different, reproducible failure exists at the boundary between oo7 and pdfs: + +1. oo7 0.7.0's `Collection::search_inner_items` returns an empty list for a locked collection. + `SearchItems` consequently returns **both** the unlocked and locked lists empty even when the + matching item is on disk. The isolated test reproduces this with pdfs's exact lookup attributes. + This is upstream [oo7 #614](https://github.com/linux-credentials/oo7/issues/614). +2. `dbus-secret-service-keyring-store` 1.0.1 maps an empty search to `keyring_core::Error::NoEntry`; + pdfs maps that to `Error::NotLoggedIn`. The client cannot offer an unlock when no locked item + is reported. +3. In pdfs 3.7.0 and the code before this change, `TokenSink::flush` treated `NotLoggedIn` as a + logout, cleared the pending rotation and returned success. It skipped the error path that + retains the new tokens in the kernel keyring and retries their persistence. The code was + unchanged between v3.7.0 and this investigation. +4. If a token refresh encounters that reply, the saved blob retains a spent refresh token. The + running process can keep working with the new tokens in memory; a restart then exposes the + stale saved credentials. A regression test failed before the fix and passes afterward. + +The same unsafe assumption also applies to transient empty lookups such as the race reported in +[oo7 #652](https://github.com/linux-credentials/oo7/issues/652), fixed in 0.7.1. That issue's +replacement race was not independently reproduced here; pdfs normally updates existing items +with `SetSecret`, so it must not be assumed to encounter the same-item `CreateItem` race. + +## Change and limits + +`TokenSink::flush` now propagates a missing-session error instead of discarding the rotation. +The existing retry, kernel-keyring recovery and unsaved-token status paths can then run. Once +the collection is unlocked and readable, the pending rotation is saved. A genuinely deleted +session is never recreated: flushing still requires a readable blob with the matching session +ID and expected refresh token. A new login causes the old rotation to be discarded as before. + +This is a confirmed pdfs defect (B284), **not a confirmed complete explanation of the reporter's +cold-boot failure**. The issue has no oo7 logs or record showing that its collection was locked +when a token rotated. A locked keyring only at startup can hide a login, but does not by itself +explain `InvalidRefreshToken`. The token-loss path requires a successful refresh while the saved +entry is unreadable. + +The fix does not make a locked keyring writable. If it stays unreadable until power-off, tokens +retained in process/kernel memory still cannot survive a cold boot. No new disk credential store +or credential format is introduced. + +Other upstream Fedora-upgrade issues were checked: [oo7 #651](https://github.com/linux-credentials/oo7/issues/651) +creates duplicate placeholder/migrated collections and refuses writes; [oo7 #662](https://github.com/linux-credentials/oo7/issues/662) +can resolve the default alias to a locked collection. Both have fixes in +[0.7.1](https://github.com/linux-credentials/oo7/releases/tag/0.7.1), released November 10, +after the reporter's November 7 retest. Neither can be assigned as this report's cause without +matching runtime evidence. + +## Reproduce without a Proton account or desktop keyring + +The oo7 fixture uses fake tokens, a temporary disk keyring and a private peer-to-peer D-Bus +connection. It asserts the observed 0.7.0 behavior, including the incorrect empty locked lookup; +passing means that behavior was reproduced. It does not simulate Proton refresh or a full boot. + +From the pdfs repository, choose an unused temporary checkout path: + +```sh +git clone --depth 1 --branch 0.7.0 https://github.com/linux-credentials/oo7.git /tmp/pdfs-oo7-repro +git -C /tmp/pdfs-oo7-repro apply "$PWD/docs/repro/oo7-0.7.0-issue78.patch" +cargo test --manifest-path /tmp/pdfs-oo7-repro/Cargo.toml \ + -p oo7-daemon --lib --features test-util issue78 -- --nocapture +``` + +Observed output: + +```text +oo7 0.7.0: locked session is on disk but SearchItems returns (0, 0) +oo7 0.7.0: all three binary SetSecret updates survived reopening the disk keyring +test item::tests::issue78_locked_lookup_hides_saved_tokens ... ok +``` + +The pdfs side is covered by `cargo test -p pdfs-core auth::tests`, specifically +`a_temporarily_missing_session_does_not_discard_a_rotation` and +`retrying_a_missing_session_never_recreates_it_or_overwrites_a_new_login`. + +Remaining live validation: reproduce on a Fedora 45 GNOME session across logout/shutdown, +correlating oo7 lock/write errors with pdfs refresh and retry logs, and checking the running +daemon's package version rather than just the CLI version. The investigation did not use a real +Proton account, alter a desktop keyring, or post to either GitHub issue. diff --git a/docs/repro/oo7-0.7.0-issue78.patch b/docs/repro/oo7-0.7.0-issue78.patch new file mode 100644 index 0000000..8c6d290 --- /dev/null +++ b/docs/repro/oo7-0.7.0-issue78.patch @@ -0,0 +1,53 @@ +diff --git a/server/src/item/tests.rs b/server/src/item/tests.rs +index 99c25dc..2ba705d 100644 +--- a/server/src/item/tests.rs ++++ b/server/src/item/tests.rs +@@ -5,6 +5,48 @@ use tokio_stream::StreamExt; + + use crate::{service::PrompterType, tests::TestServiceSetup}; + ++// Investigation for proton-drive-linux #78. Fake tokens and a private p2p bus; ++// no desktop keyring or Proton account is used. ++#[tokio::test] ++async fn issue78_locked_lookup_hides_saved_tokens() -> Result<(), Box> { ++ let setup = ++ TestServiceSetup::with_login_keyring(oo7::Secret::from("test-password-long-enough")) ++ .await?; ++ let collection = setup.default_collection().await?; ++ let attributes = [("service", "proton-drive-linux"), ("username", "session")]; ++ let secret = setup.create_dbus_secret(oo7::Secret::blob(b"fake-refresh-r1"))?; ++ collection ++ .create_item("pdfs fake session", &attributes, &secret, false, None) ++ .await?; ++ ++ let (unlocked, locked) = setup.service_api.search_items(&attributes).await?; ++ assert_eq!((unlocked.len(), locked.len()), (1, 0)); ++ setup.lock_collection(collection).await?; ++ assert!(collection.is_locked().await?); ++ ++ let (unlocked, locked) = setup.service_api.search_items(&attributes).await?; ++ // This is the problematic reply, not the Secret Service behavior expected ++ // by keyring-store: the saved item is neither unlocked nor reported locked. ++ assert_eq!((unlocked.len(), locked.len()), (0, 0)); ++ assert_eq!(setup.login_keyring().await?.items().await?.len(), 1); ++ eprintln!("oo7 0.7.0: locked session is on disk but SearchItems returns (0, 0)"); ++ ++ setup.unlock_collection(collection).await?; ++ let (unlocked, locked) = setup.service_api.search_items(&attributes).await?; ++ assert_eq!((unlocked.len(), locked.len()), (1, 0)); ++ ++ // Exercise the binary SetSecret path used by pdfs's keyring-store. Reopen ++ // the disk keyring after each update, rather than trusting a memory read. ++ for refresh in [b"fake-refresh-r2", b"fake-refresh-r3", b"fake-refresh-r4"] { ++ let secret = setup.create_dbus_secret(oo7::Secret::blob(refresh))?; ++ unlocked[0].set_secret(&secret).await?; ++ let items = setup.login_keyring().await?.items().await?; ++ assert_eq!(items[0].secret().as_bytes(), refresh); ++ } ++ eprintln!("oo7 0.7.0: all three binary SetSecret updates survived reopening the disk keyring"); ++ Ok(()) ++} ++ + #[tokio::test] + async fn label_property() -> Result<(), Box> { + let setup = TestServiceSetup::plain_session(true).await?; diff --git a/scripts/publish-wiki.sh b/scripts/publish-wiki.sh index 832a4ec..aff84b2 100755 --- a/scripts/publish-wiki.sh +++ b/scripts/publish-wiki.sh @@ -91,7 +91,7 @@ SECTIONS = [ ] # Too long or too internal for the wiki; links to them go to GitHub. -EXCLUDED = {"BUGS.md", "CHANGELOG.md", "SDK-0.11.0.md"} +EXCLUDED = {"BUGS.md", "CHANGELOG.md", "ISSUE-78.md", "SDK-0.11.0.md"} # Files outside docs/ whose links are checked but not published. CHECKED = ["README.md", "CONTRIBUTING.md", "SECURITY.md"]