Repository navigation
fix(toolset): accept a Windows tool path spelled with backslashes - #11937
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesTool-path normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change normalizes Windows tool paths before validation while preserving Unix behavior and adds focused tests and documentation. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ToolPathRequest
participant PathNormalizer
participant PathValidator
participant PathResolver
ToolPathRequest->>PathNormalizer: Rewrite Windows backslashes
PathNormalizer->>PathValidator: Validate normalized path
PathValidator->>PathResolver: Resolve accepted path
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration.md`:
- Around line 395-396: Update the Homebrew/Windows path documentation to explain
TOML quoting for native Windows paths: recommend forward slashes or show a
literal-string mise.toml example using single quotes so backslashes are not
interpreted as escapes.
In `@e2e-win/tool_path.Tests.ps1`:
- Around line 17-24: Update the suite’s BeforeAll setup to save the original
process-scoped MISE_TRUSTED_CONFIG_PATHS value, then modify AfterAll to restore
that saved value when present and remove the environment variable only when the
saved value is $null, while preserving the existing test-directory cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: cea4eebf-424d-407f-9050-ef4a6837dbd8
📒 Files selected for processing (4)
docs/configuration.mde2e-win/tool_path.Tests.ps1e2e/config/test_tool_path_validationsrc/toolset/tool_request.rs
fcabc97 to
e9a4375
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e-win/tool_path.Tests.ps1 (1)
81-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the protected Windows prefixes.
Lines 81-93 verify an ordinary forbidden character but do not exercise
\\?\and\\.\. Add TOML literal-string cases for both prefixes and assert thatmise ls --currentfails.windows_path_separatorshas a dedicated no-rewrite branch for these prefixes. This suite will not detect regressions that normalize them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-win/tool_path.Tests.ps1` around lines 81 - 93, Add TOML literal-string test cases in the `tool_path.Tests.ps1` path-validation tests for both protected Windows prefixes, `\\?\` and `\\.\`, using `mise ls --current` and asserting a nonzero exit status. Ensure the cases exercise the `windows_path_separators` no-rewrite branch and preserve the existing invalid-path coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e-win/tool_path.Tests.ps1`:
- Around line 81-93: Add TOML literal-string test cases in the
`tool_path.Tests.ps1` path-validation tests for both protected Windows prefixes,
`\\?\` and `\\.\`, using `mise ls --current` and asserting a nonzero exit
status. Ensure the cases exercise the `windows_path_separators` no-rewrite
branch and preserve the existing invalid-path coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c8d1e0f1-9584-4e92-9576-85f14f49c545
📒 Files selected for processing (2)
docs/configuration.mde2e-win/tool_path.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration.md
e9a4375 to
9b9b350
Compare
Added in assert_eq!(super::windows_path_separators(input), input, "{input:?} should not be rewritten");
assert!(accept_tool_path(input).is_err(), "{input:?}");— and lives in a module that is not The e2e cases are still worth having, for something the unit test cannot see: they go through the whole chain — Both assert the message rather than only the failure: $out | Should -BeLike '*extended-length and device paths*'That is the wording only the no-rewrite branch produces. If something started rewriting these, they would fall through to the generic Written as two plain |
9b9b350 to
1e9d7e2
Compare
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
1e9d7e2 to
6708f23
Compare
`tool.<name>.path` rejected every native Windows path:
invalid tool path "C:\Users\me\tools\bin": contains forbidden character '\'
so anything copied out of Explorer or printed by `pwd` was unusable, and only
the forward-slash spelling worked. The existing tests put "C:/Users/foo" on the
accepted side; `C:\Users\foo` was on neither, and the error does not hint that
slashes get through.
The list `path:` values are checked against was added by jdx#9814 to keep shell
syntax out of `ctx.rootPath`, which vfox plugin hooks build commands with. It
is written for a POSIX shell, where `\` is an escape. On Windows it is the path
separator.
Rather than drop `\` from the list there -- which would weaken what jdx#9814
established -- the separator is rewritten to `/` before the check. Win32 accepts
`/` wherever it accepts `\`, so the meaning is unchanged, and no backslash
reaches a plugin hook. Unix is untouched: `\` is a legal filename character
there and really is a shell escape, so it stays rejected.
Extended-length and device prefixes (`\?\`, `\.\`) are left alone, since those
are the one place Windows does not accept `/`; rewriting them would produce a
path that looks right and does not resolve. They keep being rejected, with a
message that now says why.
`path:` had no e2e coverage on either platform before this.
Closes jdx#11192
6708f23 to
07288fd
Compare
Greptile SummaryThe PR normalizes ordinary Windows backslashes before validating and resolving
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure identified. The normalization is Windows-only, runs before the existing security validation, preserves explicitly unsupported namespace prefixes for rejection, and is covered by platform-specific unit and end-to-end tests. Important Files Changed
Reviews (1): Last reviewed commit: "fix(toolset): accept a Windows tool path..." | Re-trigger Greptile |
Closes #11192.
tool.<name>.pathrejects every path spelled the way Windows spells it.The problem
Measured on 2026.8.2 windows-x64, same directory each time:
So the only spelling that works is the one nothing on Windows produces — Explorer,
pwd, andtab-completion all give backslashes. The error says a character is forbidden, not that slashes get
through, so there is nothing to act on.
The existing unit test puts
"C:/Users/foo"on the accepted side.C:\Users\foois on neitherside: the standard Windows spelling was never decided, it just fell through to the reject arm.
Correcting the analysis on the thread
There is a comment on #11192 saying validation runs after
~expansion, so anypath:using~is unusable. That is not true of current main —
validate_path_stringruns on the raw string(
tool_request.rs:110) andresolve_pathexpands afterwards, which is why the third line abovepasses. The reporter was on 2026.7.11; the order changed since.
Why the character is on the list
#9814 — "security: reject shell metacharacters in version
strings and CI inputs" — added it. A
path:value resolves intoctx.rootPathfor path-modetools, and vfox plugin hooks interpolate that into shell commands, so the fix killed that RCE class
at the Rust boundary rather than in each Lua plugin.
\is on the list because it is an escape ina POSIX shell.
That reasoning holds. It just does not describe Windows, where
\is the path separator.The change
Rewrite
\to/before validating, on Windows only. Not "allow\there".Win32 accepts
/wherever it accepts\, so the meaning of the path is unchanged — and nobackslash reaches
ctx.rootPath, so the property #9814 established is kept exactly rather thanweakened. Dropping
\from the list on Windows would have been the smaller diff and the worseanswer.
Unix is untouched: the function is a borrow-through no-op there, because
\is a legal characterin a filename — rewriting would change which file is meant — and it really is a shell escape.
resolve_pathneeded nothing:~/matching andis_absolute()both work on the rewritten form(
C:/Users/foois absolute).One code path covers both spellings of the config:
uv = { path = "…" }is folded into the stringpath:…inmise_toml.rsbefore it reachesToolRequest::new.\\?\and\\.\are deliberately not rewrittenExtended-length and device prefixes are the one place Windows does not accept
/, so rewritingwould hand back a path that looks right and does not resolve — silently. They are left alone, which
means they keep being rejected exactly as they are today; nothing regresses and nothing new
silently breaks. The error for them now says what is actually wrong instead of naming a character
the user cannot avoid.
I have not verified against a real
\\?\path that forward slashes break it — that is fromMicrosoft's documentation. The design does not depend on the claim: not rewriting is the
conservative branch either way.
Not widened here
The list is POSIX-shaped in the other direction too:
&and%are cmd's real metacharacters andpath:accepts both today. Flagging it rather than fixing it — that is a separate question aboutwhat the list should be on Windows, and this PR is about a spelling that has no alternative.
Tests
path:had no e2e coverage at all, on either platform. There were only unit tests for thevalidator.
path:arm runs it. Windows: thenative spellings are accepted and come back reduced (
C:\Users\foo\bin→C:/Users/foo/bin,and
C:\Program Files\tool, and~\.local\bin);\\?\C:\…and\\.\COM1are unchanged andstill rejected. Unix:
/tmp/\rmis unchanged and still rejected — the control for "the rewritedid not leak off Windows". Every platform: quotes, backtick and
$still rejected, which is thecontrol for "the rewrite did not widen anything but the separator". These live in the file's
existing test module, which is not
#[cfg(unix)], so CI runs them on Windows too.tool_path.Tests.ps1. Writes a nativeC:\…path intomise.tomlandasserts
mise ls --currentsucceeds and reports the rewritten path, so it checks theresolved value rather than only that the command survived. Both TOML spellings that deliver a
backslash — a literal string and a doubled basic string — plus the forward-slash spelling, which
has to keep working, and a quote in the path, which has to keep failing.
\\?\C:\Users\fooand\\.\COM1assert the message, not just the failure: only the no-rewrite branch producesextended-length and device paths …, so a change that started normalising them would fallthrough to the generic
contains forbidden characterarm — still a failure and still non-zero,which a test checking only those two things would accept.
config/test_tool_path_validation:\and$(id)still rejected, with anordinary path as the control so the two rejections are about the character rather than about
path:being broken in the sandbox.The TOML quoting matters, and review caught that I had skipped it
docs/configuration.md'spath:entry showed only a Homebrew example, so it gains the Windowscase. The first version of that sentence said
path:C:\tools\nodeworks without saying which TOMLquoting actually delivers that string, which is a trap. Measured:
The first is the bad one: not an error. TOML reads
\tas a tab, the value silently becomessomething else, and what the user sees is a complaint about a semver range with no mention of a
path. The docs now name the two spellings that work and say what the third does. The e2e-win suite
covers both of them rather than only the literal-string form the test happened to use.
docs/configuration.mdis hand-written, so nothing is regenerated.Not touched: the
semver range …wording that the mangled string produces. mise is reportingaccurately on a string TOML already broke; how unparseable tool versions are reported is a separate
question.
Not run
cargo fmt --allonly; no local build. The three measurements at the top are of a released binaryreproducing the bug, not of the fix — CI is what exercises it, and
windows-e2eis the job thatmatters.
Summary by CodeRabbit
Bug Fixes
path:tool requests now support backslash and forward-slash separators with consistent normalization.Documentation
Tests