Skip to content

Harden local state storage boundaries #3433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Hmbown wants to merge 2 commits into from
Closed

Harden local state storage boundaries #3433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4746a2
Harden local state storage boundaries
Hmbown Jun 23, 2026
095d084
WIP validate task before PR attempt artifact
Hmbown Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions crates/config/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3239,16 +3239,24 @@ pub const LEGACY_APP_DIR: &str = ".deepseek";
/// `$CODEWHALE_HOME` takes precedence when set. Otherwise defaults to
/// `$HOME/.codewhale`. This is the write target for new product state.
pub fn codewhale_home() -> Result<PathBuf> {
if let Ok(val) = std::env::var("CODEWHALE_HOME") {
let trimmed = val.trim();
if !trimmed.is_empty() {
return Ok(PathBuf::from(trimmed));
}
if let Some(path) = explicit_codewhale_home() {
return Ok(path);
}
let home = effective_home_dir().context("failed to resolve home directory")?;
Ok(home.join(CODEWHALE_APP_DIR))
}

fn explicit_codewhale_home() -> Option<PathBuf> {
std::env::var("CODEWHALE_HOME").ok().and_then(|val| {
let trimmed = val.trim();
if trimmed.is_empty() {
None
} else {
Some(PathBuf::from(trimmed))
}
})
}

/// Resolve the legacy DeepSeek home directory (`$HOME/.deepseek`).
///
/// Always returns the legacy path regardless of whether it exists.
Expand Down Expand Up @@ -3308,6 +3316,9 @@ pub fn resolve_state_dir(subdir: &str) -> Result<PathBuf> {
if primary.exists() {
return Ok(primary);
}
if explicit_codewhale_home().is_some() {
return Ok(primary);
}
let legacy = legacy_deepseek_home()?.join(subdir);
if legacy.exists() {
return Ok(legacy);
Expand All @@ -3328,7 +3339,9 @@ pub fn resolve_state_dir(subdir: &str) -> Result<PathBuf> {
pub fn ensure_state_dir(subdir: &str) -> Result<PathBuf> {
ensure_safe_state_subdir(subdir)?;
let dir = codewhale_home()?.join(subdir);
migrate_legacy_state_dir(&dir, subdir)?;
if explicit_codewhale_home().is_none() {
migrate_legacy_state_dir(&dir, subdir)?;
}
std::fs::create_dir_all(&dir)
.with_context(|| format!("failed to create {}/", dir.display()))?;
Ok(dir)
Expand Down Expand Up @@ -3599,6 +3612,9 @@ pub fn default_config_path() -> Result<PathBuf> {
if primary.exists() {
return Ok(primary);
}
if explicit_codewhale_home().is_some() {
return Ok(primary);
}
let legacy = legacy_deepseek_home()?.join(CONFIG_FILE_NAME);
if legacy.exists() {
return Ok(legacy);
Expand Down Expand Up @@ -3632,6 +3648,9 @@ pub fn migrate_config_if_needed() -> Result<Option<ConfigMigration>> {
if primary.exists() {
return Ok(None);
}
if explicit_codewhale_home().is_some() {
return Ok(None);
}
let legacy = legacy_deepseek_home()?.join(CONFIG_FILE_NAME);
if !legacy.exists() {
return Ok(None);
Expand Down
93 changes: 88 additions & 5 deletions crates/config/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1911,7 +1911,7 @@ fn migrate_config_reports_copied_legacy_path() {
unsafe {
env::set_var("HOME", &home);
env::set_var("USERPROFILE", &home);
env::set_var("CODEWHALE_HOME", &primary_dir);
env::remove_var("CODEWHALE_HOME");
}

let migration = migrate_config_if_needed()
Expand Down Expand Up @@ -1962,9 +1962,9 @@ impl Drop for StateEnvRestore {
}
}

/// Points `HOME`/`USERPROFILE`/`CODEWHALE_HOME` at a fresh temp tree so
/// `codewhale_home()` -> `<home>/.codewhale` and `legacy_deepseek_home()`
/// -> `<home>/.deepseek`. Env is restored on drop.
/// Points `HOME`/`USERPROFILE` at a fresh temp tree so `codewhale_home()` ->
/// `<home>/.codewhale` and `legacy_deepseek_home()` -> `<home>/.deepseek`.
/// Env is restored on drop.
struct StateDirEnv {
home: PathBuf,
_restore: StateEnvRestore,
Expand All @@ -1985,7 +1985,7 @@ impl StateDirEnv {
unsafe {
env::set_var("HOME", &home);
env::set_var("USERPROFILE", &home);
env::set_var("CODEWHALE_HOME", home.join(CODEWHALE_APP_DIR));
env::remove_var("CODEWHALE_HOME");
}
Self {
home,
Expand Down Expand Up @@ -2087,6 +2087,89 @@ fn resolve_state_dir_still_finds_legacy_for_backfill() {
let _ = fs::remove_dir_all(&state_env.home);
}

#[test]
fn explicit_codewhale_home_does_not_read_or_migrate_legacy_state() {
let _lock = env_lock();
let unique = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.expect("clock")
.as_nanos();
let home = std::env::temp_dir().join(format!(
"codewhale-explicit-state-home-{}-{unique}",
std::process::id()
));
let explicit = home.join("isolated-codewhale");
let restore = StateEnvRestore {
home: env::var_os("HOME"),
userprofile: env::var_os("USERPROFILE"),
codewhale_home: env::var_os("CODEWHALE_HOME"),
};
// Safety: test-only environment mutation is serialized by env_lock().
unsafe {
env::set_var("HOME", &home);
env::set_var("USERPROFILE", &home);
env::set_var("CODEWHALE_HOME", &explicit);
}
let _restore = restore;

let legacy_catalog = home.join(LEGACY_APP_DIR).join("catalog");
fs::create_dir_all(&legacy_catalog).expect("legacy catalog");
fs::write(legacy_catalog.join("state.json"), b"legacy").expect("legacy file");

assert_eq!(
resolve_state_dir("catalog").expect("resolve"),
explicit.join("catalog")
);
assert_eq!(
ensure_state_dir("catalog").expect("ensure"),
explicit.join("catalog")
);
assert!(!explicit.join("catalog").join("state.json").exists());
assert!(legacy_catalog.join("state.json").exists());

let _ = fs::remove_dir_all(home);
}

#[test]
fn explicit_codewhale_home_does_not_read_or_migrate_legacy_config() {
let _lock = env_lock();
let unique = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.expect("clock")
.as_nanos();
let home = std::env::temp_dir().join(format!(
"codewhale-explicit-config-home-{}-{unique}",
std::process::id()
));
let explicit = home.join("isolated-codewhale");
let restore = StateEnvRestore {
home: env::var_os("HOME"),
userprofile: env::var_os("USERPROFILE"),
codewhale_home: env::var_os("CODEWHALE_HOME"),
};
// Safety: test-only environment mutation is serialized by env_lock().
unsafe {
env::set_var("HOME", &home);
env::set_var("USERPROFILE", &home);
env::set_var("CODEWHALE_HOME", &explicit);
}
let _restore = restore;

let legacy_config = home.join(LEGACY_APP_DIR).join(CONFIG_FILE_NAME);
fs::create_dir_all(legacy_config.parent().expect("legacy parent")).expect("legacy dir");
fs::write(&legacy_config, b"provider = \"deepseek\"\n").expect("legacy config");

assert_eq!(
default_config_path().expect("default config path"),
explicit.join(CONFIG_FILE_NAME)
);
assert_eq!(migrate_config_if_needed().expect("migration"), None);
assert!(!explicit.join(CONFIG_FILE_NAME).exists());
assert!(legacy_config.exists());

let _ = fs::remove_dir_all(home);
}

#[test]
fn state_resolvers_reject_path_traversal_subdirs() {
// Defense against path injection (#3240 hardening): the public state
Expand Down
17 changes: 16 additions & 1 deletion crates/secrets/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,22 @@ impl FileKeyringStore {
}
}
let body = serde_json::to_string_pretty(blob)?;
fs::write(&self.path, body)?;
#[cfg(unix)]
{
use std::io::Write;
use std::os::unix::fs::OpenOptionsExt;
let mut file = fs::OpenOptions::new()
.create(true)
.truncate(true)
.write(true)
.mode(0o600)
.open(&self.path)?;
file.write_all(body.as_bytes())?;
}
#[cfg(not(unix))]
{
fs::write(&self.path, body)?;
}
Comment on lines +483 to +498

@gemini-code-assist gemini-code-assist Bot Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Writing directly to the target secrets file (self.path) can lead to data corruption or loss of all stored API keys if the write operation is interrupted (e.g., due to a sudden power loss, disk full error, or process termination). Since this file contains highly sensitive credentials, it is much safer to perform an atomic write by writing to a temporary file in the same directory and then renaming it to the target path.

        let tmp_path = self.path.with_extension("tmp");
        #[cfg(unix)]
        {
            use std::io::Write;
            use std::os::unix::fs::OpenOptionsExt;
            let mut file = fs::OpenOptions::new()
                .create(true)
                .truncate(true)
                .write(true)
                .mode(0o600)
                .open(&tmp_path)?;
            file.write_all(body.as_bytes())?;
        }
        #[cfg(not(unix))]
        {
            fs::write(&tmp_path, &body)?;
        }
        fs::rename(&tmp_path, &self.path)?;

#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
Expand Down
46 changes: 44 additions & 2 deletions crates/state/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,11 @@ impl StateStore {
}

fn conn(&self) -> Result<Connection> {
Connection::open(&self.db_path)
.with_context(|| format!("failed to open state db {}", self.db_path.display()))
let conn = Connection::open(&self.db_path)
.with_context(|| format!("failed to open state db {}", self.db_path.display()))?;
conn.execute_batch("PRAGMA foreign_keys = ON;")
.context("failed to enable sqlite foreign keys")?;
Ok(conn)
}

fn init_schema(&self) -> Result<()> {
Expand Down Expand Up @@ -1867,6 +1870,45 @@ mod tests {
assert!(err.to_string().contains("thread missing-thread not found"));
}

#[test]
fn delete_thread_cascades_child_rows() {
let store = temp_state_store("delete-thread-cascades");
store
.upsert_thread(&test_thread("thread-1"))
.expect("upsert thread");
store
.append_message("thread-1", "user", "hello", Some(serde_json::json!({})))
.expect("append message");
store
.save_checkpoint(
"thread-1",
"checkpoint-1",
&serde_json::json!({ "ok": true }),
)
.expect("save checkpoint");

store.delete_thread("thread-1").expect("delete thread");

let conn = store.conn().expect("conn");
let message_count: i64 = conn
.query_row(
"SELECT COUNT(*) FROM messages WHERE thread_id = ?1",
params!["thread-1"],
|row| row.get(0),
)
.expect("count messages");
let checkpoint_count: i64 = conn
.query_row(
"SELECT COUNT(*) FROM checkpoints WHERE thread_id = ?1",
params!["thread-1"],
|row| row.get(0),
)
.expect("count checkpoints");

assert_eq!(message_count, 0);
assert_eq!(checkpoint_count, 0);
}

#[test]
fn record_thread_goal_usage_accumulates_tokens_and_time() {
let store = temp_state_store("thread-goal-usage");
Expand Down
Loading
Loading