Repository navigation
fix(toolset): reject cmd.exe metacharacters in a Windows tool path - #11947
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWindows path validation now rejects shell metacharacters, including Windows ChangesWindows tool path validation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This change rejects Windows shell metacharacters in tool paths while preserving existing version behavior; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5a64e1c to
a718f60
Compare
Greptile SummaryThe PR strengthens Windows
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix(toolset): reject cmd.exe metacharact..." | Re-trigger Greptile |
`path:` values are checked against a denylist because the resolved path becomes `ctx.rootPath` for path-mode tools, which vfox plugin hooks build shell commands with. That list was added by jdx#9814 for a POSIX shell, so it covers `$`, backtick, quotes and `\`. On Windows the shell those hooks run through is cmd.exe, whose metacharacters are a different set and passed straight through: `&`, `|`, `<`, `>`, `^`, and `%`. `%` is the one that matters most. cmd expands `%NAME%` even inside double quotes, so a hook that quotes its interpolation correctly still cannot contain it -- the path a plugin operates on ends up being an environment variable's value rather than what was written. Measured: a `path:` value containing `%USERPROFILE%` resolves to the expanded location. Rejected on Windows only, and only for `path:`. Versions keep the POSIX-only list, since `^1.2.3` is a real npm-style range that must not become a hard error. `resolve_path` never expanded `%VAR%` in a path, so nothing that worked before is rejected now. `path:` had no e2e coverage on Windows before this.
a718f60 to
6477350
Compare
The denylist that
path:values are checked against is written for a POSIX shell. On Windows theshell is
cmd.exe, and its metacharacters pass straight through.Background
#9814 added
validate_path_stringbecause apath:valueresolves into
ctx.rootPathfor path-mode tools, which vfox plugin hooks interpolate into shellcommands. It rejects
$, backtick, quotes and\— the characters that break a POSIX shell.Those are not cmd's metacharacters. Measured on 2026.8.2 windows-x64 — every one of these is
accepted today:
%is the one that matters most. cmd expands%NAME%even inside double quotes, so a hookthat quotes its interpolation correctly still cannot contain it. The effect is that the path a
plugin ends up operating on is an environment variable's value rather than the string that was
written — I confirmed the expansion takes effect when a path-mode tool is set up, so this is not
theoretical. The others (
& | < > ^) are cmd's command-separator / redirect / escape set, thesame class
$is on the POSIX side.The change
On Windows,
path:also rejects& | < > ^ %. This is #9814's rejection, mirrored onto theplatform where cmd is the shell — not a new kind of check.
Scoped deliberately:
#[cfg(windows)]; nothing changes off Windows.path:only, not versions.^1.2.3is a real npm-style range, so^must not become a harderror in a version. The version denylist is untouched; only the new
path:-specificis_forbidden_path_charadds the cmd set.Nothing that worked before is rejected now
%—resolve_pathonly ever expanded~/, never%VAR%, so a%in apath:was alreadynon-functional. Rejecting it removes nothing that worked.
< > |— already invalid in a Windows filename, so a path containing them failed downstream indirectory creation with a bare OS error. Rejecting at validation just turns that into a clear
message.
& ^— legal in a Windows path but very rare; rejecting is the hardening.( )are deliberately not on the listThey are cmd metacharacters in the sense that a
)can close a( … )block early, but they arenot an injection vector, which is the bar the rest of this list meets. Measured, with
&as thecontrol:
The breakage case is real but needs a hook that both wraps the interpolation in a
( … )blockand leaves the path unquoted; double-quoting contains it, and the bundled hooks that build a
Windows command line double-quote. Against that,
C:\Program Files (x86)\…is one of the twostandard program directories on 64-bit Windows and exactly the kind of path
path:exists topoint at — rejecting it would be a certain, common breakage traded for a hypothetical one.
Tests
path:had no Windows coverage before this.#[cfg(windows)]: each of&,%,^,|,<,>in a path is rejected, and anordinary
C:/Program Files/toolstill passes. All-platform: a version keeps accepting^1.2.3and
1.0&x(the cmd set is path-only) while the POSIX$(id)stays rejected — the control thatthis widened the path list without touching versions. The module is not
#[cfg(unix)], so CIruns the Windows cases.
tool_path_metacharacters.Tests.ps1:%,&,^,|paths makemise ls --currentfail withinvalid tool path, plus an ordinary path as the control.docs/configuration.md'spath:entry gains a sentence. It is hand-written, so nothing isregenerated.
Not run
cargo fmt --alland an isolatedrustcprobe of the denylist; no full local build. Themeasurements above are of a released binary. CI, and
windows-e2ein particular, is what exercisesthe change.
Overlap with #11937
Resolved — #11937 has merged and this branch is rebased onto it. The two changes compose as
expected: separators are rewritten first, then the value is validated against the POSIX+cmd set.
Merging them by hand kept both sides of three conflicts rather than picking one, since the TOML
quoting note and the cmd metacharacter note are about different things, and both files' tests are
still present.
Originally written as:
#11937 also edits
validate_path_string— it rewrites\to/before validation so native Windows paths are accepted. This adds rejections insidevalidation. They compose in that order (rewrite separators, then validate against the POSIX+cmd
set) and the hunks are adjacent but distinct; whichever lands second should rebase. The two
Windows path e2e files have different names on purpose.
Summary by CodeRabbit
Bug Fixes
%,&,^, or|.Documentation