Skip to content

fix: quote file paths with shell metacharacters in generated terminal commands - #1197

Open
Bondrewd-K wants to merge 4 commits into
quarto-dev:mainfrom
Bondrewd-K:fix/vscode-shquote-parens
Open

Bondrewd-K wants to merge 4 commits into
quarto-dev:mainfrom
Bondrewd-K:fix/vscode-shquote-parens

Conversation

@Bondrewd-K

Copy link
Copy Markdown

terminalCommand() quotes the target path with shQuote(), but shQuote()
only quoted on whitespace. A path like C:/dir(name)/file.qmd was emitted
unquoted, so in the integrated PowerShell terminal (name) parsed as a
subexpression — and md is an alias for mkdir, 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. Also
fixed pythonIsCondaEnv() passing a shell-quoted path to execFileSync,
which spawns no shell.

Fixes #1194

@posit-snyk-bot

posit-snyk-bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

…-shquote-parens

# Conflicts:
#	apps/vscode/src/core/python.ts
@juliasilge

Copy link
Copy Markdown
Collaborator

@juliasilge juliasilge left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() to execFileSync().

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.
@Bondrewd-K

Copy link
Copy Markdown
Author

Thanks for the detailed review. Revised as suggested:

  • shQuote() now takes the target shell ("posix" | "powershell" | "cmd") and serializes the value as one literal argument: single quotes for POSIX sh and PowerShell ('\'' / '' for embedded quotes), double quotes for cmd.exe with %/! caret-escaped outside the quotes (" doubled per CommandLineToArgvW).
  • terminalCommand() picks the shell via targetShell(): cmd for the legacy useCmd path, PowerShell on Windows, POSIX sh elsewhere.
  • Added behavior-based tests: per-shell serialization cases plus a round-trip through real bash (printf %s <quoted>) asserting byte-identical output. The $(...)/backtick cases act as canaries. Covered: parentheses (VS Code extension: quarto preview fails when the file path contains parentheses (path not shell-quoted) #1194), ;/@ (VS Code Preview not working if path contains special characters #1102), $, $(), backticks, embedded quotes, backslashes, newlines, %VAR%/!.
  • pythonIsCondaEnv() passes the raw path to execFileSync() with no quoting (verified by inspection; the module imports the vscode API so it can't run under plain node:test).

@juliasilge juliasilge left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in strings.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:\", and CommandLineToArgvW reads \" 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.

Comment thread apps/vscode/src/core/terminal.ts Outdated
context.useCmd ? winShEscape(quarto) : shQuote(quarto),
command
];
const cmd: string[] = [shQuote(quarto, shell), command];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Suggested change
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.

Comment thread apps/vscode/src/core/terminal.ts Outdated
if (context.useCmd) {
return "cmd";
}
return os.platform() === "win32" ? "powershell" : "posix";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Make a temporary directory with special characters in its name, for example dir (x) $y.
  2. In that directory, write an executable script named python that contains #!/bin/sh and echo True.
  3. Make sure that pythonIsCondaEnv() returns true for 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.

Comment thread apps/vscode/CHANGELOG.md Outdated

## 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>).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this entry shorter and about the behavior that users see?

Suggested change
- 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));
}
}
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
);
);
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;
}
}

@Bondrewd-K

Copy link
Copy Markdown
Author

Thanks for the thorough review — all of it is addressed in the latest push.

quarto stays bare now (good catch — PowerShell parsing a line that starts with a quoted string as an expression broke every Windows preview). Shell detection reads vscode.env.shell instead of guessing from the OS, mapping pwsh/powershell → powershell, cmd → cmd, bash/zsh/sh → posix, and falls back to the old quoting when the shell isn't recognized.

Tests: added the pwsh round-trip for the serializer (your snippet, canary included), a terminalCommand() test in the extension tests, the shell-path mapping unit test, and the pythonIsCondaEnv test with a shebang script inside a dir (x) $y directory (skipped on win32).

Also fixed the cmd.exe comment to give the real reason, doubled trailing backslashes before the closing quote, removed the dead winShEscape(), and shortened the changelog entry as suggested.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VS Code extension: quarto preview fails when the file path contains parentheses (path not shell-quoted)

3 participants