Skip to content

Also return suggestion to use PSCredential for AvoidUsingPlainTextForPassword rule#1782

Merged
bergmeister merged 4 commits intoPowerShell:masterfrom
bergmeister:avoidusingplaintextforpassword-multiple-suggestions
Jul 11, 2022
Merged

Also return suggestion to use PSCredential for AvoidUsingPlainTextForPassword rule#1782
bergmeister merged 4 commits intoPowerShell:masterfrom
bergmeister:avoidusingplaintextforpassword-multiple-suggestions

Conversation

@bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Mar 28, 2022 •
edited
Loading

PR Summary

Fixes #1767 by also adding an option for PSCredential.

PR Checklist

@bergmeister
Merge branch 'master' of https://github.com/PowerShell/PSScriptAnalyzer…
e946f27 
… into avoidusingplaintextforpassword-multiple-suggestions
@bergmeister
Merge branch 'master' of https://github.com/PowerShell/PSScriptAnalyzer…
bd12868 
… into avoidusingplaintextforpassword-multiple-suggestions
@bergmeister
fix tests
30b4bcb
@bergmeister bergmeister mentioned this pull request Mar 30, 2022
Merged
@bergmeister
Copy link
Collaborator Author

bergmeister commented Mar 30, 2022 •
edited
Loading

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
@bergmeister @andyleejordan
Correctly map SuggestedCorrection to MarkerCorrection (#1749)
c49fa2a 
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 <andrew@schwartzmeyer.com>
@bergmeister bergmeister enabled auto-merge (squash) May 16, 2022 16:56
@bergmeister
Copy link
Collaborator Author

bergmeister commented May 16, 2022

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

sdwheeler
sdwheeler reviewed Jun 6, 2022
View reviewed changes
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

bergmeister commented Jun 15, 2022

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

Reviewers

@sdwheeler sdwheeler sdwheeler left review comments

+1 more reviewer

@JamesWTruher JamesWTruher JamesWTruher approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area - Rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UsePSCredentialType : on a variable $credential the quickfix proposed is [securestring] instead of [PsCredential]

3 participants

@bergmeister @JamesWTruher @sdwheeler