Add new AvoidSecretDisclosure rule - #2183
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds a new built-in PSScriptAnalyzer rule (PSAvoidSecretDisclosure) to flag common patterns that convert secrets (e.g., SecureString) into plaintext, with accompanying tests, localized strings, and documentation.
Changes:
- Introduces AvoidSecretDisclosure rule implementation to detect ConvertFrom-SecureString -AsPlainText, SecureStringTo* calls, and .Password member access.
- Adds Pester tests covering violations, compliant examples, and suppression scenarios.
- Updates rule documentation and the rules index, and adds localized strings for name/common name/description/error.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file| File | Description |
|---|---|
| Tests/Rules/AvoidSecretDisclosure.tests.ps1 | Adds Pester coverage for the new rule (violations/compliance/suppression). |
| Rules/Strings.resx | Adds localized strings for the new rule’s name/common name/description/message. |
| Rules/AvoidSecretDisclosure.cs | Implements the new analyzer rule logic and diagnostic creation. |
| docs/Rules/README.md | Registers the rule in the published rules list/table. |
| docs/Rules/AvoidSecretDisclosure.md | Adds the rule’s public documentation page and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
Sean Wheeler (sdwheeler)
left a comment
There was a problem hiding this comment.
Added suggestions for style and one question about the parameter.
Sorry, something went wrong.
Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>
Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>
Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>
Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>
Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>
…ble in a future release after giving users time to adjust.
Christoph Bergmeister (bergmeister)
left a comment
There was a problem hiding this comment.
There is already a AvoidUsingConvertToSecureStringWithPlainText therefore if we were ti have a general rule to alert on things like SecureString methods, then the old rule logic should be merged into this one and there should be configuration on what to alert for. For example on Linux, Credential does not apply. And on Linux SecureString is not implemented and since .NET 5, Securestring was marked as obsolete and not recommended any more as it is understood that practically it is and was never more secure in first place.
Since it's an optional rule, we can be opinionated but in practice many warnings will not be fixable and we found that this can be a source of frustration for community.
Sorry, something went wrong.
|
Christoph Bergmeister (@bergmeister), The difference with the existing AvoidUsingConvertToSecureStringWithPlainText rule is that this rule warns for a convert from plain text in contrast to this new AvoidSecretDisclosure rule which warns for converting a secret to plain text.
Agree, this rule should be disabled by default for continuous integration usage and enabled manually as it will certainly reveal a lot of security violations in the currently installed bases... |
Sorry, something went wrong.
Sean Wheeler (sdwheeler)
left a comment
There was a problem hiding this comment.
Docs look good.
Sorry, something went wrong.
|
Since about 3 weeks ago, I have created 6 PR for new rules (the last one was from yesterday: #2186). |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
PR Summary
Closes: #1997
Description
Disclosing a secret might result in security vulnerabilities such as memory trails or logging trails that could
be exploited by attackers. This rule identifies instances where a secret is being converted to plain text,
which can lead to unintended exposure of sensitive information.
Important
The general approach of dealing with credentials is to avoid them and instead rely on other means
to authenticate, such as certificates or Windows authentication.
How to Fix
In general, avoid any code pattern that involves converting secrets to plaintext or accessing plaintext secrets.
SecurePassword instead of accessing plaintext passwords.
Note
For custom properties named "Password", it is recommended to rename them to something that does not imply they
contain secrets, or to ensure that they do not actually contain secrets. If renaming is not possible, consider
suppressing the warning for those specific cases.
PR Checklist