Skip to content

AvoidUsingPlainTextForPassword docs do not cover all checked parameter names #1565

Open
@alekdavis

Description

@alekdavis

I have a script parameter that holds a path to the encrypted credential file named $CredentialFile for which I get the warning:

Parameter '$CredentialFile' should use SecureString, otherwise this will expose sensitive information. See ConvertTo-SecureString for more information.

There is no reason for me use SecureString here and I cannot think of a better alternative to the parameter name, so it would be nice to suppress this message for this particular instance, but not to the whole file or project. Besides, the documentation for PSAvoidUsingPlainTextForPassword needs to include Credential and whatever other strings it gets triggered of, since it only mentions one word: Password.

Activity

rjmholt

rjmholt commented on Aug 6, 2020

@rjmholt
Contributor

Addressing your title, see https://github.com/PowerShell/PSScriptAnalyzer#suppressing-rules.

Besides, the documentation for PSAvoidUsingPlainTextForPassword needs to include Credential and whatever other strings it gets triggered of, since it only mentions one word: Password.

Agreed.

It seems the list of terms is:

List<String> passwords = new List<String>() {"Password", "Passphrase", "Cred", "Credential"};

And those terms only need to appear within the parameter/variable name to trigger the rule:

if (paramName.IndexOf(password, StringComparison.OrdinalIgnoreCase) != -1)

We would welcome your PR to update the docs here

alekdavis

alekdavis commented on Aug 6, 2020

@alekdavis
Author

Oh, sweet. I did not realize that SuppressMessageAttribute must be applied to params() (originally tried it next to the parameter itself and it did not help). It's perfect when applied correctly. Thanks for your help.

changed the title [-]No way to turn AvoidUsingPlainTextForPassword off[/-] [+]AvoidUsingPlainTextForPassword docs do not cover all checked parameter names[/+] on Aug 6, 2020
rjmholt

rjmholt commented on Aug 6, 2020

@rjmholt
Contributor

Reopening to track doc issue

reopened this on Aug 6, 2020
CptMikhailov

CptMikhailov commented on Aug 25, 2021

@CptMikhailov

Is there any movement on this? I'm encountering an issue where a parameter called $CreditUnion is being incorrectly flagged. I think that the way this is implemented should be less broad, possibly. I can look at submitting a pull request after I've had a look.

Does anyone have any suggestions for how this should be reimplemented? I'm thinking maybe a more robust set of regular expression rules? Or just having a more robust list of terms, and testing for equality instead of pattern matching.

rjmholt

rjmholt commented on Aug 25, 2021

@rjmholt
Contributor

Is there any movement on this?

Not yet; we would welcome your contribution.

Does anyone have any suggestions for how this should be reimplemented?

Ultimately this is a very heuristic rule and the idea is we can infer what should be a secure string based on the name of parameters. But most users seem to appreciate that. I think a regex is probably the way to go. Something like:

([Cc]redential|[Pp]ass(word|phrase)|[Tt]oken)$
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @alekdavis@rjmholt@CptMikhailov@SydneyhSmith

        Issue actions

          AvoidUsingPlainTextForPassword docs do not cover all checked parameter names · Issue #1565 · PowerShell/PSScriptAnalyzer