Port upstream 0.67.0: stage credential writes and publish atomically

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);

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<CookieHeaderEntry> {
-        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<CookieHeaderEntry> {
+        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,57 @@ 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() {
+                assert_eq!(
+                    fs::metadata(&sibling).unwrap().len(),
+                    0,
+                    "failed store must truncate the staged cookie secret"
+                );
+            }
+        }
+    }
+
+    #[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");
+    }
 }

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))
     }
 

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);
@@ -170,7 +181,17 @@ pub(super) fn write_auth_contents(home: &Path, contents: &[u8]) -> std::io::Resu
         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() {
@@ -254,3 +275,75 @@ pub(super) fn string_value(value: &serde_json::Value, key: &str) -> Option<Strin
         .filter(|s| !s.is_empty())
         .map(str::to_string)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn dir_entries(dir: &Path) -> Vec<String> {
+        let mut names: Vec<String> = 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);
+    }
+}