Preserve Unix permissions on settings migration files

This commit is contained in:
Scott Werner 2026-10-12 13:53:39 -04:00
parent 857ea59cca
commit 20d8e8235a
5 changed files with 243 additions and 24 deletions

View file

@ -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<PathBuf> {
fn write_next_backup(
path: &Path,
contents: &str,
permissions: &std::fs::Permissions,
) -> Result<PathBuf> {
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!(

View file

@ -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<String> {
}
fn write_next_backup(path: &Path, contents: &str) -> Result<PathBuf> {
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!(

View file

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

View file

@ -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<File> {
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)
}

View file

@ -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::<std::io::Error>()
.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()
);
}