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
This commit is contained in:
nvrl committed 2026-11-14 15:18:27 +01:00
1 parent 3295eda1f0
commit ba5b6a8bf2
5 files changed
+219 -11

No files matched your search

+45 -10
View File
@@ -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::<keyring_core::mock::Cred>()
.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 {
+29
View File
@@ -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
+91
View File
@@ -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.
+53
View File
@@ -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<dyn std::error::Error>> {
+ 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<dyn std::error::Error>> {
let setup = TestServiceSetup::plain_session(true).await?;
+1 -1
View File
@@ -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"]