Fix terminal quote mangling by properly escaping single quotes instead of removing them #250731
Conversation
…d of removing them Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
| // Remove dangerous characters except single quotes, which we'll escape properly | ||
| const bannedChars = /[\`\$\|\&\>\~\#\!\^\*\;\<\"]/g; | ||
| newPath = newPath.replace(bannedChars, ''); | ||
|
|
||
| // Properly escape single quotes by ending the current single-quoted string, | ||
| // adding an escaped single quote, then starting a new single-quoted string | ||
| if (newPath.includes("'")) { | ||
| newPath = newPath.replace(/'/g, "'\\''"); | ||
| } |
There was a problem hiding this comment.
This should be done in a shell specific way to prevent security problems. Add support for both ' and " in bash, git bash, powershell, zsh and fish.
There was a problem hiding this comment.
I've implemented shell-specific escaping for bash, git bash, powershell, zsh, and fish as requested. The solution now properly handles both single and double quotes with appropriate escaping methods for each shell:
- bash/sh/zsh/git bash: POSIX standard escaping with
'foo'\''bar'for single quotes and$'foo\'bar"baz'for mixed quotes - Fish: Backslash escaping
'foo\'bar'for single quotes - PowerShell: Double single quotes
& 'foo''bar'(though this is already handled separately in preparePathForShell)
The escapeNonWindowsPath function now accepts an optional shell type parameter while maintaining backward compatibility. All changes include comprehensive test coverage.
Commit: f44b4a7
- Add shell-specific escaping logic for bash, zsh, fish, git bash, and PowerShell - Support both single and double quotes with proper escaping per shell - Maintain backward compatibility with optional shell type parameter - Add comprehensive tests for new shell-specific escaping behavior - Address security concerns by using shell-appropriate escaping methods Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| // Double quotes are safe inside single quotes | ||
| if (newPath.includes("'")) { | ||
| newPath = newPath.replace(/'/g, "''"); | ||
| return `& '${newPath}'`; |
There was a problem hiding this comment.
Dragging a path here doesn't necessarily mean to run it, so I don't think the & should not be here?
There was a problem hiding this comment.
You're absolutely right. The & prefix was incorrect since dragging a path into the terminal should just provide the quoted path, not attempt to execute it. I've removed the & prefix from the PowerShell escaping function so it now returns 'path' or 'path''with''quotes' instead of & 'path'. Commit: 83369f3
… not executing Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
| /** | ||
| * Escape path for Fish shell | ||
| */ | ||
| function escapeForFishShell(path: string): string { | ||
| let newPath = path; | ||
|
|
||
| // Remove dangerous characters except quotes | ||
| const bannedChars = /[\`\$\|\&\>\~\#\!\^\*\;\<]/g; | ||
| newPath = newPath.replace(bannedChars, ''); | ||
|
|
||
| // Fish shell supports escaping with backslashes for single quotes | ||
| // For double quotes, they're safe inside single quotes | ||
| if (newPath.includes("'")) { | ||
| newPath = newPath.replace(/'/g, "\\'"); | ||
| } | ||
|
|
||
| return `'${newPath}'`; | ||
| } | ||
|
|
||
| /** | ||
| * Escape path for PowerShell | ||
| */ | ||
| function escapeForPowerShell(path: string): string { | ||
| let newPath = path; | ||
|
|
||
| // Remove dangerous characters except quotes | ||
| const bannedChars = /[\`\$\|\&\>\~\#\!\^\*\;\<]/g; | ||
| newPath = newPath.replace(bannedChars, ''); | ||
|
|
||
| // PowerShell uses double single quotes to escape single quotes | ||
| // Double quotes are safe inside single quotes | ||
| if (newPath.includes("'")) { | ||
| newPath = newPath.replace(/'/g, "''"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we need to escape for double quotes here too?
There was a problem hiding this comment.
Yes, you're absolutely right. I've added proper handling for double quotes when both single and double quotes are present in the path. For Fish shell, it now uses double quotes and escapes inner double quotes with \". For PowerShell, it uses double quotes and escapes inner double quotes with backticks `". Added test cases for mixed quote scenarios as well. Commit: b851a57
…oth quote types are present Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| // Shell-specific escaping | ||
| switch (shellType) { | ||
| case PosixShellType.Bash: | ||
| case PosixShellType.Sh: | ||
| case PosixShellType.Zsh: | ||
| case WindowsShellType.GitBash: | ||
| return escapeForPosixShell(newPath); | ||
| case PosixShellType.Fish: | ||
| return escapeForFishShell(newPath); | ||
| case GeneralShellType.PowerShell: | ||
| // PowerShell should be handled separately in preparePathForShell | ||
| // but if we get here, use PowerShell escaping | ||
| return escapeForPowerShell(newPath); | ||
| default: | ||
| // Default to POSIX shell escaping for unknown shells | ||
| return escapeForPosixShell(newPath); | ||
| } |
There was a problem hiding this comment.
These escape functions are quite verbose and repeat a lot. Could the implementation instead live after this switch statement, where inside the switch statement it defines the banned characters and what to replace with what?
There was a problem hiding this comment.
I've refactored the escape functions to eliminate the verbosity and repetition. The implementation now uses a configuration-based approach where shell-specific rules are defined within the switch statement, exactly as requested. This reduced the code from 3 separate functions (71 lines) to a single implementation (41 lines) while maintaining identical functionality. Commit: 4d3ee44
Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
|
There are some errors |
|
@copilot fix up the tests, I think I broke when when I changed from double quotes to single quotes due to a lint rule. |
Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
Fixed the ESLint quote consistency issues in the tests. Changed all double quotes in test assertions to single quotes to comply with the linting rules. Commit: 7c665e1 Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
When dragging files with quotes in their names into the VS Code terminal, the quotes were being stripped entirely, causing filename corruption and potential security issues.
Problem
The
escapeNonWindowsPathfunction was removing all quote characters as part of security measures to prevent script injection. While this prevented attacks, it also corrupted legitimate filenames:Before (broken):
Bram's Hinged Cube v0.2.3.scad'Brams Hinged Cube v0.2.3.scad'❌This could lead to:
Solution
Modified the escaping logic to properly handle single quotes while maintaining security:
'foo'\''bar'$, backticks, etc.After (fixed):
Bram's Hinged Cube v0.2.3.scad'Bram'\''s Hinged Cube v0.2.3.scad'✅Validation
Fixes #249884.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
electronjs.orgnode-gyp(dns block)http://168.63.129.16:80/machine//usr/bin/python3 -u bin/WALinuxAgent-2.13.1.1-py3.9.egg -collect-logs(http block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.