Better way to implement rules in PS script

[rjmholt]

PSScriptAnalyzer today supports external implementation of rules in PowerShell script.

Alongside the proposal to allow custom binary rules (#1543), PSScriptAnalyzer should also look at how rules written in PowerShell are implemented to ensure that PS script rules:

• are easy to implement
• are consistent with each other and with binary rules, both in terms of having a uniform way to implement them, and a uniform way to reference them
• can be implemented without excessive boilerplate, and leverage PSScriptAnalyzer as much as possible

To address these goals, some proposals are:

Standard assumed arguments

PS script functions today have a very loose parameter contract that expects parameters that either end with the Ast suffix or the Token suffix, this means:

• We must spend time trawling through parameter names to find matching ones
• Functions must choose between using the AST and using tokens
• We can't call all functions the same way, meaning we must spend more time building the correct command call for each rule

Instead script rules should be required to accept a clear and concrete set of parameters, similar to the way TabExpansion2 does:

param(
    [ValidateNotNullOrEmpty()]
    [System.Management.Automation.Language.Ast]
    $Ast,

    [ValidateNotNullOrEmpty()]
    [System.Management.Automation.Language.Token[]]
    $Tokens,

    [string]
    $ScriptFilePath
)

Additional points:

• The API should only care about the parameters' names. Parameter attributes, [CmdletBinding()], positional and pipeline arguments don't matter to PSSA, as it will simply invoke rules with new PSCommand().AddCommand(<script rule>).AddParameter("Ast", ast).AddParameter("Tokens", tokens).AddParameter("ScriptFilePath", scriptFilePath)
• The $Tokens parameter should be immutable and ideally is a System.Collections.Generic.IReadOnlyList<Token>. But this may not work well with PowerShell, so the Token[] type may be preferable, with an understanding that mutating the token array is against the contract

Attribute based rule definition

Today, PS script rules are:

• defined by being in a module imported as a rule module
• named by function name
• namespaced by module name

To be consistent with #1543 and addressing #1545, rules instead should be declared with attributes:

[Microsoft.PowerShell.ScriptAnalyzer.Rule("AvoidAliases", "Avoid the use of aliases in PowerShell scripts")]

We could also provide a type accelerator for this attribute to reduce it to [Rule(...)].

This attribute would provide the possibility of namespace override: [Rule(..., Namespace = "MyRules")].

Write-Diagnostic instead of needing to construct diagnostic objects

Today script rules must construct and emit diagnostic objects themselves. This means:

• Script rules must use cumbersome constructor/New-Object calls
• Boilerplate is required to provide all arguments to a diagnostic constructor
• No special logic can be used to do things like:
  • Ensure the diagnostic is linked back to the rule that emitted it
  • Automatically infer properties about the diagnostic from its context
  • Suppress the diagnostic before it is created
  • Possibly perform context-dependent functionality

Instead, a PowerShell-native solution would be for PSSA to introduce a new cmdlet that hooks back into the hosting script analyzer:

Write-Diagnostic -Message <string> -Extent <IScriptExtent> [-Severity <DiagnosticSeverity>]

This command can:

• Simplify diagnostic emission
• Automatically suppress diagnostics internally
• Allow optional parameters and changes to the diagnostic constructor and write-diagnostic cmdlet without breaking APIs
• Even infer what rule it's being run from from the call stack (while this can be done, it's not clear it's a good idea)

Constrain and document the assumptions script rules can make about their executing context

Finally, today it's not clear what assumptions a script rule should make about the context in which it's run, in particular whether it shares context with the environment running Invoke-ScriptAnalyzer.

So that PSSA can be as efficient as possible, the only assumptions script rules should make about their running environment are:

• The rule-defining module and the current version of PSSA are imported into their executing runspace
• The script rule will be run on the same machine under the same user as the invoking PSSA process (which may not be using Invoke-ScriptAnalyzer, since it may be hosted)

In particular, some assumptions we don't want are:

• The script rule will be run in the same runspace or on the same thread as Invoke-ScriptAnalyzer
• Script rules will be run serially or in any defined order
• Script rules can access PSSA or other script rule module state
• Ideally, script rules should not assume they're being run in the same process as PSSA, but that may be an unhelpful and unneeded addition

Example

So an example of a rule defined with the new proposed API would be something like this:

function Test-UsesApprovedVerbs
{
    [Microsoft.PowerShell.ScriptAnalyzer.Rule("UseApprovedVerbs", "Ensure functions use approved PowerShell verbs", Namespace = "MyRules", Severity = "Information")]
    param(
        [System.Management.Automation.Language.Ast]
        $Ast,

        [System.Management.Automation.Language.Token[]]
        $Tokens,

        [string]
        $ScriptFilePath
    )

    $commandAsts = $Ast.FindAll({ <# logic to find non-compliant ASTs #> }, $true)
    foreach ($commandAst in $commandAsts)
    {
        # Extra parameter sets or transformations could be added here to make this nicer
        Write-Diagnostic -Extent $commandAst.Extent -Message "Does not comply" -Severity Information
    }
}

This rule can be referenced with MyRules/UseApprovedVerbs in settings.

Further considerations

• Should tokens be passed in an immutable IReadOnlyList<Token>?
• Should rules be required to process things in process blocks and have all inputs piped in with ValueFromPipelineByPropertyName? (This might improve performance)
• Is the assumption set too small?
• Is Write-Diagnostic the right implementation? In particular, how do we ensure every diagnostic has a RuleInfo object associated with it that links back to the providing rule (and ideally is referentially equal to all RuleInfo objects referring to the same rule)?
• Should Write-Diagnostic actually emit an object, or should it add objects to a list within PSSA?
• Should script rules be configurable? If so, how do they declare it? Should the configuration type be object, since PowerShell will handle any type there?

[jegannathanmaniganadan]

I am really looking forward on this,

• I like the idea of having attribute based rule definition. I am glad that it will solve the inconsistency behavior with the custom rules written in PS.

Few doubts,

• What would be the default if not overridden ? script name ?
• Assuming that I override the custom rule namespace, how would the suppression look like ? Does it have to be something like below ?

[SuppressMessageAttribute('MyRules\FooBar', 'Justification')] ?

Would it be nice if there is a way to register all (the new design allows me to specify more than one namespace) my custom rule namespace in PSSA, so that it will internally handle all the rules from that space ? Or something like below ?

Invoke-ScriptAnalyzer -CustomRulePath some.psm1 -CustomRuleSpace MyRule

Feel free to ignore if this sounds bad

[rjmholt]

What would be the default if not overridden ? script name ?

Currently you can't construct the attribute without a name, so you're forced to provide it. However perhaps it makes sense to use a default like:

• The name of the class minus the Rule suffix if there is one for class-defined rules
• The name of the command for command-defined rules

Assuming that I override the custom rule namespace, how would the suppression look like ?

Yeah, suppression would look pretty much as you lay out.

Would it be nice if there is a way to register all (the new design allows me to specify more than one namespace) my custom rule namespace in PSSA, so that it will internally handle all the rules from that space ?

That's actually a really good consideration. The current settings proposal includes specifying a rule collection by path and then adding settings for each rule. You would then need to specify each rule you want to enable from that collection. Thinking on that now, that's cumbersome (if nicely explicit), but the problem is that any rules that require configuration settings must have those settings provided to be able to run. This makes me think that:

• There should be a high level setting to determine whether rules must be explicitly configured to run vs implicitly run because their collections have been specified
• Configurable rules must either have a default configuration or have some way to determine that they can't be run (like TryGetDefaultConfiguration() or something)
• Script rules should also support configuration, probably by opting in with a $Configuration parameter

[iRon7]

From the start that I wrote my own custom rule a few years ago, I am still wondering how to install a rule in the environment. Meaning in the same scope as where PSScriptAnalyzer is installed (everywhere where I have access to the Invoke-ScriptAnalyzer command).
It looks like an too obvious question, so I am wondering if I might just completely missed something...

The document Creating custom rules, states:

PSScriptAnalyzer uses the Managed Extensibility Framework (MEF) to import all rules defined in the assembly. It can also consume rules written in PowerShell scripts.

but how do I install/add my custom PowerShell rule into the complete MEF framework?

When calling Invoke-ScriptAnalyzer, users can specify custom rules using the CustomizedRulePath parameter.

but I would like my PowerShell rule to be included automatically together with the standard rules

• I understand that I can add my custom rule to a PSScriptAnalyzer.psd1 settings file but that is specific a folder hierarchy.
  • I might consider to change the C:\Users\...\PowerShell\Modules\PSScriptAnalyzer\1.21.0\PSScriptAnalyzer.psd1PSScriptAnalyzer.psd1 but that will break the signature.
• I might also change the settings in VSCode, but not all of our analyzing happens in VSCode.

(See also the section Here’s how to get this on your system: in the PowerShell Injection Hunter document, are that the minimum steps to install custom rules?)

@jegannathanmaniganadan,

Would it be nice if there is a way to register all (the new design allows me to specify more than one namespace) my custom rule namespace in PSSA, so that it will internally handle all the rules from that space ?

Exactly. In fact I would even expect PSScriptAnalyzer to automatically pickup any (PowerShell) rule that I installed in my environment (through a repository as the PowerShell Gallery) or manually, e.g.:

Import-Module .\AvoidDynamicVariables.psm1

IMO, it should be possible that the command Invoke-AnalyzeScript filters the installed (PowerShell) custom rules from general commands e.g. like:

Get-Command -Verb Measure | Where-Object { $_.OutputType.Name -eq [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord] }

(or additional filters, e.g. sources that start or end with ".Rule", e.g.: .\AvoidDynamicVariables.Rule.psm1.)
And simply includes them when invoked.