From e2674c38c46cfb8da72765c46bba2af5b079b9fe Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 22 Sep 2022 20:51:48 +0100 Subject: [PATCH 1/3] PSAvoidUsingPositionalParameters: Do not warn on AZ CLI --- Rules/AvoidPositionalParameters.cs | 8 ++++++-- Tests/Rules/AvoidPositionalParameters.tests.ps1 | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/Rules/AvoidPositionalParameters.cs b/Rules/AvoidPositionalParameters.cs index d1c552957..d89bd3040 100644 --- a/Rules/AvoidPositionalParameters.cs +++ b/Rules/AvoidPositionalParameters.cs @@ -69,8 +69,12 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) // 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()); + string commandName = cmdAst.GetCommandName(); + // AZ CLI entrypoint became an az.ps1 script in 2.40.0 so do not raise for it since it is still a CLI https://github.com/PowerShell/PSScriptAnalyzer/issues/1845 + if (commandName != "az") { + yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, commandName), + cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, commandName); + } } } } diff --git a/Tests/Rules/AvoidPositionalParameters.tests.ps1 b/Tests/Rules/AvoidPositionalParameters.tests.ps1 index 73fa5d563..d3b1caa8b 100644 --- a/Tests/Rules/AvoidPositionalParameters.tests.ps1 +++ b/Tests/Rules/AvoidPositionalParameters.tests.ps1 @@ -36,6 +36,10 @@ Describe "AvoidPositionalParameters" { It "returns no violations for DSC configuration" { $noViolationsDSC.Count | Should -Be 0 } + + It "returns no violations for AZ CLI" { + Invoke-ScriptAnalyzer -ScriptDefinition 'az group deployment list' | Should -BeNullOrEmpty + } } Context "Function defined and called in script, which has 3 or more positional parameters triggers rule." { From e9dbf52418b9179d649169459634da95f5525278 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 24 Sep 2022 19:56:29 +0100 Subject: [PATCH 2/3] Add CommandAllowList configuration and make it case insensitive --- Rules/AvoidPositionalParameters.cs | 37 ++++++++++++------- .../Rules/AvoidPositionalParameters.tests.ps1 | 17 ++++++++- docs/Rules/AvoidUsingPositionalParameters.md | 21 +++++++++++ 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/Rules/AvoidPositionalParameters.cs b/Rules/AvoidPositionalParameters.cs index d89bd3040..3c6ec9626 100644 --- a/Rules/AvoidPositionalParameters.cs +++ b/Rules/AvoidPositionalParameters.cs @@ -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 @@ -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 + } + /// /// AnalyzeScript: Analyze the ast to check that positional parameters are not used. /// - public IEnumerable AnalyzeScript(Ast ast, string fileName) + public override IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); @@ -57,21 +66,21 @@ public IEnumerable 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 { - string commandName = cmdAst.GetCommandName(); - // AZ CLI entrypoint became an az.ps1 script in 2.40.0 so do not raise for it since it is still a CLI https://github.com/PowerShell/PSScriptAnalyzer/issues/1845 - if (commandName != "az") { + if (!CommandAllowList.Contains(commandName, StringComparer.OrdinalIgnoreCase)) + { yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, commandName), cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, commandName); } @@ -84,7 +93,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// GetName: Retrieves the name of this rule. /// /// The name of this rule - public string GetName() + public override string GetName() { return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidUsingPositionalParametersName); } @@ -93,7 +102,7 @@ public string GetName() /// GetCommonName: Retrieves the common name of this rule. /// /// The common name of this rule - public string GetCommonName() + public override string GetCommonName() { return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersCommonName); } @@ -102,7 +111,7 @@ public string GetCommonName() /// GetDescription: Retrieves the description of this rule. /// /// The description of this rule - public string GetDescription() + public override string GetDescription() { return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersDescription); } @@ -110,7 +119,7 @@ public string GetDescription() /// /// Method: Retrieves the type of the rule: builtin, managed or module. /// - public SourceType GetSourceType() + public override SourceType GetSourceType() { return SourceType.Builtin; } @@ -119,7 +128,7 @@ public SourceType GetSourceType() /// GetSeverity: Retrieves the severity of the rule: error, warning of information. /// /// - public RuleSeverity GetSeverity() + public override RuleSeverity GetSeverity() { return RuleSeverity.Information; } @@ -127,7 +136,7 @@ public RuleSeverity GetSeverity() /// /// Method: Retrieves the module/assembly name the rule is from. /// - public string GetSourceName() + public override string GetSourceName() { return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); } diff --git a/Tests/Rules/AvoidPositionalParameters.tests.ps1 b/Tests/Rules/AvoidPositionalParameters.tests.ps1 index d3b1caa8b..b2291e51c 100644 --- a/Tests/Rules/AvoidPositionalParameters.tests.ps1 +++ b/Tests/Rules/AvoidPositionalParameters.tests.ps1 @@ -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 'az group deployment list' -Settings @{ + IncludeRules = @('PSAvoidUsingPositionalParameters') + Rules = @{ PSAvoidUsingPositionalParameters = @{ CommandAllowList = 'Join-Path' } } + } + $violations.Count | Should -Be 1 # ensure that defaults get removed if setting is supplied + $violations.RuleName | Should -Be 'PSAvoidUsingPositionalParameters' + } } Context "When there are no violations" { @@ -37,9 +45,16 @@ Describe "AvoidPositionalParameters" { $noViolationsDSC.Count | Should -Be 0 } - It "returns no violations for AZ CLI" { + 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." { diff --git a/docs/Rules/AvoidUsingPositionalParameters.md b/docs/Rules/AvoidUsingPositionalParameters.md index 2c92b6e8f..4cecb2965 100644 --- a/docs/Rules/AvoidUsingPositionalParameters.md +++ b/docs/Rules/AvoidUsingPositionalParameters.md @@ -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. From 3a97de8edf5c2b4b5f0c5daf5599fbaccb7ea35b Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 24 Sep 2022 20:31:04 +0100 Subject: [PATCH 3/3] fix failing test on Linux as az is not a script on linux and make test not depend on AZ CLI --- Tests/Rules/AvoidPositionalParameters.tests.ps1 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/Rules/AvoidPositionalParameters.tests.ps1 b/Tests/Rules/AvoidPositionalParameters.tests.ps1 index b2291e51c..ec5a91e24 100644 --- a/Tests/Rules/AvoidPositionalParameters.tests.ps1 +++ b/Tests/Rules/AvoidPositionalParameters.tests.ps1 @@ -27,11 +27,11 @@ Describe "AvoidPositionalParameters" { } It "returns violations for command that is not in allow list of settings" { - $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'az group deployment list' -Settings @{ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Join-Path a b c d' -Settings @{ IncludeRules = @('PSAvoidUsingPositionalParameters') - Rules = @{ PSAvoidUsingPositionalParameters = @{ CommandAllowList = 'Join-Path' } } + Rules = @{ PSAvoidUsingPositionalParameters = @{ CommandAllowList = 'Test-Path' } } } - $violations.Count | Should -Be 1 # ensure that defaults get removed if setting is supplied + $violations.Count | Should -Be 1 $violations.RuleName | Should -Be 'PSAvoidUsingPositionalParameters' } }