diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 9efe0e916..c6435833d 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -119,6 +119,12 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) } string cmdletNameIfCommandNameWasAlias = Helper.Instance.GetCmdletNameFromAlias(commandName); + bool isDscResourceDeclaration = IsDscResourceDeclaration(cmdAst); + if (isDscResourceDeclaration && String.IsNullOrEmpty(cmdletNameIfCommandNameWasAlias)) + { + continue; + } + if (!String.IsNullOrEmpty(cmdletNameIfCommandNameWasAlias)) { yield return new DiagnosticRecord( @@ -186,6 +192,207 @@ private bool IgnoreCommandast(CommandAst cmdAst) return false; } + private bool IsDscResourceDeclaration(CommandAst cmdAst) + { + bool hasConfigurationAncestor = false; + for (Ast parent = cmdAst.Parent; parent != null; parent = parent.Parent) + { + if (parent is ConfigurationDefinitionAst) + { + hasConfigurationAncestor = true; + break; + } + + if (parent is FunctionDefinitionAst + || parent is HashtableAst + || parent is AssignmentStatementAst + || parent is ParenExpressionAst + || parent is SubExpressionAst + || parent is ArrayExpressionAst) + { + return false; + } + } + + if (!hasConfigurationAncestor || GetDscResourceBody(cmdAst) == null) + { + return false; + } + + for (Ast parent = cmdAst.Parent; parent != null; parent = parent.Parent) + { + if (parent is ConfigurationDefinitionAst) + { + return true; + } + + if (parent is FunctionDefinitionAst + || parent is HashtableAst + || parent is AssignmentStatementAst + || parent is ParenExpressionAst + || parent is SubExpressionAst + || parent is ArrayExpressionAst) + { + return false; + } + + if (parent is CommandExpressionAst commandExpression + && (!(commandExpression.Expression is ScriptBlockExpressionAst body) + || !IsFollowingLineDscResourceBody(body))) + { + return false; + } + + // Script resource properties contain executable code, not resource declarations. + if (parent is ScriptBlockExpressionAst scriptBlock) + { + if (scriptBlock.Parent is CommandAst enclosingCommand) + { + if (GetDscResourceBody(enclosingCommand) != scriptBlock + || !String.IsNullOrEmpty(Helper.Instance.GetCmdletNameFromAlias(enclosingCommand.GetCommandName())) + || (!String.Equals(enclosingCommand.GetCommandName(), "Node", StringComparison.OrdinalIgnoreCase) + && Helper.Instance.GetCommandInfo(enclosingCommand.GetCommandName()) != null)) + { + return false; + } + } + else if (!(scriptBlock.Parent is ConfigurationDefinitionAst) + && !(scriptBlock.Parent is DynamicKeywordStatementAst) + && !(scriptBlock.Parent is CommandExpressionAst)) + { + return false; + } + } + } + + return false; + } + + private bool IsFollowingLineDscResourceBody(ScriptBlockExpressionAst body) + { + if (!(body.Parent is CommandExpressionAst bodyExpression) + || !(bodyExpression.Parent is PipelineAst bodyPipeline) + || bodyPipeline.PipelineElements.Count != 1 + || bodyPipeline.PipelineElements[0] != bodyExpression) + { + return false; + } + + Ast statement = bodyPipeline; + for (Ast parent = bodyPipeline.Parent; parent != null; parent = parent.Parent) + { + var statements = (parent as NamedBlockAst)?.Statements + ?? (parent as StatementBlockAst)?.Statements; + if (statements == null) + { + statement = parent; + continue; + } + + int index = statements.IndexOf(statement as StatementAst); + if (index <= 0 || !(statements[index - 1] is PipelineAst declarationPipeline) + || declarationPipeline.PipelineElements.Count != 1 + || !(declarationPipeline.PipelineElements[0] is CommandAst declaration)) + { + return false; + } + + string declarationCommandName = declaration.GetCommandName(); + return GetDscResourceBody(declaration) == body + && String.IsNullOrEmpty(Helper.Instance.GetCmdletNameFromAlias(declarationCommandName)) + && (String.Equals(declarationCommandName, "Node", StringComparison.OrdinalIgnoreCase) + || Helper.Instance.GetCommandInfo(declarationCommandName) == null); + } + + return false; + } + + private ScriptBlockExpressionAst GetDscResourceBody(CommandAst cmdAst) + { + if (cmdAst.InvocationOperator != TokenKind.Unknown + || cmdAst.CommandElements.Count < 2 + || !(cmdAst.CommandElements[1] is ExpressionAst) + || cmdAst.CommandElements[1].Extent.Text == "=" + || cmdAst.Redirections.Count != 0) + { + return null; + } + + if (!(cmdAst.Parent is PipelineAst commandPipeline) + || commandPipeline.PipelineElements.Count != 1 + || commandPipeline.PipelineElements[0] != cmdAst +#if CORECLR + || commandPipeline.Background +#endif + || !(commandPipeline.Parent is NamedBlockAst || commandPipeline.Parent is StatementBlockAst)) + { + return null; + } + + if (cmdAst.CommandElements.Count == 3) + { + return cmdAst.CommandElements[2] as ScriptBlockExpressionAst; + } + + // A resource's opening brace can be on the following line. + if (cmdAst.CommandElements.Count == 2 && cmdAst.Parent is PipelineAst pipeline) + { + Ast statement = pipeline; + for (Ast parent = pipeline.Parent; parent != null; parent = parent.Parent) + { + var statements = (parent as NamedBlockAst)?.Statements + ?? (parent as StatementBlockAst)?.Statements; + if (statements == null) + { + statement = parent; + continue; + } + + int index = statements.IndexOf(statement as StatementAst); + if (index >= 0 && index + 1 < statements.Count + && statements[index + 1] is PipelineAst bodyPipeline + && bodyPipeline.PipelineElements.Count == 1 +#if CORECLR + && !bodyPipeline.Background +#endif + && bodyPipeline.PipelineElements[0] is CommandExpressionAst bodyExpression) + { + if (HasStatementSeparator(cmdAst, bodyPipeline) + || bodyExpression.Redirections.Count != 0) + { + return null; + } + + return bodyExpression.Expression as ScriptBlockExpressionAst; + } + + break; + } + } + + return null; + } + + private bool HasStatementSeparator(CommandAst statement, Ast nextStatement) + { + int separatorStart = statement.CommandElements[statement.CommandElements.Count - 1].Extent.EndOffset; + int separatorLength = nextStatement.Extent.StartOffset - separatorStart; + if (separatorLength <= 0) + { + return false; + } + + string script = statement.Extent.EndScriptPosition.GetFullScript(); + string separator = script.Substring(separatorStart, separatorLength); + if (separator.IndexOf(';') < 0 && separator.IndexOf('&') < 0) + { + return false; + } + + Parser.ParseInput(separator, out Token[] tokens, out ParseError[] errors); + return tokens.Any(token => token.Kind == TokenKind.Semi || token.Kind == TokenKind.Ampersand); + } + /// /// For a command like "gci -path c:", returns the extent of "gci" in the command /// diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 00db269a0..814574525 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -33,6 +33,19 @@ Describe "AvoidUsingAlias" { $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'verb' | Where-Object { $_.RuleName -eq $violationName } $violations.Count | Should -Be 1 } + + It "still expands aliases with resource-shaped arguments" { + $target = 'icm Server01 { Get-Process }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName + $violations.Count | Should -Be 1 + $violations[0].SuggestedCorrections[0].Text | Should -Be 'Invoke-Command' + } + + It "still adds the Get prefix to ordinary commands with resource-shaped arguments" { + $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Verb Argument {}' -IncludeRule $violationName + $violations.Count | Should -Be 1 + $violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-Verb' + } } Context "Violation Extent" { @@ -65,6 +78,215 @@ Configuration MyDscConfiguration { Get-Count | ` Should -Be 0 } + + It "does not expand DSC resource names during formatting" -skip:($IsLinux -or $IsMacOS) { + $target = @' +Configuration PackageTest { + Package PackageExample + { + Name = "TestPackage" + } +} +'@ + $settings = @{ Rules = @{ PSAvoidUsingCmdletAliases = @{ Enable = $true } } } + Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName | + Where-Object { $_.RuleName -eq $violationName } | + Get-Count | Should -Be 0 + Invoke-Formatter -ScriptDefinition $target -Settings $settings | Should -Be $target + } + } + + Context "DSC resource ASTs" { + It "does not expand nested resource names in a same-line Node declaration when node is discoverable" -skip:(!(Get-Command node -CommandType Application -ErrorAction Ignore)) { + $target = @' +{ + Node localhost { Package PackageExample {} } +} +'@ + $ast = [System.Management.Automation.Language.Parser]::ParseInput($target, [ref]$null, [ref]$null) + $body = $ast.EndBlock.Statements[0].PipelineElements[0].Expression.Copy() + $configuration = [System.Management.Automation.Language.ConfigurationDefinitionAst]::new( + $ast.Extent, $body, 'Resource', + [System.Management.Automation.Language.StringConstantExpressionAst]::new($ast.Extent, 'Test', 'BareWord')) + $rule = [Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules.AvoidAlias]::new() + + @($rule.AnalyzeScript($configuration, $null)).Count | Should -Be 0 + } + + It "ignores resource declarations but not executable commands" { + $target = @' +{ + Verb VerbExample {} + Verb NextLineExample + { + } + gci ResourceAliasExample + { + Name = "Example" + } + icm Server01 { Verb Argument {} } + & Verb Argument {} + function Get-Example { + Verb Argument {} + } + Node localhost + { + Verb NestedExample {} + Script ScriptExample { + SetScript = { + icm Server01 {} + Verb Argument {} + } + } + } + $scriptProperties = @{ + SetScript = { Verb Argument {} } + } +} +'@ + $ast = [System.Management.Automation.Language.Parser]::ParseInput($target, [ref]$null, [ref]$null) + $body = $ast.EndBlock.Statements[0].PipelineElements[0].Expression.Copy() + $configuration = [System.Management.Automation.Language.ConfigurationDefinitionAst]::new( + $ast.Extent, $body, 'Resource', + [System.Management.Automation.Language.StringConstantExpressionAst]::new($ast.Extent, 'Test', 'BareWord')) + $rule = [Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules.AvoidAlias]::new() + + $violations = @($rule.AnalyzeScript($configuration, $null)) + + $violations.Count | Should -Be 8 + $violations[0].Extent.Text | Should -Be 'gci' + $violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-ChildItem' + $violations[1].Extent.Text | Should -Be 'icm' + $violations[2].Extent.Text | Should -Be 'Verb' + $violations[3].Extent.Text | Should -Be 'Verb' + $violations[4].Extent.Text | Should -Be 'Verb' + $violations[5].Extent.Text | Should -Be 'icm' + $violations[6].Extent.Text | Should -Be 'Verb' + $violations[7].Extent.Text | Should -Be 'Verb' + } + + It "does not ignore executable commands in assignment and expression contexts or pipelines" { + $target = @' +{ + $items = ChildItem . {} + $result = Verb Argument {} + $action = { Verb Argument {} } + Name = (Verb Argument {}) + Script ScriptExample { + SetScript = + { + Verb Argument {} + } + } + Verb Argument {} | Out-Null + gci . | Out-Null + { + } +} +'@ + $ast = [System.Management.Automation.Language.Parser]::ParseInput($target, [ref]$null, [ref]$null) + $body = $ast.EndBlock.Statements[0].PipelineElements[0].Expression.Copy() + $configuration = [System.Management.Automation.Language.ConfigurationDefinitionAst]::new( + $ast.Extent, $body, 'Resource', + [System.Management.Automation.Language.StringConstantExpressionAst]::new($ast.Extent, 'Test', 'BareWord')) + $rule = [Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules.AvoidAlias]::new() + + $violations = @($rule.AnalyzeScript($configuration, $null)) + + $violations.Count | Should -Be 7 + $violations[0].Extent.Text | Should -Be 'ChildItem' + $violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-ChildItem' + $violations[1].Extent.Text | Should -Be 'Verb' + $violations[2].Extent.Text | Should -Be 'Verb' + $violations[3].Extent.Text | Should -Be 'Verb' + $violations[4].Extent.Text | Should -Be 'Verb' + $violations[5].Extent.Text | Should -Be 'Verb' + $violations[6].Extent.Text | Should -Be 'gci' + } + + It "does not mistake executable pipelines for DSC resources in fallback configuration ASTs" { + $target = @' +{ + Write-Output $(Verb Argument {}) + @(Verb Argument {}) + Verb Argument {} > $null + Verb Argument {} & + if (Verb Argument {}) {} + return Verb Argument {} + if ($true) { + Verb SameLineResource {} + Verb NextLineResource + { + } + } +} +'@ + $ast = [System.Management.Automation.Language.Parser]::ParseInput($target, [ref]$null, [ref]$null) + $body = $ast.EndBlock.Statements[0].PipelineElements[0].Expression.Copy() + $configuration = [System.Management.Automation.Language.ConfigurationDefinitionAst]::new( + $ast.Extent, $body, 'Resource', + [System.Management.Automation.Language.StringConstantExpressionAst]::new($ast.Extent, 'Test', 'BareWord')) + $rule = [Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules.AvoidAlias]::new() + + $violations = @($rule.AnalyzeScript($configuration, $null)) + + $expectedViolations = if ($PSVersionTable.PSVersion.Major -ge 7) { 6 } else { 5 } + $violations.Count | Should -Be $expectedViolations + $violations | ForEach-Object { + $_.SuggestedCorrections[0].Text | Should -Be 'Get-Verb' + } + } + + It "does not mistake following-line blocks after executable commands for DSC resources" { + $target = @' +{ + Write-Output value + { + Verb Argument {} + } +} +'@ + $ast = [System.Management.Automation.Language.Parser]::ParseInput($target, [ref]$null, [ref]$null) + $body = $ast.EndBlock.Statements[0].PipelineElements[0].Expression.Copy() + $configuration = [System.Management.Automation.Language.ConfigurationDefinitionAst]::new( + $ast.Extent, $body, 'Resource', + [System.Management.Automation.Language.StringConstantExpressionAst]::new($ast.Extent, 'Test', 'BareWord')) + $rule = [Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules.AvoidAlias]::new() + + $violations = @($rule.AnalyzeScript($configuration, $null)) + + $violations.Count | Should -Be 1 + $violations[0].Extent.Text | Should -Be 'Verb' + $violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-Verb' + } + + It "does not pair following-line blocks with commands when the block has redirections or a statement separator intervenes" { + $target = @' +{ + gci . + {} > $null + gci .; {} + gci . & + {} + gci . + {} & +} +'@ + $ast = [System.Management.Automation.Language.Parser]::ParseInput($target, [ref]$null, [ref]$null) + $body = $ast.EndBlock.Statements[0].PipelineElements[0].Expression.Copy() + $configuration = [System.Management.Automation.Language.ConfigurationDefinitionAst]::new( + $ast.Extent, $body, 'Resource', + [System.Management.Automation.Language.StringConstantExpressionAst]::new($ast.Extent, 'Test', 'BareWord')) + $rule = [Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules.AvoidAlias]::new() + + $violations = @($rule.AnalyzeScript($configuration, $null)) + + $violations.Count | Should -Be 4 + $violations | ForEach-Object { + $_.Extent.Text | Should -Be 'gci' + $_.SuggestedCorrections[0].Text | Should -Be 'Get-ChildItem' + } + } } Context "Settings file provides allowlist" {