From 14bd7098c2a46dd9f5af7160e761c5925930788d Mon Sep 17 00:00:00 2001 From: RCD <90105158+Finesssee@users.noreply.github.com> Date: Tue, 29 Sep 2026 20:10:40 +0700 Subject: [PATCH 1/2] Port upstream 0.67.0: stage credential writes and publish atomically --- rust/src/atomic_file.rs | 37 ++++++++- rust/src/browser/cookie_cache.rs | 84 +++++++++++++++++--- rust/src/codex_accounts/account_manager.rs | 12 +-- rust/src/codex_accounts/credentials.rs | 89 ++++++++++++++++++++- rust/src/secure_file.rs | 90 ++++++++++++++++++---- rust/src/settings/tests.rs | 21 +++++ 6 files changed, 293 insertions(+), 40 deletions(-) diff --git a/rust/src/atomic_file.rs b/rust/src/atomic_file.rs index 8d7cb1e1b76..ac779714a24 100644 --- a/rust/src/atomic_file.rs +++ b/rust/src/atomic_file.rs @@ -8,6 +8,22 @@ static TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); /// failure, truncate the owned temp sibling instead of deleting it so callers /// never need destructive cleanup. pub fn write_atomic(path: &Path, bytes: &[u8]) -> anyhow::Result<()> { + write_atomic_inner(path, bytes, false) +} + +/// Atomic write for credential-bearing files. +/// +/// Same publish contract as [`write_atomic`], but the staged sibling is created +/// owner-only (mode `0600` on unix, set at creation and re-applied before any +/// byte is written) so a secret is never readable by other users, neither in +/// the staged file nor in the published one. On Windows the staged file +/// inherits the protected per-user directory ACL, matching the previous +/// behavior of `secure_file`. +pub fn write_atomic_private(path: &Path, bytes: &[u8]) -> anyhow::Result<()> { + write_atomic_inner(path, bytes, true) +} + +fn write_atomic_inner(path: &Path, bytes: &[u8], private: bool) -> anyhow::Result<()> { let parent = path .parent() .filter(|p| !p.as_os_str().is_empty()) @@ -26,10 +42,23 @@ pub fn write_atomic(path: &Path, bytes: &[u8]) -> anyhow::Result<()> { )); let temp = PathBuf::from(temp_name); let result = (|| -> anyhow::Result<()> { - let mut file = std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(&temp)?; + let mut options = std::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + if private { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + let mut file = options.open(&temp)?; + #[cfg(unix)] + if private { + use std::os::unix::fs::PermissionsExt; + // Restore owner access even under a restrictive umask, before + // writing any bytes. + file.set_permissions(std::fs::Permissions::from_mode(0o600))?; + } + #[cfg(not(unix))] + let _ = private; file.write_all(bytes)?; file.sync_all()?; drop(file); diff --git a/rust/src/browser/cookie_cache.rs b/rust/src/browser/cookie_cache.rs index 0e9d89b3561..4147c28abfe 100755 --- a/rust/src/browser/cookie_cache.rs +++ b/rust/src/browser/cookie_cache.rs @@ -1,7 +1,9 @@ //! Cookie Header Cache //! //! Caches cookie headers for providers to avoid repeated browser cookie extraction. -//! Stores normalized cookie headers with timestamps and source labels. +//! Stores normalized cookie headers with timestamps and source labels. Entries +//! are session secrets, so they are written through `secure_file` (DPAPI on +//! Windows, staged and published atomically) rather than as plaintext. // The cache API is currently unused by the crate (reserved for provider // cookie reuse), so every public item would trip dead_code. @@ -11,10 +13,11 @@ )] use crate::core::ProviderId; +use crate::secure_file; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; /// Cached cookie header entry #[derive(Debug, Clone, Serialize, Deserialize)] @@ -49,8 +52,11 @@ pub struct CookieHeaderCache; impl CookieHeaderCache { /// Load cached cookie header for a provider pub fn load(provider: ProviderId) -> Option { - let path = Self::cache_path(provider)?; - let data = fs::read_to_string(&path).ok()?; + Self::load_from(&Self::cache_path(provider)?) + } + + fn load_from(path: &Path) -> Option { + let data = secure_file::read_string(path).ok()?; serde_json::from_str(&data).ok() } @@ -73,14 +79,7 @@ impl CookieHeaderCache { let entry = CookieHeaderEntry::new(normalized, source_label); let path = Self::cache_path(provider).ok_or(CookieHeaderCacheError::PathNotAvailable)?; - - // Create parent directory - if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; - } - - let json = serde_json::to_string_pretty(&entry)?; - fs::write(&path, json)?; + Self::store_to(&path, &entry)?; tracing::debug!( provider = %provider.cli_name(), @@ -91,6 +90,17 @@ impl CookieHeaderCache { Ok(()) } + /// Persist `entry` to `path`, creating the parent directory. The write is + /// staged and published atomically, so a failure keeps the previous entry. + fn store_to(path: &Path, entry: &CookieHeaderEntry) -> Result<(), CookieHeaderCacheError> { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; + } + let json = serde_json::to_string_pretty(entry)?; + secure_file::write_string(path, &json)?; + Ok(()) + } + /// Clear cached cookie header for a provider pub fn clear(provider: ProviderId) { if let Some(path) = Self::cache_path(provider) @@ -195,4 +205,54 @@ mod tests { let whitespace = CookieHeaderCache::normalize_cookie_header(" ; ; "); assert!(whitespace.is_empty()); } + + #[test] + fn store_round_trips_and_replaces_the_previous_entry() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("nested").join("codex-cookie.json"); + + CookieHeaderCache::store_to(&path, &CookieHeaderEntry::new("a=1", "Chrome")).unwrap(); + CookieHeaderCache::store_to(&path, &CookieHeaderEntry::new("a=2", "Edge")).unwrap(); + + let loaded = CookieHeaderCache::load_from(&path).expect("entry"); + assert_eq!(loaded.cookie_header, "a=2"); + assert_eq!(loaded.source_label, "Edge"); + let names: Vec<_> = fs::read_dir(path.parent().unwrap()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(names, vec![std::ffi::OsString::from("codex-cookie.json")]); + } + + #[test] + fn failed_store_leaves_no_cookie_bytes_behind() { + let dir = tempfile::tempdir().unwrap(); + // A directory at the destination makes publishing fail after the + // staged sibling was fully written. + let blocked = dir.path().join("codex-cookie.json"); + fs::create_dir(&blocked).unwrap(); + let entry = CookieHeaderEntry::new("secret=leaked-cookie", "Chrome"); + + assert!(CookieHeaderCache::store_to(&blocked, &entry).is_err()); + + assert!(blocked.is_dir()); + for sibling in fs::read_dir(dir.path()).unwrap() { + let sibling = sibling.unwrap().path(); + if sibling.is_file() { + let leftover = fs::read(&sibling).unwrap(); + assert!(!String::from_utf8_lossy(&leftover).contains("leaked-cookie")); + } + } + } + + #[test] + fn load_still_reads_entries_written_as_plaintext_json() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("codex-cookie.json"); + let entry = CookieHeaderEntry::new("legacy=1", "Chrome"); + fs::write(&path, serde_json::to_string_pretty(&entry).unwrap()).unwrap(); + + let loaded = CookieHeaderCache::load_from(&path).expect("legacy entry"); + assert_eq!(loaded.cookie_header, "legacy=1"); + } } diff --git a/rust/src/codex_accounts/account_manager.rs b/rust/src/codex_accounts/account_manager.rs index 8ef6c0361f0..9079eb3e94b 100644 --- a/rust/src/codex_accounts/account_manager.rs +++ b/rust/src/codex_accounts/account_manager.rs @@ -15,7 +15,9 @@ use thiserror::Error; use uuid::Uuid; use super::api::CodexApiError; -use super::credentials::{AuthBackedIdentity, load_identity, parse_credentials_json}; +use super::credentials::{ + AuthBackedIdentity, copy_private_file, load_identity, parse_credentials_json, +}; use super::file_locations::{ ambient_codex_home, auth_backups_directory, codex_desktop_session_root, desktop_session_snapshot_path, ensure_directories, managed_homes_directory, @@ -247,7 +249,7 @@ impl CodexAccountManager { fs::create_dir_all(ambient_codex_home())?; let backup_path = self.backup_ambient_auth()?; - fs::copy(&target_auth_path, ambient_codex_home().join("auth.json"))?; + copy_private_file(&target_auth_path, &ambient_codex_home().join("auth.json"))?; self.sync_ambient_global_state( ambient_account .as_ref() @@ -291,14 +293,14 @@ impl CodexAccountManager { // Never let a stale ambient file overwrite newer managed // credentials; that turns a working account into a dead one. if credentials_are_at_least_as_fresh(&source_auth_path, &existing_auth_path) { - fs::copy(&source_auth_path, &existing_auth_path)?; + copy_private_file(&source_auth_path, &existing_auth_path)?; } existing } None => { let fresh_home = managed_homes_directory().join(Uuid::new_v4().to_string()); fs::create_dir_all(&fresh_home)?; - fs::copy(&source_auth_path, fresh_home.join("auth.json"))?; + copy_private_file(&source_auth_path, &fresh_home.join("auth.json"))?; fresh_home } }; @@ -328,7 +330,7 @@ impl CodexAccountManager { } let backup_path = auth_backups_directory().join(format!("ambient-auth-{}.json", timestamp_slug())); - fs::copy(&auth_path, &backup_path)?; + copy_private_file(&auth_path, &backup_path)?; Ok(Some(backup_path)) } diff --git a/rust/src/codex_accounts/credentials.rs b/rust/src/codex_accounts/credentials.rs index 2ef98286c74..916bbd71613 100644 --- a/rust/src/codex_accounts/credentials.rs +++ b/rust/src/codex_accounts/credentials.rs @@ -157,10 +157,21 @@ pub fn save_credentials( } pub(super) fn write_auth_contents(home: &Path, contents: &[u8]) -> std::io::Result<()> { + write_private_file(&home.join("auth.json"), contents) +} + +/// Copy a credential file through the exclusive-staged private writer, so the +/// destination is never a partly written or world-readable copy of the secret. +pub(super) fn copy_private_file(source: &Path, destination: &Path) -> std::io::Result<()> { + write_private_file(destination, &std::fs::read(source)?) +} + +/// Stage `contents` in an exclusive owner-only sibling and publish it over +/// `path`, leaving the previous file intact when any step fails. +fn write_private_file(path: &Path, contents: &[u8]) -> std::io::Result<()> { use std::io::Write; - let auth_path = home.join("auth.json"); - // Preserve an existing auth-file symlink by replacing its resolved target. - let destination = auth_path.canonicalize().unwrap_or(auth_path); + // Preserve an existing symlink by replacing its resolved target. + let destination = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); let staged = destination.with_file_name(format!(".auth-{}.tmp", uuid::Uuid::new_v4())); let mut options = std::fs::OpenOptions::new(); options.write(true).create_new(true); @@ -254,3 +265,75 @@ pub(super) fn string_value(value: &serde_json::Value, key: &str) -> Option Vec { + let mut names: Vec = std::fs::read_dir(dir) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + names + } + + #[test] + fn copy_private_file_replaces_destination_and_leaves_no_staged_file() { + let dir = tempfile::tempdir().unwrap(); + let source = dir.path().join("source.json"); + let destination = dir.path().join("auth.json"); + std::fs::write(&source, b"new-credentials").unwrap(); + std::fs::write(&destination, b"old-credentials").unwrap(); + + copy_private_file(&source, &destination).unwrap(); + + assert_eq!(std::fs::read(&destination).unwrap(), b"new-credentials"); + assert_eq!(dir_entries(dir.path()), ["auth.json", "source.json"]); + } + + #[test] + fn copy_private_file_keeps_destination_when_source_is_unreadable() { + let dir = tempfile::tempdir().unwrap(); + let destination = dir.path().join("auth.json"); + std::fs::write(&destination, b"old-credentials").unwrap(); + + assert!(copy_private_file(&dir.path().join("missing.json"), &destination).is_err()); + + assert_eq!(std::fs::read(&destination).unwrap(), b"old-credentials"); + assert_eq!(dir_entries(dir.path()), ["auth.json"]); + } + + #[test] + fn failed_publish_keeps_destination_and_removes_the_staged_secret() { + let dir = tempfile::tempdir().unwrap(); + let destination = dir.path().join("auth.json"); + std::fs::create_dir(&destination).unwrap(); + + assert!(write_auth_contents(dir.path(), b"secret-credentials").is_err()); + + assert!(destination.is_dir()); + assert_eq!(dir_entries(dir.path()), ["auth.json"]); + } + + #[cfg(unix)] + #[test] + fn copy_private_file_publishes_owner_only_even_from_a_world_readable_source() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let source = dir.path().join("source.json"); + let destination = dir.path().join("auth.json"); + std::fs::write(&source, b"credentials").unwrap(); + std::fs::set_permissions(&source, std::fs::Permissions::from_mode(0o644)).unwrap(); + + copy_private_file(&source, &destination).unwrap(); + + let mode = std::fs::metadata(&destination) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600); + } +} diff --git a/rust/src/secure_file.rs b/rust/src/secure_file.rs index 30e2a7474cd..c3bd4aafcdd 100644 --- a/rust/src/secure_file.rs +++ b/rust/src/secure_file.rs @@ -93,11 +93,18 @@ pub fn read_string(path: &Path) -> io::Result { } /// Write a UTF-8 file, protecting it with Windows DPAPI when available. +/// +/// The protected bytes are staged in an owner-only sibling, synced, and then +/// published atomically, so a failed or interrupted write leaves the previous +/// file intact and a partly written secret is never the live file. pub fn write_string(path: &Path, contents: &str) -> io::Result<()> { let bytes = protected_file_bytes(contents)?; - std::fs::write(path, bytes)?; - restrict_file_permissions(path)?; - Ok(()) + crate::atomic_file::write_atomic_private(path, &bytes).map_err(|error| { + match error.downcast::() { + Ok(io_error) => io_error, + Err(other) => io::Error::other(other), + } + }) } #[cfg(windows)] @@ -228,19 +235,6 @@ fn unprotect(_encrypted: &[u8]) -> io::Result> { )) } -#[cfg(unix)] -fn restrict_file_permissions(path: &Path) -> io::Result<()> { - use std::os::unix::fs::PermissionsExt; - let mut perms = std::fs::metadata(path)?.permissions(); - perms.set_mode(0o600); - std::fs::set_permissions(path, perms) -} - -#[cfg(not(unix))] -fn restrict_file_permissions(_path: &Path) -> io::Result<()> { - Ok(()) -} - #[cfg(test)] mod tests { use super::*; @@ -263,6 +257,70 @@ mod tests { assert_eq!(read_string(&path).unwrap(), r#"{"secret":"value"}"#); } + #[test] + fn write_replaces_existing_file_and_leaves_only_the_destination() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secure.json"); + write_string(&path, r#"{"secret":"old"}"#).unwrap(); + write_string(&path, r#"{"secret":"new"}"#).unwrap(); + + assert_eq!(read_string(&path).unwrap(), r#"{"secret":"new"}"#); + let names: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(names, vec![std::ffi::OsString::from("secure.json")]); + } + + #[test] + fn failed_publish_keeps_destination_and_leaves_no_secret_sibling() { + let dir = tempfile::tempdir().unwrap(); + // A directory at the destination makes the publish step fail after the + // staged sibling was fully written. + let path = dir.path().join("secure.json"); + std::fs::create_dir(&path).unwrap(); + std::fs::write(path.join("keep"), b"previous").unwrap(); + + assert!(write_string(&path, r#"{"secret":"leaked-value"}"#).is_err()); + + assert!(path.is_dir()); + assert_eq!(std::fs::read(path.join("keep")).unwrap(), b"previous"); + for entry in std::fs::read_dir(dir.path()).unwrap() { + let entry_path = entry.unwrap().path(); + if entry_path.is_file() { + let leftover = std::fs::read(&entry_path).unwrap(); + assert!( + !String::from_utf8_lossy(&leftover).contains("leaked-value"), + "staged secret must not survive a failed publish" + ); + } + } + } + + #[test] + fn write_into_missing_directory_fails_without_creating_it() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("missing").join("secure.json"); + + assert!(write_string(&path, "{}").is_err()); + assert!(!dir.path().join("missing").exists()); + } + + #[cfg(unix)] + #[test] + fn write_publishes_owner_only_file() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secure.json"); + std::fs::write(&path, "old").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + write_string(&path, r#"{"secret":"value"}"#).unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + } + #[test] fn status_reports_missing_plaintext_and_protected_files() { let dir = tempfile::tempdir().unwrap(); diff --git a/rust/src/settings/tests.rs b/rust/src/settings/tests.rs index a6db4aa05b2..e7ddc60a1ee 100644 --- a/rust/src/settings/tests.rs +++ b/rust/src/settings/tests.rs @@ -1190,3 +1190,24 @@ fn legacy_visibility_setters_preserve_other_explicit_hidden_items() { vec!["metric:secondary".to_string()] ); } + +/// Cookie-denial settings written through the desktop's `Settings::save` path +/// (`secure_file::write_string` of the pretty JSON) must be read back as denied +/// by `Settings::load` (`secure_file::read_string` + JSON parse), which is what +/// every CLI code path uses. +#[test] +fn cookie_denial_survives_the_secure_write_and_load_path() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("settings.json"); + let mut settings = Settings::default(); + settings.set_cookie_source(ProviderId::Codex, "off"); + settings.set_openai_web_extras(ProviderId::Codex, false); + let json = serde_json::to_string_pretty(&settings).unwrap(); + + crate::secure_file::write_string(&path, &json).unwrap(); + let content = crate::secure_file::read_string(&path).unwrap(); + let loaded: Settings = serde_json::from_str(content.trim_start_matches('\u{feff}')).unwrap(); + + assert_eq!(loaded.cookie_source(ProviderId::Codex), "off"); + assert!(!loaded.openai_web_extras(ProviderId::Codex)); +} From acaa2716b3bf2da24c0b78d43794efdfe3586f84 Mon Sep 17 00:00:00 2001 From: RCD <90105158+Finesssee@users.noreply.github.com> Date: Wed, 30 Sep 2026 19:26:27 +0700 Subject: [PATCH 2/2] Address thermo review --- rust/src/browser/cookie_cache.rs | 7 ++++-- rust/src/codex_accounts/credentials.rs | 12 +++++++++- rust/src/secure_file.rs | 8 +++---- rust/src/settings.rs | 31 +++++++++++++++++--------- rust/src/settings/tests.rs | 15 +++++-------- 5 files changed, 46 insertions(+), 27 deletions(-) diff --git a/rust/src/browser/cookie_cache.rs b/rust/src/browser/cookie_cache.rs index 4147c28abfe..d943f2acbbc 100755 --- a/rust/src/browser/cookie_cache.rs +++ b/rust/src/browser/cookie_cache.rs @@ -239,8 +239,11 @@ mod tests { for sibling in fs::read_dir(dir.path()).unwrap() { let sibling = sibling.unwrap().path(); if sibling.is_file() { - let leftover = fs::read(&sibling).unwrap(); - assert!(!String::from_utf8_lossy(&leftover).contains("leaked-cookie")); + assert_eq!( + fs::metadata(&sibling).unwrap().len(), + 0, + "failed store must truncate the staged cookie secret" + ); } } } diff --git a/rust/src/codex_accounts/credentials.rs b/rust/src/codex_accounts/credentials.rs index 916bbd71613..45f6d1b3f32 100644 --- a/rust/src/codex_accounts/credentials.rs +++ b/rust/src/codex_accounts/credentials.rs @@ -181,7 +181,17 @@ fn write_private_file(path: &Path, contents: &[u8]) -> std::io::Result<()> { options.mode(0o600); } let mut file = options.open(&staged)?; - let written = file.write_all(contents); + let written = (|| { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + // `OpenOptionsExt::mode` is filtered through umask. Restore the + // promised mode before writing, as `secure_file` does. + file.set_permissions(std::fs::Permissions::from_mode(0o600))?; + } + file.write_all(contents)?; + file.sync_all() + })(); drop(file); let result = written.and_then(|()| std::fs::rename(&staged, &destination)); if result.is_err() { diff --git a/rust/src/secure_file.rs b/rust/src/secure_file.rs index c3bd4aafcdd..9be7093e59f 100644 --- a/rust/src/secure_file.rs +++ b/rust/src/secure_file.rs @@ -288,10 +288,10 @@ mod tests { for entry in std::fs::read_dir(dir.path()).unwrap() { let entry_path = entry.unwrap().path(); if entry_path.is_file() { - let leftover = std::fs::read(&entry_path).unwrap(); - assert!( - !String::from_utf8_lossy(&leftover).contains("leaked-value"), - "staged secret must not survive a failed publish" + assert_eq!( + std::fs::metadata(&entry_path).unwrap().len(), + 0, + "failed publish must truncate the staged secret" ); } } diff --git a/rust/src/settings.rs b/rust/src/settings.rs index 9863f194ceb..285d447907a 100755 --- a/rust/src/settings.rs +++ b/rust/src/settings.rs @@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use crate::core::ProviderId; @@ -619,19 +619,12 @@ impl Settings { /// Load settings from disk pub fn load() -> Self { + let path = Self::settings_path(); #[allow( unused_mut, reason = "mutability is needed for conditional initialization paths that the compiler cannot prove" )] - let mut settings = match Self::settings_path() { - Some(path) if path.exists() => match crate::secure_file::read_string(&path) { - Ok(content) => { - serde_json::from_str(content.trim_start_matches('\u{feff}')).unwrap_or_default() - } - Err(_) => Self::default(), - }, - _ => Self::default(), - }; + let mut settings = Self::load_from_path(path.as_deref()); // Sync autostart toggle with actual registry state and repair stale commands from older builds. #[cfg(target_os = "windows")] @@ -648,6 +641,18 @@ impl Settings { settings } + fn load_from_path(path: Option<&Path>) -> Self { + match path { + Some(path) if path.exists() => match crate::secure_file::read_string(path) { + Ok(content) => { + serde_json::from_str(content.trim_start_matches('\u{feff}')).unwrap_or_default() + } + Err(_) => Self::default(), + }, + _ => Self::default(), + } + } + /// Materialize `hidden_usage_item_ids` from the pre-0.62 per-provider /// visibility flags where the list was never persisted. Idempotent: a /// provider with an explicit list is left alone. @@ -714,13 +719,17 @@ impl Settings { let path = Self::settings_path() .ok_or_else(|| anyhow::anyhow!("Could not determine settings path"))?; + self.save_to_path(&path) + } + + fn save_to_path(&self, path: &Path) -> anyhow::Result<()> { // Ensure parent directory exists if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; } let json = serde_json::to_string_pretty(self)?; - crate::secure_file::write_string(&path, &json)?; + crate::secure_file::write_string(path, &json)?; Ok(()) } diff --git a/rust/src/settings/tests.rs b/rust/src/settings/tests.rs index e7ddc60a1ee..9bd49f5c249 100644 --- a/rust/src/settings/tests.rs +++ b/rust/src/settings/tests.rs @@ -1191,22 +1191,19 @@ fn legacy_visibility_setters_preserve_other_explicit_hidden_items() { ); } -/// Cookie-denial settings written through the desktop's `Settings::save` path -/// (`secure_file::write_string` of the pretty JSON) must be read back as denied -/// by `Settings::load` (`secure_file::read_string` + JSON parse), which is what -/// every CLI code path uses. +/// Cookie-denial settings must survive the same path-based persistence used by +/// `Settings::save` and `Settings::load`, including the secure-file wrapper +/// shared by desktop and CLI settings. #[test] -fn cookie_denial_survives_the_secure_write_and_load_path() { +fn cookie_denial_round_trips_through_settings_persistence() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("settings.json"); let mut settings = Settings::default(); settings.set_cookie_source(ProviderId::Codex, "off"); settings.set_openai_web_extras(ProviderId::Codex, false); - let json = serde_json::to_string_pretty(&settings).unwrap(); - crate::secure_file::write_string(&path, &json).unwrap(); - let content = crate::secure_file::read_string(&path).unwrap(); - let loaded: Settings = serde_json::from_str(content.trim_start_matches('\u{feff}')).unwrap(); + settings.save_to_path(&path).unwrap(); + let loaded = Settings::load_from_path(Some(&path)); assert_eq!(loaded.cookie_source(ProviderId::Codex), "off"); assert!(!loaded.openai_web_extras(ProviderId::Codex));