Repository navigation
Fix PowerShell injection in Windows tool helpers - #34
Merged
Merged
Conversation
Merged
…indows Co-authored-by: kriserickson <325934+kriserickson@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] WIP Address feedback on Tool calling pull request
Fix PowerShell injection in Windows tool helpers
Mar 1, 2026
kriserickson
approved these changes
Mar 1, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a PowerShell command-injection vulnerability on Windows by removing direct interpolation of model-supplied command names into powershell -Command strings and instead passing the values out-of-band via environment variables.
Changes:
- Update
execCommandHelp(Windows) to reference$env:HELP_COMMANDinstead offmt.Sprintf(...)interpolation. - Update
execCheckCommand(Windows) to reference$env:CHECK_COMMANDinstead offmt.Sprintf(...)interpolation.
Comments suppressed due to low confidence (3)
internal/tools/tools.go:152
- There is still a PowerShell injection vector on Windows via
execListDirectory: it builds-Commandwithfmt.Sprintf("Get-ChildItem '%s'", absPath)(tools.go:107).ValidatePathconstrains traversal but does not prevent'/;etc in the path, so model-suppliedpathcan still break out of the quoted string and execute arbitrary PowerShell. Consider applying the same out-of-band parameter pattern here as well (e.g., pass the path via env var and use-LiteralPathin the script, or otherwise avoid string interpolation in-Command).
if info.IsDir() {
return "", fmt.Errorf("%q is a directory, not a file", path)
internal/tools/tools.go:152
- The new Windows-specific injection mitigation in
execCommandHelpisn’t covered by tests (existing tests skip Windows success paths), so regressions back to string interpolation could slip in unnoticed. Consider refactoring to separate “build the exec.Cmd” from “run it”, or injecting anexecCommandContextfunction, so unit tests can assert the PowerShell arguments and thatcmd.Envis used to pass the command name.
if info.IsDir() {
return "", fmt.Errorf("%q is a directory, not a file", path)
internal/tools/tools.go:260
execCheckCommandalso changed to pass the command name viacmd.Env, but there’s no test coverage asserting the Windows code path uses environment variables (and notfmt.Sprintfinterpolation). If you refactor to make command construction testable, add a focused unit test for this function too to prevent reintroducing PowerShell-Commandinjection.
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
execCommandHelpandexecCheckCommandon Windows interpolated model-supplied input directly into PowerShell-Commandstrings, enabling arbitrary PowerShell injection.Changes
execCommandHelp: Replacefmt.Sprintf("Get-Help '%s'", command)with a static script referencing$env:HELP_COMMAND; pass the value viacmd.Env.execCheckCommand: Same pattern — replacefmt.Sprintf("Get-Command '%s'", command)with$env:CHECK_COMMANDset in the process environment.The command name is now passed out-of-band through the process environment, never interpolated into the script string.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.