From 137b88a8cddf796e609d1e5f2644475636500440 Mon Sep 17 00:00:00 2001 From: Yuting Chen Date: Fri, 24 Apr 2015 14:20:44 -0700 Subject: [PATCH 1/7] Add ChangLog to reflect changes in the development process --- CHANGELOG.MD | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 CHANGELOG.MD diff --git a/CHANGELOG.MD b/CHANGELOG.MD new file mode 100644 index 000000000..6b7c54a72 --- /dev/null +++ b/CHANGELOG.MD @@ -0,0 +1,28 @@ +##Unreleased (Apr.24, 2015) + +###Features: +- Finalized three levels of Severity - Error/Warning/Information. +- Improved PSScriptAnalyzer engine behavior: emits non-terminating errors (Ex: for failed ast parse) and continues rule application when running on multiple scripts. +- Added wild card supports for rules in Invoke-ScriptAnalyzer. Eg. Invoke-ScriptAnalyzer -IncludeRule PSAvoid* will apply all rules starting with PSAvoid* in built in rule assemblies. +- Added -Severity to Get-ScriptAnalyzerRules. Get-ScriptAnalyzer -Severity will filter rules based on the severity given. +- Added Suppression functionality. Users are now able to specify suppression on certain parts of the scripts by specifying "SupressMessageAttribute" in the scripts. More details and documentations will be coming soon in blog posts. Also comes with this feature is the ability for users to display a list of suppressed messages. + +###Rules: + +- Added DSC Rules for resources including Parameter validation, Usage of standard DSC functions and return type validation. Rule checkings also support for DSC classes. Built-in DSC rules include: + + UseStandardDSCFunctionsInResource + + UseIdenticalParametersDSC + + UseIdenticalMandatoryParametersDSC + + ReturnCorrectTypesForDSCFunctions +- Added support in the engine to detect DSC configuration/resource files and disable default rule checkings on DSC configuration and resource files. +- UseShouldProcessForStateChangingFunctions - If an advanced function has Verbs like New/Start/Stop/Restart/Reset/Set- that will change system state, it should support ShouldProcess attribute. +- AvoidUsingWMIObjectCmdlet - For PowerShell 3.0 and above, usage of WMIObject is not recommended. This rule is to detect WMIObject usage in scripts that are written for PS 3.0 and above. + +###Fixes: + +- Improved heuristics to detect usage of Username and Password instead of PSCredential type. +- Improved accuracy in the detection of uninitialized variables. +- Improved error messages to include error line numbers and file names. +- Identified usage of PSBound parameters and PowerShell supplied variables such as $MyInvocation to avoid unnecessary noise in the results returned by some of the built-in rules. +- Fixed terminating errors including "Illegal characters in Path". +- Fixed the issue where display properties in output are not consistent with the object properties. \ No newline at end of file From 5691c9a612f016e7fbeb347a636bc7effd9aa257 Mon Sep 17 00:00:00 2001 From: Yuting Chen Date: Wed, 6 May 2015 15:56:55 -0700 Subject: [PATCH 2/7] Added markdown for AvoidUsingWriteHost rule --- RuleDocumentation/AvoidUsingWriteHost.md | 37 ++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 RuleDocumentation/AvoidUsingWriteHost.md diff --git a/RuleDocumentation/AvoidUsingWriteHost.md b/RuleDocumentation/AvoidUsingWriteHost.md new file mode 100644 index 000000000..7c8e46d74 --- /dev/null +++ b/RuleDocumentation/AvoidUsingWriteHost.md @@ -0,0 +1,37 @@ +#AvoidUsingWriteHost +**Severity Level: Warning** + + +##Description + +It is generally accepted that you should never use Write-Host to create any script output whatsoever, unless your script (or function, or whatever) uses the Show verb (as in, Show-Performance). That verb explicitly means “show on the screen, with no other possibilities.” Like Show-Command. + +##How to Fix + +PTo fix a violation of this rule, please replace Write-Host with Write-Output. + +##Example + +Wrong： + +''' +function Test +{ + ... + Write-Host "Executing.." +} +''' +Correct: +''' +function Test +{ + ... + Write-Output "Executing.." +} + +function Show-Something +{ + Write-Host "show something on screen"; +} + +''' From 9ef7d48ad8589a2c7dc6db67a958ee05f046d4e4 Mon Sep 17 00:00:00 2001 From: Yuting Chen Date: Wed, 6 May 2015 15:58:49 -0700 Subject: [PATCH 3/7] updated examples --- RuleDocumentation/AvoidUsingWriteHost.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/RuleDocumentation/AvoidUsingWriteHost.md b/RuleDocumentation/AvoidUsingWriteHost.md index 7c8e46d74..890e16779 100644 --- a/RuleDocumentation/AvoidUsingWriteHost.md +++ b/RuleDocumentation/AvoidUsingWriteHost.md @@ -14,15 +14,17 @@ PTo fix a violation of this rule, please replace Write-Host with Write-Output. Wrong： -''' +``` function Test { ... Write-Host "Executing.." } -''' +``` + Correct: -''' + +``` function Test { ... @@ -33,5 +35,4 @@ function Show-Something { Write-Host "show something on screen"; } - -''' +``` From e7ee76d0eedf1d4758b095c905796c5f81d68355 Mon Sep 17 00:00:00 2001 From: quoctruong Date: Wed, 6 May 2015 16:25:06 -0700 Subject: [PATCH 4/7] Suppress AvoidUninitializedVariable for variables in the param block of get, set and test targetresource --- .../Commands/InvokeScriptAnalyzerCommand.cs | 90 ++++++++----------- Engine/Helper.cs | 35 ++++++++ Rules/AvoidUninitializedVariable.cs | 21 ++++- .../AvoidGlobalOrUnitializedVars.tests.ps1 | 5 ++ 4 files changed, 94 insertions(+), 57 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index cdfd7bd4e..1baefa989 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -502,68 +502,48 @@ private void AnalyzeFile(string filePath) } // Check if the supplied artifact is indeed part of the DSC resource - // Step 1: Check if the artifact is under the "DSCResources" folder - DirectoryInfo dscResourceParent = Directory.GetParent(filePath); - if (null != dscResourceParent) + if (Helper.Instance.IsDscResourceModule(filePath)) { - DirectoryInfo dscResourcesFolder = Directory.GetParent(dscResourceParent.ToString()); - if (null != dscResourcesFolder) - { - if (String.Equals(dscResourcesFolder.Name, "dscresources",StringComparison.OrdinalIgnoreCase)) + // Run all DSC Rules + foreach (IDSCResourceRule dscResourceRule in ScriptAnalyzer.Instance.DSCResourceRules) + { + bool includeRegexMatch = false; + bool excludeRegexMatch = false; + foreach (Regex include in includeRegexList) + { + if (include.IsMatch(dscResourceRule.GetName())) + { + includeRegexMatch = true; + break; + } + } + foreach (Regex exclude in excludeRegexList) + { + if (exclude.IsMatch(dscResourceRule.GetName())) + { + excludeRegexMatch = true; + } + } + if ((includeRule == null || includeRegexMatch) && (excludeRule == null || !excludeRegexMatch)) { - // Step 2: Ensure there is a Schema.mof in the same folder as the artifact - string schemaMofParentFolder = Directory.GetParent(filePath).ToString(); - string[] schemaMofFile = Directory.GetFiles(schemaMofParentFolder, "*.schema.mof"); + WriteVerbose(string.Format(CultureInfo.CurrentCulture, Strings.VerboseRunningMessage, dscResourceRule.GetName())); - // Ensure Schema file exists and is the only one in the DSCResource folder - if (schemaMofFile != null && schemaMofFile.Count() == 1) + // Ensure that any unhandled errors from Rules are converted to non-terminating errors + // We want the Engine to continue functioning even if one or more Rules throws an exception + try { - // Run DSC Rules only on module that matches the schema.mof file name without extension - if (schemaMofFile[0].Replace("schema.mof", "psm1") == filePath) - { - // Run all DSC Rules - foreach (IDSCResourceRule dscResourceRule in ScriptAnalyzer.Instance.DSCResourceRules) - { - bool includeRegexMatch = false; - bool excludeRegexMatch = false; - foreach (Regex include in includeRegexList) - { - if (include.IsMatch(dscResourceRule.GetName())) - { - includeRegexMatch = true; - break; - } - } - foreach (Regex exclude in excludeRegexList) - { - if (exclude.IsMatch(dscResourceRule.GetName())) - { - excludeRegexMatch = true; - } - } - if ((includeRule == null || includeRegexMatch) && (excludeRule == null || !excludeRegexMatch)) - { - WriteVerbose(string.Format(CultureInfo.CurrentCulture, Strings.VerboseRunningMessage, dscResourceRule.GetName())); - - // Ensure that any unhandled errors from Rules are converted to non-terminating errors - // We want the Engine to continue functioning even if one or more Rules throws an exception - try - { - var records = Helper.Instance.SuppressRule(dscResourceRule.GetName(), ruleSuppressions, dscResourceRule.AnalyzeDSCResource(ast, filePath).ToList()); - diagnostics.AddRange(records.Item2); - suppressed.AddRange(records.Item1); - } - catch (Exception dscResourceRuleException) - { - WriteError(new ErrorRecord(dscResourceRuleException, Strings.RuleErrorMessage, ErrorCategory.InvalidOperation, filePath)); - } - } - } - } + var records = Helper.Instance.SuppressRule(dscResourceRule.GetName(), ruleSuppressions, dscResourceRule.AnalyzeDSCResource(ast, filePath).ToList()); + diagnostics.AddRange(records.Item2); + suppressed.AddRange(records.Item1); + } + catch (Exception dscResourceRuleException) + { + WriteError(new ErrorRecord(dscResourceRuleException, Strings.RuleErrorMessage, ErrorCategory.InvalidOperation, filePath)); } } } - } + + } } #endregion diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 0e59ad803..7433ce8eb 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -156,6 +156,41 @@ public string GetCmdletNameFromAlias(String Alias) return String.Empty; } + /// + /// Given a file path, checks whether the file is part of a dsc resource module + /// + /// + /// + public bool IsDscResourceModule(string filePath) + { + DirectoryInfo dscResourceParent = Directory.GetParent(filePath); + if (null != dscResourceParent) + { + DirectoryInfo dscResourcesFolder = Directory.GetParent(dscResourceParent.ToString()); + if (null != dscResourcesFolder) + { + if (String.Equals(dscResourcesFolder.Name, "dscresources", StringComparison.OrdinalIgnoreCase)) + { + // Step 2: Ensure there is a Schema.mof in the same folder as the artifact + string schemaMofParentFolder = Directory.GetParent(filePath).ToString(); + string[] schemaMofFile = Directory.GetFiles(schemaMofParentFolder, "*.schema.mof"); + + // Ensure Schema file exists and is the only one in the DSCResource folder + if (schemaMofFile != null && schemaMofFile.Count() == 1) + { + // Run DSC Rules only on module that matches the schema.mof file name without extension + if (String.Equals(schemaMofFile[0].Replace("schema.mof", "psm1"), filePath, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + } + } + } + + return false; + } + /// /// Given a commandast, checks whether positional parameters are used or not. /// diff --git a/Rules/AvoidUninitializedVariable.cs b/Rules/AvoidUninitializedVariable.cs index 24fbf08f4..5579a068e 100644 --- a/Rules/AvoidUninitializedVariable.cs +++ b/Rules/AvoidUninitializedVariable.cs @@ -11,6 +11,7 @@ // using System; +using System.Linq; using System.Collections.Generic; using System.Management.Automation.Language; using Microsoft.Windows.Powershell.ScriptAnalyzer.Generic; @@ -51,15 +52,31 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) IEnumerable funcAsts = ast.FindAll(item => item is FunctionDefinitionAst || item is FunctionMemberAst, true); - foreach (var funcAst in funcAsts) + // Checks whether this is a dsc resource file (we don't raise this rule for get, set and test-target resource + bool isDscResourceFile = Helper.Instance.IsDscResourceModule(fileName); + + List targetResourcesFunctions = new List( new string[] { "get-targetresource", "set-targetresource", "test-targetresource" }); + + foreach (FunctionDefinitionAst funcAst in funcAsts) { // Finds all VariableExpressionAst. IEnumerable varAsts = funcAst.FindAll(testAst => testAst is VariableExpressionAst, true); + HashSet paramVariables = new HashSet(); + + if (isDscResourceFile && targetResourcesFunctions.Contains(funcAst.Name, StringComparer.OrdinalIgnoreCase)) + { + // don't raise the rules for variables in the param block. + if (funcAst.Body != null && funcAst.Body.ParamBlock != null && funcAst.Body.ParamBlock.Parameters != null) + { + paramVariables.UnionWith(funcAst.Body.ParamBlock.Parameters.Select(paramAst => paramAst.Name.VariablePath.UserPath)); + } + } + // Iterates all VariableExpressionAst and check the command name. foreach (VariableExpressionAst varAst in varAsts) { - if (Helper.Instance.IsUninitialized(varAst, funcAst)) + if (Helper.Instance.IsUninitialized(varAst, funcAst) && !paramVariables.Contains(varAst.VariablePath.UserPath)) { yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUninitializedVariableError, varAst.VariablePath.UserPath), varAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName, varAst.VariablePath.UserPath); diff --git a/Tests/Rules/AvoidGlobalOrUnitializedVars.tests.ps1 b/Tests/Rules/AvoidGlobalOrUnitializedVars.tests.ps1 index bad960927..0b90d1b71 100644 --- a/Tests/Rules/AvoidGlobalOrUnitializedVars.tests.ps1 +++ b/Tests/Rules/AvoidGlobalOrUnitializedVars.tests.ps1 @@ -5,6 +5,7 @@ $nonInitializedName = "PSAvoidUninitializedVariable" $nonInitializedMessage = "Variable 'a' is not initialized. Non-global variables must be initialized. To fix a violation of this rule, please initialize non-global variables." $directory = Split-Path -Parent $MyInvocation.MyCommand.Path $violations = Invoke-ScriptAnalyzer $directory\AvoidGlobalOrUnitializedVars.ps1 +$dscResourceViolations = Invoke-ScriptAnalyzer $directory\DSCResources\MSFT_WaitForAny\MSFT_WaitForAny.psm1 | Where-Object {$_.RuleName -eq $nonInitializedName} $globalViolations = $violations | Where-Object {$_.RuleName -eq $globalName} $nonInitializedViolations = $violations | Where-Object {$_.RuleName -eq $nonInitializedName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidGlobalOrUnitializedVarsNoViolations.ps1 @@ -17,6 +18,10 @@ Describe "AvoidGlobalVars" { $globalViolations.Count | Should Be 1 } + It "has 4 violations for dsc resources (not counting the variables in parameters)" { + $dscResourceViolations.Count | Should Be 4 + } + It "has the correct description message" { $violations[0].Message | Should Match $globalMessage } From b28f98f51c8e196581e0433f8b03df74edddfe4e Mon Sep 17 00:00:00 2001 From: GoodOlClint Date: Wed, 6 May 2015 21:14:01 -0500 Subject: [PATCH 5/7] Do not require Write-Verbose in functions without the CmdletBinding attribute --- Rules/ProvideVerboseMessage.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Rules/ProvideVerboseMessage.cs b/Rules/ProvideVerboseMessage.cs index a69447c0e..fb4b3f15b 100644 --- a/Rules/ProvideVerboseMessage.cs +++ b/Rules/ProvideVerboseMessage.cs @@ -12,10 +12,12 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Management.Automation.Language; using Microsoft.Windows.Powershell.ScriptAnalyzer.Generic; using System.ComponentModel.Composition; using System.Globalization; +using System.Management.Automation; namespace Microsoft.Windows.Powershell.ScriptAnalyzer.BuiltinRules { @@ -57,6 +59,15 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst fun return AstVisitAction.SkipChildren; } + //Write-Verbose is not required for non-advanced functions + if (funcAst.Body == null || funcAst.Body.ParamBlock == null + || funcAst.Body.ParamBlock.Attributes == null || + funcAst.Body.ParamBlock.Parameters == null || + !funcAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute))) + { + return AstVisitAction.Continue; + } + var commandAsts = funcAst.Body.FindAll(testAst => testAst is CommandAst, false); bool hasVerbose = false; From 2523307bbc76545b099f312c4ffe48fed2cb0866 Mon Sep 17 00:00:00 2001 From: GoodOlClint Date: Wed, 6 May 2015 21:14:52 -0500 Subject: [PATCH 6/7] Updated unit tests for PSProvideVerboseMessage test --- Tests/Rules/GoodCmdlet.ps1 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Tests/Rules/GoodCmdlet.ps1 b/Tests/Rules/GoodCmdlet.ps1 index 7c1dfa240..a294af4df 100644 --- a/Tests/Rules/GoodCmdlet.ps1 +++ b/Tests/Rules/GoodCmdlet.ps1 @@ -81,4 +81,10 @@ function Get-File if ($pscmdlet.ShouldContinue("Yes", "No")) { } } +} + +#Write-Verbose should not be required because this is not an advanced function +function Get-SimpleFunc +{ + } \ No newline at end of file From fce5e415b4a0dbefd353515204179cb4f63c5f0b Mon Sep 17 00:00:00 2001 From: "Yuting Chen[MSFT]" Date: Thu, 7 May 2015 14:26:16 -0700 Subject: [PATCH 7/7] Update CHANGELOG.MD New changelog for the upcoming release --- CHANGELOG.MD | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.MD b/CHANGELOG.MD index 6b7c54a72..1602860be 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -1,9 +1,27 @@ -##Unreleased (Apr.24, 2015) +## Unreleased (May.7, 2015) +###Features: +- Integrated with waffle.io for Project Management. +- Added documentation for writing script rules. + +###Rules: +- AvoidUsingWMICmdlet rule: For PowerShell 3.0 and above, usage of WMI cmdlets is not recommended. This rule is to detect WMI cmdlet usage in scripts that are written for PS 3.0 and above. +- DSCTestsPresent rule: Resource module contains Tests folder with tests for given resource. +- UseOutputTypeCorrectly rule: If we can identify the type of an object that is outputted to the pipeline by a cmdlet, then that type must be listed in the OutputType attribute. + +###Fixes: + +- PSProvideVerboseMessage only throws warnings in non-advanced functions. +- Fix the issue in importing customized rule + + + + +##Relesed on Apr.24, 2015 ###Features: - Finalized three levels of Severity - Error/Warning/Information. - Improved PSScriptAnalyzer engine behavior: emits non-terminating errors (Ex: for failed ast parse) and continues rule application when running on multiple scripts. -- Added wild card supports for rules in Invoke-ScriptAnalyzer. Eg. Invoke-ScriptAnalyzer -IncludeRule PSAvoid* will apply all rules starting with PSAvoid* in built in rule assemblies. +- Added wild card supports for rules in Invoke-ScriptAnalyzer and Get-ScriptAnalyzer. Eg. Invoke-ScriptAnalyzer -IncludeRule PSAvoid* will apply all rules starting with PSAvoid* in built in rule assemblies. - Added -Severity to Get-ScriptAnalyzerRules. Get-ScriptAnalyzer -Severity will filter rules based on the severity given. - Added Suppression functionality. Users are now able to specify suppression on certain parts of the scripts by specifying "SupressMessageAttribute" in the scripts. More details and documentations will be coming soon in blog posts. Also comes with this feature is the ability for users to display a list of suppressed messages. @@ -16,7 +34,7 @@ + ReturnCorrectTypesForDSCFunctions - Added support in the engine to detect DSC configuration/resource files and disable default rule checkings on DSC configuration and resource files. - UseShouldProcessForStateChangingFunctions - If an advanced function has Verbs like New/Start/Stop/Restart/Reset/Set- that will change system state, it should support ShouldProcess attribute. -- AvoidUsingWMIObjectCmdlet - For PowerShell 3.0 and above, usage of WMIObject is not recommended. This rule is to detect WMIObject usage in scripts that are written for PS 3.0 and above. + ###Fixes: @@ -25,4 +43,4 @@ - Improved error messages to include error line numbers and file names. - Identified usage of PSBound parameters and PowerShell supplied variables such as $MyInvocation to avoid unnecessary noise in the results returned by some of the built-in rules. - Fixed terminating errors including "Illegal characters in Path". -- Fixed the issue where display properties in output are not consistent with the object properties. \ No newline at end of file +