From 24b3e6809489cb0576a99210851c000e23e2b95d Mon Sep 17 00:00:00 2001 From: Cylae <13425054+Cylae@users.noreply.github.com> Date: Fri, 11 Sep 2026 00:05:42 +0000 Subject: [PATCH] security: enforce path traversal validation and deterministic user listing --- server_manager/src/core/atomic_io.rs | 1 + server_manager/src/core/journal.rs | 1 + server_manager/src/core/lock.rs | 1 + server_manager/src/core/users.rs | 23 +++++++++++++++++++++- server_manager/tests/contract_atomic_io.rs | 6 ++++++ server_manager/tests/contract_journal.rs | 6 ++++++ server_manager/tests/contract_locking.rs | 6 ++++++ 7 files changed, 43 insertions(+), 1 deletion(-) diff --git a/server_manager/src/core/atomic_io.rs b/server_manager/src/core/atomic_io.rs index 07e2030..af14fc4 100644 --- a/server_manager/src/core/atomic_io.rs +++ b/server_manager/src/core/atomic_io.rs @@ -16,6 +16,7 @@ use std::os::unix::fs::OpenOptionsExt; /// 5. Atomically renames the temporary file to the destination path. pub fn atomic_write>(path: P, content: &[u8], mode: u32) -> Result<()> { let dest = path.as_ref(); + crate::core::validate::validate_safe_path(dest)?; let parent = dest.parent().unwrap_or_else(|| Path::new(".")); if !parent.as_os_str().is_empty() { fs::create_dir_all(parent) diff --git a/server_manager/src/core/journal.rs b/server_manager/src/core/journal.rs index e4adc45..555a3a3 100644 --- a/server_manager/src/core/journal.rs +++ b/server_manager/src/core/journal.rs @@ -67,6 +67,7 @@ impl Journal { /// Opens or creates the journal file with 0600 permissions. pub fn open_or_create>(path: P) -> Result { let target = path.as_ref(); + crate::core::validate::validate_safe_path(target)?; let parent = target.parent().unwrap_or_else(|| Path::new(".")); if !parent.as_os_str().is_empty() { fs::create_dir_all(parent) diff --git a/server_manager/src/core/lock.rs b/server_manager/src/core/lock.rs index fb3e8cd..c195eba 100644 --- a/server_manager/src/core/lock.rs +++ b/server_manager/src/core/lock.rs @@ -16,6 +16,7 @@ impl ProcessLock { /// If `non_blocking` is true and the lock is already held, returns an error immediately. pub fn acquire>(path: P, non_blocking: bool) -> Result { let target = path.as_ref(); + crate::core::validate::validate_safe_path(target)?; let parent = target.parent().unwrap_or_else(|| Path::new(".")); if !parent.as_os_str().is_empty() { let _ = std::fs::create_dir_all(parent); diff --git a/server_manager/src/core/users.rs b/server_manager/src/core/users.rs index 46ff68a..dfc78e3 100644 --- a/server_manager/src/core/users.rs +++ b/server_manager/src/core/users.rs @@ -363,7 +363,10 @@ impl UserManager { } pub fn list_users(&self) -> Vec<&User> { - self.users.values().collect() + let mut list: Vec<&User> = Vec::with_capacity(self.users.len()); + list.extend(self.users.values()); + list.sort_by(|a, b| a.username.cmp(&b.username)); + list } } @@ -537,4 +540,22 @@ mod tests { assert!(!Role::Observer.can_trigger_updates()); assert!(!Role::Auditor.can_trigger_updates()); } + + #[test] + fn test_list_users_deterministic_sorting() { + let mut manager = UserManager::default(); + manager + .add_user("charlie", "pass123", Role::Observer, None) + .expect("add charlie"); + manager + .add_user("alice", "pass123", Role::Admin, None) + .expect("add alice"); + manager + .add_user("bob", "pass123", Role::Operator, None) + .expect("add bob"); + + let users = manager.list_users(); + let usernames: Vec<&str> = users.iter().map(|u| u.username.as_str()).collect(); + assert_eq!(usernames, vec!["alice", "bob", "charlie"]); + } } diff --git a/server_manager/tests/contract_atomic_io.rs b/server_manager/tests/contract_atomic_io.rs index 1c96f20..7934a24 100644 --- a/server_manager/tests/contract_atomic_io.rs +++ b/server_manager/tests/contract_atomic_io.rs @@ -31,6 +31,12 @@ fn test_atomic_write_creates_file_with_content() { let _ = fs::remove_dir_all(&temp_dir); } +#[test] +fn test_atomic_write_path_traversal_forbidden() { + let invalid_path = std::path::Path::new("subdir/../forbidden.txt"); + assert!(atomic_write_str(invalid_path, "forbidden", 0o600).is_err()); +} + #[test] fn test_atomic_write_overwrites_existing_file() { let temp_dir = std::env::temp_dir().join(format!( diff --git a/server_manager/tests/contract_journal.rs b/server_manager/tests/contract_journal.rs index 6ed31d8..02d7955 100644 --- a/server_manager/tests/contract_journal.rs +++ b/server_manager/tests/contract_journal.rs @@ -45,6 +45,12 @@ fn test_journal_creation_and_append() { let _ = fs::remove_dir_all(&temp_dir); } +#[test] +fn test_journal_path_traversal_forbidden() { + let invalid_path = std::path::Path::new("subdir/../forbidden_journal.jsonl"); + assert!(Journal::open_or_create(invalid_path).is_err()); +} + #[test] fn test_journal_compensatory_rollback_in_reverse_order() { let temp_dir = std::env::temp_dir().join(format!( diff --git a/server_manager/tests/contract_locking.rs b/server_manager/tests/contract_locking.rs index 49bbbc3..893de45 100644 --- a/server_manager/tests/contract_locking.rs +++ b/server_manager/tests/contract_locking.rs @@ -39,3 +39,9 @@ fn test_process_lock_acquisition_and_mutual_exclusion() { let _ = fs::remove_dir_all(&temp_dir); } + +#[test] +fn test_process_lock_path_traversal_forbidden() { + let invalid_path = std::path::Path::new("subdir/../forbidden.lock"); + assert!(ProcessLock::acquire(invalid_path, true).is_err()); +}