From 91c0a7fe944e8778af2855018c27bf2258fade20 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 13 Oct 2026 16:27:44 +0000 Subject: [PATCH] test(archive-props): keep shifted symlinks pointing where they did `entries_without_top_level_symlink` moves every relative ComfyUI entry under `a/`, but left the generator's relative link targets as they were, so `../outside` written one level deeper resolved to `dest/outside` and no longer escaped. A moved symlink's relative target now gains a `../`, keeping the "link, then write through it" escape the property is meant to exercise, and a unit test checks the resolved target is unchanged by the move. Also share one `ESCAPES_ONLY` between the uv and ComfyUI properties, and say on `CaseOutcome::tree` that its `shared_inodes` is always empty because `run_case` reports those as escapes. Signed-off-by: Roman Inflianskas --- apps/rocm/src/comfyui.rs | 108 ++++++++++++++++++++++---- crates/rocm-core/src/archive_props.rs | 11 ++- crates/rocm-core/src/uv.rs | 7 +- 3 files changed, 102 insertions(+), 24 deletions(-) diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 8a8e80416..30144eaca 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -3115,8 +3115,8 @@ mod archive_properties { use proptest::prelude::*; use crate::archive_props::{ - Entry, Expect, Kind, Layout, Modes, entries, note_mode_scope, run_case, run_property, - run_round_trip, tar_gz_bytes, + ESCAPES_ONLY, Entry, Expect, Kind, Layout, Modes, entries, note_mode_scope, run_case, + run_property, run_round_trip, tar_gz_bytes, }; /// The unpack plus the first write `install` makes into the result: the @@ -3133,6 +3133,25 @@ mod archive_properties { Ok(()) } + /// `entry` moved under `a/`, unless its name is absolute (re-rooted, it + /// already lands deep inside the extraction root). A relative symlink + /// target resolves from the link's own directory, now one level deeper, + /// so it climbs one more to land where it did unmoved: `../outside` + /// still escapes rather than stopping at `dest/outside`. + fn moved_one_level_down(mut entry: Entry) -> Entry { + if entry.name.starts_with(b"/") || entry.name.starts_with(b"@") { + return entry; + } + entry.name.splice(0..0, *b"a/"); + if let Kind::Symlink(target) = &mut entry.kind + && !target.starts_with(b"/") + && !target.starts_with(b"@") + { + target.splice(0..0, *b"../"); + } + entry + } + /// [`entries`] with every relative name moved one level down, under /// `a/`, and every symlink dropped that could still land directly in the /// extraction root: the shape that hits the known #522 defect @@ -3142,7 +3161,9 @@ mod archive_properties { /// left alone: re-rooted, they land deep inside the extraction root. /// Moving names down rather than only dropping top-level links keeps /// "link, then write through it" in play, one level down where the - /// source tree is. + /// source tree is; a moved symlink's relative target gains a `../` so it + /// still resolves to the same place, and an escaping target still + /// escapes. fn entries_without_top_level_symlink() -> impl Strategy> { /// Lexical components, or `None` when the name climbs above the root. fn components(name: &[u8]) -> Option> { @@ -3161,15 +3182,7 @@ mod archive_properties { entries().prop_map(|entries| { let mut links: Vec> = Vec::new(); let mut kept = Vec::new(); - let moved: Vec = entries - .into_iter() - .map(|mut entry| { - if !entry.name.starts_with(b"/") && !entry.name.starts_with(b"@") { - entry.name.splice(0..0, *b"a/"); - } - entry - }) - .collect(); + let moved: Vec = entries.into_iter().map(moved_one_level_down).collect(); for entry in &moved { if matches!(entry.kind, Kind::Symlink(_)) && !entry.name.starts_with(b"/") @@ -3189,6 +3202,72 @@ mod archive_properties { }) } + /// Moving an entry down must not move where its symlink points: the + /// target, resolved lexically from the link's directory, names the same + /// place relative to the extraction root before and after. + #[test] + fn moving_an_entry_down_keeps_its_symlink_target_in_place() { + /// `dir` joined with `target`, `..` applied lexically; a leading `..` + /// that climbs above the root is kept. + fn resolve(dir: &[u8], target: &[u8]) -> Vec> { + let mut out: Vec> = Vec::new(); + for part in dir + .split(|b| *b == b'/') + .chain(target.split(|b| *b == b'/')) + { + match part { + b"" | b"." => {} + b".." if out.last().is_some_and(|last| last != b"..") => { + out.pop(); + } + _ => out.push(part.to_vec()), + } + } + out + } + for target in [ + &b"../outside"[..], + b"../../outside", + b"../victim", + b".", + b"l/../..", + b"a", + ] { + let moved = moved_one_level_down(Entry { + name: b"l".to_vec(), + kind: Kind::Symlink(target.to_vec()), + mode: 0o777, + }); + assert_eq!(moved.name, b"a/l"); + let Kind::Symlink(moved_target) = moved.kind else { + panic!("still a symlink"); + }; + assert_eq!( + resolve(b"a", &moved_target), + resolve(b"", target), + "{} moved to {}", + String::from_utf8_lossy(target), + String::from_utf8_lossy(&moved_target) + ); + } + // Absolute targets and absolute names are left alone. + let absolute = moved_one_level_down(Entry { + name: b"l".to_vec(), + kind: Kind::Symlink(b"@OUT/sentinel".to_vec()), + mode: 0o777, + }); + assert!(matches!(absolute.kind, Kind::Symlink(t) if t == b"@OUT/sentinel")); + assert_eq!( + moved_one_level_down(Entry { + name: b"/x".to_vec(), + kind: Kind::Dir, + mode: 0o755 + }) + .name, + b"/x" + ); + } + fn property( label: &str, strategy: impl Strategy>, @@ -3221,11 +3300,6 @@ mod archive_properties { }) } - const ESCAPES_ONLY: Expect = Expect { - modes: Modes::Unchecked, - no_outward_links: false, - }; - /// What holds today: with the #522 shape (a top-level symlink) left out, /// the unpack and the first install write stay inside the app root. #[test] diff --git a/crates/rocm-core/src/archive_props.rs b/crates/rocm-core/src/archive_props.rs index b7532f6bb..bcbae898d 100644 --- a/crates/rocm-core/src/archive_props.rs +++ b/crates/rocm-core/src/archive_props.rs @@ -877,7 +877,9 @@ pub struct CaseOutcome { /// removed outside it, hard links shared with outside files, and the /// extraction root itself removed or replaced by a non-directory. pub escapes: Vec, - /// The state of the tree left inside the root. + /// The state of the tree left inside the root. Its `shared_inodes` is + /// always empty: a hard link shared with an outside file is a write + /// outside, so [`run_case`] moves those into `escapes`. pub tree: Violations, } @@ -1053,6 +1055,13 @@ pub struct Expect { pub no_outward_links: bool, } +/// Only escapes are judged: the contract for an extractor that makes no +/// promise about modes or outward links. +pub const ESCAPES_ONLY: Expect = Expect { + modes: Modes::Unchecked, + no_outward_links: false, +}; + impl CaseOutcome { /// `Ok` when the outcome meets `expect`, else the outcome as text. pub fn judge(&self, expect: Expect) -> Result<(), String> { diff --git a/crates/rocm-core/src/uv.rs b/crates/rocm-core/src/uv.rs index b8238df9e..2754af4b6 100644 --- a/crates/rocm-core/src/uv.rs +++ b/crates/rocm-core/src/uv.rs @@ -1331,7 +1331,7 @@ mod archive_properties { use super::{extract_archive, find_binary_in, make_executable, uv_binary_name}; use crate::archive_props::{ - Entry, Expect, Kind, Layout, Modes, OUTSIDE_TOKEN, concretize, entries, + ESCAPES_ONLY, Entry, Expect, Kind, Layout, Modes, OUTSIDE_TOKEN, concretize, entries, naive_sanitizing_unpack, naive_unpack, note_mode_scope, run_case, run_property, run_round_trip, tar_gz_bytes, }; @@ -1355,11 +1355,6 @@ mod archive_properties { }) } - const ESCAPES_ONLY: Expect = Expect { - modes: Modes::Unchecked, - no_outward_links: false, - }; - #[test] fn uv_archive_extraction_never_writes_outside_the_staging_dir() { property("uv", ESCAPES_ONLY, extract_archive).unwrap();