Skip to content

PSAvoidUsingPositionalParameters: Do not warn on AZ CLI #1846

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

Merged
merged 3 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 26 additions & 13 deletions Rules/AvoidPositionalParameters.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Management.Automation.Language;
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
using System.Linq;
#if !CORECLR
using System.ComponentModel.Composition;
#endif
Expand All @@ -18,12 +19,20 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
#if !CORECLR
[Export(typeof(IScriptRule))]
#endif
public class AvoidPositionalParameters : IScriptRule
public class AvoidPositionalParameters : ConfigurableRule
{
[ConfigurableRuleProperty(defaultValue: new string[] { "az" })]
public string[] CommandAllowList { get; set; }

public AvoidPositionalParameters()
{
Enable = true; // keep it enabled by default, user can still override this with settings
}

/// <summary>
/// AnalyzeScript: Analyze the ast to check that positional parameters are not used.
/// </summary>
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);

Expand Down Expand Up @@ -57,20 +66,24 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
PipelineAst parent = cmdAst.Parent as PipelineAst;

string commandName = cmdAst.GetCommandName();
if (parent != null && parent.PipelineElements.Count > 1)
{
// raise if it's the first element in pipeline. otherwise no.
if (parent.PipelineElements[0] == cmdAst)
if (parent.PipelineElements[0] == cmdAst && !CommandAllowList.Contains(commandName, StringComparer.OrdinalIgnoreCase))
{
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, cmdAst.GetCommandName()),
cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, cmdAst.GetCommandName());
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, commandName),
cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, commandName);
}
}
// not in pipeline so just raise it normally
else
{
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, cmdAst.GetCommandName()),
cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, cmdAst.GetCommandName());
if (!CommandAllowList.Contains(commandName, StringComparer.OrdinalIgnoreCase))
{
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, commandName),
cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, commandName);
}
}
}
}
Expand All @@ -80,7 +93,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
/// GetName: Retrieves the name of this rule.
/// </summary>
/// <returns>The name of this rule</returns>
public string GetName()
public override string GetName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidUsingPositionalParametersName);
}
Expand All @@ -89,7 +102,7 @@ public string GetName()
/// GetCommonName: Retrieves the common name of this rule.
/// </summary>
/// <returns>The common name of this rule</returns>
public string GetCommonName()
public override string GetCommonName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersCommonName);
}
Expand All @@ -98,15 +111,15 @@ public string GetCommonName()
/// GetDescription: Retrieves the description of this rule.
/// </summary>
/// <returns>The description of this rule</returns>
public string GetDescription()
public override string GetDescription()
{
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersDescription);
}

/// <summary>
/// Method: Retrieves the type of the rule: builtin, managed or module.
/// </summary>
public SourceType GetSourceType()
public override SourceType GetSourceType()
{
return SourceType.Builtin;
}
Expand All @@ -115,15 +128,15 @@ public SourceType GetSourceType()
/// GetSeverity: Retrieves the severity of the rule: error, warning of information.
/// </summary>
/// <returns></returns>
public RuleSeverity GetSeverity()
public override RuleSeverity GetSeverity()
{
return RuleSeverity.Information;
}

/// <summary>
/// Method: Retrieves the module/assembly name the rule is from.
/// </summary>
public string GetSourceName()
public override string GetSourceName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
}
Expand Down
19 changes: 19 additions & 0 deletions Tests/Rules/AvoidPositionalParameters.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ Describe "AvoidPositionalParameters" {
$violations.RuleName | Should -Contain 'PSAvoidUsingCmdletAliases'
}

It "returns violations for command that is not in allow list of settings" {
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Join-Path a b c d' -Settings @{
IncludeRules = @('PSAvoidUsingPositionalParameters')
Rules = @{ PSAvoidUsingPositionalParameters = @{ CommandAllowList = 'Test-Path' } }
}
$violations.Count | Should -Be 1
$violations.RuleName | Should -Be 'PSAvoidUsingPositionalParameters'
}
}

Context "When there are no violations" {
Expand All @@ -36,6 +44,17 @@ Describe "AvoidPositionalParameters" {
It "returns no violations for DSC configuration" {
$noViolationsDSC.Count | Should -Be 0
}

It "returns no violations for AZ CLI by default" {
Invoke-ScriptAnalyzer -ScriptDefinition 'az group deployment list' | Should -BeNullOrEmpty
}

It "returns no violations for command from allow list defined in settings and is case invariant" {
Invoke-ScriptAnalyzer -ScriptDefinition 'join-patH a b c' -Settings @{
IncludeRules = @('PSAvoidUsingPositionalParameters')
Rules = @{ PSAvoidUsingPositionalParameters = @{ CommandAllowList = 'az', 'Join-Path' } }
} | Should -BeNullOrEmpty
}
}

Context "Function defined and called in script, which has 3 or more positional parameters triggers rule." {
Expand Down
21 changes: 21 additions & 0 deletions docs/Rules/AvoidUsingPositionalParameters.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,27 @@ rule from being too noisy, this rule gets only triggered when there are 3 or mor
supplied. A simple example where the risk of using positional parameters is negligible, is
`Test-Path $Path`.

## Configuration

```powershell
Rules = @{
AvoidUsingPositionalParameters = @{
CommandAllowList = 'az', 'Join-Path'
Enable = $true
}
}
```

### Parameters

#### AvoidUsingPositionalParameters: string[] (Default value is 'az')

Commands to be excluded from this rule. `az` is excluded by default because starting with version 2.40.0 the entrypoint of the AZ CLI became an `az.ps1` script but this script does not have any named parameters and just passes them on using `$args` as is to the Python process that it starts, therefore it is still a CLI and not a PowerShell command.

#### Enable: bool (Default value is `$true`)

Enable or disable the rule during ScriptAnalyzer invocation.

## How

Use full parameter names when calling commands.
Expand Down