From 347aa397d04539d4e6f535554ee0b4f33578dc24 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 1 Oct 2026 13:45:08 +0000 Subject: [PATCH 1/2] fix(runtime): compare paths by the folder they name, not by their text `runtime_install_root_is_protected` decides whether a folder may be handed to `fs::remove_dir_all`. It compared normalized path TEXT, and off Windows that normalization is `value.to_owned()` -- a no-op. So the check `path == root || root in path.ancestors()` saw `/etc/` and `/etc` as different folders, and nothing resolved `.` or `..` at all. Measured against the real gate, `ensure_runtime_install_root_is_safe_to_remove` accepted every one of `/etc/`, `//etc`, `/./etc`, `/etc/.`, `/usr/` and `$HOME/../../etc`, refusing only the exact spelling `/etc`. The trailing-slash case needs no adversary: a `dirname`, a shell variable or a hand-edit produces it. The `..` case defeats the guard differently -- `Path::ancestors()` treats `..` as an ordinary component, so a path that escapes `$HOME` still looks like it is inside it and takes the home exemption's early return. The guard's own comment says it exists for "a hand-edited or corrupted registry entry [pointing] `install_root` at a protected system location". It failed at exactly that. The existing test passed throughout because it used `/etc/rocm-cli-test-runtime` -- a subpath, the one shape that works. Both comparisons now resolve `.`, `..`, repeated separators and a trailing separator before comparing. Folding `\` to `/` stays gated on Windows: off Windows a backslash is an ordinary filename byte, so a directory named `..\tmp` inside `/usr` must not be rewritten into `/usr/../tmp`. Raw-text comparison tolerated an unconditional fold because equality never *removed* components; resolving `..` does, which would have turned this fix into a new bypass. The resolution is lexical because these paths often do not exist yet, so `canonicalize` has nothing to resolve. It is NOT symlink-safe, and the comments say so rather than claiming otherwise: because the home exemption is consulted first, a symlink the user owns is enough -- with `$HOME/link -> /etc`, `$HOME/link/child` is lexically inside home, so the guard reports it removable while the kernel lands in `/etc`. Closing that needs a decision about canonicalizing the part of the path that does exist, and belongs in its own change. Two further consumers of the same comparison are fixed by this: `chat_install_prefix_is_system`, which gates an install prefix that arrives as model-generated JSON, and `rocmd`'s `system_prefix_requires_ack`, which stopped demanding acknowledgement for a `..` path that escaped home. The guard is pinned by properties rather than more examples, because the contract is about every spelling of a path and an example only ever covers the spellings someone thought of. The reference the properties measure against is derived from POSIX semantics, not restated from the code, so it does not inherit the code's mistakes -- it is what makes a one-component change to the generator alphabet catch the backslash case above. The comparison helpers are now parameterised by platform instead of reading `runtime_is_windows()`, so the Windows path-resolution rules are exercised from a Linux host. Note this covers the helpers only: the branch selecting the Windows protected-root list still reads `runtime_is_windows()` and remains reachable only on the Windows lane. Parameterising also required the containment test to compare bytes rather than `&path[..n]`, which splits a non-ASCII path mid-character and panics. On Windows this trades one inaccuracy for another, deliberately: direct respellings (`C:/Windows/`, `c:/windows`, `C:/Windows//System32`) are now caught, while `C:\Windows\..\ProgramData` -- previously refused only because the text walk matched `C:\Windows` as a literal ancestor -- now correctly resolves to `C:/ProgramData` and is accepted, because that path is not on the list. The three-entry Windows list omits the drive root, `C:/Users` and `C:/ProgramData`, and hardcodes drive `C:` and English folder names. That is an incomplete policy rather than a broken comparison, it was previously masked by the ancestor-walk bug, and it deserves its own change. Signed-off-by: Roman Inflianskas --- Cargo.lock | 31 ++ MANIFEST.md | 3 + apps/rocm/src/main.rs | 58 ++ crates/rocm-core/Cargo.toml | 10 + .../proptest-regressions/runtime.txt | 11 + crates/rocm-core/src/runtime.rs | 515 +++++++++++++++++- 6 files changed, 612 insertions(+), 16 deletions(-) create mode 100644 crates/rocm-core/proptest-regressions/runtime.txt diff --git a/Cargo.lock b/Cargo.lock index b86229f32..2036089c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3178,6 +3178,21 @@ dependencies = [ "version_check", ] +[[package]] +name = "proptest" +version = "1.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b45fcc2344c680f5025fe57779faef368840d0bd1f42f216291f0dc4ace4744" +dependencies = [ + "bitflags 2.13.0", + "num-traits", + "rand 0.9.4", + "rand_chacha 0.9.0", + "rand_xorshift", + "regex-syntax", + "unarray", +] + [[package]] name = "pulldown-cmark" version = "0.12.2" @@ -3334,6 +3349,15 @@ dependencies = [ "getrandom 0.3.4", ] +[[package]] +name = "rand_xorshift" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "513962919efc330f829edb2535844d1b912b0fbe2ca165d613e4e8788bb05a5a" +dependencies = [ + "rand_core 0.9.5", +] + [[package]] name = "ratatui" version = "0.30.2" @@ -3672,6 +3696,7 @@ dependencies = [ "cc", "directories", "libc", + "proptest", "rand 0.9.4", "regex", "rsa", @@ -5219,6 +5244,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicase" version = "2.9.0" diff --git a/MANIFEST.md b/MANIFEST.md index 5e6c7ed20..1c9c7c242 100644 --- a/MANIFEST.md +++ b/MANIFEST.md @@ -353,6 +353,7 @@ repository. | proc-macro-crate | 3.5.0 | MIT OR Apache-2.0 | | proc-macro2 | 1.0.106 | MIT OR Apache-2.0 | | proc-macro2-diagnostics | 0.10.1 | MIT/Apache-2.0 | +| proptest | 1.11.0 | MIT OR Apache-2.0 | | pulldown-cmark | 0.12.2 | MIT | | quick-xml | 0.39.4 | MIT | | quinn | 0.11.11 | MIT OR Apache-2.0 | @@ -367,6 +368,7 @@ repository. | rand_chacha | 0.9.0 | MIT OR Apache-2.0 | | rand_core | 0.6.4 | MIT OR Apache-2.0 | | rand_core | 0.9.5 | MIT OR Apache-2.0 | +| rand_xorshift | 0.4.0 | MIT OR Apache-2.0 | | ratatui | 0.30.2 | MIT | | ratatui-core | 0.1.2 | MIT | | ratatui-crossterm | 0.1.2 | MIT | @@ -515,6 +517,7 @@ repository. | typenum | 1.20.1 | MIT OR Apache-2.0 | | ucd-trie | 0.1.7 | MIT OR Apache-2.0 | | uds_windows | 1.2.1 | MIT | +| unarray | 0.1.4 | MIT OR Apache-2.0 | | unicase | 2.9.0 | MIT OR Apache-2.0 | | unicode-ident | 1.0.24 | (MIT OR Apache-2.0) AND Unicode-3.0 | | unicode-linebreak | 0.1.5 | Apache-2.0 | diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index e600faf20..1dac9e994 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -33986,6 +33986,64 @@ ID_LIKE="suse opensuse" assert!(err.to_string().contains("protected system location")); } + /// The gate above is the last thing between a registry entry and + /// `fs::remove_dir_all`, and the path it is handed is text somebody else + /// wrote. One folder has many spellings, so the refusal has to survive all + /// of them — a trailing separator, a doubled one, a `.`, or a `..` that + /// walks back out of the user's home directory. Each of these names `/etc`. + #[test] + #[cfg(unix)] + fn ensure_runtime_install_root_rejects_every_spelling_of_a_protected_path() { + let home = rocm_core::runtime_home_dir().expect("a home directory"); + let home = home.display().to_string(); + let spellings = [ + "/etc/".to_owned(), + "//etc".to_owned(), + "/./etc".to_owned(), + "/etc/.".to_owned(), + format!("{home}/../../etc"), + ]; + + let accepted: Vec = spellings + .into_iter() + .filter(|text| ensure_runtime_install_root_is_safe_to_remove(Path::new(text)).is_ok()) + .collect(); + + assert!( + accepted.is_empty(), + "accepted as safe to recursively delete: {accepted:?}" + ); + } + + /// The same question — "is this a system location?" — also keeps the local + /// assistant from installing into one, and there the folder arrives as + /// free-form JSON from a model rather than from a file the user wrote. A + /// trailing separator is exactly the sort of thing generated text carries, + /// so the refusal has to survive every spelling here too. + #[test] + #[cfg(unix)] + fn the_assistant_cannot_name_a_system_install_folder_by_respelling_it() { + let home = rocm_core::runtime_home_dir().expect("a home directory"); + let home = home.display().to_string(); + let spellings = [ + "/usr/".to_owned(), + "/opt/".to_owned(), + "//usr".to_owned(), + "/./opt".to_owned(), + format!("{home}/../../usr"), + ]; + + let accepted: Vec = spellings + .into_iter() + .filter(|text| !chat_install_prefix_is_system(Path::new(text))) + .collect(); + + assert!( + accepted.is_empty(), + "accepted as a user install folder: {accepted:?}" + ); + } + #[test] fn plan_runtime_uninstall_does_not_mutate() -> Result<()> { let (root, paths) = test_paths("runtime-uninstall-plan-dry-run"); diff --git a/crates/rocm-core/Cargo.toml b/crates/rocm-core/Cargo.toml index e96bc4af9..05969b4cc 100644 --- a/crates/rocm-core/Cargo.toml +++ b/crates/rocm-core/Cargo.toml @@ -27,5 +27,15 @@ sysinfo.workspace = true toml = "0.8" ureq = { version = "2.12", features = ["native-certs"] } +[dev-dependencies] +# Property-based tests for the path comparison that decides whether a folder +# may be recursively deleted. The contract — "a path that resolves inside a +# protected system root is never reported unprotected" — has to hold for EVERY +# spelling of that path, which is exactly the shape an example test misses: the +# pre-existing example used `/etc/rocm-cli-test-runtime`, the one shape that +# happened to work, while `/etc/` and `$HOME/../../etc` sailed through. +# Test-only, so it adds nothing to any shipped binary. +proptest = { version = "1", default-features = false, features = ["std"] } + [target.'cfg(target_os = "windows")'.dependencies] windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_System_Registry", "Win32_System_SystemInformation", "Win32_System_Threading"] } diff --git a/crates/rocm-core/proptest-regressions/runtime.txt b/crates/rocm-core/proptest-regressions/runtime.txt new file mode 100644 index 000000000..0f1be8be5 --- /dev/null +++ b/crates/rocm-core/proptest-regressions/runtime.txt @@ -0,0 +1,11 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 44838f606145dd05b27407447aa76a8a775e5a0d7a547bd898e4fb6488871799 # shrinks to text = "/bin/" +cc 5dc93bbc0c3b8a6fda0a1f14eba8364422f99adb63c0a4c3df90a55c903fd631 # shrinks to text = "/bin/" +cc a25d13c5221f176eaf8f3b0647babe58fe2984d7f0d3190b1a1b1fdc2e74d273 # shrinks to path = "/opt/", base = "/opt" +cc aa4f3b9ad94cb072d17df2d8c448929748d0a76fad6f04a0a76463f97ada9d60 # shrinks to text = "/bin/..\\tmp/.." +cc 00244038f28f97b7497323fe552197d8efe1e5006c07a42a7e8bbd83ba7f956a # shrinks to text = "/bin/..\\tmp" diff --git a/crates/rocm-core/src/runtime.rs b/crates/rocm-core/src/runtime.rs index c7842e778..729fb1c69 100644 --- a/crates/rocm-core/src/runtime.rs +++ b/crates/rocm-core/src/runtime.rs @@ -485,16 +485,123 @@ pub fn runtime_drive_root_for_key(ch: char) -> Option { path.is_dir().then_some(path) } +/// Split a forward-slash path into the prefix that `..` must never climb past +/// and the components that follow it. +/// +/// An absolute path's root, a Windows drive, and a UNC share are all anchors: +/// `/..` is `/` on every POSIX system, and no amount of `..` leaves `C:\`. A +/// relative path has no anchor, so a leading `..` there is meaningful and is +/// kept. +fn split_runtime_path_anchor(value: &str, platform: RuntimePlatform) -> (&str, &str) { + // A drive letter and a UNC share are anchors only where they mean anything. + // On Linux `C:/x` is an ordinary relative name and `//etc` is just `/etc`, + // so reading either as a root would invent a path that is not there. + // + // The platform is a parameter rather than `runtime_is_windows()` so that the + // Windows side of a guard against recursive deletion can be tested from a + // Linux host, which is the only host this repository runs clippy and most + // of its unit tests on. + if platform.is_windows() { + if let Some(rest) = value.strip_prefix("//") { + // `//server/share/...`: the share itself is the anchor. + let share_end = rest + .match_indices('/') + .nth(1) + .map_or(rest.len(), |(index, _)| index); + let (anchor_tail, rest) = rest.split_at(share_end); + return (&value[..2 + anchor_tail.len()], rest); + } + let bytes = value.as_bytes(); + if bytes.len() >= 2 && bytes[0].is_ascii_alphabetic() && bytes[1] == b':' { + let drive_end = if bytes.len() > 2 && bytes[2] == b'/' { + 3 + } else { + 2 + }; + return value.split_at(drive_end); + } + } + if value.starts_with('/') { + return value.split_at(1); + } + ("", value) +} + +/// Collapse `.`, `..`, repeated separators and a trailing separator, so that +/// every spelling of one folder reduces to one string. +/// +/// Purely lexical, and deliberately so: this feeds the comparisons that decide +/// whether a folder may be recursively deleted, and those run against paths +/// that often do not exist yet (or exist only in a registry entry), where +/// `canonicalize` has nothing to resolve. +/// +/// This is NOT symlink-safe, and the gap is not merely theoretical. Because +/// [`runtime_install_root_is_protected`] exempts anything inside the user's +/// home before it consults the protected-root list, a symlink the user owns is +/// enough: with `$HOME/link -> /etc`, the path `$HOME/link/child` is lexically +/// inside home, so the guard reports it removable while the kernel lands in +/// `/etc`. Resolving lexically is strictly better than the raw-text comparison +/// it replaces — it closes every *spelling* of a protected path — but it does +/// not close that hole, and closing it needs a decision about canonicalizing +/// the part of the path that does exist, which is a separate change. +fn lexically_resolved_runtime_path_text(value: &str, platform: RuntimePlatform) -> String { + let (anchor, rest) = split_runtime_path_anchor(value, platform); + let anchored = !anchor.is_empty(); + let mut parts: Vec<&str> = Vec::new(); + for part in rest.split('/') { + match part { + "" | "." => {} + ".." => match parts.last() { + // A relative path keeps the `..` it cannot resolve: `../a` names + // a real place, just not one this function can name differently. + None if !anchored => parts.push(".."), + Some(&"..") => parts.push(".."), + // Anchored and already at the top: `/..` is `/`. + None => {} + Some(_) => { + parts.pop(); + } + }, + other => parts.push(other), + } + } + let joined = parts.join("/"); + if anchored { + format!("{}{joined}", anchor.trim_end_matches('/').to_owned() + "/") + } else if joined.is_empty() { + ".".to_owned() + } else { + joined + } +} + +/// Reduce a path to the single text that names its folder, whatever spelling it +/// arrived in. +fn comparable_runtime_path_text(path: &Path, platform: RuntimePlatform) -> String { + let text = normalize_runtime_path_text_for_platform(&path.display().to_string(), platform); + // Folding `\` to `/` is a WINDOWS rule and must stay gated on the platform. + // Off Windows a backslash is an ordinary filename byte, so a directory + // genuinely named `..\tmp` inside `/usr` would otherwise be rewritten to + // `/usr/../tmp` and the `..` resolution below would walk it straight out of + // the protected root — turning a guard into a bypass. Raw-text comparison + // tolerated the unconditional fold because equality never *removed* + // components; resolving `..` does. + let text = if platform.is_windows() { + text.replace('\\', "/") + } else { + text + }; + lexically_resolved_runtime_path_text(&text, platform) +} + pub fn runtime_paths_equivalent(left: &Path, right: &Path) -> bool { - let left = normalize_runtime_path_for_host(left) - .display() - .to_string() - .replace('\\', "/"); - let right = normalize_runtime_path_for_host(right) - .display() - .to_string() - .replace('\\', "/"); - if runtime_is_windows() { + runtime_paths_equivalent_on(left, right, RuntimePlatform::current()) +} + +fn runtime_paths_equivalent_on(left: &Path, right: &Path, platform: RuntimePlatform) -> bool { + let left = comparable_runtime_path_text(left, platform); + let right = comparable_runtime_path_text(right, platform); + if platform.is_windows() { left.eq_ignore_ascii_case(&right) } else { left == right @@ -502,14 +609,27 @@ pub fn runtime_paths_equivalent(left: &Path, right: &Path) -> bool { } pub fn runtime_path_is_same_or_inside(path: &Path, base: &Path) -> bool { - let path = normalize_runtime_path_for_host(path); - let base = normalize_runtime_path_for_host(base); - if runtime_paths_equivalent(&path, &base) { - return true; + runtime_path_is_same_or_inside_on(path, base, RuntimePlatform::current()) +} + +fn runtime_path_is_same_or_inside_on(path: &Path, base: &Path, platform: RuntimePlatform) -> bool { + // Compared after resolution rather than by walking `Path::ancestors`, which + // treats `..` as an ordinary component and so counts a path that climbs + // back OUT of `base` as still inside it. + let path = comparable_runtime_path_text(path, platform); + let base = comparable_runtime_path_text(base, platform); + let inside_prefix = format!("{}/", base.trim_end_matches('/')); + if path.len() <= inside_prefix.len() { + return runtime_paths_equivalent_on(Path::new(&path), Path::new(&base), platform); + } + // Compared as bytes: a path may hold any UTF-8, and slicing a `str` at a + // byte offset that lands mid-character panics. + let head = &path.as_bytes()[..inside_prefix.len()]; + if platform.is_windows() { + head.eq_ignore_ascii_case(inside_prefix.as_bytes()) + } else { + head == inside_prefix.as_bytes() } - path.ancestors() - .skip(1) - .any(|ancestor| runtime_paths_equivalent(ancestor, &base)) } const MANAGED_RUNTIME_FORMATS: [&str; 2] = ["wheel", "tarball"]; @@ -1278,4 +1398,367 @@ mod tests { assert_eq!(split_windows_path_list_text(&joined), entries.to_vec()); } + + /// The Windows side of the same comparisons, exercised from whatever host + /// runs the tests. `runtime_is_windows()` is decided at compile time, so + /// without a platform parameter this branch would be checked only by the + /// one CI lane that runs on Windows — and it is the branch that decides + /// whether `C:\Windows` may be recursively deleted. + #[test] + fn windows_containment_sees_through_spelling_and_case() { + let windows = RuntimePlatform::Windows; + let system_root = Path::new("C:/Windows"); + + for inside in [ + r"C:\Windows", + "C:/Windows/", + "c:/windows", + "C:/Windows/./System32", + "C:/Program Files/../Windows/System32", + "C:/Windows//System32", + ] { + assert!( + runtime_path_is_same_or_inside_on(Path::new(inside), system_root, windows), + "{inside} names C:/Windows or something under it" + ); + } + + for outside in [ + "C:/Users/dev/.rocm", + "C:/Windows/../Users/dev", + "C:/WindowsApps", + "D:/Windows", + ] { + assert!( + !runtime_path_is_same_or_inside_on(Path::new(outside), system_root, windows), + "{outside} is not inside C:/Windows" + ); + } + + // A drive is an anchor: `..` can never climb off it onto another one. + // (The resolver is handed forward slashes; the conversion happens in + // `comparable_runtime_path_text`, which the assertions above go through.) + assert_eq!( + lexically_resolved_runtime_path_text("C:/../../Windows", windows), + "C:/Windows" + ); + // A UNC share is an anchor too. + assert_eq!( + lexically_resolved_runtime_path_text("//server/share/../../rocm", windows), + "//server/share/rocm" + ); + } + + /// The same text means different things on the two platforms, so the + /// resolution must not borrow Windows' reading on Linux: `//etc` is `/etc` + /// there, and `C:/x` is an ordinary relative name, not a drive. + #[test] + fn linux_resolution_does_not_borrow_windows_anchors() { + let linux = RuntimePlatform::Linux; + + assert_eq!(lexically_resolved_runtime_path_text("//etc", linux), "/etc"); + assert_eq!( + lexically_resolved_runtime_path_text("/etc/./../etc/", linux), + "/etc" + ); + // `/..` is `/` on every POSIX system. + assert_eq!(lexically_resolved_runtime_path_text("/../..", linux), "/"); + // Relative: a leading `..` names a real place this cannot rename. + assert_eq!( + lexically_resolved_runtime_path_text("../a/../b", linux), + "../b" + ); + assert_eq!(lexically_resolved_runtime_path_text("a/..", linux), "."); + assert_eq!( + lexically_resolved_runtime_path_text("C:/x", linux), + "C:/x", + "a drive letter is not a root on Linux" + ); + } + + // ── Properties: the recursive-delete guard ───────────────────── + // + // `runtime_install_root_is_protected` is the single source of truth for + // "may ROCm CLI `remove_dir_all` this folder?". Everything below states a + // contract it must satisfy for EVERY spelling of a path, because the whole + // point of the guard is that it is handed a path somebody (or something) + // else wrote down — a registry entry, an `--prefix` argument, a tool call + // from the local assistant. Such a path is text, and text has many + // spellings for one folder. + // + // Unix-only: the protected-root list the guard consults is the Unix one, + // and `runtime_is_windows()` is decided at compile time, so the Windows + // branch cannot be exercised from here. All pure and in-process. + #[cfg(unix)] + mod delete_guard_properties { + use super::*; + use proptest::prelude::*; + + /// The Unix roots `runtime_install_root_is_protected` refuses, restated + /// here so a property compares the guard against the policy rather than + /// against itself. + const PROTECTED_ROOTS: [&str; 13] = [ + "/bin", "/boot", "/dev", "/etc", "/lib", "/lib64", "/opt", "/proc", "/root", "/sbin", + "/sys", "/usr", "/var", + ]; + + /// Resolve `.`, `..`, repeated and trailing separators lexically — what + /// `realpath --no-symlinks`, every shell, and `Path::components` on + /// Windows all do, and what the kernel does for a path with no symlinks + /// in it. + /// + /// This is the reference the guard is measured against, and it is + /// derived from POSIX semantics rather than from the implementation on + /// purpose: an oracle restated from the code under test shares that + /// code's mistakes and proves nothing. Keep it that way — note it does + /// NOT treat `\` specially, which is exactly what catches an + /// unconditional backslash fold leaking into `..` resolution. + /// + /// It says nothing about symlinks. The guard is not symlink-safe (see + /// `lexically_resolved_runtime_path_text`); this reference only pins + /// that every *spelling* of one folder resolves alike. + fn lexically_resolved(path: &str) -> String { + let mut parts: Vec<&str> = Vec::new(); + for part in path.split('/') { + match part { + "" | "." => {} + // POSIX: `/..` is `/`, so popping an empty stack is a no-op. + ".." => { + parts.pop(); + } + other => parts.push(other), + } + } + format!("/{}", parts.join("/")) + } + + /// Does `path` really name `base` or something under it, once both are + /// resolved? + fn lexically_same_or_inside(path: &str, base: &str) -> bool { + let path = lexically_resolved(path); + let base = lexically_resolved(base); + path == base || path.starts_with(&format!("{}/", base.trim_end_matches('/'))) + } + + /// Does `path` really resolve to a protected system location? + fn lexically_protected(path: &str) -> bool { + let resolved = lexically_resolved(path); + resolved == "/" + || PROTECTED_ROOTS + .iter() + .any(|root| lexically_same_or_inside(&resolved, root)) + } + + fn home_text() -> Option { + runtime_home_dir().map(|home| home.display().to_string()) + } + + /// The guard deliberately exempts anything STRICTLY inside the user's + /// own home directory, so a user install under `~/.rocm` stays + /// removable even when home itself sits under a protected root (`/root` + /// for a root user). A property about protection has to grant the same + /// exemption, or it would just be re-litigating that decision. + fn exempt_as_user_owned(path: &str) -> bool { + home_text().is_some_and(|home| { + lexically_same_or_inside(path, &home) + && lexically_resolved(path) != lexically_resolved(&home) + }) + } + + /// The spellings [`a_protected_location_is_refused_however_it_is_spelled`] + /// shrank to, pinned as examples so each stays named even if the + /// generator is retuned. Every one of these names `/etc` (or `/`), and + /// every one of them is a plausible way for a path to be written down + /// by hand, assembled by a script, or produced by joining. + #[test] + fn the_delete_guard_refuses_every_spelling_of_a_protected_root() { + let home = runtime_home_dir().expect("a home directory"); + let home = home.display().to_string(); + let spellings = [ + "/etc".to_owned(), + "/etc/".to_owned(), + "//etc".to_owned(), + "/./etc".to_owned(), + "/etc/.".to_owned(), + "/etc//".to_owned(), + "/usr/../etc".to_owned(), + "/etc/..".to_owned(), + format!("{home}/../../etc"), + format!("{home}/.rocm/../../../etc"), + ]; + let reported: Vec<(String, bool)> = spellings + .into_iter() + .map(|text| { + let protected = runtime_install_root_is_protected(Path::new(&text)); + (text, protected) + }) + .collect(); + let removable: Vec<&str> = reported + .iter() + .filter(|(_, protected)| !protected) + .map(|(text, _)| text.as_str()) + .collect(); + + assert!( + removable.is_empty(), + "reported removable: {removable:?}, full result: {reported:?}" + ); + } + + /// A naive generator is worthless here. Drawing arbitrary strings would + /// spend every draw on paths that resolve nowhere near a protected + /// root, and would pass against a guard that is wide open. So the + /// alphabet is tiny and entirely made of the components that matter: + /// `..` and `.` (the ones nothing in the guard resolves), the empty + /// string (which produces a doubled separator once joined), and the + /// names of real protected roots. + fn component() -> impl Strategy { + prop_oneof![ + 6 => Just(".."), + 3 => Just("."), + 2 => Just(""), + 3 => Just("etc"), + 2 => Just("usr"), + 2 => Just("rocm"), + 2 => Just("runtimes"), + // Off Windows a backslash is an ordinary filename byte, so this + // is ONE legitimate directory name, not two components. It is in + // the alphabet because folding `\` to `/` unconditionally and + // then resolving `..` silently walks out of a protected root; + // without this component every property below still passes. + 2 => Just(r"..\tmp"), + // The leaf names a real managed `install_root` ends in, so the + // generated paths look like the ones the guard actually sees. + 2 => Just("wheel"), + 2 => Just("tarball"), + ] + } + + /// Seeded with the places a real `install_root` is written down: the + /// protected roots themselves, the user's home, and an ordinary + /// unprotected folder. + fn prefix() -> impl Strategy { + let mut seeds: Vec = PROTECTED_ROOTS + .iter() + .map(|&root| root.to_owned()) + .collect(); + seeds.push("/".to_owned()); + seeds.push("/tmp".to_owned()); + seeds.push("/home".to_owned()); + if let Some(home) = home_text() { + seeds.push(format!("{home}/.rocm")); + seeds.push(home); + } + proptest::sample::select(seeds) + } + + /// Paths rooted at the user's own home, so the exemption that keeps + /// `~/.rocm/...` removable is sampled densely instead of being drowned + /// out by the thirteen protected prefixes. + fn user_owned_text() -> impl Strategy { + let home = home_text().unwrap_or_else(|| "/home/rocm".to_owned()); + let seeds = vec![ + home.clone(), + format!("{home}/.rocm"), + format!("{home}/.rocm/data"), + ]; + path_text(proptest::sample::select(seeds)) + } + + /// A path is text, and the guard must answer for the folder that text + /// names, not for the characters it happens to be spelled with. + fn install_root_text() -> impl Strategy { + path_text(prefix()) + } + + fn path_text(prefix: impl Strategy) -> impl Strategy { + ( + prefix, + proptest::collection::vec(component(), 0..4), + prop_oneof![4 => Just(""), 2 => Just("/"), 1 => Just("/."), 1 => Just("/..")], + ) + .prop_map(|(prefix, parts, trailing)| { + let mut text = prefix; + for part in parts { + text.push('/'); + text.push_str(part); + } + text.push_str(trailing); + text + }) + } + + proptest::proptest! { + /// The contract the guard exists for: no path that really resolves + /// into a protected system location may be reported removable, + /// however it is spelled. A counterexample here is `remove_dir_all` + /// on a system directory. + #[test] + fn a_protected_location_is_refused_however_it_is_spelled( + text in install_root_text(), + ) { + prop_assume!(!exempt_as_user_owned(&text)); + prop_assume!(lexically_protected(&text)); + + prop_assert!( + runtime_install_root_is_protected(Path::new(&text)), + "{text} resolves to {} but was reported removable", + lexically_resolved(&text) + ); + } + + /// The guard must not swing the other way either: a folder that + /// really is the user's own stays removable, or `runtimes uninstall` + /// refuses the very folder it created. + #[test] + fn a_user_owned_location_stays_removable(text in user_owned_text()) { + // Strictly-inside-home is the whole contract: the exemption is + // unconditional, so this must hold even when home itself sits + // under a protected root (`/root` for a root user). Filtering + // those draws out with `!lexically_protected` would both assume + // away the interesting case AND reject every draw on such a + // host, which proptest reports as a hard abort rather than a + // skip. + prop_assume!(exempt_as_user_owned(&text)); + + prop_assert!( + !runtime_install_root_is_protected(Path::new(&text)), + "{text} resolves to {} but was refused", + lexically_resolved(&text) + ); + } + + /// Containment is what both the home exemption and the protected + /// -root check are built out of, so it has to agree with where the + /// paths actually resolve — in both directions. Answering "inside" + /// for a path that escaped lets a caller out of the guard; + /// answering "outside" for one that did not hides a protected root. + #[test] + fn containment_agrees_with_where_the_paths_resolve( + path in install_root_text(), + base in prefix(), + ) { + prop_assert_eq!( + runtime_path_is_same_or_inside(Path::new(&path), Path::new(&base)), + lexically_same_or_inside(&path, &base), + "containment of {} in {} disagrees with {} in {}", + path.clone(), + base.clone(), + lexically_resolved(&path), + lexically_resolved(&base) + ); + } + + /// Two spellings of one folder are one folder. + #[test] + fn equivalence_sees_through_spelling(text in install_root_text()) { + let resolved = lexically_resolved(&text); + prop_assert!( + runtime_paths_equivalent(Path::new(&text), Path::new(&resolved)), + "{text} and {resolved} name the same folder but compared unequal" + ); + } + + } + } } From c104da017b24120490b146cc26a752ae3a9ab1e0 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 2 Oct 2026 08:48:12 +0000 Subject: [PATCH 2/2] test(runtime): prove the respelling fix with regressions, not reading Three reviewer nits on the respelling fix for runtime_install_root_is_protected, addressed with tests only: - rocmd's system_prefix_requires_ack was only claimed as a fixed consumer; its existing test used an already-canonical prefix, so it never drove the `..`-escaping bug. Add a regression built from the real home directory plus `/../../usr`, confirmed red against the pre-fix comparison and green against the fix. - The new respelling tests are unix-only, so the public gate's Windows arm (home exemption plus the three protected roots) was never exercised on the Windows lane end to end. Add a test, following the existing cfg!(windows)-at-runtime fixture-selection pattern, that checks respelled Windows paths when actually running on Windows and the Unix answer otherwise. - The property suite's POSIX reference oracle silently assumed absolute input, true only because every generator seed happens to be absolute today. Pin that assumption with an assertion so a future generator change can't desync the oracle from the code it measures. Signed-off-by: Roman Inflianskas --- apps/rocmd/src/lib.rs | 22 +++++++++++++++ crates/rocm-core/src/runtime.rs | 48 +++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index 3762212e3..c65f057e4 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -6233,6 +6233,28 @@ mod tests { ); } + /// The test above only ever hands `system_prefix_requires_ack` an + /// already-canonical path, so it cannot catch the bug this crate's fix + /// addresses: a `..`-respelled prefix that escapes `$HOME` used to compare + /// equal to a path still inside it (`Path::ancestors()` treats `..` as an + /// ordinary component), so acknowledgement was never required. Drive the + /// same check with a prefix built by walking `..` out of the real home + /// directory, which is exactly the shape the original bug let through. + #[test] + #[cfg(unix)] + fn install_sdk_rejects_system_prefix_reached_by_escaping_home() { + let home = rocm_core::runtime_home_dir().expect("a home directory"); + let escaped_prefix = format!("{}/../../usr", home.display()); + + let arguments = + serde_json::Map::from_iter([("prefix".to_owned(), Value::String(escaped_prefix))]); + let error = build_install_sdk_args(&arguments, false).unwrap_err(); + assert!( + error.to_string().contains("allow_system_prefix=true"), + "{error:#}" + ); + } + /// The `install_sdk` MCP tool spawns `rocm` with null stdin, so a real /// install over an active default managed runtime would hit the approval /// gate's non-interactive refusal and bail asking for a flag no MCP caller diff --git a/crates/rocm-core/src/runtime.rs b/crates/rocm-core/src/runtime.rs index 729fb1c69..011a64cbd 100644 --- a/crates/rocm-core/src/runtime.rs +++ b/crates/rocm-core/src/runtime.rs @@ -1476,6 +1476,41 @@ mod tests { ); } + /// `windows_containment_sees_through_spelling_and_case` above exercises the + /// Windows path-resolution rules from a Linux host, but only through the + /// platform-parameterised helpers. The public gate itself, + /// [`runtime_install_root_is_protected`], still decides which protected-root + /// list to consult by reading [`runtime_is_windows()`] — a compile-time + /// answer — so its Windows arm is reachable only when this binary is + /// actually running on Windows. Not `#[cfg(unix)]`-gated, so it compiles + /// and runs on every lane: it follows the same `cfg!(windows)`-at-runtime + /// shape `ensure_runtime_install_root_rejects_protected_system_path` (in + /// `apps/rocm`) already uses to pick host-appropriate fixtures, so a lane + /// that is not Windows still exercises the gate end-to-end against the + /// Unix answer it already owns. + #[test] + fn the_public_gate_refuses_every_spelling_of_a_protected_root_on_its_own_host() { + let spellings: Vec = if cfg!(windows) { + vec![ + PathBuf::from("C:/Windows/"), // trailing separator + PathBuf::from("c:/windows"), // mixed case + PathBuf::from("C:/Windows//System32"), // doubled separator + PathBuf::from("C:/Program Files//"), // doubled separator + PathBuf::from("C:/PROGRAM FILES (X86)"), // mixed case + ] + } else { + vec![PathBuf::from("/etc/"), PathBuf::from("//etc")] + }; + + let accepted: Vec = spellings + .into_iter() + .filter(|path| !runtime_install_root_is_protected(path)) + .map(|path| path.display().to_string()) + .collect(); + + assert!(accepted.is_empty(), "accepted as removable: {accepted:?}"); + } + // ── Properties: the recursive-delete guard ───────────────────── // // `runtime_install_root_is_protected` is the single source of truth for @@ -1517,7 +1552,20 @@ mod tests { /// It says nothing about symlinks. The guard is not symlink-safe (see /// `lexically_resolved_runtime_path_text`); this reference only pins /// that every *spelling* of one folder resolves alike. + /// + /// Absolute input only: every generator seed in this module + /// (`PROTECTED_ROOTS`, `prefix()`, `home_text()`) is already rooted at + /// `/`, so a relative path never reaches this oracle today. That is a + /// property of the generators, not of this function's logic, so it is + /// asserted rather than merely assumed — a generator change that starts + /// seeding relative text would otherwise desync the oracle from + /// `runtime_install_root_is_protected` (which does handle relative + /// paths) without a single property failing to say so. fn lexically_resolved(path: &str) -> String { + assert!( + path.starts_with('/'), + "lexically_resolved is a POSIX-absolute-path oracle; got relative input {path:?}" + ); let mut parts: Vec<&str> = Vec::new(); for part in path.split('/') { match part {