From b0ff6ee44139dfb1cbe508620d88c0737efdedb2 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Fri, 16 Oct 2026 10:32:05 +0200 Subject: [PATCH] fix(core): never write defaults over a config file that does not parse load_config gives defaults for a file it cannot read, and every setter loads, changes one field and saves. The first settings change after the file was damaged therefore wrote defaults over the mountpoint, the adopted device and the ignore rules. save_config now refuses while the file on disk does not parse, and the first-run default is written atomically with mode 0600 (B223). --- crates/pdfs-core/src/config.rs | 51 ++++++++++++++++++++++++++++++++-- docs/BUGS.md | 26 +++++++++++++++++ 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/crates/pdfs-core/src/config.rs b/crates/pdfs-core/src/config.rs index 74b7342..5d60d74 100644 --- a/crates/pdfs-core/src/config.rs +++ b/crates/pdfs-core/src/config.rs @@ -310,6 +310,22 @@ impl AppConfig { } } +/// Fail when the config file at `path` exists but does not parse, so that +/// [`AppDirs::save_config`] never writes over settings it could not read. +fn refuse_to_replace_unreadable(path: &Path) -> Result<()> { + match std::fs::read_to_string(path) { + Ok(content) => match serde_json::from_str::(&content) { + Ok(_) => Ok(()), + Err(e) => Err(Error::Other(format!( + "{} cannot be read ({e}); fix it or move it aside before changing settings", + path.display() + ))), + }, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e.into()), + } +} + /// Resolved XDG locations for state, cache, and the default mountpoint. pub struct AppDirs { dirs: ProjectDirs, @@ -341,6 +357,7 @@ impl AppDirs { // Preserve the original bytes for diagnosis/recovery. In // particular, never replace a truncated config with defaults // and silently lose mount, device, or ignore settings. + // `save_config` refuses to write over it for the same reason. tracing::error!(path = %path.display(), error = %e, "config is unreadable; using in-memory defaults without overwriting it"); return AppConfig::default(); } @@ -348,9 +365,8 @@ impl AppDirs { } let default_config = AppConfig::default(); - if let Ok(content) = serde_json::to_string_pretty(&default_config) { - let _ = std::fs::create_dir_all(self.dirs.config_dir()); - let _ = std::fs::write(&path, content); + if let Err(e) = self.save_config(&default_config) { + tracing::warn!(path = %path.display(), error = %e, "could not write the default config"); } default_config } @@ -358,8 +374,15 @@ impl AppDirs { /// Persist `config` to [`config_path`](Self::config_path), creating the /// config dir if missing. The Settings page calls this after editing the /// cache budget or mountpoint; the daemon re-reads the file on its next mount. + /// + /// Refuses while the file on disk does not parse. Every caller loads, + /// changes one field and saves, and [`load_config`](Self::load_config) + /// gives defaults for a file it cannot read, so the first change after + /// the file was damaged wrote defaults over the mountpoint, the device and + /// the ignore rules the user had (`docs/BUGS.md` B223). pub fn save_config(&self, config: &AppConfig) -> Result<()> { let path = self.config_path(); + refuse_to_replace_unreadable(&path)?; let parent = path .parent() .ok_or_else(|| Error::Other("configuration path has no parent".into()))?; @@ -526,6 +549,28 @@ mod tests { std::fs::metadata(p).unwrap().permissions().mode() & 0o777 } + /// A damaged config must not be replaced by defaults the next time a + /// setting changes (`docs/BUGS.md` B223). + #[test] + fn an_unreadable_config_is_never_written_over() { + let dir = std::env::temp_dir().join(format!( + "pdfs-cfg-guard-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("config.json"); + + assert!(refuse_to_replace_unreadable(&path).is_ok(), "no file yet"); + std::fs::write(&path, serde_json::to_vec(&AppConfig::default()).unwrap()).unwrap(); + assert!(refuse_to_replace_unreadable(&path).is_ok()); + std::fs::write(&path, br#"{"mountpoint": "/home/me/Pro"#).unwrap(); + assert!(refuse_to_replace_unreadable(&path).is_err(), "truncated"); + + let _ = std::fs::remove_dir_all(&dir); + } + /// `docs/BUGS.md` B6. A permissive mode here is not a cosmetic problem: these /// directories hold decrypted content, a plaintext index of every node name, /// and the control socket that commands the daemon's session. diff --git a/docs/BUGS.md b/docs/BUGS.md index 78bea66..7fd0806 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -100,6 +100,7 @@ a live account since. | Entry | Title | |---|---| +| [B223](#b223--a-damaged-config-file-is-replaced-with-defaults-on-the-next-settings-change) | A damaged config file is replaced with defaults on the next settings change | | [B224](#b224--fallocate-zero-ranges-are-undone-and-collapse-ranges-corrupt-the-file) | `fallocate` zero ranges are undone and collapse ranges corrupt the file | | [B222](#b222--a-full-disk-at-close-is-reported-to-nobody-and-fsync-on-a-read-only-file-fails) | A full disk at close is reported to nobody, and `fsync` on a read-only file fails | | [B221](#b221--a-cold-listing-holds-the-global-lock-across-a-database-transaction) | A cold listing holds the global lock across a database transaction | @@ -326,6 +327,31 @@ a live account since. Newest first. +### B223 — A damaged config file is replaced with defaults on the next settings change + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.10). +**Where:** `AppDirs::save_config`, `AppDirs::load_config` and `refuse_to_replace_unreadable` in +`crates/pdfs-core/src/config.rs` + +**Repro:** Truncate `~/.config/proton-drive-linux/config.json`, then change any setting in the +app, or let the daemon record its device. + +**Cause.** `load_config` gives defaults for a file it cannot parse, and logs that it leaves the +file alone. But every setter loads, changes one field and saves, in the daemon and in the app. +So the first change after the damage wrote the defaults over the file, losing the mountpoint, +the device the client had adopted, the ignore rules and the `open_with` choices. ARCHITECTURE +§10.5 says the file is never replaced with defaults. The first-run default was also written with a +plain `std::fs::write`, which is not atomic and takes the umask's mode. + +**Fix.** `save_config` refuses while the file on disk exists and does not parse, and says which +file to fix. The first-run default goes through `save_config`, so it is written atomically with +mode 0600. + +**Test:** `config::tests::an_unreadable_config_is_never_written_over`. + +--- + ### B224 — `fallocate` zero ranges are undone and collapse ranges corrupt the file **Status:** Fixed, not verified live