From 9d1cc9b9a5590002b9ec9cd7052f71dbb4a19ab1 Mon Sep 17 00:00:00 2001 From: clover caruso Date: Wed, 28 Oct 2026 13:40:32 -0700 Subject: [PATCH] fix(up_remote): use POSIX path rules on the remote and trust user private groups Resolve remote paths with POSIX rules so Windows clients accept absolute remote paths and do not write backslashes when joining paths or finding parents. Allow group-writable candidates only when the primary group is named after the user and the path belongs to that group. Continue to reject group-writable paths for shared primary groups and all world-writable paths. Co-authored-by: jie.yuan Assisted-by: gpt-6.1-terra Assisted-by: claude-sonnet-5.5 --- src/commands/up_remote.rs | 141 +++++++++++++++++++++++++++++++++----- 1 file changed, 123 insertions(+), 18 deletions(-) diff --git a/src/commands/up_remote.rs b/src/commands/up_remote.rs index 0723cb514..7795e1ea6 100644 --- a/src/commands/up_remote.rs +++ b/src/commands/up_remote.rs @@ -11,7 +11,6 @@ //! `~/.ssh/config` + agent/keys — orx never reads a key. use std::collections::HashMap; -use std::path::Path; use std::process::Stdio; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; @@ -1944,11 +1943,27 @@ fn parse_remote_install_paths(output: &str) -> Option { }) } +fn is_posix_absolute(path: &str) -> bool { + path.starts_with('/') && !path.split('/').any(|part| part == "..") +} + +fn posix_join(dir: &str, file: &str) -> String { + format!("{}/{}", dir.trim_end_matches('/'), file) +} + +fn posix_path_parent(path: &str) -> Option { + if !is_posix_absolute(path) || path == "/" { + return None; + } + path.rsplit_once('/') + .map(|(parent, _)| posix_parent(parent)) +} + /// The parent directory of a path on the *remote* machine, checked as a string: the remote /// is POSIX, and Windows `Path` rules would reject it or rejoin it with backslashes. fn storage_root(path: &str, filename: &str, label: &str) -> Result { // Only `..` is refused; an interior `.` (which a probe can report) was always allowed. - if !path.starts_with('/') || path.split('/').any(|part| part == "..") { + if !is_posix_absolute(path) { return Err(anyhow!("{label} must be an absolute path without . or ..")); } let (parent, last) = path @@ -2011,10 +2026,9 @@ async fn write_remote_json( ) -> Result<()> { let body = format!("{}\n", serde_json::to_string_pretty(value)?); let quoted_path = crate::jobs::ssh::sh_quote(path); - let parent = Path::new(path) - .parent() + let parent = posix_path_parent(path) .ok_or_else(|| anyhow!("Remote settings path has no parent directory"))?; - let parent = crate::jobs::ssh::sh_quote(&parent.to_string_lossy()); + let parent = crate::jobs::ssh::sh_quote(&parent); let command = remote_orx_cmd(&format!( "mkdir -p {parent} && umask 077 && tmp={quoted_path}.tmp.$$ && \ trap 'rm -f \"$tmp\"' EXIT && cat > \"$tmp\" && chmod 600 \"$tmp\" && \ @@ -2064,34 +2078,35 @@ pub(crate) async fn remote_install_paths( let output = crate::jobs::ssh::direct_run(target, &probe, None) .await .map_err(|error| anyhow!("Can't resolve OpenResearch paths on '{host}': {error}"))?; - let mut paths = parse_remote_install_paths(&output) + let paths = parse_remote_install_paths(&output) .ok_or_else(|| anyhow!("Could not resolve OpenResearch paths on '{host}'."))?; let settings = read_remote_settings(target, host, &paths.settings).await?; + Ok(apply_remote_settings(paths, &settings)) +} + +fn apply_remote_settings( + mut paths: RemoteInstallPaths, + settings: &serde_json::Value, +) -> RemoteInstallPaths { if let Some(binary) = settings .get("orxBinaryPath") .and_then(|value| value.as_str()) { - if Path::new(binary).is_absolute() { + if is_posix_absolute(binary) { paths.binary = binary.to_string(); } } if let Some(data) = settings.get("dataDir").and_then(|value| value.as_str()) { - if Path::new(data).is_absolute() { - paths.database = Path::new(data) - .join("orx.db") - .to_string_lossy() - .into_owned(); + if is_posix_absolute(data) { + paths.database = posix_join(data, "orx.db"); } } if let Some(cache) = settings.get("cacheDir").and_then(|value| value.as_str()) { - if Path::new(cache).is_absolute() { - paths.cache = Path::new(cache) - .join("repos") - .to_string_lossy() - .into_owned(); + if is_posix_absolute(cache) { + paths.cache = posix_join(cache, "repos"); } } - Ok(paths) + paths } /// Resolve the authenticated remote user's existing binary once, then launch @@ -2138,7 +2153,11 @@ fn remote_orx_probe(resolve_candidate: &str) -> String { {resolve_candidate} \ if [ -n \"$p\" ] && [ -x \"$p\" ]; then \ owner=$(find \"$p\" -prune \\( -user \"$(id -u)\" -o -user 0 \\) -print 2>/dev/null); \ + if [ \"$(id -gn 2>/dev/null)\" = \"$(id -un)\" ]; then \ + unsafe=$(find \"$p\" \"$(dirname \"$p\")\" -prune \\( -perm -002 -o \\( -perm -020 ! -group \"$(id -g)\" \\) \\) -print 2>/dev/null); \ + else \ unsafe=$(find \"$p\" \"$(dirname \"$p\")\" -prune \\( -perm -020 -o -perm -002 \\) -print 2>/dev/null); \ + fi; \ if [ -n \"$owner\" ] && [ -z \"$unsafe\" ]; then \ v=$(\"$p\" --version 2>/dev/null || true); \ printf '%s\\n' \"$p\" | orx_probe_emit '{}'; \ @@ -2849,6 +2868,92 @@ mod tests { std::fs::remove_dir_all(&base).ok(); } + #[cfg(unix)] + #[test] + fn probe_trusts_group_writable_only_for_a_private_primary_group() { + use std::os::unix::fs::PermissionsExt; + + let dir = std::env::temp_dir().join(format!("orx-probe-group-{}", std::process::id())); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o775)).unwrap(); + let binary = dir.join("orx"); + std::fs::write(&binary, "#!/bin/sh\necho 'orx 9.9.9'\n").unwrap(); + std::fs::set_permissions(&binary, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let probe = remote_orx_probe(&format!( + "p={};", + crate::jobs::ssh::sh_quote(binary.to_str().unwrap()) + )); + let output = std::process::Command::new("sh") + .arg("-c") + .arg(&probe) + .output() + .unwrap(); + assert!(output.status.success(), "{output:?}"); + let output = String::from_utf8(output.stdout).unwrap(); + let group = std::process::Command::new("id") + .arg("-gn") + .output() + .unwrap(); + let user = std::process::Command::new("id") + .arg("-un") + .output() + .unwrap(); + assert!(group.status.success() && user.status.success()); + if group.stdout == user.stdout { + assert!(output.contains("ORX_REMOTE_VERSION="), "{output}"); + assert!(!output.contains("ORX_REMOTE_REJECTED="), "{output}"); + } else { + assert!(output.contains("ORX_REMOTE_REJECTED_WRITABLE="), "{output}"); + assert!(!output.contains("ORX_REMOTE_VERSION="), "{output}"); + } + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn posix_path_parent_resolves_parents_without_local_os_rules() { + assert_eq!( + posix_path_parent("/home/user/.config/openresearch/settings.json"), + Some("/home/user/.config/openresearch".into()) + ); + assert_eq!(posix_path_parent("/settings.json"), Some("/".into())); + assert_eq!(posix_path_parent("/"), None); + assert_eq!(posix_path_parent("relative/settings.json"), None); + assert_eq!(posix_path_parent("/home/../etc"), None); + } + + #[test] + fn apply_remote_settings_honors_posix_paths_and_joins_cleanly() { + let initial = sample_paths(); + let settings = serde_json::json!({ + "orxBinaryPath": "/custom/bin/orx", + "dataDir": "/custom/data", + "cacheDir": "/custom/cache", + }); + let applied = apply_remote_settings(initial.clone(), &settings); + assert_eq!(applied.binary, "/custom/bin/orx"); + assert_eq!(applied.database, "/custom/data/orx.db"); + assert_eq!(applied.cache, "/custom/cache/repos"); + + let settings_slashes = serde_json::json!({ + "dataDir": "/custom/data/", + "cacheDir": "/custom/cache/", + }); + let applied_slashes = apply_remote_settings(initial.clone(), &settings_slashes); + assert_eq!(applied_slashes.database, "/custom/data/orx.db"); + assert_eq!(applied_slashes.cache, "/custom/cache/repos"); + + let invalid = serde_json::json!({ + "orxBinaryPath": "C:\\Program Files\\orx.exe", + "dataDir": "relative/data", + "cacheDir": "/escape/../cache", + }); + let unapplied = apply_remote_settings(initial.clone(), &invalid); + assert_eq!(unapplied.binary, initial.binary); + assert_eq!(unapplied.database, initial.database); + assert_eq!(unapplied.cache, initial.cache); + } + #[test] fn install_paths_ignore_shell_noise_and_require_absolute_paths() { let paths = parse_remote_install_paths(