diff --git a/RuleDocumentation/UseManifestExportFields.md b/RuleDocumentation/UseManifestExportFields.md new file mode 100644 index 000000000..e69de29bb diff --git a/RuleDocumentation/UseToExportFieldsInManifest.md b/RuleDocumentation/UseToExportFieldsInManifest.md new file mode 100644 index 000000000..11ae40353 --- /dev/null +++ b/RuleDocumentation/UseToExportFieldsInManifest.md @@ -0,0 +1,28 @@ +#UseToExportFieldsInManifest +**Severity Level: Warning** + + +##Description + +In a module manifest, AliasesToExport, CmdletsToExport, FunctionsToExport and VariablesToExport fields should not use wildcards or $null in their entries. During module auto-discovery, if any of these entries are missing or $null or wildcard, PowerShell does some potentially expensive work to analyze the rest of the module. + +##How to Fix + +Please consider using an explicit list. + +##Example 1 + +Wrong: + FunctionsToExport = $null + +Correct: + FunctionToExport = @() + +##Example 2 +Suppose there are only two functions in your module, Get-Foo and Set-Foo that you want to export. Then, + +Wrong: + FunctionsToExport = '*' + +Correct: + FunctionToExport = @(Get-Foo, Set-Foo) \ No newline at end of file diff --git a/Rules/ScriptAnalyzerBuiltinRules.csproj b/Rules/ScriptAnalyzerBuiltinRules.csproj index 3862c8b8f..9138a7e33 100644 --- a/Rules/ScriptAnalyzerBuiltinRules.csproj +++ b/Rules/ScriptAnalyzerBuiltinRules.csproj @@ -95,6 +95,7 @@ Strings.resx + diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index f9679f1b5..aae951eeb 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1995,6 +1995,42 @@ internal static string UseStandardDSCFunctionsInResourceName { } } + /// + /// Looks up a localized string similar to Use the *ToExport module manifest fields.. + /// + internal static string UseToExportFieldsInManifestCommonName { + get { + return ResourceManager.GetString("UseToExportFieldsInManifestCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to In a module manifest, AliasesToExport, CmdletsToExport, FunctionsToExport and VariablesToExport fields should not use wildcards or $null in their entries. During module auto-discovery, if any of these entries are missing or $null or wildcard, PowerShell does some potentially expensive work to analyze the rest of the module.. + /// + internal static string UseToExportFieldsInManifestDescription { + get { + return ResourceManager.GetString("UseToExportFieldsInManifestDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Do not use wildcard or $null in this field. Explicitly specify a list for {0}. . + /// + internal static string UseToExportFieldsInManifestError { + get { + return ResourceManager.GetString("UseToExportFieldsInManifestError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to UseToExportFieldsInManifest. + /// + internal static string UseToExportFieldsInManifestName { + get { + return ResourceManager.GetString("UseToExportFieldsInManifestName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Use Type At Variable Assignment. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 0b2fad980..233f99a5e 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -798,4 +798,16 @@ AvoidNullOrEmptyHelpMessageAttribute + + Use the *ToExport module manifest fields. + + + In a module manifest, AliasesToExport, CmdletsToExport, FunctionsToExport and VariablesToExport fields should not use wildcards or $null in their entries. During module auto-discovery, if any of these entries are missing or $null or wildcard, PowerShell does some potentially expensive work to analyze the rest of the module. + + + Do not use wildcard or $null in this field. Explicitly specify a list for {0}. + + + UseToExportFieldsInManifest + \ No newline at end of file diff --git a/Rules/UseToExportFieldsInManifest.cs b/Rules/UseToExportFieldsInManifest.cs new file mode 100644 index 000000000..f7623c085 --- /dev/null +++ b/Rules/UseToExportFieldsInManifest.cs @@ -0,0 +1,183 @@ +// +// Copyright (c) Microsoft Corporation. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// + +using System; +using System.Collections.Generic; +using System.Management.Automation.Language; +using System.Management.Automation; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using System.ComponentModel.Composition; +using System.Globalization; +using System.Text.RegularExpressions; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ + /// + /// UseToExportFieldsInManifest: Checks if AliasToExport, CmdletsToExport, FunctionsToExport and VariablesToExport + /// fields do not use wildcards and $null in their entries. + /// + [Export(typeof(IScriptRule))] + public class UseToExportFieldsInManifest : IScriptRule + { + /// + /// AnalyzeScript: Analyzes the AST to check if AliasToExport, CmdletsToExport, FunctionsToExport + /// and VariablesToExport fields do not use wildcards and $null in their entries. + /// + /// The script's ast + /// The script's file name + /// A List of diagnostic results of this rule + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException(Strings.NullAstErrorMessage); + } + + if (fileName == null || !fileName.EndsWith(".psd1", StringComparison.OrdinalIgnoreCase)) + { + yield break; + } + + if (!IsValidManifest(ast, fileName)) + { + yield break; + } + + String[] manifestFields = {"FunctionsToExport", "CmdletsToExport", "VariablesToExport", "AliasesToExport"}; + var hashtableAst = ast.Find(x => x is HashtableAst, false) as HashtableAst; + + if (hashtableAst == null) + { + yield break; + } + + foreach(String field in manifestFields) + { + IScriptExtent extent; + if (!HasAcceptableExportField(field, hashtableAst, ast.Extent.Text, out extent) && extent != null) + { + yield return new DiagnosticRecord(GetError(field), extent, GetName(), DiagnosticSeverity.Warning, fileName); + } + } + + } + + /// + /// Checks if the manifest file is valid. + /// + /// + /// + /// A boolean value indicating the validity of the manifest file. + private bool IsValidManifest(Ast ast, string fileName) + { + var missingManifestRule = new MissingModuleManifestField(); + return !missingManifestRule.AnalyzeScript(ast, fileName).GetEnumerator().MoveNext(); + + } + + /// + /// Checks if the *ToExport fields are explicitly set to arrays, eg. @(...), and the array entries do not contain any wildcard. + /// + /// + /// + /// + /// + /// A boolean value indicating if the the ToExport fields are explicitly set to arrays or not. + private bool HasAcceptableExportField(string key, HashtableAst hast, string scriptText, out IScriptExtent extent) + { + extent = null; + foreach (var pair in hast.KeyValuePairs) + { + if (key.Equals(pair.Item1.Extent.Text.Trim(), StringComparison.OrdinalIgnoreCase)) + { + // checks if the right hand side of the assignment is an array. + var arrayAst = pair.Item2.Find(x => x is ArrayLiteralAst || x is ArrayExpressionAst, true); + if (arrayAst == null) + { + extent = pair.Item2.Extent; + return false; + } + else + { + //checks if any entry within the array has a wildcard. + var elementWithWildcard = arrayAst.Find(x => x is StringConstantExpressionAst + && x.Extent.Text.Contains("*"), false); + if (elementWithWildcard != null) + { + extent = elementWithWildcard.Extent; + return false; + } + return true; + } + } + } + return true; + } + + public string GetError(string field) + { + return string.Format(CultureInfo.CurrentCulture, Strings.UseToExportFieldsInManifestError, field); + } + + /// + /// GetName: Retrieves the name of this rule. + /// + /// The name of this rule + public string GetName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseToExportFieldsInManifestName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return String.Format(CultureInfo.CurrentCulture, Strings.UseToExportFieldsInManifestCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return String.Format(CultureInfo.CurrentCulture, Strings.UseToExportFieldsInManifestDescription); + } + + /// + /// Method: Retrieves the type of the rule: builtin, managed or module. + /// + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + + /// + /// GetSeverity: Retrieves the severity of the rule: error, warning of information. + /// + /// + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// Method: Retrieves the module/assembly name the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + } +} diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index e9bb657f5..ef2cefc31 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -56,7 +56,7 @@ Describe "Test Name parameters" { It "Get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $defaultRules.Count | Should be 40 + $defaultRules.Count | Should be 41 } } diff --git a/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 b/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 new file mode 100644 index 000000000..bc7994035 Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadAll.psd1 b/Tests/Rules/TestManifest/ManifestBadAll.psd1 new file mode 100644 index 000000000..eeed4260d Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestBadAll.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadCmdletsWildcard.psd1 b/Tests/Rules/TestManifest/ManifestBadCmdletsWildcard.psd1 new file mode 100644 index 000000000..3782c9e7b Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestBadCmdletsWildcard.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psd1 b/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psd1 new file mode 100644 index 000000000..c8fc4b534 Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psd1 b/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psd1 new file mode 100644 index 000000000..3c253844d Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadFunctionsWildcardInArray.psd1 b/Tests/Rules/TestManifest/ManifestBadFunctionsWildcardInArray.psd1 new file mode 100644 index 000000000..acb43a6e9 Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestBadFunctionsWildcardInArray.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadVariablesWildcard.psd1 b/Tests/Rules/TestManifest/ManifestBadVariablesWildcard.psd1 new file mode 100644 index 000000000..946701bc3 Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestBadVariablesWildcard.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestGood.psd1 b/Tests/Rules/TestManifest/ManifestGood.psd1 new file mode 100644 index 000000000..cd360aed1 Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestGood.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestInvalid.psd1 b/Tests/Rules/TestManifest/ManifestInvalid.psd1 new file mode 100644 index 000000000..6a1d32571 Binary files /dev/null and b/Tests/Rules/TestManifest/ManifestInvalid.psd1 differ diff --git a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 new file mode 100644 index 000000000..50255b5dc --- /dev/null +++ b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 @@ -0,0 +1,91 @@ +Import-Module PSScriptAnalyzer +$directory = Split-Path -Parent $MyInvocation.MyCommand.Path +$testManifestPath = Join-Path $directory "TestManifest" +$testManifestBadFunctionsWildcardPath = "ManifestBadFunctionsWildcard.psd1" +$testManifestBadFunctionsWildcardInArrayPath = "ManifestBadFunctionsWildcardInArray.psd1" +$testManifestBadFunctionsNullPath = "ManifestBadFunctionsNull.psd1" +$testManifestBadCmdletsWildcardPath = "ManifestBadCmdletsWildcard.psd1" +$testManifestBadAliasesWildcardPath = "ManifestBadAliasesWildcard.psd1" +$testManifestBadVariablesWildcardPath = "ManifestBadVariablesWildcard.psd1" +$testManifestBadAllPath = "ManifestBadAll.psd1" +$testManifestGoodPath = "ManifestGood.psd1" +$testManifestInvalidPath = "ManifestInvalid.psd1" + +Function Run-PSScriptAnalyzerRule +{ + Param( + [Parameter(Mandatory)] + [String] $ManifestPath + ) + + Invoke-ScriptAnalyzer -Path (Resolve-Path (Join-Path $testManifestPath $ManifestPath))` + -IncludeRule PSUseToExportFieldsInManifest +} + +Describe "UseManifestExportFields" { + + Context "Invalid manifest file" { + It "does not process the manifest" { + $results = Run-PSScriptAnalyzerRule $testManifestInvalidPath + $results | Should BeNullOrEmpty + } + } + + Context "Manifest contains violations" { + + It "detects FunctionsToExport with wildcard" { + $results = Run-PSScriptAnalyzerRule $testManifestBadFunctionsWildcardPath + $results.Count | Should be 1 + $results[0].Extent.Text | Should be "'*'" + } + + It "detects FunctionsToExport with null" { + $results = Run-PSScriptAnalyzerRule $testManifestBadFunctionsNullPath + $results.Count | Should be 1 + $results[0].Extent.Text | Should be '$null' + } + + It "detects array element containing wildcard" { + $results = Run-PSScriptAnalyzerRule $testManifestBadFunctionsWildcardInArrayPath + $results.Count | Should be 3 + $results.Where({$_.Message -match "FunctionsToExport"}).Extent.Text | Should be "'Get-*'" + $results.Where({$_.Message -match "CmdletsToExport"}).Extent.Text | Should be "'Update-*'" + + # if more than two elements contain wildcard we can show only the first one as of now. + $results.Where({$_.Message -match "VariablesToExport"}).Extent.Text | Should be "'foo*'" + } + + + It "detects CmdletsToExport with wildcard" { + $results = Run-PSScriptAnalyzerRule $testManifestBadCmdletsWildcardPath + $results.Count | Should be 1 + $results[0].Extent.Text | Should be "'*'" + } + + It "detects AliasesToExport with wildcard" { + $results = Run-PSScriptAnalyzerRule $testManifestBadAliasesWildcardPath + $results.Count | Should be 1 + $results[0].Extent.Text | Should be "'*'" + } + + It "detects VariablesToExport with wildcard" { + $results = Run-PSScriptAnalyzerRule $testManifestBadVariablesWildcardPath + $results.Count | Should be 1 + $results[0].Extent.Text | Should be "'*'" + } + + It "detects all the *ToExport violations" { + $results = Run-PSScriptAnalyzerRule $testManifestBadAllPath + $results.Count | Should be 4 + } + } + + Context "Manifest contains no violations" { + It "detects all the *ToExport fields explicitly stating lists" { + $results = Run-PSScriptAnalyzerRule $testManifestGoodPath + $results.Count | Should be 0 + } + } +} + +