Skip to content

fix: use POSIX path rules on the remote and trust user private groups #816

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

Merged
alphaxiv-bot merged 2 commits into from
Oct 28, 2026
+123 −18
Merged

fix: use POSIX path rules on the remote and trust user private groups #816

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d1cc9b
fix(up_remote): use POSIX path rules on the remote and trust user pri…
paperclover Oct 28, 2026
1bc1017
Merge branch 'main' into clo/use-posix-path-rules-on-the-remote-and-t…
alphaxiv-bot[bot] Oct 28, 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
141 changes: 123 additions & 18 deletions src/commands/up_remote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1944,11 +1943,27 @@ fn parse_remote_install_paths(output: &str) -> Option<RemoteInstallPaths> {
})
}

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<String> {
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<String> {
// 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
Expand Down Expand Up @@ -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\" && \
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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); \
Comment on lines +2156 to +2157

@greptile-apps greptile-apps Bot Oct 28, 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.

P1 security Shared groups are trusted

Matching the primary group name to the username does not prove that the group is private. If another account belongs to alice, it can change an alice:alice executable with mode 0775. The owner check still passes, and the new exception lets the probe run the changed executable as Alice. Verify that no other accounts belong to the group before allowing group writes, or keep rejecting group-writable candidates.

How this was verified: The probe checks group names and file ownership, then directly runs "$p" --version without checking whether other accounts belong to the group.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/commands/up_remote.rs
Line: 2156-2157

Comment:
**Shared groups are trusted**

Matching the primary group name to the username does not prove that the group is private. If another account belongs to `alice`, it can change an `alice:alice` executable with mode `0775`. The owner check still passes, and the new exception lets the probe run the changed executable as Alice. Verify that no other accounts belong to the group before allowing group writes, or keep rejecting group-writable candidates.

**How this was verified:** The probe checks group names and file ownership, then directly runs `"$p" --version` without checking whether other accounts belong to the group.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

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 '{}'; \
Expand Down Expand Up @@ -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();
Comment on lines +2876 to +2878

@greptile-apps greptile-apps Bot Oct 28, 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.

P2 Test assumes the directory's group

The new test assumes the temporary directory belongs to the user's primary group, but it only sets its mode. If TMPDIR points to a shared directory whose set-group-ID bit is set, the fixture inherits that directory's group. When the user's primary group matches their username, the test expects acceptance even though the probe correctly rejects the fixture's different group. This can fail tests for developers using shared temporary directories. Set the fixture's group explicitly or account for its actual group before choosing the expected result.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/commands/up_remote.rs
Line: 2876-2878

Comment:
**Test assumes the directory's group**

The new test assumes the temporary directory belongs to the user's primary group, but it only sets its mode. If `TMPDIR` points to a shared directory whose set-group-ID bit is set, the fixture inherits that directory's group. When the user's primary group matches their username, the test expects acceptance even though the probe correctly rejects the fixture's different group. This can fail tests for developers using shared temporary directories. Set the fixture's group explicitly or account for its actual group before choosing the expected result.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

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(
Expand Down
Loading