From 07288fd576cae2b8a37ca3885e9464faddbd3470 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Thu, 13 Aug 2026 10:00:18 +0900 Subject: [PATCH] fix(toolset): accept a Windows tool path spelled with backslashes `tool..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 #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 #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 #11192 --- docs/configuration.md | 6 +- e2e-win/tool_path.Tests.ps1 | 120 +++++++++++++++++++++++++++ e2e/config/test_tool_path_validation | 30 +++++++ src/toolset/tool_request.rs | 120 ++++++++++++++++++++++++++- 4 files changed, 273 insertions(+), 3 deletions(-) create mode 100644 e2e-win/tool_path.Tests.ps1 create mode 100644 e2e/config/test_tool_path_validation diff --git a/docs/configuration.md b/docs/configuration.md index 5b9adc73de5..2b8fcd71cc2 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -418,7 +418,11 @@ Both `mise.toml` and `.tool-versions` support "scopes" which modify the behavior - `prefix:` - use the latest version that matches the prefix. Useful for Go since `1.20` would only match `1.20` exactly but `prefix:1.20` will match `1.20.1` and `1.20.2` etc. - `path:` - use a custom compiled version at the given path. One use-case is to re-use - Homebrew tools (e.g.: `path:/opt/homebrew/opt/node@20`). + Homebrew tools (e.g.: `path:/opt/homebrew/opt/node@20`). On Windows both separators work and + mise stores the forward-slash form either way, but mind the TOML quoting: a backslash is an + escape inside a _basic_ (double-quoted) string, so write `{ path = 'C:\tools\node' }` as a + literal string, or double them as `"C:\\tools\\node"`. `"C:\tools\node"` is not rejected — TOML + reads `\t` as a tab — so the path silently becomes something else. - `sub-:` - resolves `ORIG_VERSION`, subtracts the numeric components in `PARTIAL_VERSION` from the corresponding resolved version components, then resolves the result as a version prefix. For example, `sub-2:lts` resolves `lts` and subtracts 2 from its major diff --git a/e2e-win/tool_path.Tests.ps1 b/e2e-win/tool_path.Tests.ps1 new file mode 100644 index 00000000000..3afd4a515e0 --- /dev/null +++ b/e2e-win/tool_path.Tests.ps1 @@ -0,0 +1,120 @@ +Describe 'tool-path' { + BeforeAll { + $script:OriginalDir = Get-Location + $script:TestRoot = Join-Path $TestDrive ([System.Guid]::NewGuid().ToString()) + New-Item -ItemType Directory -Path $script:TestRoot | Out-Null + + # A directory to point `path:` at. Nothing is installed into it -- `path:` means "the + # tool is already here", so parsing and resolving the value is the whole of it, and + # parsing is where this used to fail. + $script:ToolDir = Join-Path $script:TestRoot "tools\bin" + New-Item -ItemType Directory -Path $script:ToolDir | Out-Null + + # `mise ls` reports the resolved path with forward slashes, so this is what the native + # spelling below has to come back as. + $script:Forward = $script:ToolDir -replace '\\', '/' + + # Saved rather than just cleared afterwards: Pester runs every suite in one process, so + # removing an inherited value would leave the next file without it. + $script:OriginalTrusted = [Environment]::GetEnvironmentVariable('MISE_TRUSTED_CONFIG_PATHS', 'Process') + $env:MISE_TRUSTED_CONFIG_PATHS = $script:TestRoot + Set-Location $script:TestRoot + } + + AfterAll { + Set-Location $script:OriginalDir + Remove-Item -Path $script:TestRoot -Recurse -Force -ErrorAction Ignore + if ($null -eq $script:OriginalTrusted) { + Remove-Item Env:MISE_TRUSTED_CONFIG_PATHS -ErrorAction Ignore + } else { + [Environment]::SetEnvironmentVariable('MISE_TRUSTED_CONFIG_PATHS', $script:OriginalTrusted, 'Process') + } + } + + It 'accepts a tool path spelled the way Windows spells it' { + # Backslashes are what Explorer shows and what `pwd` prints, so this is the spelling a + # Windows user actually has. It used to be rejected outright: + # invalid tool path "C:\\...": contains forbidden character '\' + # + # Written as a TOML literal string, which is what takes backslashes as-is. In a basic + # string they are escapes -- see the doubled form below. + @" +[tools] +node = { path = '$($script:ToolDir)' } +"@ | Out-File (Join-Path $script:TestRoot "mise.toml") + + $out = mise ls --current 2>&1 | Out-String + $LASTEXITCODE | Should -Be 0 + # Asserting the resolved value, not just that the command survived: the separators are + # rewritten rather than allowed through, and this is where that shows. + $out | Should -BeLike "*path:$($script:Forward)*" + } + + It 'accepts the same path written as a TOML basic string' { + # The other spelling that actually delivers backslashes to mise, and the one the docs + # offer as an alternative: doubled inside a double-quoted string. `"C:\tools\node"` is a + # third thing entirely -- TOML reads `\t` as a tab and the path silently becomes + # something else -- which is why the docs say which two to use. + $doubled = $script:ToolDir -replace '\\', '\\' + @" +[tools] +node = { path = "$doubled" } +"@ | Out-File (Join-Path $script:TestRoot "mise.toml") + + $out = mise ls --current 2>&1 | Out-String + $LASTEXITCODE | Should -Be 0 + $out | Should -BeLike "*path:$($script:Forward)*" + } + + It 'accepts the same path written with forward slashes' { + # The workaround people were given while the above was broken. It has to keep working. + @" +[tools] +node = { path = '$($script:Forward)' } +"@ | Out-File (Join-Path $script:TestRoot "mise.toml") + + $out = mise ls --current 2>&1 | Out-String + $LASTEXITCODE | Should -Be 0 + $out | Should -BeLike "*path:$($script:Forward)*" + } + + It 'still rejects a path containing a quote' { + # The control for the two above. Only the separator is rewritten; the rest of the list + # that `path:` values are checked against is untouched, and a quote is on it because the + # resolved path reaches plugin hooks that build shell commands with it. + @" +[tools] +node = { path = 'C:/a"b' } +"@ | Out-File (Join-Path $script:TestRoot "mise.toml") + + $out = mise ls --current 2>&1 | Out-String + $LASTEXITCODE | Should -Not -Be 0 + $out | Should -BeLike '*invalid tool path*' + } + + # `\\?\` and `\\.\` are the one place Windows does not accept `/`, so they are the branch that + # is deliberately *not* rewritten. Both assert the message rather than only the failure: it is + # the one that branch produces, so a change that started rewriting them would surface here as + # the generic "forbidden character" wording instead of as a passing test. + It 'still rejects an extended-length path, and says why' { + @" +[tools] +node = { path = '\\?\C:\Users\foo' } +"@ | Out-File (Join-Path $script:TestRoot "mise.toml") + + $out = mise ls --current 2>&1 | Out-String + $LASTEXITCODE | Should -Not -Be 0 + $out | Should -BeLike '*extended-length and device paths*' + } + + It 'still rejects a device path, and says why' { + @" +[tools] +node = { path = '\\.\COM1' } +"@ | Out-File (Join-Path $script:TestRoot "mise.toml") + + $out = mise ls --current 2>&1 | Out-String + $LASTEXITCODE | Should -Not -Be 0 + $out | Should -BeLike '*extended-length and device paths*' + } +} diff --git a/e2e/config/test_tool_path_validation b/e2e/config/test_tool_path_validation new file mode 100644 index 00000000000..799a580b5f4 --- /dev/null +++ b/e2e/config/test_tool_path_validation @@ -0,0 +1,30 @@ +#!/usr/bin/env bash + +# A `path:` value is resolved into `ctx.rootPath` for path-mode tools, which plugin hooks build +# shell commands with, so it is checked against a list of shell-quote-breaking characters (#9814). +# +# On Windows `\` is the path separator rather than a shell escape, so it is rewritten to `/` before +# that check. Here it is a shell escape and a legal character in a filename, so it stays rejected. +# This is the end-to-end guard that the rewrite did not leak off Windows. + +mkdir -p tools/bin + +cat >mise.toml <<'EOF' +[tools] +node = { path = '/tmp/\rm' } +EOF +assert_fail_contains "mise ls --current" "invalid tool path" + +cat >mise.toml <<'EOF' +[tools] +node = { path = '/tmp/$(id)' } +EOF +assert_fail_contains "mise ls --current" "invalid tool path" + +# The control: an ordinary path resolves, so the assertions above are about the character and not +# about `path:` being broken in this sandbox. +cat >mise.toml <<'EOF' +[tools] +node = { path = "./tools/bin" } +EOF +assert_contains "mise ls --current" "tools/bin" diff --git a/src/toolset/tool_request.rs b/src/toolset/tool_request.rs index c97a8959e2f..63707e50090 100644 --- a/src/toolset/tool_request.rs +++ b/src/toolset/tool_request.rs @@ -107,8 +107,9 @@ impl ToolRequest { } } Some(("path", p)) => { - validate_path_string(p)?; - let path = resolve_path(p, &source); + let p = windows_path_separators(p); + validate_path_string(&p)?; + let path = resolve_path(&p, &source); Self::Path { path, options: backend.opts(), @@ -519,6 +520,10 @@ fn validate_ref_string(s: &str) -> Result<()> { /// entry in a project config cannot inject shell syntax. Path traversal is /// intentionally not rejected here because `path:../tools/foo` is a normal /// relative-path use case. +/// +/// The list is written for a POSIX shell, which is why `\` is on it. On Windows `\` is a path +/// separator instead, so it is rewritten by [`windows_path_separators`] before it gets here rather +/// than being allowed through — see that function. fn validate_path_string(s: &str) -> Result<()> { if s.is_empty() { return Ok(()); @@ -528,11 +533,49 @@ fn validate_path_string(s: &str) -> Result<()> { // and quote/expansion rejection, but allow `/` since paths need it. is_forbidden_version_char(*c) }) { + // The only `\` that survives the rewrite is an extended-length or device prefix, so say + // what is actually wrong instead of naming a character the user cannot avoid. + #[cfg(windows)] + if c == '\\' { + bail!( + "invalid tool path {s:?}: extended-length and device paths (\\\\?\\, \\\\.\\) are not supported" + ); + } bail!("invalid tool path {s:?}: contains forbidden character {c:?}"); } Ok(()) } +/// Rewrite `\` to `/` in a `path:` value on Windows. +/// +/// `\` is the path separator there, not a shell metacharacter, so [`validate_path_string`] used to +/// reject every native path — anything copied out of Explorer or printed by `pwd`. Win32 accepts +/// `/` wherever it accepts `\`, so rewriting is what makes those usable *without* letting a `\` +/// reach a vfox hook's `ctx.rootPath`, which is the thing the list exists to prevent (#9814). The +/// alternative — dropping `\` from the list on Windows — would have weakened that. +/// +/// Extended-length and device prefixes (`\\?\`, `\\.\`) are left alone. Those are the one place +/// Windows does not accept `/`, so rewriting would hand back a path that looks right and does not +/// resolve; they keep being rejected, as they are today. +#[cfg(windows)] +fn windows_path_separators(s: &str) -> std::borrow::Cow<'_, str> { + if s.starts_with(r"\\?\") || s.starts_with(r"\\.\") { + return std::borrow::Cow::Borrowed(s); + } + if s.contains('\\') { + std::borrow::Cow::Owned(s.replace('\\', "/")) + } else { + std::borrow::Cow::Borrowed(s) + } +} + +/// No-op off Windows: `\` is a legal character in a unix filename, so rewriting it would change +/// which file is meant, and there it really is a shell escape — it stays rejected. +#[cfg(not(windows))] +fn windows_path_separators(s: &str) -> std::borrow::Cow<'_, str> { + std::borrow::Cow::Borrowed(s) +} + fn is_forbidden_version_char(c: char) -> bool { if (c as u32) < 0x20 || c == '\x7f' { return true; @@ -783,6 +826,8 @@ mod tests { #[test] fn test_validate_path_string() { + // `validate_path_string` in isolation. On Windows the `path:` arm rewrites separators + // first, so a `\` in a real config does not reach here — that pair is covered below. use super::validate_path_string; // valid paths for p in [ @@ -815,6 +860,77 @@ mod tests { } } + /// Rewrite then validate, in the order the `path:` arm does it. + fn accept_tool_path(p: &str) -> Result { + let p = super::windows_path_separators(p); + super::validate_path_string(&p)?; + Ok(p.into_owned()) + } + + #[test] + fn test_tool_path_rewrite_does_not_widen_the_rest() { + // The control for the two platform-specific tests below: quote and expansion characters + // stay rejected everywhere, and a path with no separator to rewrite comes back untouched. + assert_eq!( + accept_tool_path("/home/user/tools/foo").unwrap(), + "/home/user/tools/foo" + ); + assert_eq!(accept_tool_path("C:/Users/foo").unwrap(), "C:/Users/foo"); + for p in [ + "/tmp/$HOME", + "/tmp/`id`", + "/tmp/$(id)", + "/tmp/'rm", + "/tmp/\"rm", + "/tmp/\nrm", + ] { + assert!( + accept_tool_path(p).is_err(), + "expected path {p:?} to be rejected" + ); + } + } + + #[cfg(windows)] + #[test] + fn test_native_windows_tool_path_is_accepted() { + // The reported case: a path spelled the way Windows spells it. Rewritten rather than + // allowed through, so no `\` reaches a plugin hook's `ctx.rootPath`. + for (input, expected) in [ + (r"C:\Users\foo\bin", "C:/Users/foo/bin"), + (r"~\.local\bin", "~/.local/bin"), + (r"..\tools\foo", "../tools/foo"), + (r"C:\Program Files\tool", "C:/Program Files/tool"), + ] { + assert_eq!(accept_tool_path(input).unwrap(), expected, "{input:?}"); + } + } + + #[cfg(windows)] + #[test] + fn test_extended_length_tool_path_is_left_alone_and_rejected() { + // `\\?\` and `\\.\` are the one place Windows does not accept `/`, so rewriting them + // would produce a path that looks right and does not resolve. They stay rejected, as + // they are on every version before this change. + for input in [r"\\?\C:\Users\foo", r"\\.\COM1"] { + assert_eq!( + super::windows_path_separators(input), + input, + "{input:?} should not be rewritten" + ); + assert!(accept_tool_path(input).is_err(), "{input:?}"); + } + } + + #[cfg(unix)] + #[test] + fn test_backslash_is_still_rejected_on_unix() { + // The control for the Windows tests: `\` is a legal character in a unix filename and a + // shell escape there, so none of the rewriting reaches this platform. + assert_eq!(super::windows_path_separators(r"/tmp/\rm"), r"/tmp/\rm"); + assert!(accept_tool_path(r"/tmp/\rm").is_err()); + } + #[test] fn test_version_sub() { assert_str_eq!(version_sub("18.2.3", "2").unwrap(), "16");