From 4afd3fdaf64d0485965107259a56c3d68393165c Mon Sep 17 00:00:00 2001 From: Henry Su Date: Sat, 22 Aug 2026 17:01:26 -0500 Subject: [PATCH] test(adapters): collapse the sanitize_path suite, and fix the bypass it found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hand-written assertion lists (`sanitize_strips_traversal` and `sanitize_path_neutralizes_traversal_and_drive_letters`) had grown to 88 assert_eq! lines covering one 25-line pure function. 69 of those arrived one per commit — 12% of the repository's history is single-assert commits against sanitize_path, which is unreviewable and still only ever tests inputs someone thought of. Replaced with: - `sanitize_path_table`: all 87 unique cases preserved verbatim as data, grouped by what they demonstrate. Extracted mechanically (with a parser that handles Rust raw strings ending in a backslash, e.g. r"src\app\") and cross-checked against the assert_eq! count in each block, so no case was dropped in translation. Adding a case is now a row, not a commit. - `sanitize_path_invariants`: six properties that must hold for ALL input — no `..` segment survives, no empty/`.` segments, never absolute, no control or zero-width characters, no drive prefix, and idempotence. The property test immediately found a real bypass in sanitize_path itself: sanitize_path("A\x7f:") -> "A:" (expected "") The drive-letter check ran against the RAW segment, before control and zero-width characters were filtered. Putting any such character between the letter and the colon made the segment longer than 2 bytes, so the check missed it; the character was then stripped and the drive prefix survived into the output. "C\u{200b}:/Users/dev/x.rs" leaked through the same way. Fixed by filtering first and testing the filtered segment. Windows drive prefix neutralization is a stated boundary-hardening control, so this closes a real hole in it rather than a theoretical one. Four regression cases added to the table alongside the property. proptest is a dev-dependency only, with default-features off (no fork/timeout machinery) to keep the dependency graph small for the egress audit. Verified: fmt, clippy, cargo test --locked --all-targets (346 pass / 4 ignored), no_network_in_archive, egress-check.sh, toolchain-pin-check.sh. The property test was run 5 times over fresh case sets to confirm stability. --- Cargo.lock | 69 +++++ crates/lore-core/Cargo.toml | 5 + crates/lore-core/src/adapters/common.rs | 320 +++++++++++++++--------- 3 files changed, 271 insertions(+), 123 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 49a7816..4cb82b4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2845,6 +2845,7 @@ dependencies = [ "gix", "lore-ipc", "notify", + "proptest", "rusqlite", "serde_json", "tempfile", @@ -3441,6 +3442,15 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "439ee305def115ba05938db6eb1644ff94165c5ab5e9420d1c1bcedbba909391" +[[package]] +name = "ppv-lite86" +version = "0.2.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85eae3c4ed2f50dcfe72643da4befc30deadb458a9b590d720cde2f2b1e97da9" +dependencies = [ + "zerocopy", +] + [[package]] name = "precomputed-hash" version = "0.1.1" @@ -3518,6 +3528,21 @@ dependencies = [ "parking_lot", ] +[[package]] +name = "proptest" +version = "1.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b45fcc2344c680f5025fe57779faef368840d0bd1f42f216291f0dc4ace4744" +dependencies = [ + "bitflags 2.13.1", + "num-traits", + "rand", + "rand_chacha", + "rand_xorshift", + "regex-syntax", + "unarray", +] + [[package]] name = "quick-xml" version = "0.41.0" @@ -3548,6 +3573,44 @@ version = "6.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" +[[package]] +name = "rand" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9ef1d0d795eb7d84685bca4f72f3649f064e6641543d3a8c415898726a57b41" +dependencies = [ + "rand_chacha", + "rand_core", +] + +[[package]] +name = "rand_chacha" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3022b5f1df60f26e1ffddd6c66e8aa15de382ae63b3a0c1bfc0e4d3e3f325cb" +dependencies = [ + "ppv-lite86", + "rand_core", +] + +[[package]] +name = "rand_core" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76afc826de14238e6e8c374ddcc1fa19e374fd8dd986b0d2af0d02377261d83c" +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", +] + [[package]] name = "raw-window-handle" version = "0.6.2" @@ -4948,6 +5011,12 @@ version = "1.20.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6f5e870be6c3b371b77fe0ee0bafb859fa4964b4404c27de1d380043c4dda20" +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unic-char-property" version = "0.9.0" diff --git a/crates/lore-core/Cargo.toml b/crates/lore-core/Cargo.toml index d429fdc..9c044bb 100644 --- a/crates/lore-core/Cargo.toml +++ b/crates/lore-core/Cargo.toml @@ -37,3 +37,8 @@ gix = { version = "0.86", default-features = false, features = ["sha1", "status" [dev-dependencies] tempfile = "3" +# Property testing for pure input-sanitizing functions (adapters::common). +# default-features off drops the `fork`/`timeout` machinery (and its rusty-fork +# + wait-timeout deps): we only need the generator + shrinker, and a leaner dev +# tree keeps the egress dependency audit small. +proptest = { version = "1", default-features = false, features = ["std"] } diff --git a/crates/lore-core/src/adapters/common.rs b/crates/lore-core/src/adapters/common.rs index 9209973..826f1c6 100644 --- a/crates/lore-core/src/adapters/common.rs +++ b/crates/lore-core/src/adapters/common.rs @@ -127,18 +127,21 @@ pub(crate) fn non_negative_int_field(obj: &serde_json::Value, key: &str) -> Opti pub(crate) fn sanitize_path(raw: &str) -> String { let mut parts: Vec = Vec::new(); for seg in raw.split(['/', '\\']) { - let clean_seg = if seg.len() == 2 - && seg.as_bytes()[1] == b':' - && seg.as_bytes()[0].is_ascii_alphabetic() - { - "" - } else { - seg - }; - let filtered: String = clean_seg + // Filter BEFORE the drive-letter check, not after. Testing the raw + // segment lets a control or zero-width character sit between the letter + // and the colon ("A\x7f:", "C\u{200b}:"): the length check then fails, + // the character is stripped a moment later, and a drive prefix survives + // into the output. Found by `sanitize_path_invariants`. + let filtered: String = seg .chars() .filter(|c| !c.is_control() && !crate::is_zero_width(*c)) .collect(); + let bytes = filtered.as_bytes(); + let filtered = if bytes.len() == 2 && bytes[1] == b':' && bytes[0].is_ascii_alphabetic() { + String::new() + } else { + filtered + }; match filtered.as_str() { "" | "." => {} ".." => { @@ -199,31 +202,192 @@ pub(crate) fn resolve_file_event_segments(session: &mut crate::model::ParsedSess mod tests { use super::*; + /// Every `sanitize_path` case, as data. + /// + /// This replaces two hand-written assertion lists (`sanitize_strips_traversal` + /// and `sanitize_path_neutralizes_traversal_and_drive_letters`) that had grown + /// to 88 `assert_eq!` lines between them, 69 of which arrived one per commit. + /// Each row below is one of those cases, preserved verbatim — a table is the + /// right shape for enumerated inputs, and adding a case is now a row, not a + /// commit. Invariants that should hold for *all* input are property-tested in + /// `sanitize_path_invariants` below rather than enumerated here. #[test] - fn sanitize_strips_traversal() { - assert_eq!(sanitize_path("../../a/b"), "a/b"); - assert_eq!(sanitize_path(r"..\..\a\b"), "a/b"); - assert_eq!(sanitize_path(r"src\..\src\app.ts"), "src/app.ts"); - assert_eq!(sanitize_path(r"foo/bar\baz"), "foo/bar/baz"); - assert_eq!(sanitize_path("foo///bar\\\\baz"), "foo/bar/baz"); - assert_eq!(sanitize_path("./a/./b/./c.ts"), "a/b/c.ts"); - assert_eq!(sanitize_path("a/b/c/../../d.ts"), "a/d.ts"); - assert_eq!( - sanitize_path(r"C:\Users\dev\project\file.ts"), - "Users/dev/project/file.ts" - ); - assert_eq!( - sanitize_path(r"d:/project/src/index.js"), - "project/src/index.js" - ); - assert_eq!( - sanitize_path("src/\0poison\r/app\u{200b}.ts"), - "src/poison/app.ts" - ); - assert_eq!( - sanitize_path("\u{feff}src/\x1b[31mred\x1b[0m/\u{2060}main\u{200d}.rs"), - "src/[31mred[0m/main.rs" - ); + fn sanitize_path_table() { + // (input, expected) + const CASES: &[(&str, &str)] = &[ + // parent-directory traversal is neutralized. + ("../../a/b", "a/b"), + (r"..\..\a\b", "a/b"), + (r"src\..\src\app.ts", "src/app.ts"), + ("a/b/c/../../d.ts", "a/d.ts"), + ("../../foo/bar.rs", "foo/bar.rs"), + ("a/b/c/../../d.rs", "a/d.rs"), + ("my..dir/file.rs", "my..dir/file.rs"), + ("/a/../b/", "b"), + ("/a/./../b/", "b"), + (r"\a\..\b\", "b"), + (r"\a\.\..\b\", "b"), + ("///..///", ""), + (r"\\\..\\\", ""), + (r"///..\\\\", ""), + (r"\\\..///", ""), + ("..", ""), + // Windows drive prefixes are dropped — including when a control or + // zero-width character is used to hide the prefix from the check + // (regressions for the bypass `sanitize_path_invariants` found). + ("A\u{7f}:", ""), + ("C\u{200b}:", ""), + ("C\u{200b}:/Users/dev/x.rs", "Users/dev/x.rs"), + ("a/C\u{feff}:/b.rs", "a/b.rs"), + // Windows drive prefixes are dropped. + (r"C:\Users\dev\project\file.ts", "Users/dev/project/file.ts"), + (r"d:/project/src/index.js", "project/src/index.js"), + ("C:\\Users\\test\\file.txt", "Users/test/file.txt"), + // control and zero-width characters are filtered out. + ("src/\0poison\r/app\u{200b}.ts", "src/poison/app.ts"), + ( + "\u{feff}src/\x1b[31mred\x1b[0m/\u{2060}main\u{200d}.rs", + "src/[31mred[0m/main.rs", + ), + // three or more dots are ordinary name characters, not traversal. + (".../src/lib.rs", ".../src/lib.rs"), + ("..../src/lib.rs", "..../src/lib.rs"), + ("app.min...js", "app.min...js"), + ("/a/./.../b/", "a/.../b"), + ("/a/.../b/", "a/.../b"), + ("/a/..../b/", "a/..../b"), + (r"\a\.\...\b\", "a/.../b"), + (r"\a\...\b\", "a/.../b"), + (r"\a\....\b\", "a/..../b"), + ("a/b...c/d.rs", "a/b...c/d.rs"), + (r"a\b...c\d.rs", "a/b...c/d.rs"), + ("a/b....c/d.rs", "a/b....c/d.rs"), + (r"a\b....c\d.rs", "a/b....c/d.rs"), + ("///...///", "..."), + (r"\\\...\\\", "..."), + (r"///...\\\\", "..."), + (r"\\\...///", "..."), + ("///....///", "...."), + (r"\\\....\\\", "...."), + (r"///....\\\\", "...."), + (r"\\\....///", "...."), + ("...", "..."), + ("....", "...."), + // single dots: segment-only `.` is dropped, dots inside names are kept. + ("./a/./b/./c.ts", "a/b/c.ts"), + ("./src/./main.rs", "src/main.rs"), + ("/absolute/path/file.rs", "absolute/path/file.rs"), + (r"\\server\share\file.rs", "server/share/file.rs"), + ("a///b///c.rs", "a/b/c.rs"), + (r"a\\\b\\\c.rs", "a/b/c.rs"), + ("x.rs", "x.rs"), + ("a.b", "a.b"), + ("./file.rs", "file.rs"), + (".file.rs", ".file.rs"), + (".dir/.file.rs", ".dir/.file.rs"), + ("a./b.rs", "a./b.rs"), + ("a.rs.", "a.rs."), + ("a/b/c.rs", "a/b/c.rs"), + ("a/./b/./c.rs", "a/b/c.rs"), + ("/a/./b/", "a/b"), + (r"\a\.\b\", "a/b"), + (r"a/b\c/d\file.rs", "a/b/c/d/file.rs"), + ("a/b.c/d.rs", "a/b.c/d.rs"), + (r"a\b.c\d.rs", "a/b.c/d.rs"), + ("archive.tar.gz", "archive.tar.gz"), + ("a/b/archive.tar.gz", "a/b/archive.tar.gz"), + (r"a\b\archive.tar.gz", "a/b/archive.tar.gz"), + ("///.///", ""), + (r"\\\.\\\", ""), + (r"///.\\\\", ""), + (r"\\\.///", ""), + (".", ""), + ("./", ""), + // separators are normalized and redundant ones collapsed. + (r"foo/bar\baz", "foo/bar/baz"), + ("foo///bar\\\\baz", "foo/bar/baz"), + ("src/app/", "src/app"), + (r"src\app\", "src/app"), + ("a", "a"), + ("a/b////", "a/b"), + (r"a\b\\\\", "a/b"), + ("///a/b///", "a/b"), + (r"\\\a\b\\\", "a/b"), + ("///", ""), + (r"\\\", ""), + (r"\", ""), + (r"\\", ""), + (r"////\\\\", ""), + ]; + + for (input, expected) in CASES { + assert_eq!( + sanitize_path(input), + *expected, + "sanitize_path({input:?}) should be {expected:?}" + ); + } + } + + proptest::proptest! { + /// Properties that must hold for *every* input, including inputs no one + /// thought to enumerate. These are the actual security contract of + /// `sanitize_path`: its output is joined onto a display path, so it must + /// never escape upward, never re-anchor to a root or a drive, and never + /// carry characters that can forge what a path looks like on screen. + #[test] + fn sanitize_path_invariants(raw in ".{0,120}") { + let out = sanitize_path(&raw); + + // An empty output is a legitimate result: input that is entirely + // separators, dot-segments, or stripped characters sanitizes to "". + // Segment checks only apply once there is a path to check, because + // "".split('/') yields a single empty segment. + if !out.is_empty() { + // 1. No traversal survives. A literal ".." segment is what lets a + // path escape its parent, so it must never appear in the output. + proptest::prop_assert!( + !out.split('/').any(|seg| seg == ".."), + "output {out:?} contains a `..` segment (from {raw:?})" + ); + + // 2. No empty or bare-`.` segments: both are meaningless, and an + // empty leading segment would make the path absolute. + proptest::prop_assert!( + !out.split('/').any(|seg| seg.is_empty() || seg == "."), + "output {out:?} contains an empty or `.` segment (from {raw:?})" + ); + } + + // 3. Never absolute, on either separator convention. + proptest::prop_assert!(!out.starts_with('/'), "output {out:?} is absolute"); + proptest::prop_assert!(!out.contains('\\'), "output {out:?} kept a backslash"); + + // 4. No control or zero-width characters. These are the characters + // that let a recorded path lie about itself in the UI. + proptest::prop_assert!( + !out.chars().any(|c| c.is_control() || crate::is_zero_width(c)), + "output {out:?} kept a control or zero-width char (from {raw:?})" + ); + + // 5. No Windows drive prefix is re-anchored. + let first = out.split('/').next().unwrap_or(""); + proptest::prop_assert!( + !(first.len() == 2 + && first.as_bytes()[1] == b':' + && first.as_bytes()[0].is_ascii_alphabetic()), + "output {out:?} still starts with a drive prefix" + ); + + // 6. Idempotent — re-sanitizing a sanitized path changes nothing. + // Without this, output could still be an input that needs cleaning. + proptest::prop_assert_eq!( + sanitize_path(&out), + out.clone(), + "sanitize_path is not idempotent for {:?}", + raw + ); + } } #[test] @@ -437,96 +601,6 @@ mod tests { assert_eq!(epoch_ms(tabs_ts), expected); } - #[test] - fn sanitize_path_neutralizes_traversal_and_drive_letters() { - assert_eq!(sanitize_path("../../foo/bar.rs"), "foo/bar.rs"); - assert_eq!( - sanitize_path("C:\\Users\\test\\file.txt"), - "Users/test/file.txt" - ); - assert_eq!(sanitize_path("./src/./main.rs"), "src/main.rs"); - assert_eq!( - sanitize_path("/absolute/path/file.rs"), - "absolute/path/file.rs" - ); - assert_eq!(sanitize_path("a/b/c/../../d.rs"), "a/d.rs"); - assert_eq!(sanitize_path("src/app/"), "src/app"); - assert_eq!(sanitize_path(r"src\app\"), "src/app"); - assert_eq!( - sanitize_path(r"\\server\share\file.rs"), - "server/share/file.rs" - ); - assert_eq!(sanitize_path(".../src/lib.rs"), ".../src/lib.rs"); - assert_eq!(sanitize_path("..../src/lib.rs"), "..../src/lib.rs"); - assert_eq!(sanitize_path("a///b///c.rs"), "a/b/c.rs"); - assert_eq!(sanitize_path(r"a\\\b\\\c.rs"), "a/b/c.rs"); - assert_eq!(sanitize_path("a"), "a"); - assert_eq!(sanitize_path("x.rs"), "x.rs"); - assert_eq!(sanitize_path("a.b"), "a.b"); - assert_eq!(sanitize_path("./file.rs"), "file.rs"); - assert_eq!(sanitize_path(".file.rs"), ".file.rs"); - assert_eq!(sanitize_path(".dir/.file.rs"), ".dir/.file.rs"); - assert_eq!(sanitize_path("app.min...js"), "app.min...js"); - assert_eq!(sanitize_path("my..dir/file.rs"), "my..dir/file.rs"); - assert_eq!(sanitize_path("a./b.rs"), "a./b.rs"); - assert_eq!(sanitize_path("a.rs."), "a.rs."); - assert_eq!(sanitize_path("a/b/c.rs"), "a/b/c.rs"); - assert_eq!(sanitize_path("a/./b/./c.rs"), "a/b/c.rs"); - assert_eq!(sanitize_path("/a/./b/"), "a/b"); - assert_eq!(sanitize_path("/a/../b/"), "b"); - assert_eq!(sanitize_path("/a/./../b/"), "b"); - assert_eq!(sanitize_path("/a/./.../b/"), "a/.../b"); - assert_eq!(sanitize_path("/a/.../b/"), "a/.../b"); - assert_eq!(sanitize_path("/a/..../b/"), "a/..../b"); - assert_eq!(sanitize_path(r"\a\.\b\"), "a/b"); - assert_eq!(sanitize_path(r"\a\..\b\"), "b"); - assert_eq!(sanitize_path(r"\a\.\..\b\"), "b"); - assert_eq!(sanitize_path(r"\a\.\...\b\"), "a/.../b"); - assert_eq!(sanitize_path(r"\a\...\b\"), "a/.../b"); - assert_eq!(sanitize_path(r"\a\....\b\"), "a/..../b"); - assert_eq!(sanitize_path(r"a/b\c/d\file.rs"), "a/b/c/d/file.rs"); - assert_eq!(sanitize_path("a/b.c/d.rs"), "a/b.c/d.rs"); - assert_eq!(sanitize_path(r"a\b.c\d.rs"), "a/b.c/d.rs"); - assert_eq!(sanitize_path("archive.tar.gz"), "archive.tar.gz"); - assert_eq!(sanitize_path("a/b/archive.tar.gz"), "a/b/archive.tar.gz"); - assert_eq!(sanitize_path(r"a\b\archive.tar.gz"), "a/b/archive.tar.gz"); - assert_eq!(sanitize_path("a/b...c/d.rs"), "a/b...c/d.rs"); - assert_eq!(sanitize_path(r"a\b...c\d.rs"), "a/b...c/d.rs"); - assert_eq!(sanitize_path("a/b....c/d.rs"), "a/b....c/d.rs"); - assert_eq!(sanitize_path(r"a\b....c\d.rs"), "a/b....c/d.rs"); - assert_eq!(sanitize_path("a/b////"), "a/b"); - assert_eq!(sanitize_path(r"a\b\\\\"), "a/b"); - assert_eq!(sanitize_path("///a/b///"), "a/b"); - assert_eq!(sanitize_path(r"\\\a\b\\\"), "a/b"); - assert_eq!(sanitize_path("///"), ""); - assert_eq!(sanitize_path("///.///"), ""); - assert_eq!(sanitize_path(r"\\\"), ""); - assert_eq!(sanitize_path(r"\\\.\\\"), ""); - assert_eq!(sanitize_path(r"///.\\\\"), ""); - assert_eq!(sanitize_path(r"\\\.///"), ""); - assert_eq!(sanitize_path("///..///"), ""); - assert_eq!(sanitize_path(r"\\\..\\\"), ""); - assert_eq!(sanitize_path(r"///..\\\\"), ""); - assert_eq!(sanitize_path(r"\\\..///"), ""); - assert_eq!(sanitize_path("///...///"), "..."); - assert_eq!(sanitize_path(r"\\\...\\\"), "..."); - assert_eq!(sanitize_path(r"///...\\\\"), "..."); - assert_eq!(sanitize_path(r"\\\...///"), "..."); - assert_eq!(sanitize_path("///....///"), "...."); - assert_eq!(sanitize_path(r"\\\....\\\"), "...."); - assert_eq!(sanitize_path(r"///....\\\\"), "...."); - assert_eq!(sanitize_path(r"\\\....///"), "...."); - assert_eq!(sanitize_path("."), ""); - assert_eq!(sanitize_path("./"), ""); - assert_eq!(sanitize_path(".."), ""); - assert_eq!(sanitize_path("..."), "..."); - assert_eq!(sanitize_path("...."), "...."); - assert_eq!(sanitize_path(r"\"), ""); - assert_eq!(sanitize_path(r"\\"), ""); - assert_eq!(sanitize_path(r"\\\"), ""); - assert_eq!(sanitize_path(r"////\\\\"), ""); - } - #[test] fn bounded_safely_truncates_multibyte_characters() { let ascii = "a".repeat(100);