fix(runtimes): make path guards and binary lookup correct on Windows#503
Conversation
Two of the guards used Path::is_absolute(), which on Windows requires a
drive prefix AND a root — so unix-absolute paths slipped through:
- validate_runtime_name accepted "/etc" (and drive-prefixed names like
"C:evil", which Path::join lets replace the runtimes root entirely).
Reject on has_root() or any Prefix component instead.
- extract_tar_gz's tar-slip guard accepted entries like "/tmp/evil" on
Windows. Replace the ParentDir+is_absolute check with a single
components guard: any component that is not Normal/CurDir (ParentDir,
RootDir, or a drive Prefix) rejects the entry. "./"-prefixed entries
stay accepted.
Also fix the ensure_reports_needs_download_again_after_remove fixture to
write binary_file_name("bun") — the lookup expects bun.exe on Windows.
These three tests were among the 38 Windows failures that became visible
once Xoshbin#499 made cargo test loadable on Windows (see Xoshbin#498). New regression
tests cover the drive-prefixed/rootful forms on Windows.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH
There was a problem hiding this comment.
Code Review
This pull request enhances security and path traversal guards across platforms, particularly addressing Windows-specific path behaviors. It updates the tar-slip guard in extract_tar_gz to reject any non-normal or non-current directory components, and improves validate_runtime_name to reject absolute, rootful, or drive-prefixed paths. The review feedback correctly identifies a critical vulnerability where . is accepted as a valid runtime name, which could lead to the accidental deletion of the entire runtimes directory, and suggests restricting the validation to a single normal component along with adding corresponding test coverage.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Gemini review caught a pre-existing hole the has_root/Prefix check kept: "." passes validation, and remove_from_root(root, ".") resolves to the runtimes root itself — remove_dir_all would delete every installed runtime. Require exactly one Component::Normal instead, which rejects ".", "./x", separator-containing names, rootful and drive-prefixed paths in one predicate. New regression test covers "." and multi-component names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH
…th separators and directory aliases
First slice of fixing the 38 Windows test failures that became visible once #499 made
cargo testloadable on Windows (context in #498). Starting here because two of the three are product bugs in security guards, not test issues.The bugs
Path::is_absolute()on Windows requires a drive prefix and a root —Path::new("/etc").is_absolute()isfalse. Two guards relied on it:validate_runtime_nameaccepted"/etc"on Windows. It also accepted drive-prefixed names in both forms — andPathBuf::joinreplaces the entire base path when joining anything with a drive prefix, so a hypothetical name likeC:evilwouldn't land under the runtimes root at all.extract_tar_gz's tar-slip guard accepted entries like/tmp/evilon Windows (the..half of the check was fine). Exploitability was already limited becauseunpack_instrips a leading/itself, but the explicit up-front guard exists to be stricter than that fallback — and on Windows it wasn't.The fix
validate_runtime_name: reject onhas_root()(true for/etcand\etcon Windows, and for/etcon Unix) or anyComponent::Prefix(catchesC:\eviland drive-relativeC:evil).extract_tar_gz: theParentDir+is_absolutepair becomes one stricter predicate — any component that is notNormal/CurDirrejects the entry (covers.., rootful, and drive-prefixed in one)../-prefixed entries, common in tarballs, stay accepted.Test changes
ensure_reports_needs_download_again_after_removefixture now writesbinary_file_name("bun")instead of a literal"bun"— the lookup expectsbun.exeon Windows (third of the three failures).#[cfg(windows)]regression tests:validate_runtime_namerejectingC:\evil/C:evil/\etc, andextract_tar_gzrejecting a drive-prefixed entry.Verification
cargo test runtimesgreen, including the previously-failing three and the new Windows-only tests.cargo test— 2856 passed; the one failure is the known WSL environment issue (fs_watcherundermax_user_instances=128, reproduces on cleanmain).cargo clippy --all-targets -- -D warningsandcargo fmtclean.🤖 Generated with Claude Code