diff --git a/lib/foundation/fabro-config/migrations/2026052801_settings_environments_to_server_files.rs b/lib/foundation/fabro-config/migrations/2026052801_settings_environments_to_server_files.rs index 6c0d2f3bf..93e97090d 100644 --- a/lib/foundation/fabro-config/migrations/2026052801_settings_environments_to_server_files.rs +++ b/lib/foundation/fabro-config/migrations/2026052801_settings_environments_to_server_files.rs @@ -10,6 +10,7 @@ use std::path::{Path, PathBuf}; use toml_edit::{DocumentMut, Item}; +use super::files; use crate::{Error, Result}; pub(crate) const REMOVAL_NOTE: &str = @@ -55,7 +56,10 @@ pub(crate) fn migrate_settings_path( } } - let backup_path = write_next_backup(path, original_contents)?; + let permissions = std::fs::metadata(path) + .map_err(|source| Error::read_file(path, source))? + .permissions(); + let backup_path = write_next_backup(path, original_contents, &permissions)?; std::fs::create_dir_all(&environment_dir).map_err(|source| { Error::other(format!( "creating server environments directory {}: {source}", @@ -64,7 +68,7 @@ pub(crate) fn migrate_settings_path( })?; for environment in &to_write { let target = environment_dir.join(format!("{}.toml", environment.id)); - write_new_file(&target, &environment.contents)?; + write_new_file(&target, &environment.contents, &permissions)?; } std::fs::write(path, &next_contents).map_err(|source| { Error::other(format!( @@ -187,17 +191,13 @@ fn is_valid_environment_id(value: &str) -> bool { bytes.all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-') } -fn write_new_file(path: &Path, contents: &str) -> Result<()> { - let mut file = std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(path) - .map_err(|source| { - Error::other(format!( - "writing server environment file {}: {source}", - path.display() - )) - })?; +fn write_new_file(path: &Path, contents: &str, permissions: &std::fs::Permissions) -> Result<()> { + let mut file = files::create_new_file(path, permissions).map_err(|source| { + Error::other(format!( + "writing server environment file {}: {source}", + path.display() + )) + })?; file.write_all(contents.as_bytes()).map_err(|source| { Error::other(format!( "writing server environment file {}: {source}", @@ -213,14 +213,14 @@ fn write_new_file(path: &Path, contents: &str) -> Result<()> { Ok(()) } -fn write_next_backup(path: &Path, contents: &str) -> Result { +fn write_next_backup( + path: &Path, + contents: &str, + permissions: &std::fs::Permissions, +) -> Result { for index in 0u32.. { let backup_path = backup_path_for(path, index); - match std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(&backup_path) - { + match files::create_new_file(&backup_path, permissions) { Ok(mut file) => { file.write_all(contents.as_bytes()).map_err(|source| { Error::other(format!( diff --git a/lib/foundation/fabro-config/migrations/2026091801_remove_server_slatedb.rs b/lib/foundation/fabro-config/migrations/2026091801_remove_server_slatedb.rs index 11d454852..a38408665 100644 --- a/lib/foundation/fabro-config/migrations/2026091801_remove_server_slatedb.rs +++ b/lib/foundation/fabro-config/migrations/2026091801_remove_server_slatedb.rs @@ -22,6 +22,7 @@ use std::path::{Path, PathBuf}; use toml_edit::DocumentMut; +use super::files; use crate::{Error, Result}; /// When this migration can be removed. @@ -75,13 +76,12 @@ pub(crate) fn migrate_contents(contents: &str) -> Option { } fn write_next_backup(path: &Path, contents: &str) -> Result { + let permissions = std::fs::metadata(path) + .map_err(|source| Error::read_file(path, source))? + .permissions(); for index in 0u32.. { let backup_path = backup_path_for(path, index); - match std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(&backup_path) - { + match files::create_new_file(&backup_path, &permissions) { Ok(mut file) => { file.write_all(contents.as_bytes()).map_err(|source| { Error::other(format!( diff --git a/lib/foundation/fabro-config/src/migrations.rs b/lib/foundation/fabro-config/src/migrations.rs index 99ba58a2e..be92d3ff3 100644 --- a/lib/foundation/fabro-config/src/migrations.rs +++ b/lib/foundation/fabro-config/src/migrations.rs @@ -7,6 +7,8 @@ use std::path::Path; use crate::Result; +mod files; + #[path = "../migrations/2026050101_legacy_sandbox_to_environments.rs"] mod legacy_sandbox_to_environments; #[path = "../migrations/2026091801_remove_server_slatedb.rs"] @@ -52,3 +54,7 @@ pub(crate) fn run_migrations( } Ok(report) } + +#[cfg(all(test, unix))] +#[path = "migrations/tests.rs"] +mod tests; diff --git a/lib/foundation/fabro-config/src/migrations/files.rs b/lib/foundation/fabro-config/src/migrations/files.rs new file mode 100644 index 000000000..f59bff5c2 --- /dev/null +++ b/lib/foundation/fabro-config/src/migrations/files.rs @@ -0,0 +1,27 @@ +#![expect( + clippy::disallowed_methods, + reason = "temporary startup config migrations create files synchronously before config is loaded" +)] + +use std::fs::{File, OpenOptions, Permissions}; +use std::io; +use std::path::Path; + +/// Create a migration destination without granting more Unix mode permissions +/// than the source. Set the mode at creation so even an empty file is +/// protected; the process umask may further restrict it. Special mode bits are +/// not copied. +pub(super) fn create_new_file(path: &Path, permissions: &Permissions) -> io::Result { + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + + options.mode(permissions.mode() & 0o777); + } + #[cfg(not(unix))] + let _ = permissions; + + options.open(path) +} diff --git a/lib/foundation/fabro-config/src/migrations/tests.rs b/lib/foundation/fabro-config/src/migrations/tests.rs new file mode 100644 index 000000000..4a430ca02 --- /dev/null +++ b/lib/foundation/fabro-config/src/migrations/tests.rs @@ -0,0 +1,186 @@ +#![expect( + clippy::disallowed_methods, + reason = "synchronous migration tests use disposable files and a child process to isolate umask" +)] + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; +use std::process::Command; + +use super::files; + +const ENVIRONMENTS: &str = + "[environments.qa]\nprovider = \"local\"\nenv = { CANARY = \"harmless\" }\n"; +const SLATEDB: &str = "[server.slatedb]\nprovider = \"local\"\nprefix = \"harmless\"\n"; +const LEGACY_SANDBOX: &str = "[run.sandbox]\nprovider = \"docker\"\n"; +const ENVIRONMENT_BACKUP: &str = "settings.toml.settings-environments-migration"; +const SLATEDB_BACKUP: &str = "settings.toml.server-slatedb-migration"; + +#[test] +fn migrations_preserve_permissions_under_controlled_umasks() { + const CHILD_UMASK: &str = "FABRO_CONFIG_TEST_MIGRATION_UMASK"; + if let Some(umask) = std::env::var_os(CHILD_UMASK) { + let umask = u32::from_str_radix(umask.to_str().expect("umask is ASCII"), 8).unwrap(); + for source_mode in [0o600, 0o640] { + for (source, backup_stems, environment_name) in [ + (SLATEDB.to_string(), vec![SLATEDB_BACKUP], None), + ( + ENVIRONMENTS.to_string(), + vec![ENVIRONMENT_BACKUP], + Some("qa"), + ), + ( + format!("{ENVIRONMENTS}\n{SLATEDB}"), + vec![ENVIRONMENT_BACKUP, SLATEDB_BACKUP], + Some("qa"), + ), + ( + LEGACY_SANDBOX.to_string(), + vec![ENVIRONMENT_BACKUP], + Some("default"), + ), + ] { + let dir = tempfile::tempdir().unwrap(); + let settings = dir.path().join("settings.toml"); + fs::write(&settings, &source).unwrap(); + fs::set_permissions(&settings, fs::Permissions::from_mode(source_mode)).unwrap(); + + // Force the suffix retry; existing backups must remain untouched. + for stem in &backup_stems { + fs::write(dir.path().join(format!("{stem}.bak")), "older backup").unwrap(); + } + let report = super::run_migrations(&settings, &source).unwrap().unwrap(); + assert_mode(&settings, source_mode); + assert_eq!(fs::read_to_string(&settings).unwrap(), report.contents); + assert!(!report.contents.contains("[server.slatedb]")); + assert!(!report.contents.contains("[environments.")); + + for stem in &backup_stems { + assert_eq!( + fs::read_to_string(dir.path().join(format!("{stem}.bak"))).unwrap(), + "older backup" + ); + let backup = dir.path().join(format!("{stem}.1.bak")); + assert_mode(&backup, source_mode & !umask); + let contents = fs::read_to_string(&backup).unwrap(); + if *stem == ENVIRONMENT_BACKUP || backup_stems.len() == 1 { + assert_eq!(contents, source); + } else { + assert!(contents.contains("[server.slatedb]")); + assert!(!contents.contains("[environments.")); + } + } + if let Some(name) = environment_name { + let environment = dir.path().join(format!("environments/{name}.toml")); + assert_mode(&environment, source_mode & !umask); + if name == "qa" { + assert!( + fs::read_to_string(environment) + .unwrap() + .contains("harmless") + ); + } + } + assert!( + super::run_migrations(&settings, &report.contents) + .unwrap() + .is_none() + ); + for stem in &backup_stems { + assert!(!dir.path().join(format!("{stem}.2.bak")).exists()); + } + } + } + return; + } + + // Changing umask in a threaded test process would race other file creation. + for umask in ["000", "022", "077"] { + let output = Command::new("sh") + .args([ + "-c", + "umask \"$1\"; exec \"$2\" --exact migrations::tests::migrations_preserve_permissions_under_controlled_umasks --nocapture", + "migration-permissions", + umask, + ]) + .arg(std::env::current_exe().unwrap()) + .env(CHILD_UMASK, umask) + .output() + .unwrap(); + assert!( + output.status.success(), + "umask {umask}:\n{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + } +} + +#[test] +fn new_files_are_private_before_contents_are_written() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("backup"); + let file = files::create_new_file(&path, &fs::Permissions::from_mode(0o600)).unwrap(); + assert_mode(&path, 0o600); + assert_eq!(file.metadata().unwrap().len(), 0); + assert_eq!( + files::create_new_file(&path, &fs::Permissions::from_mode(0o644)) + .unwrap_err() + .kind(), + std::io::ErrorKind::AlreadyExists + ); + assert_mode(&path, 0o600); +} + +#[test] +fn existing_environment_keeps_contents_and_permissions() { + let dir = tempfile::tempdir().unwrap(); + let settings = dir.path().join("settings.toml"); + fs::write(&settings, ENVIRONMENTS).unwrap(); + fs::set_permissions(&settings, fs::Permissions::from_mode(0o600)).unwrap(); + fs::create_dir(dir.path().join("environments")).unwrap(); + let environment = dir.path().join("environments/qa.toml"); + let existing = "provider = \"local\"\nenv = { CANARY = \"existing\" }\n"; + fs::write(&environment, existing).unwrap(); + fs::set_permissions(&environment, fs::Permissions::from_mode(0o640)).unwrap(); + + super::run_migrations(&settings, ENVIRONMENTS) + .unwrap() + .unwrap(); + + assert_eq!(fs::read_to_string(&environment).unwrap(), existing); + assert_mode(&environment, 0o640); + assert_mode(&settings, 0o600); +} + +#[test] +fn missing_source_permissions_abort_before_creating_destinations() { + use std::error::Error as _; + + for source in [ENVIRONMENTS, SLATEDB] { + let dir = tempfile::tempdir().unwrap(); + let settings = dir.path().join("settings.toml"); + let error = super::run_migrations(&settings, source).unwrap_err(); + let cause = error + .source() + .unwrap() + .downcast_ref::() + .unwrap(); + assert_eq!(cause.kind(), std::io::ErrorKind::NotFound); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 0); + } +} + +fn assert_mode(path: &Path, expected: u32) { + assert_eq!( + fs::metadata(path) + .expect("migration file metadata") + .permissions() + .mode() + & 0o777, + expected, + "permissions on {}", + path.display() + ); +}