Fix encoding bug, command injection, and $-expansion in profile path#3
Closed
Fix encoding bug, command injection, and $-expansion in profile path#3
Conversation
…alls - Convert all sp.run(string, shell=True) to sp.run([list], shell=False) using ["pwsh", "-c", "..."] so PowerShell is explicitly targeted and no OS-level shell expansion occurs (fixes paths with spaces) - Fix get_current_encoding() which already had shell=False but still passed a string (incorrect); now passes a proper list - Add _pwsh_escape() helper to escape single quotes in paths when embedding them in PowerShell single-quoted strings - Change profile-append command from '\"path\" | Invoke-Expression' to '& \"path\"' syntax, which correctly handles paths with spaces - Quote all paths in PowerShell commands with single quotes Fixes #2 Co-authored-by: StephLin <20769958+StephLin@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] [click-pwsh#2] Fix issues with space causing command errors
Fix space-in-path breakage in install/update by using shell=False with list-based subprocess calls
Mar 15, 2026
StephLin
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates click_pwsh’s install/update flows to be robust to PowerShell profile paths containing spaces by avoiding shell=True string commands and improving PowerShell-side path quoting.
Changes:
- Switch all
subprocess.run()invocations to list-based args withshell=False(directly executingpwsh). - Add
_pwsh_escape()and wrap redirected file paths in single quotes inside PowerShell commands. - Replace
Invoke-Expression-based script invocation with the PowerShell call operator (&).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
28
to
33
| encoding = sp.run( | ||
| ["pwsh", "-c", "(chcp | Out-String).Split(' ')[-1].Trim()"], | ||
| shell=False, | ||
| capture_output=True, | ||
| ) | ||
| encoding = ''.join(chr(byte) for byte in encoding.stdout if byte not in (b'\r', b'\n')) |
Owner
There was a problem hiding this comment.
@copilot This should be an irrelevant issue though it’s important. Please open another PR to target this issue.
Author
There was a problem hiding this comment.
I'm unable to open new PRs. Please open a new issue or PR to track this — the fix would be to replace the byte-by-byte iteration with encoding.stdout.decode("ascii").strip() (or "utf-8") in get_current_encoding(), since chcp output is always ASCII.
Comment on lines
+57
to
+62
| "$env:{0} = 'pwsh_source'; {1} > '{2}'; $env:{0} = $null".format( | ||
| completion_varname, | ||
| command, | ||
| _pwsh_escape(str(completion_profile)), | ||
| ), | ||
| ], |
Comment on lines
+69
to
+70
| "echo '& \"{}\"' >> '{}'".format( | ||
| str(completion_profile), |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: StephLin <20769958+StephLin@users.noreply.github.com>
Copilot
AI
changed the title
Fix space-in-path breakage in install/update scripts
Fix encoding bug, command injection, and $-expansion in profile path
Mar 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three correctness and security issues in
click_pwsh/__main__.pyidentified during review of the space-in-path fix.Changes
get_current_encoding()fix — iteratingbytesyieldsints, sobyte not in (b'\r', b'\n')was alwaysTrueand CR/LF could leak into the returned encoding string. Replaced withstdout.decode("utf-8").strip().Command injection guard —
commandwas interpolated directly into the PowerShell-cstring with no validation, allowing arbitrary PS execution (e.g.foo; Remove-Item ...). Added_validate_command()enforcing^[A-Za-z0-9][A-Za-z0-9_-]*$before anysp.run()call.$-safe profile invocation — the line appended to the user's profile was& "path"(double-quoted), so any$in the filename would be expanded at profile load time. Changed to single-quoted& 'path'via_pwsh_escape():Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.