Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Also return suggestion to use PSCredential for AvoidUsingPlainTextForPassword rule #1782

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Mar 28, 2022

PR Summary

Fixes #1767 by also adding an option for PSCredential.

PR Checklist

… into avoidusingplaintextforpassword-multiple-suggestions
… into avoidusingplaintextforpassword-multiple-suggestions
@bergmeister
Copy link
Collaborator Author

bergmeister commented Mar 30, 2022

PR that fixes the UI bug in the extension, which only displayed one code action: PowerShell/PowerShellEditorServices#1749

andyleejordan added a commit to PowerShell/PowerShellEditorServices that referenced this pull request Mar 31, 2022
With the recent fix in PR #1718, PSES processes now all Correction objects from PSSA but the message specifically was just taken from the last correction here. I did not notice this at first because I thought I had to tweak my rule first to emit two different messages until I realized it was another bug in PSES.

Related: PowerShell/PSScriptAnalyzer#1782

Co-authored-by: Andy Schwartzmeyer <[email protected]>
@bergmeister bergmeister enabled auto-merge (squash) May 16, 2022 16:56
@bergmeister
Copy link
Collaborator Author

@JamesWTruher Can you please help force merge it due to the old checks removed in master that are blocking the merge?

Copy link
Collaborator

@sdwheeler sdwheeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the severity of this rule be DiagnosticSeverity.Error to match the severity of AvoidUsingConvertToSecureStringWithPlainText?

@bergmeister
Copy link
Collaborator Author

Should the severity of this rule be DiagnosticSeverity.Error to match the severity of AvoidUsingConvertToSecureStringWithPlainText?

I think existing severities of old rules like this one are very inconsistent but my thinking is that Error should only be set when there would be a runtime problem and the rule is certain on not being a false positive, i.e. a 'must fix'. Therefore I'd rather downgrade severity of AvoidUsingConvertToSecureStringWithPlainText TBH

@bergmeister bergmeister merged commit 87199e5 into PowerShell:master Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants