From 9f9cd243f33b59fa18c8e45fbdff2c9745ad00ca Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Mon, 21 Sep 2026 18:11:39 +0000 Subject: [PATCH 1/3] fix(fs): build VFS paths with forward slashes on every host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PathBuf::join`/`push` append the host separator, so on Windows every VFS path built that way came out mixed: `grep -r` and `du` printed `/d/proj\src\main.rs`, and the read-error path of tail/rev/tac/shuf/ sort/uniq/wc reported the same shape. `InMemoryFs::rename` was worse than cosmetic — its directory fan-out wrote the new entry-map keys itself, so renamed children would have been unreachable afterwards. Add `vfs_join`, which joins with `/` on every host, keeps `Path::join` semantics (absolute child replaces the base) and leaves `.`/`..` for the backends, and route every VFS-path join through it. Host paths (RealFs under its root, host mount mapping) keep `Path::join`, where the host separator is correct. Guards: a static scan over `crates/bashkit/src/` for the `cwd.join(` and `.join(&entry.name)` shapes, and `windows_containment_*` tests next to `vfs_join`, `grep`, `du`, `wc` and `InMemoryFs::rename` that the Windows CI job runs. Fixes #2425 Claude-Session: https://claude.ai/code/session_01MtasrnXfdKmZqJEtP1Exby --- crates/bashkit/src/builtins/archive.rs | 3 +- .../bashkit/src/builtins/awk/interpreter.rs | 5 +- crates/bashkit/src/builtins/awk/mod.rs | 5 +- crates/bashkit/src/builtins/cat.rs | 3 +- crates/bashkit/src/builtins/checksum.rs | 3 +- crates/bashkit/src/builtins/column.rs | 3 +- crates/bashkit/src/builtins/comm.rs | 5 +- crates/bashkit/src/builtins/cuttr.rs | 3 +- crates/bashkit/src/builtins/diff.rs | 5 +- crates/bashkit/src/builtins/dirstack.rs | 3 +- crates/bashkit/src/builtins/disk.rs | 34 ++++- crates/bashkit/src/builtins/fileops.rs | 5 +- crates/bashkit/src/builtins/git/client.rs | 139 +++++++++--------- crates/bashkit/src/builtins/grep.rs | 62 +++++++- crates/bashkit/src/builtins/headtail.rs | 5 +- crates/bashkit/src/builtins/hextools.rs | 3 +- crates/bashkit/src/builtins/ls/find.rs | 3 +- crates/bashkit/src/builtins/ls/list.rs | 5 +- crates/bashkit/src/builtins/mod.rs | 83 ++++++++++- crates/bashkit/src/builtins/navigation.rs | 3 +- crates/bashkit/src/builtins/nl.rs | 3 +- crates/bashkit/src/builtins/paste.rs | 3 +- crates/bashkit/src/builtins/path.rs | 3 +- crates/bashkit/src/builtins/python.rs | 5 +- crates/bashkit/src/builtins/rg/mod.rs | 30 ++-- crates/bashkit/src/builtins/sed.rs | 5 +- crates/bashkit/src/builtins/shuf.rs | 5 +- crates/bashkit/src/builtins/sortuniq.rs | 9 +- crates/bashkit/src/builtins/sqlite/mod.rs | 7 +- crates/bashkit/src/builtins/strings.rs | 3 +- crates/bashkit/src/builtins/test.rs | 3 +- crates/bashkit/src/builtins/textrev.rs | 5 +- crates/bashkit/src/builtins/tree.rs | 3 +- crates/bashkit/src/builtins/typescript.rs | 3 +- crates/bashkit/src/builtins/wc.rs | 38 ++++- crates/bashkit/src/builtins/yq.rs | 3 +- crates/bashkit/src/builtins/zip_cmd.rs | 5 +- crates/bashkit/src/fs/memory.rs | 29 +++- crates/bashkit/src/fs/mod.rs | 92 ++++++++++++ crates/bashkit/src/fs/mountable.rs | 3 +- crates/bashkit/src/fs/namespace.rs | 7 +- crates/bashkit/src/fs/overlay.rs | 18 ++- crates/bashkit/src/interpreter/glob.rs | 11 +- crates/bashkit/src/interpreter/mod.rs | 6 +- crates/bashkit/src/lib.rs | 2 +- knowledge/foundations/vfs.md | 36 +++++ 46 files changed, 559 insertions(+), 158 deletions(-) diff --git a/crates/bashkit/src/builtins/archive.rs b/crates/bashkit/src/builtins/archive.rs index 35f1460c9..1253b6ead 100644 --- a/crates/bashkit/src/builtins/archive.rs +++ b/crates/bashkit/src/builtins/archive.rs @@ -31,6 +31,7 @@ use std::path::{Path, PathBuf}; use super::limits::ARCHIVE_MAX_DECOMPRESSION_RATIO as MAX_DECOMPRESSION_RATIO; use super::{Builtin, Context, resolve_path}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; use crate::limits::{BudgetedBytes, BudgetedString, BudgetedVec, LimitExceeded}; @@ -586,7 +587,7 @@ fn add_directory_to_tar<'a>( // Add directory contents let entries = ctx.fs.read_dir(path).await?; for entry in entries { - let child_path = path.join(&entry.name); + let child_path = vfs_join(path, &entry.name); let child_name = format!("{}/{}", name, entry.name); if entry.metadata.file_type.is_dir() { diff --git a/crates/bashkit/src/builtins/awk/interpreter.rs b/crates/bashkit/src/builtins/awk/interpreter.rs index da75ebb4d..f19d2bc4e 100644 --- a/crates/bashkit/src/builtins/awk/interpreter.rs +++ b/crates/bashkit/src/builtins/awk/interpreter.rs @@ -18,6 +18,7 @@ use crate::fs::{FileSystem, normalize_path}; use crate::limits::ExecutionLimits; // On wasm32 there is no OS thread to bridge the sync AWK evaluator to the async // VFS, so redirected reads/writes drive the VFS future inline with now_or_never. +use crate::fs::vfs_join; #[cfg(target_family = "wasm")] use futures_util::FutureExt; @@ -234,7 +235,7 @@ impl AwkInterpreter { let resolved = if path_str.starts_with('/') { PathBuf::from(path_str) } else { - self.cwd.join(path_str) + vfs_join(&self.cwd, path_str) }; normalize_path(&resolved).to_string_lossy().to_string() } @@ -1200,7 +1201,7 @@ impl AwkInterpreter { let resolved = if path.starts_with('/') { PathBuf::from(path) } else { - self.cwd.join(path) + vfs_join(&self.cwd, path) }; let key = resolved.to_string_lossy().into_owned(); // Bound the number of distinct redirect targets (DoS guard, parity with diff --git a/crates/bashkit/src/builtins/awk/mod.rs b/crates/bashkit/src/builtins/awk/mod.rs index 5efe09b46..1cca8c804 100644 --- a/crates/bashkit/src/builtins/awk/mod.rs +++ b/crates/bashkit/src/builtins/awk/mod.rs @@ -451,6 +451,7 @@ fn normalize_awk_newlines(input: &str) -> String { mod interpreter; mod parser; +use crate::fs::vfs_join; use interpreter::{AwkFlow, AwkInterpreter}; use parser::AwkParser; @@ -535,7 +536,7 @@ impl Builtin for Awk { let path = if ctx.args[i].starts_with('/') { std::path::PathBuf::from(&ctx.args[i]) } else { - ctx.cwd.join(&ctx.args[i]) + vfs_join(ctx.cwd, &ctx.args[i]) }; program_str = match read_text_file(&*ctx.fs, &path, "awk").await { Ok(t) => t, @@ -615,7 +616,7 @@ impl Builtin for Awk { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; let text = match read_text_file(&*ctx.fs, &path, "awk").await { diff --git a/crates/bashkit/src/builtins/cat.rs b/crates/bashkit/src/builtins/cat.rs index b58255b3e..5abefa942 100644 --- a/crates/bashkit/src/builtins/cat.rs +++ b/crates/bashkit/src/builtins/cat.rs @@ -12,6 +12,7 @@ use std::path::Path; use super::{Builtin, Context}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; pub struct Cat; @@ -72,7 +73,7 @@ impl Builtin for Cat { let path = if Path::new(file).is_absolute() { file.clone() } else { - ctx.cwd.join(file).to_string_lossy().into_owned() + vfs_join(ctx.cwd, file).to_string_lossy().into_owned() }; match ctx.fs.read_file(Path::new(&path)).await { Ok(bytes) => raw.extend_from_slice(&bytes), diff --git a/crates/bashkit/src/builtins/checksum.rs b/crates/bashkit/src/builtins/checksum.rs index 9f7632386..c34b4fd72 100644 --- a/crates/bashkit/src/builtins/checksum.rs +++ b/crates/bashkit/src/builtins/checksum.rs @@ -7,6 +7,7 @@ use sha2::{Digest, Sha256}; use super::{Builtin, Context}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// md5sum builtin - compute MD5 message digest @@ -100,7 +101,7 @@ async fn checksum_execute(ctx: &Context<'_>, cmd: &str) -> Result text.lines().map(|l| l.to_string()).collect(), @@ -100,7 +101,7 @@ impl Builtin for Comm { let path = if files[1].starts_with('/') { std::path::PathBuf::from(&files[1]) } else { - ctx.cwd.join(&files[1]) + vfs_join(ctx.cwd, &files[1]) }; match read_text_file(&*ctx.fs, &path, "comm").await { Ok(text) => text.lines().map(|l| l.to_string()).collect(), diff --git a/crates/bashkit/src/builtins/cuttr.rs b/crates/bashkit/src/builtins/cuttr.rs index 13d23512d..b7c379ae4 100644 --- a/crates/bashkit/src/builtins/cuttr.rs +++ b/crates/bashkit/src/builtins/cuttr.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use super::{Builtin, BuiltinHelper, Context, read_text_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; // Keep tr set expansion bounded before stdin/output controls can apply. @@ -189,7 +190,7 @@ impl Builtin for Cut { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; let text = match read_text_file(&*ctx.fs, &path, "cut").await { diff --git a/crates/bashkit/src/builtins/diff.rs b/crates/bashkit/src/builtins/diff.rs index 36764875a..e21a7a6c4 100644 --- a/crates/bashkit/src/builtins/diff.rs +++ b/crates/bashkit/src/builtins/diff.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use super::{Builtin, Context, read_text_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The diff builtin - compare files line by line. @@ -348,7 +349,7 @@ impl Builtin for Diff { let path = if files[0].starts_with('/') { std::path::PathBuf::from(&files[0]) } else { - ctx.cwd.join(&files[0]) + vfs_join(ctx.cwd, &files[0]) }; match read_text_file(&*ctx.fs, &path, "diff").await { Ok(text) => text.lines().map(|l| l.to_string()).collect(), @@ -365,7 +366,7 @@ impl Builtin for Diff { let path = if files[1].starts_with('/') { std::path::PathBuf::from(&files[1]) } else { - ctx.cwd.join(&files[1]) + vfs_join(ctx.cwd, &files[1]) }; match read_text_file(&*ctx.fs, &path, "diff").await { Ok(text) => text.lines().map(|l| l.to_string()).collect(), diff --git a/crates/bashkit/src/builtins/dirstack.rs b/crates/bashkit/src/builtins/dirstack.rs index 65c6bfd9d..bca7e08bd 100644 --- a/crates/bashkit/src/builtins/dirstack.rs +++ b/crates/bashkit/src/builtins/dirstack.rs @@ -12,13 +12,14 @@ use std::path::PathBuf; use super::limits::DIRSTACK_MAX_SIZE as MAX_DIRSTACK_SIZE; use super::{Builtin, Context}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; fn normalize_path(base: &std::path::Path, target: &str) -> PathBuf { let path = if target.starts_with('/') { PathBuf::from(target) } else { - base.join(target) + vfs_join(base, target) }; super::resolve_path(&PathBuf::from("/"), &path.to_string_lossy()) } diff --git a/crates/bashkit/src/builtins/disk.rs b/crates/bashkit/src/builtins/disk.rs index d1d5665f0..52d4c9518 100644 --- a/crates/bashkit/src/builtins/disk.rs +++ b/crates/bashkit/src/builtins/disk.rs @@ -5,6 +5,7 @@ use std::path::Path; use super::{Builtin, Context}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The du builtin - estimate file space usage. @@ -69,7 +70,7 @@ impl Builtin for Du { } else if path_str == "." { ctx.cwd.clone() } else { - ctx.cwd.join(path_str) + vfs_join(ctx.cwd, path_str) }; match calculate_size(&ctx, &path, summary_only, human_readable).await { @@ -129,7 +130,7 @@ fn calculate_size_recursive<'a>( ctx.consume_budget_work(u64::try_from(entries.len()).unwrap_or(u64::MAX))?; for entry in entries { - let child_path = path.join(&entry.name); + let child_path = vfs_join(path, &entry.name); total += calculate_size_recursive( ctx, &child_path, @@ -294,6 +295,35 @@ mod tests { // ==================== du tests ==================== + // Issue #2425: `du` prints the paths it builds, so every separator in + // them must be `/` even when the host separator is `\`. + #[tokio::test] + async fn windows_containment_du_prints_slash_separated_paths() { + let (fs, mut cwd, mut variables) = create_test_ctx().await; + let env = HashMap::new(); + + fs.mkdir(&cwd.join("proj/src"), true).await.unwrap(); + fs.write_file(&cwd.join("proj/src/main.rs"), b"fn main() {}") + .await + .unwrap(); + + let args = vec!["proj".to_string()]; + let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); + + let result = Du.execute(ctx).await.unwrap(); + assert_eq!(result.exit_code, 0); + assert!( + result.stdout.contains("/home/user/proj/src/main.rs"), + "recursive descent path is not slash-separated:\n{}", + result.stdout + ); + assert!( + !result.stdout.contains('\\'), + "output leaked a host separator:\n{}", + result.stdout + ); + } + #[tokio::test] async fn test_du_file() { let (fs, mut cwd, mut variables) = create_test_ctx().await; diff --git a/crates/bashkit/src/builtins/fileops.rs b/crates/bashkit/src/builtins/fileops.rs index f9777fe97..3fdd278b2 100644 --- a/crates/bashkit/src/builtins/fileops.rs +++ b/crates/bashkit/src/builtins/fileops.rs @@ -13,6 +13,7 @@ use std::path::Path; use super::limits::MKTEMP_MAX_ATTEMPTS; use super::{Builtin, Context, resolve_path}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The mkdir builtin - create directories. @@ -320,7 +321,7 @@ impl Builtin for Cp { .file_name() .map(|s| s.to_string_lossy().to_string()) .unwrap_or_else(|| source.to_string()); - dest_path.join(&filename) + vfs_join(&dest_path, &filename) } else { dest_path.clone() }; @@ -428,7 +429,7 @@ impl Builtin for Mv { .file_name() .map(|s| s.to_string_lossy().to_string()) .unwrap_or_else(|| source.to_string()); - dest_path.join(&filename) + vfs_join(&dest_path, &filename) } else { dest_path.clone() }; diff --git a/crates/bashkit/src/builtins/git/client.rs b/crates/bashkit/src/builtins/git/client.rs index 9248d9805..f505dfe7e 100644 --- a/crates/bashkit/src/builtins/git/client.rs +++ b/crates/bashkit/src/builtins/git/client.rs @@ -18,6 +18,7 @@ use crate::error::{Error, Result}; use crate::fs::FileSystem; use super::config::GitConfig; +use crate::fs::vfs_join; /// Git client for virtual operations. /// @@ -92,7 +93,7 @@ impl GitClient { /// The path must be within the virtual filesystem. Path traversal /// attacks are blocked by the VFS layer. pub async fn init(&self, fs: &Arc, repo_path: &Path) -> Result { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); // Check if already initialized if fs.exists(&git_dir).await? { @@ -104,13 +105,13 @@ impl GitClient { // Create .git directory structure fs.mkdir(&git_dir, true).await?; - fs.mkdir(&git_dir.join("objects"), true).await?; - fs.mkdir(&git_dir.join("refs"), true).await?; - fs.mkdir(&git_dir.join("refs/heads"), true).await?; - fs.mkdir(&git_dir.join("refs/tags"), true).await?; + fs.mkdir(&vfs_join(&git_dir, "objects"), true).await?; + fs.mkdir(&vfs_join(&git_dir, "refs"), true).await?; + fs.mkdir(&vfs_join(&git_dir, "refs/heads"), true).await?; + fs.mkdir(&vfs_join(&git_dir, "refs/tags"), true).await?; // Create HEAD pointing to master - fs.write_file(&git_dir.join("HEAD"), b"ref: refs/heads/master\n") + fs.write_file(&vfs_join(&git_dir, "HEAD"), b"ref: refs/heads/master\n") .await?; // Create config with author info @@ -124,11 +125,11 @@ impl GitClient { \temail = {}\n", self.config.author_name, self.config.author_email ); - fs.write_file(&git_dir.join("config"), config_content.as_bytes()) + fs.write_file(&vfs_join(&git_dir, "config"), config_content.as_bytes()) .await?; // Create empty index - fs.write_file(&git_dir.join("index"), b"").await?; + fs.write_file(&vfs_join(&git_dir, "index"), b"").await?; Ok(format!( "Initialized empty Git repository in {}/.git/\n", @@ -148,7 +149,7 @@ impl GitClient { repo_path: &Path, key: &str, ) -> Result> { - let config_path = repo_path.join(".git/config"); + let config_path = vfs_join(repo_path, ".git/config"); if !fs.exists(&config_path).await? { return Err(Error::Internal(format!( @@ -202,7 +203,7 @@ impl GitClient { key: &str, value: &str, ) -> Result<()> { - let config_path = repo_path.join(".git/config"); + let config_path = vfs_join(repo_path, ".git/config"); if !fs.exists(&config_path).await? { return Err(Error::Internal(format!( @@ -286,8 +287,8 @@ impl GitClient { repo_path: &Path, paths: &[&str], ) -> Result<()> { - let git_dir = repo_path.join(".git"); - let index_path = git_dir.join("index"); + let git_dir = vfs_join(repo_path, ".git"); + let index_path = vfs_join(&git_dir, "index"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -313,7 +314,7 @@ impl GitClient { let path = if Path::new(path_str).is_absolute() { PathBuf::from(path_str) } else { - repo_path.join(path_str) + vfs_join(repo_path, path_str) }; // Handle "." to add all files @@ -362,7 +363,7 @@ impl GitClient { continue; } - let path = dir.join(name); + let path = vfs_join(dir, name); let rel_path = path .strip_prefix(repo_path) .unwrap_or(&path) @@ -390,9 +391,9 @@ impl GitClient { repo_path: &Path, message: &str, ) -> Result { - let git_dir = repo_path.join(".git"); - let index_path = git_dir.join("index"); - let commits_path = git_dir.join("commits"); + let git_dir = vfs_join(repo_path, ".git"); + let index_path = vfs_join(&git_dir, "index"); + let commits_path = vfs_join(&git_dir, "commits"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -447,7 +448,7 @@ impl GitClient { fs.write_file(&commits_path, commits.as_bytes()).await?; // Track committed files by adding staged files to the tracked set - let tracked_path = git_dir.join("tracked"); + let tracked_path = vfs_join(&git_dir, "tracked"); let mut tracked: HashSet = HashSet::new(); if fs.exists(&tracked_path).await? { let content = fs.read_file(&tracked_path).await?; @@ -475,7 +476,7 @@ impl GitClient { fs.write_file(&index_path, b"").await?; // Update HEAD - let head_ref_path = git_dir.join("refs/heads/master"); + let head_ref_path = vfs_join(&git_dir, "refs/heads/master"); fs.write_file(&head_ref_path, hash.as_bytes()).await?; Ok(format!( @@ -487,8 +488,8 @@ impl GitClient { /// Get repository status. pub async fn status(&self, fs: &Arc, repo_path: &Path) -> Result { - let git_dir = repo_path.join(".git"); - let index_path = git_dir.join("index"); + let git_dir = vfs_join(repo_path, ".git"); + let index_path = vfs_join(&git_dir, "index"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -503,7 +504,7 @@ impl GitClient { }; // Read HEAD to get branch - let head_path = git_dir.join("HEAD"); + let head_path = vfs_join(&git_dir, "HEAD"); if fs.exists(&head_path).await? { let content = fs.read_file(&head_path).await?; let content = String::from_utf8_lossy(&content); @@ -526,7 +527,7 @@ impl GitClient { } // Load tracked (committed) files - let tracked_path = git_dir.join("tracked"); + let tracked_path = vfs_join(&git_dir, "tracked"); let mut tracked_files: HashSet = HashSet::new(); if fs.exists(&tracked_path).await? { let content = fs.read_file(&tracked_path).await?; @@ -573,7 +574,7 @@ impl GitClient { continue; } - let path = dir.join(name); + let path = vfs_join(dir, name); let rel_path = path .strip_prefix(repo_path) .unwrap_or(&path) @@ -598,8 +599,8 @@ impl GitClient { repo_path: &Path, limit: Option, ) -> Result> { - let git_dir = repo_path.join(".git"); - let commits_path = git_dir.join("commits"); + let git_dir = vfs_join(repo_path, ".git"); + let commits_path = vfs_join(&git_dir, "commits"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -732,7 +733,7 @@ impl GitClient { name: &str, url: &str, ) -> Result<()> { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -745,7 +746,7 @@ impl GitClient { self.config.is_url_allowed(url).map_err(Error::Internal)?; // Store remote in .git/remotes file - let remotes_path = git_dir.join("remotes"); + let remotes_path = vfs_join(&git_dir, "remotes"); let mut remotes = String::new(); if fs.exists(&remotes_path).await? { let content = fs.read_file(&remotes_path).await?; @@ -778,8 +779,8 @@ impl GitClient { repo_path: &Path, name: &str, ) -> Result<()> { - let git_dir = repo_path.join(".git"); - let remotes_path = git_dir.join("remotes"); + let git_dir = vfs_join(repo_path, ".git"); + let remotes_path = vfs_join(&git_dir, "remotes"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -822,8 +823,8 @@ impl GitClient { fs: &Arc, repo_path: &Path, ) -> Result> { - let git_dir = repo_path.join(".git"); - let remotes_path = git_dir.join("remotes"); + let git_dir = vfs_join(repo_path, ".git"); + let remotes_path = vfs_join(&git_dir, "remotes"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -979,8 +980,8 @@ impl GitClient { fs: &Arc, repo_path: &Path, ) -> Result> { - let git_dir = repo_path.join(".git"); - let refs_heads = git_dir.join("refs/heads"); + let git_dir = vfs_join(repo_path, ".git"); + let refs_heads = vfs_join(&git_dir, "refs/heads"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -1027,8 +1028,8 @@ impl GitClient { fs: &Arc, repo_path: &Path, ) -> Result { - let git_dir = repo_path.join(".git"); - let head_path = git_dir.join("HEAD"); + let git_dir = vfs_join(repo_path, ".git"); + let head_path = vfs_join(&git_dir, "HEAD"); if fs.exists(&head_path).await? { let content = fs.read_file(&head_path).await?; @@ -1098,7 +1099,9 @@ impl GitClient { for component in path.components() { match component { - Component::Normal(part) => clean.push(part), + // `PathBuf::push` would insert the host separator here, so the + // cleaned pathspec would not be a VFS path on Windows (#2425). + Component::Normal(part) => clean = vfs_join(&clean, part), Component::CurDir => {} Component::ParentDir | Component::RootDir | Component::Prefix(_) => { return Err(Error::Internal(format!( @@ -1134,9 +1137,9 @@ impl GitClient { name: &str, ) -> Result<()> { Self::validate_ref_name(name)?; - let git_dir = repo_path.join(".git"); - let refs_heads = git_dir.join("refs/heads"); - let branch_path = refs_heads.join(name); + let git_dir = vfs_join(repo_path, ".git"); + let refs_heads = vfs_join(&git_dir, "refs/heads"); + let branch_path = vfs_join(&refs_heads, name); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -1153,7 +1156,7 @@ impl GitClient { } // Get current HEAD commit - let head_ref = refs_heads.join(self.get_current_branch(fs, repo_path).await?); + let head_ref = vfs_join(&refs_heads, self.get_current_branch(fs, repo_path).await?); let commit_hash = if fs.exists(&head_ref).await? { let content = fs.read_file(&head_ref).await?; String::from_utf8_lossy(&content).trim().to_string() @@ -1181,8 +1184,8 @@ impl GitClient { name: &str, ) -> Result<()> { Self::validate_ref_name(name)?; - let git_dir = repo_path.join(".git"); - let branch_path = git_dir.join("refs/heads").join(name); + let git_dir = vfs_join(repo_path, ".git"); + let branch_path = vfs_join(&git_dir, "refs/heads").join(name); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -1220,9 +1223,9 @@ impl GitClient { target: &str, ) -> Result { Self::validate_ref_name(target)?; - let git_dir = repo_path.join(".git"); - let head_path = git_dir.join("HEAD"); - let branch_path = git_dir.join("refs/heads").join(target); + let git_dir = vfs_join(repo_path, ".git"); + let head_path = vfs_join(&git_dir, "HEAD"); + let branch_path = vfs_join(&git_dir, "refs/heads").join(target); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -1264,7 +1267,7 @@ impl GitClient { _from: Option<&str>, _to: Option<&str>, ) -> Result { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -1286,7 +1289,7 @@ impl GitClient { mode: &str, target: Option<&str>, ) -> Result { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( @@ -1300,7 +1303,7 @@ impl GitClient { match mode { "--soft" | "--mixed" | "--hard" => { // Clear staged files (index) - let index_path = git_dir.join("index"); + let index_path = vfs_join(&git_dir, "index"); fs.write_file(&index_path, b"").await?; Ok(match mode { @@ -1344,7 +1347,7 @@ impl GitClient { repo_path: &Path, target: Option<&str>, ) -> Result { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( "fatal: not a git repository: {}", @@ -1364,7 +1367,7 @@ impl GitClient { ))); } let safe_path = Self::validate_repo_pathspec(path)?; - let file_path = repo_path.join(&safe_path); + let file_path = vfs_join(repo_path, &safe_path); if fs.exists(&file_path).await? { let content = fs.read_file(&file_path).await?; // Sanitize file content from VFS (TM-GIT-015) @@ -1404,7 +1407,7 @@ impl GitClient { fs: &Arc, repo_path: &Path, ) -> Result> { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( "fatal: not a git repository: {}", @@ -1412,7 +1415,7 @@ impl GitClient { ))); } - let tracked_path = git_dir.join("tracked"); + let tracked_path = vfs_join(&git_dir, "tracked"); let mut files = Vec::new(); if fs.exists(&tracked_path).await? { let content = fs.read_file(&tracked_path).await?; @@ -1425,7 +1428,7 @@ impl GitClient { } // Also include staged but not yet committed files - let index_path = git_dir.join("index"); + let index_path = vfs_join(&git_dir, "index"); if fs.exists(&index_path).await? { let content = fs.read_file(&index_path).await?; let content = String::from_utf8_lossy(&content); @@ -1450,7 +1453,7 @@ impl GitClient { repo_path: &Path, args: &[&str], ) -> Result { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal( "fatal: not a git repository (or any parent up to mount point /)\n\ @@ -1476,7 +1479,7 @@ impl GitClient { "--abbrev-ref" => { i += 1; if i < args.len() && args[i] == "HEAD" { - let head_content = fs.read_file(&git_dir.join("HEAD")).await?; + let head_content = fs.read_file(&vfs_join(&git_dir, "HEAD")).await?; let head = String::from_utf8_lossy(&head_content); if let Some(branch) = head.trim().strip_prefix("ref: refs/heads/") { // Sanitize branch name from HEAD ref (TM-GIT-015) @@ -1491,11 +1494,11 @@ impl GitClient { } "HEAD" => { // Resolve HEAD to commit hash - let head_content = fs.read_file(&git_dir.join("HEAD")).await?; + let head_content = fs.read_file(&vfs_join(&git_dir, "HEAD")).await?; let head = String::from_utf8_lossy(&head_content); if let Some(branch) = head.trim().strip_prefix("ref: refs/heads/") { Self::validate_ref_name(branch)?; - let ref_path = git_dir.join("refs/heads").join(branch); + let ref_path = vfs_join(&git_dir, "refs/heads").join(branch); if fs.exists(&ref_path).await? { let hash = fs.read_file(&ref_path).await?; // Sanitize hash read from ref file (TM-GIT-015) @@ -1520,7 +1523,7 @@ impl GitClient { arg )) })?; - let ref_path = git_dir.join("refs/heads").join(arg); + let ref_path = vfs_join(&git_dir, "refs/heads").join(arg); if fs.exists(&ref_path).await? { let hash = fs.read_file(&ref_path).await?; // Sanitize hash read from ref file (TM-GIT-015) @@ -1552,7 +1555,7 @@ impl GitClient { paths: &[&str], staged: bool, ) -> Result { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( "fatal: not a git repository: {}", @@ -1562,7 +1565,7 @@ impl GitClient { if staged { // Unstage files (remove from index) - let index_path = git_dir.join("index"); + let index_path = vfs_join(&git_dir, "index"); if fs.exists(&index_path).await? { let content = fs.read_file(&index_path).await?; let content = String::from_utf8_lossy(&content); @@ -1580,7 +1583,7 @@ impl GitClient { // Restore working tree files from tracked versions // Since we don't store per-commit snapshots, this is a no-op message for path in paths { - let file_path = repo_path.join(path); + let file_path = vfs_join(repo_path, path); if !fs.exists(&file_path).await? { return Err(Error::Internal(format!( "error: pathspec '{}' did not match any file(s) known to git", @@ -1603,7 +1606,7 @@ impl GitClient { ref1: &str, ref2: &str, ) -> Result { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( "fatal: not a git repository: {}", @@ -1647,7 +1650,7 @@ impl GitClient { pattern: &str, paths: &[&str], ) -> Result { - let git_dir = repo_path.join(".git"); + let git_dir = vfs_join(repo_path, ".git"); if !fs.exists(&git_dir).await? { return Err(Error::Internal(format!( "fatal: not a git repository: {}", @@ -1685,7 +1688,7 @@ impl GitClient { let mut output = String::new(); for file in &files { - let file_path = repo_path.join(file); + let file_path = vfs_join(repo_path, file); if !fs.exists(&file_path).await? { continue; } @@ -1715,11 +1718,11 @@ impl GitClient { refspec: &str, ) -> Result { if refspec == "HEAD" { - let head_content = fs.read_file(&git_dir.join("HEAD")).await?; + let head_content = fs.read_file(&vfs_join(git_dir, "HEAD")).await?; let head = String::from_utf8_lossy(&head_content); if let Some(branch) = head.trim().strip_prefix("ref: refs/heads/") { Self::validate_ref_name(branch)?; - let ref_path = git_dir.join("refs/heads").join(branch); + let ref_path = vfs_join(git_dir, "refs/heads").join(branch); if fs.exists(&ref_path).await? { let hash = fs.read_file(&ref_path).await?; return Ok(String::from_utf8_lossy(&hash).trim().to_string()); @@ -1730,7 +1733,7 @@ impl GitClient { // Try as branch name. Invalid ref names cannot be resolved via the VFS. if Self::validate_ref_name(refspec).is_ok() { - let ref_path = git_dir.join("refs/heads").join(refspec); + let ref_path = vfs_join(git_dir, "refs/heads").join(refspec); if fs.exists(&ref_path).await? { let hash = fs.read_file(&ref_path).await?; return Ok(String::from_utf8_lossy(&hash).trim().to_string()); diff --git a/crates/bashkit/src/builtins/grep.rs b/crates/bashkit/src/builtins/grep.rs index b2ded3973..2ffa0a05e 100644 --- a/crates/bashkit/src/builtins/grep.rs +++ b/crates/bashkit/src/builtins/grep.rs @@ -43,6 +43,7 @@ use super::search_common::{ }; use super::{Builtin, Context}; use crate::error::{Error, Result}; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// grep command - pattern matching @@ -560,7 +561,7 @@ impl Builtin for Grep { let path = if pattern_file.starts_with('/') { std::path::PathBuf::from(pattern_file) } else { - ctx.cwd.join(pattern_file) + vfs_join(ctx.cwd, pattern_file) }; match ctx.fs.read_file(&path).await { Ok(content) => { @@ -627,7 +628,7 @@ impl Builtin for Grep { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; dirs_to_process.push(path); } @@ -635,7 +636,7 @@ impl Builtin for Grep { while let Some(path) = dirs_to_process.pop() { if let Ok(entries) = ctx.fs.read_dir(&path).await { for entry in entries { - let entry_path = path.join(&entry.name); + let entry_path = vfs_join(&path, &entry.name); if entry.metadata.file_type.is_dir() { // Skip dirs matching --exclude-dir patterns if opts @@ -673,7 +674,7 @@ impl Builtin for Grep { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; match ctx.fs.read_file(&path).await { @@ -1003,7 +1004,7 @@ async fn try_indexed_search( let root = crate::fs::normalize_path(&if file.starts_with('/') { std::path::PathBuf::from(file) } else { - cwd.join(file) + vfs_join(cwd, file) }); let provider = sc.search_provider(&root)?; let caps = provider.capabilities(); @@ -1054,7 +1055,7 @@ async fn try_indexed_search( let candidate = if m.path.is_absolute() { crate::fs::normalize_path(&m.path) } else { - crate::fs::normalize_path(&root.join(&m.path)) + crate::fs::normalize_path(&vfs_join(&root, &m.path)) }; if !candidate.starts_with(&root) || !seen_paths.insert(candidate.clone()) { @@ -1458,6 +1459,55 @@ mod tests { assert!(!result.stdout.contains("b.log")); } + // Issue #2425: `grep -r` prints the paths it walks, so a relative operand + // resolved against the cwd and every directory entry below it must stay + // `/`-separated even where the host separator is `\`. + #[tokio::test] + async fn windows_containment_grep_recursive_prints_slash_separated_paths() { + let grep = Grep; + let fs = Arc::new(InMemoryFs::new()); + fs.mkdir(&PathBuf::from("/d/proj/src"), true).await.unwrap(); + fs.write_file(&PathBuf::from("/d/proj/src/main.rs"), b"fn main() {}\n") + .await + .unwrap(); + + let mut vars = HashMap::new(); + let mut cwd = PathBuf::from("/d/proj"); + let args: Vec = ["-r", "main", "src"] + .iter() + .map(|s| s.to_string()) + .collect(); + + let ctx = Context { + args: &args, + env: &HashMap::new(), + variables: &mut vars, + cwd: &mut cwd, + fs, + stdin: None, + #[cfg(feature = "http_client")] + http_client: None, + #[cfg(feature = "git")] + git_client: None, + #[cfg(feature = "ssh")] + ssh_client: None, + shell: None, + }; + + let result = grep.execute(ctx).await.unwrap(); + assert_eq!(result.exit_code, 0); + assert!( + result.stdout.starts_with("/d/proj/src/main.rs:"), + "match path is not slash-separated:\n{}", + result.stdout + ); + assert!( + !result.stdout.contains('\\'), + "output leaked a host separator:\n{}", + result.stdout + ); + } + #[tokio::test] async fn test_grep_recursive_single_file() { let grep = Grep; diff --git a/crates/bashkit/src/builtins/headtail.rs b/crates/bashkit/src/builtins/headtail.rs index 75cb5b45a..0850c7258 100644 --- a/crates/bashkit/src/builtins/headtail.rs +++ b/crates/bashkit/src/builtins/headtail.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use super::{Builtin, Context, read_text_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// Default number of lines to output @@ -61,7 +62,7 @@ impl Builtin for Head { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; match ctx.fs.read_file(&path).await { @@ -141,7 +142,7 @@ impl Builtin for Tail { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; let text = match read_text_file(&*ctx.fs, &path, "tail").await { diff --git a/crates/bashkit/src/builtins/hextools.rs b/crates/bashkit/src/builtins/hextools.rs index e76281f19..3a83f711a 100644 --- a/crates/bashkit/src/builtins/hextools.rs +++ b/crates/bashkit/src/builtins/hextools.rs @@ -5,6 +5,7 @@ use async_trait::async_trait; use super::{Builtin, Context}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The od builtin - dump files in octal and other formats. @@ -651,7 +652,7 @@ async fn collect_input( let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - cwd.join(file) + vfs_join(cwd, file) }; let content = fs diff --git a/crates/bashkit/src/builtins/ls/find.rs b/crates/bashkit/src/builtins/ls/find.rs index 20a3a2153..826ce6e75 100644 --- a/crates/bashkit/src/builtins/ls/find.rs +++ b/crates/bashkit/src/builtins/ls/find.rs @@ -9,6 +9,7 @@ use super::glob_match; use crate::builtins::limits::FIND_MAX_OUTPUT_BYTES; use crate::builtins::{Builtin, Context, ExecutionPlan, SubCommand, resolve_path}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::{ControlFlow, ExecResult}; /// Options for find command @@ -534,7 +535,7 @@ fn find_recursive<'a>( sorted_entries.sort_by(|a, b| a.name.cmp(&b.name)); for entry in sorted_entries { - let child_path = path.join(&entry.name); + let child_path = vfs_join(path, &entry.name); let child_display = if display_path == "." { format!("./{}", entry.name) } else { diff --git a/crates/bashkit/src/builtins/ls/list.rs b/crates/bashkit/src/builtins/ls/list.rs index d8e163f13..51f17f3c6 100644 --- a/crates/bashkit/src/builtins/ls/list.rs +++ b/crates/bashkit/src/builtins/ls/list.rs @@ -13,6 +13,7 @@ use crate::builtins::generated::ls_args::LS_ENV_DEFAULTS; use crate::builtins::{Builtin, Context, resolve_path}; use crate::error::Result; use crate::fs::FileType; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// Argument IDs from the generated `ls_command()` that bashkit currently @@ -301,7 +302,7 @@ async fn list_directory( output.push_str(&line); if opts.recursive && entry.metadata.file_type.is_dir() { subdirs.push(( - path.join(&entry.name), + vfs_join(path, &entry.name), format!("{}/{}", display_path, entry.name), )); } @@ -317,7 +318,7 @@ async fn list_directory( names.push(name); if opts.recursive && entry.metadata.file_type.is_dir() { subdirs.push(( - path.join(&entry.name), + vfs_join(path, &entry.name), format!("{}/{}", display_path, entry.name), )); } diff --git a/crates/bashkit/src/builtins/mod.rs b/crates/bashkit/src/builtins/mod.rs index 9d77dd566..50af392e4 100644 --- a/crates/bashkit/src/builtins/mod.rs +++ b/crates/bashkit/src/builtins/mod.rs @@ -601,7 +601,7 @@ pub fn resolve_path(cwd: &Path, path_str: &str) -> PathBuf { let joined = if path.is_absolute() { path.to_path_buf() } else { - cwd.join(path) + vfs_join(cwd, path) }; // Normalize the path to handle . and .. components normalize_path(&joined) @@ -609,6 +609,7 @@ pub fn resolve_path(cwd: &Path, path_str: &str) -> PathBuf { // Re-export shared normalize_path for use by builtins use crate::fs::normalize_path; +use crate::fs::vfs_join; /// Execution context for builtin commands. /// @@ -1469,6 +1470,86 @@ mod tests { ); } + // ------------------------------------------------------------------------- + // Issue #2425: VFS paths are Unix-style on every host, but + // `PathBuf::join`/`push` insert the *host* separator, so on Windows a + // joined path prints as `/d/proj\src\main.rs`. + // + // Static guard — the shapes that build a VFS path from a cwd or from a + // directory entry must go through `crate::fs::vfs_join`. The dynamic + // counterpart is the `windows_containment_*` tests next to `vfs_join` and + // the affected builtins, which the Windows CI job runs. + // ------------------------------------------------------------------------- + + #[test] + fn no_host_separator_joins_on_vfs_paths() { + // `Path::join` shapes seen on VFS paths: a cwd-relative operand and a + // directory entry appended during a recursive walk. + let shapes = ["cwd.join(", ".join(&entry.name)", ".join(&e.name)"]; + let manifest = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + let mut violations = Vec::new(); + + fn walk( + dir: &std::path::Path, + root: &std::path::Path, + shapes: &[&str], + violations: &mut Vec, + ) { + for entry in std::fs::read_dir(dir).expect("read source dir") { + let entry = entry.unwrap(); + let path = entry.path(); + if path.is_dir() { + walk(&path, root, shapes, violations); + continue; + } + if path.extension().is_none_or(|e| e != "rs") { + continue; + } + // Whole test files build VFS paths for fixtures, never for + // output (inline test modules are skipped below). + if path.file_name().is_some_and(|n| n == "tests.rs") { + continue; + } + let src = std::fs::read_to_string(&path).expect("read source"); + // Skip inline `#[cfg(test)] mod ... { ... }` blocks for the + // same reason. A `#[cfg(test)] mod tests;` declaration is not a + // block, and the file it names is skipped by name above. + let mut in_test_mod = false; + let mut prev_attr = false; + for (i, line) in src.lines().enumerate() { + if in_test_mod { + in_test_mod = line != "}"; + continue; + } + if prev_attr && line.starts_with("mod ") && line.ends_with('{') { + in_test_mod = true; + prev_attr = false; + continue; + } + prev_attr = line == "#[cfg(test)]"; + if line.contains("// host-join-ok:") { + continue; + } + if shapes.iter().any(|shape| line.contains(shape)) { + let rel = path.strip_prefix(root).unwrap_or(&path); + violations.push(format!("{}:{}: {}", rel.display(), i + 1, line.trim())); + } + } + } + } + + walk(&manifest.join("src"), manifest, &shapes, &mut violations); + + assert!( + violations.is_empty(), + "`Path::join` used to build a VFS path. On Windows it inserts the \ + host separator, so the path prints as `/d/proj\\src\\main.rs` \ + (issue #2425). Use `crate::fs::vfs_join` instead, or add \ + `// host-join-ok: ` for a genuine host path.\n\nViolations:\n{}", + violations.join("\n") + ); + } + /// TM-INF-024: clap `Arg::env(...)` reads defaults from the real /// process environment. uutils ships `.env("TABSIZE")` / /// `.env("TIME_STYLE")` on `ls`, but bashkit isolates scripts inside diff --git a/crates/bashkit/src/builtins/navigation.rs b/crates/bashkit/src/builtins/navigation.rs index 5656e1bc5..f3bca5d73 100644 --- a/crates/bashkit/src/builtins/navigation.rs +++ b/crates/bashkit/src/builtins/navigation.rs @@ -31,7 +31,7 @@ impl Builtin for Cd { .map(PathBuf::from) .unwrap_or_else(|| ctx.cwd.clone()) } else { - ctx.cwd.join(target) + vfs_join(ctx.cwd, target) }; // Normalize the path @@ -73,3 +73,4 @@ impl Builtin for Pwd { } use crate::fs::normalize_path; +use crate::fs::vfs_join; diff --git a/crates/bashkit/src/builtins/nl.rs b/crates/bashkit/src/builtins/nl.rs index e3f488ac5..5d2496412 100644 --- a/crates/bashkit/src/builtins/nl.rs +++ b/crates/bashkit/src/builtins/nl.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use super::{Builtin, Context, MAX_FORMAT_WIDTH, read_text_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The nl builtin - number lines of files. @@ -189,7 +190,7 @@ impl Builtin for Nl { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; let text = match read_text_file(&*ctx.fs, &path, "nl").await { diff --git a/crates/bashkit/src/builtins/paste.rs b/crates/bashkit/src/builtins/paste.rs index be78445e0..a534bf6cc 100644 --- a/crates/bashkit/src/builtins/paste.rs +++ b/crates/bashkit/src/builtins/paste.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use super::{Builtin, Context, read_text_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The paste builtin - merge lines of files. @@ -170,7 +171,7 @@ impl Builtin for Paste { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; let text = match read_text_file(&*ctx.fs, &path, "paste").await { diff --git a/crates/bashkit/src/builtins/path.rs b/crates/bashkit/src/builtins/path.rs index 29173fd39..0381db6d6 100644 --- a/crates/bashkit/src/builtins/path.rs +++ b/crates/bashkit/src/builtins/path.rs @@ -6,6 +6,7 @@ use std::path::Path; use super::{Builtin, Context}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The basename builtin - strip directory and suffix from filenames. @@ -478,7 +479,7 @@ async fn follow_readlink_symlinks( let mut next = if target.is_absolute() { target } else { - link_parent.join(target) + vfs_join(link_parent, target) }; for remaining in components.iter().skip(idx + 1) { diff --git a/crates/bashkit/src/builtins/python.rs b/crates/bashkit/src/builtins/python.rs index 2f04b1c9b..daefe4221 100644 --- a/crates/bashkit/src/builtins/python.rs +++ b/crates/bashkit/src/builtins/python.rs @@ -37,6 +37,7 @@ use std::time::Duration; use super::{Builtin, Context, ExecutionDeadline, RuntimeLimits, resolve_path}; use crate::error::Result; +use crate::fs::vfs_join; use crate::fs::{FileSystem, FileType}; use crate::interpreter::ExecResult; @@ -978,7 +979,7 @@ async fn handle_os_call( let items: Vec = entries .into_iter() .map(|e| { - let child = path.join(&e.name); + let child = vfs_join(&path, &e.name); MontyObject::Path(child.to_string_lossy().to_string()) }) .collect(); @@ -1089,7 +1090,7 @@ fn resolve_python_path(path_str: &str, cwd: &Path) -> PathBuf { if p.is_absolute() { p.to_owned() } else { - cwd.join(p) + vfs_join(cwd, p) } } diff --git a/crates/bashkit/src/builtins/rg/mod.rs b/crates/bashkit/src/builtins/rg/mod.rs index ff7c8e99a..4e06bc62d 100644 --- a/crates/bashkit/src/builtins/rg/mod.rs +++ b/crates/bashkit/src/builtins/rg/mod.rs @@ -28,6 +28,7 @@ use std::sync::OnceLock; use super::search_common::build_regex_opts; use super::{Builtin, Context, read_text_file, resolve_path}; use crate::error::{Error, Result}; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; use crate::limits::ExecutionLimits; @@ -3193,7 +3194,8 @@ async fn load_rg_global_ignore_files( let mut configured_paths = Vec::new(); if let Some(home) = non_empty_env_path(env, "HOME") { - configured_paths.extend(git_config_excludes_files(fs, &home.join(".gitconfig")).await?); + configured_paths + .extend(git_config_excludes_files(fs, &vfs_join(&home, ".gitconfig")).await?); } if configured_paths.is_empty() { @@ -3236,8 +3238,10 @@ fn default_git_global_ignore_path( env: &std::collections::HashMap, ) -> Option { non_empty_env_path(env, "XDG_CONFIG_HOME") - .map(|path| path.join("git/ignore")) - .or_else(|| non_empty_env_path(env, "HOME").map(|path| path.join(".config/git/ignore"))) + .map(|path| vfs_join(&path, "git/ignore")) + .or_else(|| { + non_empty_env_path(env, "HOME").map(|path| vfs_join(&path, ".config/git/ignore")) + }) } async fn git_config_excludes_files( @@ -3287,7 +3291,7 @@ fn expand_git_config_path(value: &str, home: &Path) -> PathBuf { return home.to_path_buf(); } if let Some(rest) = value.strip_prefix("~/") { - return home.join(rest); + return vfs_join(home, rest); } PathBuf::from(value) } @@ -3329,7 +3333,7 @@ async fn load_local_ignore_rules( if !opts.no_ignore_exclude { load_optional_ignore_file_with_rule_base( fs, - &dir.join(".git/info/exclude"), + &vfs_join(dir, ".git/info/exclude"), dir, opts.ignore_file_case_insensitive, inherited_rule_count, @@ -3339,7 +3343,7 @@ async fn load_local_ignore_rules( } load_optional_ignore_file_with_rule_base( fs, - &dir.join(".gitignore"), + &vfs_join(dir, ".gitignore"), dir, opts.ignore_file_case_insensitive, inherited_rule_count, @@ -3350,7 +3354,7 @@ async fn load_local_ignore_rules( if !opts.no_ignore_dot { load_optional_ignore_file_with_rule_base( fs, - &dir.join(".ignore"), + &vfs_join(dir, ".ignore"), dir, opts.ignore_file_case_insensitive, inherited_rule_count, @@ -3359,7 +3363,7 @@ async fn load_local_ignore_rules( .await?; load_optional_ignore_file_with_rule_base( fs, - &dir.join(".rgignore"), + &vfs_join(dir, ".rgignore"), dir, opts.ignore_file_case_insensitive, inherited_rule_count, @@ -3491,7 +3495,7 @@ async fn has_git_dir_in_ancestors(fs: &dyn crate::fs::FileSystem, dir: &Path, ro for ancestor in dir.ancestors() { if ancestor.starts_with(root) && fs - .stat(&ancestor.join(".git")) + .stat(&vfs_join(ancestor, ".git")) .await .is_ok_and(|meta| meta.file_type.is_dir()) { @@ -3650,8 +3654,8 @@ async fn collect_rg_files_recursive( }; if let Ok(entries) = fs.read_dir(&item.actual).await { for entry in entries { - let path = item.logical.join(&entry.name); - let actual_path = item.actual.join(&entry.name); + let path = vfs_join(&item.logical, &entry.name); + let actual_path = vfs_join(&item.actual, &entry.name); let hidden_entry = !opts.hidden && is_hidden_name(&entry.name); let entry_depth = item.depth + 1; let (entry_actual_path, entry_metadata) = @@ -3889,7 +3893,7 @@ async fn try_indexed_search( let root = if p.starts_with('/') { PathBuf::from(p) } else { - cwd.join(p) + vfs_join(cwd, p) }; (root, Some(p.clone())) }) @@ -3934,7 +3938,7 @@ async fn try_indexed_search( let candidate = if m.path.is_absolute() { crate::fs::normalize_path(&m.path) } else { - crate::fs::normalize_path(&root.join(&m.path)) + crate::fs::normalize_path(&vfs_join(&root, &m.path)) }; let explicit_file_match = explicit_file_root && candidate == root; diff --git a/crates/bashkit/src/builtins/sed.rs b/crates/bashkit/src/builtins/sed.rs index 7b2a9ee62..0680054d1 100644 --- a/crates/bashkit/src/builtins/sed.rs +++ b/crates/bashkit/src/builtins/sed.rs @@ -26,6 +26,7 @@ use super::search_common::{REGEX_DFA_SIZE_LIMIT, REGEX_SIZE_LIMIT, build_regex, use super::limits::SED_MAX_GROUP_NESTING_DEPTH as MAX_GROUP_NESTING_DEPTH; use super::{Builtin, Context, read_text_file}; use crate::error::{Error, Result}; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// Regex wrapper that falls back to fancy-regex for patterns with backreferences. @@ -961,7 +962,7 @@ impl Builtin for Sed { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; let text = match read_text_file(&*ctx.fs, &path, "sed").await { @@ -1132,7 +1133,7 @@ impl Builtin for Sed { let path = if filename.starts_with('/') { std::path::PathBuf::from(&filename) } else { - ctx.cwd.join(&filename) + vfs_join(ctx.cwd, &filename) }; if let Err(e) = ctx.fs.write_file(&path, content.as_bytes()).await { diff --git a/crates/bashkit/src/builtins/shuf.rs b/crates/bashkit/src/builtins/shuf.rs index 1ce942b79..b0f1c9f82 100644 --- a/crates/bashkit/src/builtins/shuf.rs +++ b/crates/bashkit/src/builtins/shuf.rs @@ -18,6 +18,7 @@ use std::path::Path; use super::{Builtin, Context, read_text_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; use crate::limits::ExecutionLimits; @@ -93,7 +94,7 @@ impl Builtin for Shuf { let path = if Path::new(file).is_absolute() { file.clone() } else { - ctx.cwd.join(file).to_string_lossy().into_owned() + vfs_join(ctx.cwd, file).to_string_lossy().into_owned() }; match read_text_file(&*ctx.fs, Path::new(&path), "shuf").await { Ok(t) => t, @@ -121,7 +122,7 @@ impl Builtin for Shuf { let resolved = if path.is_absolute() { path.clone() } else { - ctx.cwd.join(&path) + vfs_join(ctx.cwd, &path) }; if let Err(e) = ctx.fs.write_file(&resolved, out.as_bytes()).await { return Ok(ExecResult::err( diff --git a/crates/bashkit/src/builtins/sortuniq.rs b/crates/bashkit/src/builtins/sortuniq.rs index 7e67735eb..afcdbc960 100644 --- a/crates/bashkit/src/builtins/sortuniq.rs +++ b/crates/bashkit/src/builtins/sortuniq.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use super::{Builtin, Context, read_text_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The sort builtin - sort lines of text. @@ -409,7 +410,7 @@ impl Builtin for Sort { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; let text = match read_text_file(&*ctx.fs, &path, "sort").await { @@ -431,7 +432,7 @@ impl Builtin for Sort { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; let text = match read_text_file(&*ctx.fs, &path, "sort").await { Ok(t) => t, @@ -607,7 +608,7 @@ impl Builtin for Sort { let path = if outfile.starts_with('/') { std::path::PathBuf::from(outfile) } else { - ctx.cwd.join(outfile) + vfs_join(ctx.cwd, outfile) }; if let Err(e) = ctx.fs.write_file(&path, output.as_bytes()).await { return Ok(ExecResult::err(format!("sort: {}: {}\n", outfile, e), 1)); @@ -704,7 +705,7 @@ impl Builtin for Uniq { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; match read_text_file(&*ctx.fs, &path, "uniq").await { diff --git a/crates/bashkit/src/builtins/sqlite/mod.rs b/crates/bashkit/src/builtins/sqlite/mod.rs index cf92c2605..a32a09b8f 100644 --- a/crates/bashkit/src/builtins/sqlite/mod.rs +++ b/crates/bashkit/src/builtins/sqlite/mod.rs @@ -48,6 +48,7 @@ use crate::fs::FileSystem; use crate::interpreter::ExecResult; use super::{Builtin, Context, check_help_version, resolve_path}; +use crate::fs::vfs_join; use dot_commands::{DotError, DotOutcome}; use engine::{QueryLimits, SqliteEngine}; use formatter::{OutputMode, OutputOpts, render}; @@ -839,7 +840,11 @@ async fn run_statements( Ok(DotOutcome::Configured) => {} Ok(DotOutcome::Quit) => return Ok(()), Ok(DotOutcome::Read(p)) => { - let abs = if p.is_absolute() { p } else { cwd.join(&p) }; + let abs = if p.is_absolute() { + p + } else { + vfs_join(cwd, &p) + }; let bytes = fs .read_file(&abs) .await diff --git a/crates/bashkit/src/builtins/strings.rs b/crates/bashkit/src/builtins/strings.rs index 74d715169..0245fa6f7 100644 --- a/crates/bashkit/src/builtins/strings.rs +++ b/crates/bashkit/src/builtins/strings.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use super::{Builtin, Context}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The strings builtin - find printable strings in files. @@ -184,7 +185,7 @@ impl Builtin for Strings { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; match ctx.fs.read_file(&path).await { diff --git a/crates/bashkit/src/builtins/test.rs b/crates/bashkit/src/builtins/test.rs index c131c5e60..2f4f0bdc6 100644 --- a/crates/bashkit/src/builtins/test.rs +++ b/crates/bashkit/src/builtins/test.rs @@ -9,6 +9,7 @@ use async_trait::async_trait; use super::{Builtin, Context}; use crate::error::Result; use crate::fs::FileSystem; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The test builtin command. @@ -71,7 +72,7 @@ fn resolve_file_path(cwd: &Path, arg: &str) -> PathBuf { if p.is_absolute() { p.to_path_buf() } else { - cwd.join(p) + vfs_join(cwd, p) } } diff --git a/crates/bashkit/src/builtins/textrev.rs b/crates/bashkit/src/builtins/textrev.rs index c5d4cd7f2..fe2f20053 100644 --- a/crates/bashkit/src/builtins/textrev.rs +++ b/crates/bashkit/src/builtins/textrev.rs @@ -11,6 +11,7 @@ use std::path::Path; use super::{Builtin, Context, read_text_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// Read input from files or stdin, returning the raw text. Used by `rev`. @@ -40,7 +41,7 @@ async fn read_input(ctx: &Context<'_>) -> std::result::Result Option { if p.is_absolute() { Some(p.to_owned()) } else { - Some(cwd.join(p)) + Some(vfs_join(&cwd, p)) } } _ => None, diff --git a/crates/bashkit/src/builtins/wc.rs b/crates/bashkit/src/builtins/wc.rs index 8c19124b5..bb7cd1559 100644 --- a/crates/bashkit/src/builtins/wc.rs +++ b/crates/bashkit/src/builtins/wc.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use super::{Builtin, Context, read_stream_file}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; /// The wc builtin - print newline, word, and byte counts. @@ -143,7 +144,7 @@ impl Builtin for Wc { let path = if file.starts_with('/') { std::path::PathBuf::from(file) } else { - ctx.cwd.join(file) + vfs_join(ctx.cwd, file) }; match read_stream_file(&*ctx.fs, &path, "wc").await { @@ -295,6 +296,41 @@ mod tests { Wc.execute(ctx).await.unwrap() } + // Issue #2425: a failed read reports the resolved VFS path, which must be + // `/`-separated even where the host separator is `\`. + #[tokio::test] + async fn windows_containment_wc_error_path_is_slash_separated() { + let fs = Arc::new(InMemoryFs::new()); + let mut variables = HashMap::new(); + let env = HashMap::new(); + let mut cwd = PathBuf::from("/d/proj"); + + let args = vec!["src/missing.rs".to_string()]; + let ctx = Context { + args: &args, + env: &env, + variables: &mut variables, + cwd: &mut cwd, + fs, + stdin: None, + #[cfg(feature = "http_client")] + http_client: None, + #[cfg(feature = "git")] + git_client: None, + #[cfg(feature = "ssh")] + ssh_client: None, + shell: None, + }; + + let result = Wc.execute(ctx).await.unwrap(); + assert_eq!(result.exit_code, 1); + assert!( + result.stderr.starts_with("wc: /d/proj/src/missing.rs:"), + "error path is not slash-separated:\n{}", + result.stderr + ); + } + #[tokio::test] async fn test_wc_all() { let result = run_wc(&[], Some("one two three\nfour five\n")).await; diff --git a/crates/bashkit/src/builtins/yq.rs b/crates/bashkit/src/builtins/yq.rs index 75c730bae..0d1a22edc 100644 --- a/crates/bashkit/src/builtins/yq.rs +++ b/crates/bashkit/src/builtins/yq.rs @@ -12,6 +12,7 @@ use serde::Deserialize; use super::{Builtin, Context, Jq, read_text_file, resolve_path}; use crate::StreamData; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; use crate::limits::ExecutionLimits; @@ -681,7 +682,7 @@ async fn atomic_replace( .iter() .map(|byte| format!("{byte:02x}")) .collect::(); - let candidate = parent.join(format!(".bashkit-yq-{suffix}.tmp")); + let candidate = vfs_join(parent, format!(".bashkit-yq-{suffix}.tmp")); if !fs .exists(&candidate) .await diff --git a/crates/bashkit/src/builtins/zip_cmd.rs b/crates/bashkit/src/builtins/zip_cmd.rs index 720ee5508..55056daeb 100644 --- a/crates/bashkit/src/builtins/zip_cmd.rs +++ b/crates/bashkit/src/builtins/zip_cmd.rs @@ -23,6 +23,7 @@ use std::path::{Component, Path, PathBuf}; use super::{Builtin, Context, resolve_path}; use crate::error::Result; +use crate::fs::vfs_join; use crate::interpreter::ExecResult; use crate::limits::BudgetedBytes; @@ -266,7 +267,7 @@ async fn collect_files_recursive( while let Some((current, current_prefix)) = dirs.pop() { if let Ok(entries) = fs.read_dir(¤t).await { for entry in entries { - let path = current.join(&entry.name); + let path = vfs_join(¤t, &entry.name); let entry_prefix = if current_prefix.is_empty() { entry.name.clone() } else { @@ -454,7 +455,7 @@ impl Builtin for Unzip { )); } }; - let target = extract_base.join(entry_path); + let target = vfs_join(&extract_base, entry_path); // Check if file exists and overwrite not set if !opts.overwrite diff --git a/crates/bashkit/src/fs/memory.rs b/crates/bashkit/src/fs/memory.rs index 83fc561ab..c7a4cd560 100644 --- a/crates/bashkit/src/fs/memory.rs +++ b/crates/bashkit/src/fs/memory.rs @@ -50,6 +50,7 @@ use super::limits::{FsLimits, FsUsage}; use super::traits::{DirEntry, FileSystem, FileSystemExt, FileType, Metadata}; use crate::error::Result; +use super::vfs_join; #[cfg(feature = "failpoints")] use fail::fail_point; @@ -1680,7 +1681,7 @@ impl FileSystem for InMemoryFs { .filter(|(path, _)| path.starts_with(&from)) .map(|(path, entry)| { let suffix = path.strip_prefix(&from).expect("prefix checked"); - (to.join(suffix), path.clone(), entry.clone()) + (vfs_join(&to, suffix), path.clone(), entry.clone()) }) .collect::>(); for (_, old_path, _) in &moved { @@ -1908,6 +1909,32 @@ impl FileSystemExt for InMemoryFs { mod tests { use super::*; + // Issue #2425: the rename fan-out writes the new entry-map keys itself, so + // a host separator there would leave the renamed children unreachable + // through the normalized paths every lookup uses. + #[tokio::test] + async fn windows_containment_rename_dir_keeps_children_reachable() { + let fs = InMemoryFs::new(); + fs.mkdir(Path::new("/src/nested"), true).await.unwrap(); + fs.write_file(Path::new("/src/nested/main.rs"), b"fn main() {}") + .await + .unwrap(); + + fs.rename(Path::new("/src"), Path::new("/dst")) + .await + .unwrap(); + + assert_eq!( + fs.read_file(Path::new("/dst/nested/main.rs")) + .await + .unwrap(), + b"fn main() {}" + ); + let entries = fs.read_dir(Path::new("/dst/nested")).await.unwrap(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].name, "main.rs"); + } + #[tokio::test] async fn test_write_and_read_file() { let fs = InMemoryFs::new(); diff --git a/crates/bashkit/src/fs/mod.rs b/crates/bashkit/src/fs/mod.rs index 0aa488efd..ed4eb235f 100644 --- a/crates/bashkit/src/fs/mod.rs +++ b/crates/bashkit/src/fs/mod.rs @@ -507,6 +507,38 @@ impl FileSystem for DisabledFs { } } +/// Join `child` onto the VFS path `base`, always using `/` as the separator. +/// +/// `PathBuf::join`/`push` insert the *host* separator (`\` on Windows) whenever +/// the buffer does not already end in one, so `/d/proj` + `src` becomes +/// `/d/proj\src` there. The VFS is Unix-style on every host (see +/// [`normalize_path`]), and builtins print these paths verbatim, so a host +/// separator leaks into command output and error messages (issue #2425). +/// Every VFS path built from a cwd, an operand, or a directory entry goes +/// through this helper instead of `Path::join`; host paths (`RealFs` under its +/// root, host mount mapping) keep using `Path::join`, where the host separator +/// is the correct one. +/// +/// Semantics match `Path::join` for VFS paths: an absolute `child` replaces +/// `base`, and no `.`/`..` resolution happens here (backends normalize on +/// entry). It differs only for an empty `child`, where the result is `base` +/// without a trailing separator. +pub fn vfs_join(base: &Path, child: impl AsRef) -> PathBuf { + let child = child.as_ref(); + // Non-UTF-8 pieces cannot be reassembled as a string; they never occur in + // the VFS (paths come from shell words and `DirEntry::name`, both `String`), + // so fall back rather than mangle them through a lossy conversion. + let (Some(base), Some(child)) = (base.to_str(), child.to_str()) else { + return base.join(child); + }; + match (base, child) { + ("", _) => PathBuf::from(child), + (_, "") => PathBuf::from(base), + (_, _) if child.starts_with('/') => PathBuf::from(child), + _ => PathBuf::from(format!("{}/{}", base.trim_end_matches('/'), child)), + } +} + /// Normalize a virtual filesystem path by resolving `.` and `..` components. /// /// Returns a canonical absolute path. If the result would be empty (e.g., from @@ -653,3 +685,63 @@ pub async fn verify_filesystem_requirements(fs: &dyn FileSystem) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod vfs_join_tests { + use super::*; + + /// The separator is `/` on every host, so these assertions are the same + /// ones the Windows CI job checks — there `Path::join` would produce + /// `/d/proj\src`. + #[test] + fn windows_containment_vfs_join_always_uses_forward_slashes() { + let base = Path::new("/d/proj"); + let src = vfs_join(base, "src"); + assert_eq!(src.to_string_lossy(), "/d/proj/src"); + // Nested joins are where `PathBuf::push` accumulates host separators. + let file = vfs_join(&src, "main.rs"); + assert_eq!(file.to_string_lossy(), "/d/proj/src/main.rs"); + assert!(!file.to_string_lossy().contains('\\')); + } + + #[test] + fn windows_containment_vfs_join_matches_path_join_semantics() { + // Absolute child replaces the base, like `Path::join`. + assert_eq!( + vfs_join(Path::new("/d/proj"), "/etc/hosts").to_string_lossy(), + "/etc/hosts" + ); + // A trailing separator on the base is not doubled. + assert_eq!( + vfs_join(Path::new("/d/proj/"), "src").to_string_lossy(), + "/d/proj/src" + ); + // Root is still a single separator. + assert_eq!(vfs_join(Path::new("/"), "etc").to_string_lossy(), "/etc"); + // Multi-segment children keep their own separators. + assert_eq!( + vfs_join(Path::new("/d/proj"), "src/main.rs").to_string_lossy(), + "/d/proj/src/main.rs" + ); + // `.`/`..` are left for the backends to normalize. + assert_eq!( + vfs_join(Path::new("/d/proj"), "../other").to_string_lossy(), + "/d/proj/../other" + ); + // Empty pieces: relative bases and empty children stay usable. + assert_eq!(vfs_join(Path::new(""), "src").to_string_lossy(), "src"); + assert_eq!( + vfs_join(Path::new("/d/proj"), "").to_string_lossy(), + "/d/proj" + ); + } + + #[test] + fn windows_containment_vfs_join_result_normalizes_like_a_backend() { + // What the backends do to the joined path must not change. + assert_eq!( + normalize_path(&vfs_join(Path::new("/d/proj"), "../other/./x")), + Path::new("/d/other/x") + ); + } +} diff --git a/crates/bashkit/src/fs/mountable.rs b/crates/bashkit/src/fs/mountable.rs index 703b2b380..a822f807e 100644 --- a/crates/bashkit/src/fs/mountable.rs +++ b/crates/bashkit/src/fs/mountable.rs @@ -16,6 +16,7 @@ use std::sync::{Arc, RwLock}; use super::limits::{FsLimits, FsUsage}; use super::traits::{DirEntry, FileSystem, FileSystemExt, FileType, Metadata}; +use super::vfs_join; use crate::error::Result; use std::io::ErrorKind; @@ -452,7 +453,7 @@ impl MountableFs { let resolved = if relative.as_os_str().is_empty() { PathBuf::from("/") } else { - PathBuf::from("/").join(relative) + vfs_join(Path::new("/"), relative) }; (Arc::clone(fs), resolved) diff --git a/crates/bashkit/src/fs/namespace.rs b/crates/bashkit/src/fs/namespace.rs index d6c7801a2..37f9414c8 100644 --- a/crates/bashkit/src/fs/namespace.rs +++ b/crates/bashkit/src/fs/namespace.rs @@ -10,6 +10,7 @@ use std::io::{Error as IoError, ErrorKind}; use std::path::{Component, Path, PathBuf}; use std::sync::Arc; +use super::vfs_join; use super::{ DirEntry, FileSystem, FileSystemExt, FileType, FsLimits, FsUsage, Metadata, ReadOnlyFs, normalize_path, @@ -227,7 +228,7 @@ impl NamespaceFs { let source_path = if relative.as_os_str().is_empty() { mount.source_root.clone() } else { - mount.source_root.join(relative) + vfs_join(&mount.source_root, relative) }; ResolvedPath { index, @@ -288,9 +289,9 @@ impl NamespaceFs { continue; }; children.insert(if path == Path::new("/") { - PathBuf::from("/").join(name) + vfs_join(Path::new("/"), name) } else { - path.join(name) + vfs_join(path, name) }); } children.into_iter().collect() diff --git a/crates/bashkit/src/fs/overlay.rs b/crates/bashkit/src/fs/overlay.rs index 1a66aadde..91422e0c7 100644 --- a/crates/bashkit/src/fs/overlay.rs +++ b/crates/bashkit/src/fs/overlay.rs @@ -41,6 +41,7 @@ use std::sync::{Arc, RwLock}; use super::limits::{FsLimits, FsUsage}; use super::memory::InMemoryFs; use super::traits::{DirEntry, FileSystem, FileSystemExt, FileType, Metadata}; +use super::vfs_join; use crate::error::Result; /// Copy-on-write overlay filesystem. @@ -332,7 +333,7 @@ impl OverlayFs { async fn hide_lower_children_recursive(&self, dir: &Path) { if let Ok(entries) = self.lower.read_dir(dir).await { for entry in entries { - let child = dir.join(&entry.name); + let child = vfs_join(dir, &entry.name); if self.is_whiteout(&child) { continue; } @@ -734,7 +735,7 @@ impl FileSystem for OverlayFs { if is_dir_lower && let Ok(lower_entries) = self.lower.read_dir(&path).await { for entry in lower_entries { // Skip whited out entries - let entry_path = path.join(&entry.name); + let entry_path = vfs_join(&path, &entry.name); if !self.is_whiteout(&entry_path) { entries.insert(entry.name.clone(), entry); } @@ -827,7 +828,10 @@ impl FileSystem for OverlayFs { FileType::Directory => { captured.push(CapturedEntry::Directory(relative.clone(), metadata.mode)); for entry in self.read_dir(&path).await? { - pending.push((path.join(&entry.name), relative.join(&entry.name))); + pending.push(( + vfs_join(&path, &entry.name), + vfs_join(&relative, &entry.name), + )); } } FileType::File => captured.push(CapturedEntry::File( @@ -867,22 +871,22 @@ impl FileSystem for OverlayFs { for entry in captured { match entry { CapturedEntry::Directory(path, mode) => { - let path = to.join(path); + let path = vfs_join(&to, path); self.mkdir(&path, true).await?; self.chmod(&path, mode).await?; } CapturedEntry::File(path, content, mode) => { - let path = to.join(path); + let path = vfs_join(&to, path); self.write_file(&path, &content).await?; self.chmod(&path, mode).await?; } CapturedEntry::Symlink(path, target, mode) => { - let path = to.join(path); + let path = vfs_join(&to, path); self.symlink(&target, &path).await?; self.chmod(&path, mode).await?; } CapturedEntry::Fifo(path, content, mode) => { - let path = to.join(path); + let path = vfs_join(&to, path); self.mkfifo(&path, mode).await?; self.append_file(&path, &content).await?; } diff --git a/crates/bashkit/src/interpreter/glob.rs b/crates/bashkit/src/interpreter/glob.rs index b5a77deff..62ba970ee 100644 --- a/crates/bashkit/src/interpreter/glob.rs +++ b/crates/bashkit/src/interpreter/glob.rs @@ -9,6 +9,7 @@ use std::path::{Path, PathBuf}; use crate::error::Result; use super::Interpreter; +use crate::fs::vfs_join; /// Expand a POSIX character class name into a list of characters. pub(super) fn expand_posix_class(name: &str, out: &mut Vec) { @@ -858,7 +859,7 @@ impl Interpreter { matched.sort(); for name in matched { let output = Self::glob_join_output(out, &name, is_absolute); - next.push((dir.join(&name), output)); + next.push((vfs_join(dir, &name), output)); } } } else { @@ -866,7 +867,7 @@ impl Interpreter { // that must not reach the filesystem or the expanded word. let literal = Self::glob_path_unescape(component); for (dir, out) in &candidates { - let path = crate::fs::normalize_path(&dir.join(&literal)); + let path = crate::fs::normalize_path(&vfs_join(dir, &literal)); // Intermediate literals are validated implicitly by the next // `read_dir`; only the final component needs an existence check. if is_last && !self.fs.exists(&path).await.unwrap_or(false) { @@ -941,7 +942,7 @@ impl Interpreter { continue; } if !entry.metadata.file_type.is_dir() { - matches.push(dir.join(&entry.name).to_string_lossy().to_string()); + matches.push(vfs_join(dir, &entry.name).to_string_lossy().to_string()); } } } @@ -955,7 +956,7 @@ impl Interpreter { continue; } if self.glob_match_impl(&entry.name, pat, nocase, 0) { - matches.push(dir.join(&entry.name).to_string_lossy().to_string()); + matches.push(vfs_join(dir, &entry.name).to_string_lossy().to_string()); } } } @@ -985,7 +986,7 @@ impl Interpreter { if entry.name.starts_with('.') && !dotglob { continue; } - let subdir = dir.join(&entry.name); + let subdir = vfs_join(dir, &entry.name); result.push(subdir.clone()); self.collect_dirs_recursive(&subdir, result, max_depth - 1, dotglob) .await; diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 458ccb80c..724e039e5 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -3681,7 +3681,7 @@ impl Interpreter { let joined = if path.is_absolute() { path.to_path_buf() } else { - self.cwd.join(path) + crate::fs::vfs_join(&self.cwd, path) }; crate::fs::normalize_path(&joined) }; @@ -4258,7 +4258,7 @@ impl Interpreter { let parent = path.parent().unwrap_or_else(|| Path::new("/")); let id = TIME_REPORT_COUNTER.fetch_add(1, Ordering::Relaxed); - let temp = parent.join(format!(".bashkit-time-{id}.tmp")); + let temp = crate::fs::vfs_join(parent, format!(".bashkit-time-{id}.tmp")); if let Err(error) = self.fs.write_file(&temp, report).await { let _ = self.fs.remove(&temp, false).await; return Err(error); @@ -8743,7 +8743,7 @@ impl Interpreter { let joined = if p.is_absolute() { p.to_path_buf() } else { - self.cwd.join(p) + crate::fs::vfs_join(&self.cwd, p) }; crate::fs::normalize_path(&joined) } diff --git a/crates/bashkit/src/lib.rs b/crates/bashkit/src/lib.rs index 20a8f7dff..7a1fafda3 100644 --- a/crates/bashkit/src/lib.rs +++ b/crates/bashkit/src/lib.rs @@ -500,7 +500,7 @@ pub use fs::{ InMemoryFs, LazyLoader, Metadata, MountableFs, NamespaceAccess, NamespaceFs, NamespaceFsBuilder, OverlayFs, PosixFs, ReadOnlyFs, SearchCapabilities, SearchCapable, SearchMatch, SearchProvider, SearchQuery, SearchResults, VfsEntry, VfsEntryKind, VfsSnapshot, - normalize_path, verify_filesystem_requirements, + normalize_path, verify_filesystem_requirements, vfs_join, }; #[cfg(feature = "realfs")] pub use fs::{RealFs, RealFsMode}; diff --git a/knowledge/foundations/vfs.md b/knowledge/foundations/vfs.md index a06c4d173..aa1a9fb39 100644 --- a/knowledge/foundations/vfs.md +++ b/knowledge/foundations/vfs.md @@ -176,6 +176,42 @@ All `FileSystem` implementations MUST enforce: 5. Cross-backend rename restores the destination on failure, or is rejected before mutation when that rollback contract cannot be supported +### Path Separators Are Always `/` + +The VFS is Unix-style on every host: `normalize_path` rebuilds paths from their +components with `/`, and every backend runs it on entry. Host path syntax +(drive-relative, drive-absolute, UNC, device) is discarded there +(THREAT[TM-ESC-033]). + +`std::path::PathBuf::join`/`push` break that rule on Windows: they append +`MAIN_SEPARATOR` (`\`) whenever the buffer does not already end in a separator, +so `/d/proj` + `src` becomes `/d/proj\src`. The result still *resolves* (the +backends normalize `\` away), but builtins print these paths verbatim, so +`grep -r` and `du` emitted mixed separators such as `/d/proj\src\main.rs`, and +the read-error path in `tail`/`rev`/`tac`/`shuf`/`sort`/`uniq`/`wc` did the same +(issue #2425). + +`InMemoryFs::rename` was worse than cosmetic: its directory fan-out wrote +`to.join(suffix)` straight into the entry map, so on Windows the renamed +children were keyed `\`-style and every normalized lookup afterwards missed +them. + +Decision: VFS paths are built with `crate::fs::vfs_join`, never `Path::join`. +It joins with `/` on every host, keeps `Path::join` semantics (an absolute +child replaces the base) and leaves `.`/`..` for the backends to normalize. +It is re-exported as `bashkit::vfs_join`, since custom `FileSystem` +implementations have the same obligation. Two guards keep it that way: +- `builtins::tests::no_host_separator_joins_on_vfs_paths` — static + scan of `crates/bashkit/src/` for the `cwd.join(` and `.join(&entry.name)` + shapes, with a `// host-join-ok: ` escape hatch. +- `windows_containment_*` unit tests next to `vfs_join`, `grep`, `du`, `wc` and + `InMemoryFs::rename`, which the Windows CI job runs (`cargo test -p bashkit + --lib --features realfs windows_containment`). + +Host paths are the exception and keep using `Path::join`: `RealFs` joining +under its host root, and `HostMounts` mapping a VFS path back to a host path. +There the host separator is the correct one. + ### Security Conformance Certification `tests/support/filesystem_security_conformance.rs` is the shared, private From 764d171d4c0222bd05cbf8b3b99c72711729c594 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Mon, 21 Sep 2026 18:15:46 +0000 Subject: [PATCH 2/3] fix(typescript): drop needless borrow in vfs_join call `cargo clippy --all-targets --all-features -- -D warnings` rejected `vfs_join(&cwd, p)`: `cwd` is already `&Path`. Claude-Session: https://claude.ai/code/session_01MtasrnXfdKmZqJEtP1Exby --- crates/bashkit/src/builtins/typescript.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bashkit/src/builtins/typescript.rs b/crates/bashkit/src/builtins/typescript.rs index e9c9dc975..05a63635a 100644 --- a/crates/bashkit/src/builtins/typescript.rs +++ b/crates/bashkit/src/builtins/typescript.rs @@ -999,7 +999,7 @@ fn extract_path(args: &[Value], cwd: &Path) -> Option { if p.is_absolute() { Some(p.to_owned()) } else { - Some(vfs_join(&cwd, p)) + Some(vfs_join(cwd, p)) } } _ => None, From a31a24e9441887d514041603d9b620514057fa9a Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Mon, 21 Sep 2026 18:40:57 +0000 Subject: [PATCH 3/3] fix(fs): close the rest of the host-vs-VFS path class Issue #2425 is one shape of a wider class: host path semantics applied to VFS paths. Two more shapes were still open. `PathBuf::push` has the same separator behavior as `join`, and two sites built VFS paths with it (the `realpath` symlink walk, the git pathspec cleaner). Both normalized downstream, so they were consistency gaps rather than live bugs; they now go through `vfs_join`. `Path::is_absolute()` is false on Windows for a VFS-absolute path like `/etc/passwd`, because it carries no drive prefix. Two security guards tested it, so neither fired there: the RealFs absolute-symlink-target guard and the tar path-traversal guard. Containment held in both cases via their later checks (canonicalize-and-contain, and `starts_with(extract_base)`), so this was latent fragility rather than an escape. Both now test `has_root()`, true for a `/`-rooted path on every host, and the RealFs guard gets a Windows test. Guards widened to match the class: - the static scan now covers every workspace crate's `src/`, not just this one (verified by planting a violation in bashkit-cli) - a Windows-only test pins the platform behavior these rules exist for: `Path::join` inserting `\`, `is_absolute()` being false for `/etc/passwd`, and `\` comparing equal to `/` Claude-Session: https://claude.ai/code/session_01MtasrnXfdKmZqJEtP1Exby --- crates/bashkit/src/builtins/archive.rs | 7 ++++++- crates/bashkit/src/builtins/mod.rs | 13 +++++++++++-- crates/bashkit/src/builtins/path.rs | 2 +- crates/bashkit/src/builtins/rg/mod.rs | 5 ++++- crates/bashkit/src/fs/mod.rs | 26 ++++++++++++++++++++++++++ crates/bashkit/src/fs/realfs.rs | 8 +++++++- crates/bashkit/tests/realfs_tests.rs | 26 ++++++++++++++++++++++++++ knowledge/foundations/vfs.md | 22 +++++++++++++++++++--- 8 files changed, 100 insertions(+), 9 deletions(-) diff --git a/crates/bashkit/src/builtins/archive.rs b/crates/bashkit/src/builtins/archive.rs index 1253b6ead..d6f5c5814 100644 --- a/crates/bashkit/src/builtins/archive.rs +++ b/crates/bashkit/src/builtins/archive.rs @@ -662,7 +662,12 @@ fn validate_tar_for_extraction( if !to_stdout { let entry = Path::new(name.as_ref()); - if entry.is_absolute() + // `has_root()` rather than `is_absolute()`: a `/etc/passwd` entry + // carries no drive prefix, so on Windows `is_absolute()` is false + // and this arm never fired (issue #2425's class). The + // `starts_with(extract_base)` arm below still blocked the write. + if entry.has_root() + || entry.is_absolute() || entry .components() .any(|component| matches!(component, std::path::Component::ParentDir)) diff --git a/crates/bashkit/src/builtins/mod.rs b/crates/bashkit/src/builtins/mod.rs index 50af392e4..6be483304 100644 --- a/crates/bashkit/src/builtins/mod.rs +++ b/crates/bashkit/src/builtins/mod.rs @@ -1486,7 +1486,11 @@ mod tests { // `Path::join` shapes seen on VFS paths: a cwd-relative operand and a // directory entry appended during a recursive walk. let shapes = ["cwd.join(", ".join(&entry.name)", ".join(&e.name)"]; - let manifest = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + // Every crate in the workspace, not just this one: the sibling crates + // (CLI, bindings, bench) build VFS paths too. + let workspace = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .expect("crates/ dir"); let mut violations = Vec::new(); fn walk( @@ -1538,7 +1542,12 @@ mod tests { } } - walk(&manifest.join("src"), manifest, &shapes, &mut violations); + for crate_dir in std::fs::read_dir(workspace).expect("read crates dir") { + let src = crate_dir.expect("crate entry").path().join("src"); + if src.is_dir() { + walk(&src, workspace, &shapes, &mut violations); + } + } assert!( violations.is_empty(), diff --git a/crates/bashkit/src/builtins/path.rs b/crates/bashkit/src/builtins/path.rs index 0381db6d6..ca2b816dc 100644 --- a/crates/bashkit/src/builtins/path.rs +++ b/crates/bashkit/src/builtins/path.rs @@ -484,7 +484,7 @@ async fn follow_readlink_symlinks( for remaining in components.iter().skip(idx + 1) { if let Component::Normal(part) = remaining { - next.push(part); + next = vfs_join(&next, part); } } diff --git a/crates/bashkit/src/builtins/rg/mod.rs b/crates/bashkit/src/builtins/rg/mod.rs index 4e06bc62d..b3c0574c5 100644 --- a/crates/bashkit/src/builtins/rg/mod.rs +++ b/crates/bashkit/src/builtins/rg/mod.rs @@ -3587,7 +3587,10 @@ async fn resolve_rg_symlink_target( let resolved = if target.is_absolute() { crate::fs::normalize_path(&target) } else { - crate::fs::normalize_path(&link_path.parent().unwrap_or(Path::new("/")).join(target)) + crate::fs::normalize_path(&vfs_join( + link_path.parent().unwrap_or(Path::new("/")), + target, + )) }; // rg may emulate ripgrep's -L behavior only inside the requested VFS search root; // rejecting escapes preserves TM-ESC-002's inert-symlink sandbox boundary. diff --git a/crates/bashkit/src/fs/mod.rs b/crates/bashkit/src/fs/mod.rs index ed4eb235f..8d23745e6 100644 --- a/crates/bashkit/src/fs/mod.rs +++ b/crates/bashkit/src/fs/mod.rs @@ -578,6 +578,32 @@ pub fn normalize_path(path: &Path) -> PathBuf { mod windows_containment_tests { use super::*; + /// Pins the host behavior the VFS rules exist for, so the reasoning behind + /// [`vfs_join`] and the `has_root()` guards is checked on Windows rather + /// than asserted in a comment. If std ever changed these, this test says so + /// before the rules start looking arbitrary. + #[test] + fn windows_containment_host_path_hazards_are_still_real() { + // Why `vfs_join` exists: `Path::join` inserts the host separator. + assert_eq!( + Path::new("/d/proj").join("src").to_string_lossy(), + r"/d/proj\src" + ); + assert_eq!( + vfs_join(Path::new("/d/proj"), "src").to_string_lossy(), + "/d/proj/src" + ); + + // Why absolute-target guards test `has_root()`: a VFS-absolute path + // carries no drive prefix, so `is_absolute()` is false here. + assert!(!Path::new("/etc/passwd").is_absolute()); + assert!(Path::new("/etc/passwd").has_root()); + + // Why a mixed-separator path still resolves: both are separators here, + // which is what keeps the leak cosmetic rather than functional. + assert_eq!(Path::new(r"/d/proj\src"), Path::new("/d/proj/src")); + } + #[test] fn windows_containment_normalizes_host_path_syntax_into_vfs_root() { assert_eq!( diff --git a/crates/bashkit/src/fs/realfs.rs b/crates/bashkit/src/fs/realfs.rs index fed6b6d8b..1a0a2d39e 100644 --- a/crates/bashkit/src/fs/realfs.rs +++ b/crates/bashkit/src/fs/realfs.rs @@ -641,7 +641,13 @@ impl FsBackend for RealFs { // Absolute targets always escape the mount root on disk // THREAT[TM-ESC-033]: `C:target` is drive-relative, not absolute, but // still carries host namespace semantics and cannot be a VFS target. - if target.is_absolute() + // `has_root()` rather than `is_absolute()`: a VFS-absolute target like + // `/etc/passwd` has no drive prefix, so on Windows `is_absolute()` is + // false and this guard never fired there (issue #2425's class). The + // canonicalize-and-contain check below still blocked it, but only as a + // backstop — reject it here, where the error names the real reason. + if target.has_root() + || target.is_absolute() || target .components() .any(|component| matches!(component, std::path::Component::Prefix(_))) diff --git a/crates/bashkit/tests/realfs_tests.rs b/crates/bashkit/tests/realfs_tests.rs index 0ab2303fc..b8c848d09 100644 --- a/crates/bashkit/tests/realfs_tests.rs +++ b/crates/bashkit/tests/realfs_tests.rs @@ -98,6 +98,32 @@ mod windows_containment { ); } + /// Issue #2425's class: a VFS-absolute target has no drive prefix, so + /// `is_absolute()` is false on Windows and the guard that rejects absolute + /// symlink targets did not fire. Containment held via the + /// canonicalize-and-contain backstop; the guard now rejects it directly. + #[tokio::test] + async fn windows_containment_realfs_rejects_vfs_absolute_symlink_target() { + let root = tempfile::tempdir().unwrap(); + let fs = RealFs::open(root.path(), RealFsMode::ReadWrite) + .await + .unwrap(); + + let error = fs + .symlink( + Path::new("/Windows/System32/drivers/etc/hosts"), + Path::new("/link"), + ) + .await + .expect_err("VFS-absolute symlink target must be rejected"); + assert!( + error + .to_string() + .contains("absolute or drive-relative target"), + "expected the absolute-target guard to fire, got: {error}" + ); + } + #[tokio::test] async fn windows_containment_realfs_rejects_drive_relative_symlink_target() { let root = tempfile::tempdir().unwrap(); diff --git a/knowledge/foundations/vfs.md b/knowledge/foundations/vfs.md index aa1a9fb39..a0c81acd9 100644 --- a/knowledge/foundations/vfs.md +++ b/knowledge/foundations/vfs.md @@ -196,14 +196,30 @@ the read-error path in `tail`/`rev`/`tac`/`shuf`/`sort`/`uniq`/`wc` did the same children were keyed `\`-style and every normalized lookup afterwards missed them. -Decision: VFS paths are built with `crate::fs::vfs_join`, never `Path::join`. +The same host/VFS mismatch has a second shape: `Path::is_absolute()` is false +on Windows for a VFS-absolute path like `/etc/passwd`, because there is no +drive prefix. Two security guards tested it — the RealFs absolute-symlink-target +guard and the tar extraction traversal guard — so neither fired on Windows. +Containment held in both cases via their later checks (canonicalize-and-contain +for RealFs, `starts_with(extract_base)` for tar), so this was latent fragility +rather than an escape, but the guards now test `has_root()`, which is true for a +`/`-rooted path on every host. + +Decision: VFS paths are built with `crate::fs::vfs_join`, never `Path::join` +(or `PathBuf::push`), and a guard that asks "is this path absolute?" about a VFS +path asks `has_root()`, never `is_absolute()`. It joins with `/` on every host, keeps `Path::join` semantics (an absolute child replaces the base) and leaves `.`/`..` for the backends to normalize. It is re-exported as `bashkit::vfs_join`, since custom `FileSystem` implementations have the same obligation. Two guards keep it that way: -- `builtins::tests::no_host_separator_joins_on_vfs_paths` — static - scan of `crates/bashkit/src/` for the `cwd.join(` and `.join(&entry.name)` +- `builtins::tests::no_host_separator_joins_on_vfs_paths` — static scan of + every workspace crate's `src/` for the `cwd.join(` and `.join(&entry.name)` shapes, with a `// host-join-ok: ` escape hatch. +- `windows_containment_host_path_hazards_are_still_real` — a Windows-only test + pinning the platform behavior these rules exist for (`Path::join` inserting + `\`, `is_absolute()` being false for `/etc/passwd`, `\` and `/` comparing + equal). If std or the platform ever changed, it says so before the rules + start looking arbitrary. - `windows_containment_*` unit tests next to `vfs_join`, `grep`, `du`, `wc` and `InMemoryFs::rename`, which the Windows CI job runs (`cargo test -p bashkit --lib --features realfs windows_containment`).