- Notifications
You must be signed in to change notification settings - Fork8.1k
Add CodeQL suppressions for EventLog, NativeCommandProcessor, TypeCatalogGen, Process methods#26467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR adds CodeQL suppression comments to document expected security-related behaviors in PowerShell's codebase. The changes clarify that certain security warnings (command-line injection, SSRF, weak cryptography) are intentional design decisions based on PowerShell's trust model where users have full control and responsibility for the code they execute.
Key changes:
- Corrects spelling of "Poweshell" to "PowerShell" in 7 existing CodeQL suppression comments
- Adds 5 new CodeQL suppression comments for command-line injection warnings in process-related code
- Adds 1 new CodeQL suppression comment for SHA-1 weak cryptography usage (backwards compatibility)
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TypeCatalogGen.cs | Adds weak-crypto suppression for SHA-1 hash algorithm used for backwards compatibility |
| FileSystemProvider.cs | Corrects "Poweshell" to "PowerShell" in command-line-injection-shell-execution suppression |
| UpdatableHelpSystem.cs | Corrects "Poweshell" to "PowerShell" in two SSRF suppressions |
| RunspaceConnectionInfo.cs | Corrects "Poweshell" to "PowerShell" in SSH process command-line-injection suppression |
| NativeCommandProcessor.cs | Adds new command-line-injection suppression and corrects "Poweshell" to "PowerShell" in two existing suppressions |
| WebRequestPSCmdlet.Common.cs | Corrects "Poweshell" to "PowerShell" in SSRF suppression |
| Process.cs | Adds two new command-line-injection suppressions for working directory paths and corrects "Poweshell" to "PowerShell" in existing suppression |
| Eventlog.cs | Adds new command-line-injection suppression for ComputerName parameter in event viewer launch |
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for7 days. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
| // codeql[cs/microsoft/command-line-injection-shell-execution] - This is expectedPoweshell behavior where user inputted paths are supported for the context of this method. The user assumes trust for the file path specified, so any file executed in the runspace would be in the user's local system/process or a system they have access to in which case restricted remoting security guidelines should be used. | ||
| ProcessStartInfostartInfo=new(filePath); | ||
| // codeql[cs/microsoft/command-line-injection-shell-execution] - This is expectedPowerShell behavior where user inputted paths are supported for the context of this method. The user assumes trust for the file path specified, so any file executed in the runspace would be in the user's local system/process or a system they have access to in which case restricted remoting security guidelines should be used. | ||
| System.Diagnostics.ProcessStartInfostartInfo=newSystem.Diagnostics.ProcessStartInfo(filePath); |
CopilotAIDec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The change fromProcessStartInfo toSystem.Diagnostics.ProcessStartInfo is inconsistent with the existing code style in this file and adds unnecessary verbosity. TheProcessStartInfo type is already imported viausing System.Diagnostics; at the top of the file, so the fully qualified name is redundant. This change should be reverted to maintain consistency with the rest of the codebase.
| System.Diagnostics.ProcessStartInfostartInfo=newSystem.Diagnostics.ProcessStartInfo(filePath); | |
| ProcessStartInfostartInfo=newProcessStartInfo(filePath); |
Uh oh!
There was an error while loading.Please reload this page.
Add CodeQL suppression comments to clarify that certain potential security issues (such as command-line injection and weak cryptography) are expected behaviors in PowerShell, given its design and user trust model. These comments provide context for static analysis tools and future maintainers. No functional code changes are included. Also fixed typo and updated prior CodeQL suppressions.
These changes do not affect runtime behavior but improve code documentation and static analysis compliance.
PR Summary
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header