Repository navigation
UsePSCredentialType : on a variable $credential the quickfix proposed is [securestring] instead of [PsCredential] #1767
Description
Summary of the new feature
When I have a variable called $credential, the proposed quickfix is to set it as a [securestring] instead of [PsCredential] object :
Proposed technical implementation details (optional)
The Quick Fix should be [PsCredential]
What is the latest version of PSScriptAnalyzer at the point of writing
1.20.0
Activity
StevenBucher98 commentedon Feb 1, 2022
Thanks for pointing this out Thomas F. (@zamothh)! We will leave this open for discussion, adding extra logic in the rule to check for the
$credentialand recommend[PsCredential]would be reasonable.StevenBucher98 commentedon Feb 1, 2022
What do you think Christoph Bergmeister (@bergmeister)?
JustinGrote commentedon Feb 2, 2022
I think [PsCredential] is what most people are thinking. There's already the separate rule saying parameters named $Password should be [SecureString], that should stay as is, but Credential specifically almost always in Powershell nomenclature means a PSCredential (username/password pair)
bergmeister commentedon Feb 4, 2022
At the moment, PSSA would be happy with either pscredential or securestring type. It's worthwhile remembering that the way things work is the PSSA rule emits the SuggestedCorrections property, which is an
IEnumerableso we could return more than one correction. We'd have to check if current code actually would propagate this end to end in PSSA and the extension (code somewhere might lazily just take the first element). As I have worked on the extension before happy to look into it. Therefore we wouldn't have to think about which is the best. Let me know what you thinkbergmeister commentedon Feb 6, 2022
I had a more detailed look: The
AvoidUsingPlainTextForPasswordrule gets triggered here if the parameter name is eitherPassword,Passphrase,CredorCredential. It seems that the rule does some type analysis and somehow the rule does not get triggered for types such as[int], [pscustomobjector[pscredential, hence why my above observation around pscredential was a red herring (or rather a defect in the rule). Just by looking at the code here, which determines whether a warning gets emitted based on type analysis, it seems the rule is just relying on the above defect to emit a warning rather than looking for a specific type like SecureString or PSCredential. I propose to fix the rule to only alert if the type is neither pscredential nor securestring and always emit a warning otherwise. WDYT Steven Bucher (@StevenBucher98)Also, I did a small PoC to make the rule return two local suggested corrections and this works end to end in the PSScriptAnalyzer module but not in the VS-Code UI, I suspect there is code somewhere to just take the first element of the list, I've contributed to the vs-code extension and PSES before so I should be able to fix that myself as well.
Return a code action for each diagnostic record
StevenBucher98 commentedon Mar 1, 2022
Thanks for the PR Christoph Bergmeister (@bergmeister), just so I am following it correctly, your fix helps with suggesting multiple warnings for the VSCode UI? There was no fix for the alert to add "Set $credential to
[PsCredential]" correct?bergmeister commentedon Mar 1, 2022
The first PR in PSES was to fix a bug in the VS Code extension where it would only ever display one suggested correction. My next PR will be in PSSA and will make the rule return 2 suggested corrections as part of the single warning. This way users can choose whether they prefer to fix it with pscredential or the securestring. I can show you a screenshot when I open the PSSA PR, which should make it clearer