Repository navigation
fix: quote file paths with shell metacharacters in generated terminal commands - #1197
Bondrewd-K wants to merge 4 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…-shquote-parens # Conflicts: # apps/vscode/src/core/python.ts
|
Thank you so much for this contribution, @Bondrewd-K! |
juliasilge
left a comment
There was a problem hiding this comment.
Thanks so much for working on this! This does fix the reported cases with parentheses and semicolons, and the execFileSync() change also looks correct; because it does not invoke a shell, it should receive the raw executable path.
I think we need to revise the generalized shQuote() change, though. The new implementation detects more shell metacharacters but only wraps the value in double quotes; it does not escape the value inside those quotes. In POSIX shells, values such as $HOME, $(command), backticks, backslashes, and embedded double quotes can still be expanded, executed, or parsed incorrectly. PowerShell also performs expansion inside double quotes, while cmd.exe has different behavior again, including %VAR% expansion.
Could you please change this so that the original path is preserved as one literal argument for the shell that will execute the generated terminal command? This likely requires shell-aware argument serialization rather than one generic double-quote wrapper. At a minimum, please add behavior-based tests that verify argument round-tripping for:
- paths with parentheses, covering #1194 and posit-dev/positron#15907;
- paths with
;and@, covering #1102; $,$(), backticks, embedded single and double quotes, backslashes, and newlines;%VAR%and!for Windows command-shell behavior; and- the raw executable path passed by
pythonIsCondaEnv()toexecFileSync().
The current tests only verify the produced string, so they currently accept outputs such as "a$b" even though a shell may not pass that value through literally. A round-trip test through the relevant shell, or focused tests for a shell-specific serializer, would give us confidence that fixing these three reports does not introduce argument corruption or command execution for other valid paths.
Address review feedback on quarto-dev#1197: replace the generic double-quote wrapper with shell-aware argument serialization so a path survives as one literal argument in the shell that will run it -- single quotes for POSIX sh and PowerShell, double quotes with %-caret-escaping for cmd.exe. Adds behavior-based round-trip tests through a real shell.
|
Thanks for the detailed review. Revised as suggested:
|
juliasilge
left a comment
There was a problem hiding this comment.
Thank you so much for these improvements! The POSIX serializer and the bash round-trip test look great. I left a couple of inline review comments for changes to make, plus here are some smaller items:
- The cmd.exe comment gives the wrong reason why
%is safe (packages/core/src/strings.ts, and the same text instrings.test.ts). Interactive cmd expands%NAME%before it processes carets, so^%does not stop the expansion. The output is safe because the"and^characters break the variable name: cmd sees%"PATH"^%, that variable is not defined, and cmd keeps the text. Can you change the comment to give this reason? - The cmd.exe form breaks on a trailing backslash.
C:\becomes"C:\", andCommandLineToArgvWreads\"as a literal quote, not as the closing quote. The same thing occurs for any part that ends in\before a%or!, because each part gets its own quotes. To correct this, double the backslashes at the end of each part before you add the closing quote. winShEscape()is now dead code (packages/core/src/strings.ts). This PR removed its last caller, so you can remove it.
| context.useCmd ? winShEscape(quarto) : shQuote(quarto), | ||
| command | ||
| ]; | ||
| const cmd: string[] = [shQuote(quarto, shell), command]; |
There was a problem hiding this comment.
IIUC on Windows, this line now makes a command that PowerShell cannot run. shQuote("quarto", "powershell") returns 'quarto', so the terminal gets:
'quarto' preview 'C:/path/to/doc.qmd'
When a PowerShell line starts with a quoted string, PowerShell parses the line as an expression, not as a command. The result is a parse error ("Unexpected token 'preview' in expression or statement"), and Quarto does not start. This occurs for every path, not only for paths with special characters. On main, shQuote("quarto") returned quarto without quotes, because the word has no whitespace.
quarto is a constant, so it does not need quotes. Can you keep it bare and quote only the target path?
| const cmd: string[] = [shQuote(quarto, shell), command]; | |
| const cmd: string[] = [quarto, command]; |
If you prefer to quote it, PowerShell needs the call operator: & 'quarto' preview ....
Can you also add a test for the full terminalCommand() output in PowerShell? The GitHub ubuntu runners have pwsh installed, so a round trip through pwsh -NoProfile -Command can run in CI. The current tests examine only shQuote(), so they cannot find a problem in how terminalCommand() makes the full line.
| if (context.useCmd) { | ||
| return "cmd"; | ||
| } | ||
| return os.platform() === "win32" ? "powershell" : "posix"; |
There was a problem hiding this comment.
This function uses the operating system to select the shell, but the terminal does not always use that shell. terminalOptions() does not set shellPath, so window.createTerminal(options) starts the default profile of the user. On Windows, that profile can be PowerShell, cmd.exe, Git Bash, or WSL. On macOS and Linux, it can be pwsh or fish.
If the selected shell is incorrect, the quoting can break paths that work on main now. Instead, can you detect the shell that the terminal will actually use? One possible method is to read vscode.env.shell (the default shell of the user) and map the basename of the file: pwsh or powershell to "powershell", cmd to "cmd", and bash, zsh, or sh to "posix". If the shell is not known, a safe fallback is the old behavior: no quotes when the path has no special characters.
The CI cannot easily test each shell, so a unit test for the mapping from shell path to TerminalShell is sufficient here.
| // Note: execFileSync does not spawn a shell, so the executable path must | ||
| // not be shell-quoted here (shQuote is only for command strings typed | ||
| // into a terminal). | ||
| child_process.execFileSync(python, args, { |
There was a problem hiding this comment.
Thank you for this fix! My earlier review also asked for a test of this change, and I don't see one quite yet.
The vscode import stops this module from running under plain node:test, but the VS Code extension tests in apps/vscode/src/test/ run inside the extension host, where vscode is available. Can you add a test there with npm run test-vscode?
The test must fail with the old shell-quoted path and pass with the raw path. A false result does not show this, because the catch returns false for both. One way to get a true result without conda:
- Make a temporary directory with special characters in its name, for example
dir (x) $y. - In that directory, write an executable script named
pythonthat contains#!/bin/shandecho True. - Make sure that
pythonIsCondaEnv()returnstruefor the script path.
With the old code, the quoted path does not exist, so the function returns false and the test fails. A shebang script does not run on Windows, so skip this test on win32.
|
|
||
| ## 1.139.0 (Unreleased) | ||
|
|
||
| - Fixed "Quarto: Preview" (and render) stalling on Windows PowerShell when the document path contains parentheses but no spaces (e.g. `Article(md)`): arguments in generated terminal commands are now serialized as single literal arguments for the target shell — PowerShell, cmd.exe, or POSIX sh — rather than one generic double-quote wrapper, so paths containing `$`, backticks, `%`, or quotes also survive intact (<https://github.com/quarto-dev/quarto/issues/1194>). |
There was a problem hiding this comment.
Can you make this entry shorter and about the behavior that users see?
| - Fixed "Quarto: Preview" (and render) stalling on Windows PowerShell when the document path contains parentheses but no spaces (e.g. `Article(md)`): arguments in generated terminal commands are now serialized as single literal arguments for the target shell — PowerShell, cmd.exe, or POSIX sh — rather than one generic double-quote wrapper, so paths containing `$`, backticks, `%`, or quotes also survive intact (<https://github.com/quarto-dev/quarto/issues/1194>). | |
| - Fixed "Quarto: Preview" and render in the terminal when the document path contains shell special characters, such as parentheses, `;`, `@`, `$`, or quotes (<https://github.com/quarto-dev/quarto/pull/1197>). |
| assert.equal(roundTripped, value, JSON.stringify(value)); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
The PowerShell and cmd.exe tests compare only strings. For PowerShell, a round trip like the bash test is possible in CI, because the GitHub ubuntu runners have pwsh installed. Can you add a test like this one? It sends each value from pwsh to a native executable, the same way the terminal command sends the target path to quarto.
This test examines only the serializer. It does not replace the test of the full terminalCommand() line that I asked for as well (where the 'quarto' problem is). Because terminalCommand() imports vscode, that test probably goes in the extension tests in apps/vscode/src/test/. Note that this test starts the command with &, because PowerShell does not run a line that starts with a quoted string as a command. The generated terminal command has the same problem.
| ); | |
| ); | |
| test( | |
| "shQuote powershell output round-trips through pwsh to a native command", | |
| { skip: !hasPwsh() && "pwsh is not installed" }, | |
| () => { | |
| // Behavior-based check: pass each serialized value from pwsh to a native | |
| // executable (node), the same way the generated terminal command passes | |
| // the target path to quarto. The $(...) case is a canary: if the value | |
| // were not literal, pwsh would run it and print "PWNED". | |
| const node = shQuote(process.execPath, "powershell"); | |
| const script = shQuote("process.stdout.write(process.argv[1])", "powershell"); | |
| const values = [ | |
| "C:/Users/me/Article(md)/01_introduction.md", | |
| "a b", | |
| "a$b", | |
| "$HOME", | |
| "$(echo PWNED)", | |
| "a`b", | |
| "a'b", | |
| 'a"b', | |
| "a\\b", | |
| "a\nb", | |
| "a;b", | |
| "a@b", | |
| "a&b", | |
| "a|b", | |
| "a#b", | |
| "%PATH%", | |
| "", | |
| ]; | |
| for (const value of values) { | |
| const quoted = shQuote(value, "powershell"); | |
| const roundTripped = execFileSync( | |
| "pwsh", | |
| ["-NoProfile", "-NonInteractive", "-Command", `& ${node} -e ${script} ${quoted}`], | |
| { encoding: "utf-8" } | |
| ); | |
| assert.equal(roundTripped, value, JSON.stringify(value)); | |
| } | |
| } | |
| ); | |
| function hasPwsh(): boolean { | |
| try { | |
| execFileSync("pwsh", ["-NoProfile", "-Command", "exit 0"]); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } |
|
Thanks for the thorough review — all of it is addressed in the latest push.
Tests: added the pwsh round-trip for the serializer (your snippet, canary included), a Also fixed the cmd.exe comment to give the real reason, doubled trailing backslashes before the closing quote, removed the dead One honest caveat: I couldn't run the suites locally in this environment, so CI is the real gate here. Let me know if anything still looks off. |
terminalCommand()quotes the target path withshQuote(), butshQuote()only quoted on whitespace. A path like
C:/dir(name)/file.qmdwas emittedunquoted, so in the integrated PowerShell terminal
(name)parsed as asubexpression — and
mdis an alias formkdir, stalling "Quarto: Preview"at a
Path[0]:prompt. (Bare parens are equally a syntax error in bash.)shQuote()now quotes on whitespace or any shell-special character. Alsofixed
pythonIsCondaEnv()passing a shell-quoted path toexecFileSync,which spawns no shell.
Fixes #1194