diff --git a/Engine/Generic/CorrectionExtent.cs b/Engine/Generic/CorrectionExtent.cs new file mode 100644 index 000000000..0974bea74 --- /dev/null +++ b/Engine/Generic/CorrectionExtent.cs @@ -0,0 +1,144 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Management.Automation.Language; +using System.Text; +using System.Threading.Tasks; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic +{ + public class CorrectionExtent + { + public int EndColumnNumber + { + get + { + return endColumnNumber; + } + } + + public int EndLineNumber + { + get + { + return endLineNumber; + } + } + + public string File + { + get + { + return file; + } + } + + public int StartColumnNumber + { + get + { + return startColumnNumber; + } + } + + public int StartLineNumber + { + get + { + return startLineNumber; + } + } + + public string Text + { + get + { + return text; + } + } + + public string Description + { + get + { + return description; + } + } + + private string file; + private int startLineNumber; + private int endLineNumber; + private int startColumnNumber; + private int endColumnNumber; + private string text; + private string description; + + public CorrectionExtent( + int startLineNumber, + int endLineNumber, + int startColumnNumber, + int endColumnNumber, + string text, + string file) + : this( + startLineNumber, + endLineNumber, + startColumnNumber, + endColumnNumber, + text, + file, + null) + { + } + + public CorrectionExtent( + int startLineNumber, + int endLineNumber, + int startColumnNumber, + int endColumnNumber, + string text, + string file, + string description) + { + this.startLineNumber = startLineNumber; + this.endLineNumber = endLineNumber; + this.startColumnNumber = startColumnNumber; + this.endColumnNumber = endColumnNumber; + this.file = file; + this.text = text; + this.description = description; + ThrowIfInvalidArguments(); + } + + + + private void ThrowIfInvalidArguments() + { + ThrowIfNull(file, "filename"); + ThrowIfNull(text, "text"); + ThrowIfDecreasing(startLineNumber, endLineNumber, "start line number cannot be less than end line number"); + if (startLineNumber == endLineNumber) + { + ThrowIfDecreasing(StartColumnNumber, endColumnNumber, "start column number cannot be less than end column number for a one line extent"); + } + } + + private void ThrowIfDecreasing(int start, int end, string message) + { + if (start > end) + { + throw new ArgumentException(message); + } + } + + private void ThrowIfNull(T arg, string argName) + { + if (arg == null) + { + throw new ArgumentNullException(argName); + } + } + + } +} diff --git a/Engine/Generic/DiagnosticRecord.cs b/Engine/Generic/DiagnosticRecord.cs index 5425d9d9a..a8487a94a 100644 --- a/Engine/Generic/DiagnosticRecord.cs +++ b/Engine/Generic/DiagnosticRecord.cs @@ -10,6 +10,7 @@ // THE SOFTWARE. // +using System.Collections.Generic; using System.Management.Automation.Language; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic @@ -26,6 +27,7 @@ public class DiagnosticRecord private DiagnosticSeverity severity; private string scriptName; private string ruleSuppressionId; + private List suggestedCorrections; /// /// Represents a string from the rule about why this diagnostic was created. @@ -91,6 +93,15 @@ public string RuleSuppressionID set { ruleSuppressionId = value; } } + /// + /// Returns suggested correction + /// return value can be null + /// + public IEnumerable SuggestedCorrections + { + get { return suggestedCorrections; } + } + /// /// DiagnosticRecord: The constructor for DiagnosticRecord class. /// @@ -98,23 +109,25 @@ public DiagnosticRecord() { } - + /// - /// DiagnosticRecord: The constructor for DiagnosticRecord class. + /// DiagnosticRecord: The constructor for DiagnosticRecord class that takes in suggestedCorrection /// /// A string about why this diagnostic was created /// The place in the script this diagnostic refers to /// The name of the rule that created this diagnostic /// The severity of this diagnostic /// The name of the script file being analyzed - public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, DiagnosticSeverity severity, string scriptName, string ruleId = null) + /// The correction suggested by the rule to replace the extent text + public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, DiagnosticSeverity severity, string scriptName, string ruleId = null, List suggestedCorrections = null) { - Message = string.IsNullOrEmpty(message) ? string.Empty : message; + Message = string.IsNullOrEmpty(message) ? string.Empty : message; RuleName = string.IsNullOrEmpty(ruleName) ? string.Empty : ruleName; - Extent = extent; + Extent = extent; Severity = severity; ScriptName = string.IsNullOrEmpty(scriptName) ? string.Empty : scriptName; ruleSuppressionId = ruleId; + this.suggestedCorrections = suggestedCorrections; } } diff --git a/Engine/Helper.cs b/Engine/Helper.cs index a15e5166b..0b6fbecb2 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -19,6 +19,7 @@ using System.Management.Automation.Language; using System.Globalization; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using System.Management.Automation.Runspaces; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -104,7 +105,6 @@ internal set private string[] functionScopes = new string[] { "global:", "local:", "script:", "private:"}; private string[] variableScopes = new string[] { "global:", "local:", "script:", "private:", "variable:", ":"}; - #endregion /// @@ -112,6 +112,7 @@ internal set /// private Helper() { + } /// @@ -229,6 +230,61 @@ public bool IsDscResourceModule(string filePath) return false; } + + /// + /// Gets the module manifest + /// + /// + /// + /// Returns a object of type PSModuleInfo + public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable errorRecord) + { + errorRecord = null; + PSModuleInfo psModuleInfo = null; + Collection psObj = null; + var ps = System.Management.Automation.PowerShell.Create(); + try + { + ps.AddCommand("Test-ModuleManifest"); + ps.AddParameter("Path", filePath); + ps.AddParameter("WarningAction", ActionPreference.SilentlyContinue); + psObj = ps.Invoke(); + } + catch (CmdletInvocationException e) + { + // Invoking Test-ModuleManifest on a module manifest that doesn't have all the valid keys + // throws a NullReferenceException. This is probably a bug in Test-ModuleManifest and hence + // we consume it to allow execution of the of this method. + if (e.InnerException == null || e.InnerException.GetType() != typeof(System.NullReferenceException)) + { + throw; + } + } + if (ps.HadErrors && ps.Streams != null && ps.Streams.Error != null) + { + var errorRecordArr = new ErrorRecord[ps.Streams.Error.Count]; + ps.Streams.Error.CopyTo(errorRecordArr, 0); + errorRecord = errorRecordArr; + } + if (psObj != null && psObj.Any() && psObj[0] != null) + { + psModuleInfo = psObj[0].ImmediateBaseObject as PSModuleInfo; + } + ps.Dispose(); + return psModuleInfo; + } + + /// + /// Checks if the error record is MissingMemberException + /// + /// + /// Returns a boolean value indicating the presence of MissingMemberException + public static bool IsMissingManifestMemberException(ErrorRecord errorRecord) + { + return errorRecord.CategoryInfo != null + && errorRecord.CategoryInfo.Category == ErrorCategory.ResourceUnavailable + && string.Equals("MissingMemberException", errorRecord.CategoryInfo.Reason, StringComparison.OrdinalIgnoreCase); + } /// /// Get the list of exported function by analyzing the ast @@ -1317,8 +1373,7 @@ public static string[] ProcessCustomRulePaths(string[] rulePaths, SessionState s } - - #endregion + #endregion Methods } diff --git a/Engine/ScriptAnalyzerEngine.csproj b/Engine/ScriptAnalyzerEngine.csproj index 368214b56..84442e131 100644 --- a/Engine/ScriptAnalyzerEngine.csproj +++ b/Engine/ScriptAnalyzerEngine.csproj @@ -34,23 +34,23 @@ Microsoft.Windows.PowerShell.ScriptAnalyzer - true - bin\PSV3 Debug\ - TRACE;DEBUG;PSV3 - full - AnyCPU - prompt - MinimumRecommendedRules.ruleset - - - bin\PSV3 Release\ - TRACE;PSV3 - true - pdbonly - AnyCPU - prompt - MinimumRecommendedRules.ruleset - + true + bin\PSV3 Debug\ + TRACE;DEBUG;PSV3 + full + AnyCPU + prompt + MinimumRecommendedRules.ruleset + + + bin\PSV3 Release\ + TRACE;PSV3 + true + pdbonly + AnyCPU + prompt + MinimumRecommendedRules.ruleset + @@ -70,6 +70,7 @@ + @@ -101,7 +102,7 @@ - + diff --git a/Engine/about_PSScriptAnalyzer.help.txt b/Engine/about_PSScriptAnalyzer.help.txt index 43bf6e88c..63a57a1a8 100644 --- a/Engine/about_PSScriptAnalyzer.help.txt +++ b/Engine/about_PSScriptAnalyzer.help.txt @@ -183,6 +183,47 @@ RULE SUPPRESSSION Target="*")] param() } + +VIOLATION CORRECTION + +Most violations can be fixed by replacing the violation causing content with the correct alternative. In an attempt to provide the user with the ability to correct the violation we provide a property - `SuggestedCorrections`, in each DiagnosticRecord instance. This property contains the information needed to rectify the violation. For example, consider a script `C:\tmp\test.ps1` with the following content. + +PS> Get-Content C:\tmp\test.ps1 +gci C:\ + +Invoking PSScriptAnalyzer on the file gives the following output. + +PS>$diagnosticRecord = Invoke-ScriptAnalyzer -Path C:\tmp\test.p1 +PS>$diagnosticRecord | select SuggestedCorrections | Format-Custom + +class DiagnosticRecord +{ + SuggestedCorrections = + [ + class CorrectionExtent + { + EndColumnNumber = 4 + EndLineNumber = 1 + File = C:\Users\kabawany\tmp\test3.ps1 + StartColumnNumber = 1 + StartLineNumber = 1 + Text = Get-ChildItem + Description = Replace gci with Get-ChildItem + } + ] + +} + +The *LineNumber and *ColumnNumber properties give the region of the script that can be replaced by the contents of Text property, i.e., replace gci with Get-ChildItem. + +The main motivation behind having SuggestedCorrections is to enable quick-fix like scenarios in editors like VSCode, Sublime, etc. At present, we provide valid SuggestedCorrection only for the following rules, while gradually adding this feature to more rules. + + * AvoidAlias.cs + * AvoidUsingPlainTextForPassword.cs + * MisleadingBacktick.cs + * MissingModuleManifestField.cs + * UseToExportFieldsInManifest.cs + EXTENSIBILITY diff --git a/README.md b/README.md index df8d7587d..47cbe380f 100644 --- a/README.md +++ b/README.md @@ -215,6 +215,50 @@ public System.Collections.Generic.IEnumerable GetRule(string[] moduleName string[] ruleNames) ``` +Violation Correction +==================== + +Most violations can be fixed by replacing the violation causing content with the correct alternative. In an attempt to provide the user with the ability to correct the violation we provide a property - `SuggestedCorrections`, in each DiagnosticRecord instance. This property contains the information needed to rectify the violation. For example, consider a script `C:\tmp\test.ps1` with the following content. + +```powershell +PS> Get-Content C:\tmp\test.ps1 +gci C:\ +``` + +Invoking PSScriptAnalyzer on the file gives the following output. + +```powershell +PS>$diagnosticRecord = Invoke-ScriptAnalyzer -Path C:\tmp\test.p1 +PS>$diagnosticRecord | select SuggestedCorrections | Format-Custom + +class DiagnosticRecord +{ + SuggestedCorrections = + [ + class CorrectionExtent + { + EndColumnNumber = 4 + EndLineNumber = 1 + File = C:\Users\kabawany\tmp\test3.ps1 + StartColumnNumber = 1 + StartLineNumber = 1 + Text = Get-ChildItem + Description = Replace gci with Get-ChildItem + } + ] + +} +``` + +The `*LineNumber` and `*ColumnNumber` properties give the region of the script that can be replaced by the contents of `Text` property, i.e., replace gci with Get-ChildItem. + +The main motivation behind having `SuggestedCorrections` is to enable quick-fix like scenarios in editors like VSCode, Sublime, etc. At present, we provide valid `SuggestedCorrection` only for the following rules, while gradually adding this feature to more rules. + + * AvoidAlias.cs + * AvoidUsingPlainTextForPassword.cs + * MisleadingBacktick.cs + * MissingModuleManifestField.cs + * UseToExportFieldsInManifest.cs Building the Code ================= diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 1b3e05779..dee522973 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -40,21 +40,56 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { CommandAst cmdAst = (CommandAst)foundAst; string aliasName = cmdAst.GetCommandName(); + // Handles the exception caused by commands like, {& $PLINK $args 2> $TempErrorFile}. // You can also review the remark section in following document, // MSDN: CommandAst.GetCommandName Method - if (aliasName == null) continue; - - string cmdletName = Microsoft.Windows.PowerShell.ScriptAnalyzer.Helper.Instance.GetCmdletNameFromAlias(aliasName); - + if (aliasName == null) + { + continue; + } + string cmdletName = Helper.Instance.GetCmdletNameFromAlias(aliasName); if (!String.IsNullOrEmpty(cmdletName)) { - yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, aliasName, cmdletName), - cmdAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName, aliasName); + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, aliasName, cmdletName), + cmdAst.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + aliasName, + suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletName)); } } } + /// + /// Creates a list containing suggested correction + /// + /// Command AST of an alias + /// Full name of the alias + /// Retruns a list of suggested corrections + private List GetCorrectionExtent(CommandAst cmdAst, string cmdletName) + { + var corrections = new List(); + var ext = cmdAst.Extent; + var alias = cmdAst.GetCommandName(); + string description = string.Format( + CultureInfo.CurrentCulture, + Strings.AvoidUsingCmdletAliasesCorrectionDescription, + alias, + cmdletName); + corrections.Add(new CorrectionExtent( + ext.StartLineNumber, + ext.EndLineNumber, + cmdAst.CommandElements[0].Extent.StartColumnNumber, + cmdAst.CommandElements[0].Extent.EndColumnNumber, + cmdletName, + ext.File, + description)); + return corrections; + } + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Rules/AvoidUsingPlainTextForPassword.cs b/Rules/AvoidUsingPlainTextForPassword.cs index 766aa78ee..0da7230bb 100644 --- a/Rules/AvoidUsingPlainTextForPassword.cs +++ b/Rules/AvoidUsingPlainTextForPassword.cs @@ -17,6 +17,7 @@ using System.ComponentModel.Composition; using System.Globalization; using System.Reflection; +using System.Text; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -60,11 +61,64 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { yield return new DiagnosticRecord( String.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPlainTextForPasswordError, paramAst.Name), - paramAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); + paramAst.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + suggestedCorrections: GetCorrectionExtent(paramAst)); } } } + private List GetCorrectionExtent(ParameterAst paramAst) + { + //Find the parameter type extent and replace that with secure string + IScriptExtent extent; + var typeAttributeAst = GetTypeAttributeAst(paramAst); + var corrections = new List(); + string correctionText; + if (typeAttributeAst == null) + { + // cannot find any type attribute + extent = paramAst.Name.Extent; + correctionText = string.Format("[SecureString] {0}", paramAst.Name.Extent.Text); + } + else + { + // replace only the existing type with [SecureString] + extent = typeAttributeAst.Extent; + correctionText = typeAttributeAst.TypeName.IsArray ? "[SecureString[]]" : "[SecureString]"; + } + string description = string.Format( + CultureInfo.CurrentCulture, + Strings.AvoidUsingPlainTextForPasswordCorrectionDescription, + paramAst.Name.Extent.Text); + corrections.Add(new CorrectionExtent( + extent.StartLineNumber, + extent.EndLineNumber, + extent.StartColumnNumber, + extent.EndColumnNumber, + correctionText.ToString(), + paramAst.Extent.File, + description)); + return corrections; + } + + private TypeConstraintAst GetTypeAttributeAst(ParameterAst paramAst) + { + if (paramAst.Attributes != null) + { + foreach(var attr in paramAst.Attributes) + { + if (attr.GetType() == typeof(TypeConstraintAst)) + { + return attr as TypeConstraintAst; + } + } + } + return null; + } + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Rules/MisleadingBacktick.cs b/Rules/MisleadingBacktick.cs index 1da366faf..abada65ac 100644 --- a/Rules/MisleadingBacktick.cs +++ b/Rules/MisleadingBacktick.cs @@ -55,16 +55,40 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { int lineNumber = ast.Extent.StartLineNumber + i; - ScriptPosition start = new ScriptPosition(fileName, lineNumber, match.Index, line); - ScriptPosition end = new ScriptPosition(fileName, lineNumber, match.Index + match.Length, line); - + var start = new ScriptPosition(fileName, lineNumber, match.Index + 1, line); + var end = new ScriptPosition(fileName, lineNumber, match.Index + match.Length + 1, line); + var extent = new ScriptExtent(start, end); yield return new DiagnosticRecord( string.Format(CultureInfo.CurrentCulture, Strings.MisleadingBacktickError), - new ScriptExtent(start, end), GetName(), DiagnosticSeverity.Warning, fileName); + extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + suggestedCorrections: GetCorrectionExtent(extent)); } } } + /// + /// Creates a list containing suggested correction + /// + /// + /// Returns a list of suggested corrections + private List GetCorrectionExtent(IScriptExtent violationExtent) + { + var corrections = new List(); + string description = "Remove trailing whilespace"; + corrections.Add(new CorrectionExtent( + violationExtent.StartLineNumber , + violationExtent.EndLineNumber, + violationExtent.StartColumnNumber + 1, + violationExtent.EndColumnNumber, + String.Empty, + violationExtent.File, + description)); + return corrections; + } + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Rules/MissingModuleManifestField.cs b/Rules/MissingModuleManifestField.cs index 6df37980c..0acd85278 100644 --- a/Rules/MissingModuleManifestField.cs +++ b/Rules/MissingModuleManifestField.cs @@ -38,41 +38,25 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (String.Equals(System.IO.Path.GetExtension(fileName), ".psd1", StringComparison.OrdinalIgnoreCase)) { - var ps = System.Management.Automation.PowerShell.Create(); - - try - { - ps.AddCommand("Test-ModuleManifest"); - ps.AddParameter("Path", fileName); - - // Suppress warnings emitted during the execution of Test-ModuleManifest - // ModuleManifest rule must catch any violations (warnings/errors) and generate DiagnosticRecord(s) - ps.AddParameter("WarningAction", ActionPreference.SilentlyContinue); - ps.Invoke(); - - } catch { } - - if (ps != null && ps.HadErrors && ps.Streams != null && ps.Streams.Error != null) + IEnumerable errorRecords; + var psModuleInfo = Helper.Instance.GetModuleManifest(fileName, out errorRecords); + if (errorRecords != null) { - foreach (var errorRecord in ps.Streams.Error) + foreach (var errorRecord in errorRecords) { - if (errorRecord.CategoryInfo != null && errorRecord.CategoryInfo.Category == System.Management.Automation.ErrorCategory.ResourceUnavailable - && String.Equals("MissingMemberException", errorRecord.CategoryInfo.Reason, StringComparison.OrdinalIgnoreCase)) + if (Helper.IsMissingManifestMemberException(errorRecord)) { System.Diagnostics.Debug.Assert(errorRecord.Exception != null && !String.IsNullOrWhiteSpace(errorRecord.Exception.Message), Strings.NullErrorMessage); - yield return new DiagnosticRecord(errorRecord.Exception.Message, ast.Extent, GetName(), DiagnosticSeverity.Warning, fileName); } } } - - ps.Dispose(); } } - + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index c8bc1fe79..ee3aaebe2 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -528,6 +528,15 @@ internal static string AvoidUsingCmdletAliasesCommonName { } } + /// + /// Looks up a localized string similar to Replace {0} with {1}. + /// + internal static string AvoidUsingCmdletAliasesCorrectionDescription { + get { + return ResourceManager.GetString("AvoidUsingCmdletAliasesCorrectionDescription", resourceCulture); + } + } + /// /// Looks up a localized string similar to An alias is an alternate name or nickname for a cmdlet or for a command element, such as a function, script, file, or executable file. But when writing scripts that will potentially need to be maintained over time, either by the original author or another Windows PowerShell scripter, please consider using full cmdlet name instead of alias. Aliases can introduce these problems, readability, understandability and availability.. /// @@ -780,6 +789,15 @@ internal static string AvoidUsingPlainTextForPasswordCommonName { } } + /// + /// Looks up a localized string similar to Set {0} type to SecureString. + /// + internal static string AvoidUsingPlainTextForPasswordCorrectionDescription { + get { + return ResourceManager.GetString("AvoidUsingPlainTextForPasswordCorrectionDescription", resourceCulture); + } + } + /// /// Looks up a localized string similar to Password parameters that take in plaintext will expose passwords and compromise the security of your system.. /// @@ -2004,6 +2022,15 @@ internal static string UseToExportFieldsInManifestCommonName { } } + /// + /// Looks up a localized string similar to Replace {0} with {1}. + /// + internal static string UseToExportFieldsInManifestCorrectionDescription { + get { + return ResourceManager.GetString("UseToExportFieldsInManifestCorrectionDescription", 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.. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index a7c969482..a07448d27 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -810,4 +810,13 @@ UseToExportFieldsInManifest + + Replace {0} with {1} + + + Set {0} type to SecureString + + + Replace {0} with {1} + \ No newline at end of file diff --git a/Rules/UseToExportFieldsInManifest.cs b/Rules/UseToExportFieldsInManifest.cs index 324c60487..5521642bc 100644 --- a/Rules/UseToExportFieldsInManifest.cs +++ b/Rules/UseToExportFieldsInManifest.cs @@ -19,6 +19,8 @@ using System.ComponentModel.Composition; using System.Globalization; using System.Text.RegularExpressions; +using System.Diagnostics; +using System.Text; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -29,6 +31,11 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules [Export(typeof(IScriptRule))] public class UseToExportFieldsInManifest : IScriptRule { + + private const string functionsToExport = "FunctionsToExport"; + private const string cmdletsToExport = "CmdletsToExport"; + private const string aliasesToExport = "AliasesToExport"; + /// /// AnalyzeScript: Analyzes the AST to check if AliasToExport, CmdletsToExport, FunctionsToExport /// and VariablesToExport fields do not use wildcards and $null in their entries. @@ -48,12 +55,14 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) yield break; } - if (!IsValidManifest(ast, fileName)) - { + // check if valid module manifest + IEnumerable errorRecord = null; + PSModuleInfo psModuleInfo = Helper.Instance.GetModuleManifest(fileName, out errorRecord); + if ((errorRecord != null && errorRecord.Count() > 0) || psModuleInfo == null) + { yield break; } - - String[] manifestFields = {"FunctionsToExport", "CmdletsToExport", "VariablesToExport", "AliasesToExport"}; + var hashtableAst = ast.Find(x => x is HashtableAst, false) as HashtableAst; if (hashtableAst == null) @@ -61,30 +70,112 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) yield break; } - foreach(String field in manifestFields) + string[] manifestFields = { functionsToExport, cmdletsToExport, aliasesToExport }; + + 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); + yield return new DiagnosticRecord( + GetError(field), + extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + suggestedCorrections: GetCorrectionExtent(field, extent, psModuleInfo)); + } + else + { + } } } - - /// - /// Checks if the manifest file is valid. - /// - /// - /// - /// A boolean value indicating the validity of the manifest file. - private bool IsValidManifest(Ast ast, string fileName) + + private string GetListLiteral(Dictionary exportedItems) { - var missingManifestRule = new MissingModuleManifestField(); - return !missingManifestRule.AnalyzeScript(ast, fileName).GetEnumerator().MoveNext(); - + const int lineWidth = 64; + if (exportedItems == null || exportedItems.Keys == null) + { + return null; + } + var sbuilder = new StringBuilder(); + sbuilder.Append("@("); + var sbuilderInner = new StringBuilder(); + int charLadder = lineWidth; + int keyCount = exportedItems.Keys.Count; + foreach (var key in exportedItems.Keys) + { + sbuilderInner.Append("'"); + sbuilderInner.Append(key); + sbuilderInner.Append("'"); + if (--keyCount > 0) + { + sbuilderInner.Append(", "); + if (sbuilderInner.Length > charLadder) + { + charLadder += lineWidth; + sbuilderInner.AppendLine(); + sbuilderInner.Append('\t', 2); + } + } + } + sbuilder.Append(sbuilderInner); + sbuilder.Append(")"); + return sbuilder.ToString(); + } + + + private List GetCorrectionExtent(string field, IScriptExtent extent, PSModuleInfo psModuleInfo) + { + Debug.Assert(field != null); + Debug.Assert(psModuleInfo != null); + Debug.Assert(extent != null); + var corrections = new List(); + string correctionText = null; + switch (field) + { + case functionsToExport: + correctionText = GetListLiteral(psModuleInfo.ExportedFunctions); + break; + case cmdletsToExport: + correctionText = GetListLiteral(psModuleInfo.ExportedCmdlets); + break; + case aliasesToExport: + correctionText = GetListLiteral(psModuleInfo.ExportedAliases); + break; + default: + throw new NotImplementedException(string.Format("{0} not implemented", field)); + } + string description = string.Format( + Strings.UseToExportFieldsInManifestCorrectionDescription, + extent.Text, + correctionText); + corrections.Add(new CorrectionExtent( + extent.StartLineNumber, + extent.EndLineNumber, + extent.StartColumnNumber, + extent.EndColumnNumber, + correctionText, + extent.File, + description)); + return corrections; } + ///// + ///// 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 ast contains wildcard character. /// diff --git a/Tests/Engine/CorrectionExtent.tests.ps1 b/Tests/Engine/CorrectionExtent.tests.ps1 new file mode 100644 index 000000000..e4f1c36ef --- /dev/null +++ b/Tests/Engine/CorrectionExtent.tests.ps1 @@ -0,0 +1,34 @@ +if (!(Get-Module PSScriptAnalyzer)) +{ + Import-Module PSScriptAnalyzer +} + +Describe "Correction Extent" { + $type = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.CorrectionExtent] + + Context "Object construction" { + It "creates the object with correct properties" { + $correctionExtent = $type::new(1, 1, 1, 3, "get-childitem", "newfile", "cool description") + + $correctionExtent.StartLineNumber | Should Be 1 + $correctionExtent.EndLineNumber | Should Be 1 + $correctionExtent.StartColumnNumber | Should Be 1 + $correctionExtent.EndColumnNumber | Should be 3 + $correctionExtent.Text | Should Be "get-childitem" + $correctionExtent.File | Should Be "newfile" + $correctionExtent.Description | Should Be "cool description" + } + + It "throws if end line number is less than start line number" { + $text = "Get-ChildItem" + {$type::new(2, 1, 1, $text.Length + 1, $text, "newfile")} | Should Throw "start line number" + } + + It "throws if end column number is less than start column number for same line" { + $text = "start-process" + {$type::new(1, 1, 2, 1, $text, "newfile")} | Should Throw "start column number" + } + } +} + + diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 1a57c4f36..f304b0318 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -2,8 +2,10 @@ $violationMessage = "'cls' is an alias of 'Clear-Host'. Alias can introduce possible problems and make scripts hard to maintain. Please consider changing alias to its full content." $violationName = "PSAvoidUsingCmdletAliases" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violations = Invoke-ScriptAnalyzer $directory\AvoidUsingAlias.ps1 | Where-Object {$_.RuleName -eq $violationName} +$violationFilepath = Join-Path $directory 'AvoidUsingAlias.ps1' +$violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidUsingAliasNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} +Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Describe "AvoidUsingAlias" { Context "When there are violations" { @@ -14,6 +16,14 @@ Describe "AvoidUsingAlias" { It "has the correct description message" { $violations[1].Message | Should Match $violationMessage } + + It "suggests correction" { + Test-CorrectionExtent $violationFilepath $violations[0] 1 'iex' 'Invoke-Expression' + $violations[0].SuggestedCorrections[0].Description | Should Be 'Replace iex with Invoke-Expression' + + Test-CorrectionExtent $violationFilepath $violations[1] 1 'cls' 'Clear-Host' + $violations[1].SuggestedCorrections[0].Description | Should Be 'Replace cls with Clear-Host' + } } Context "When there are no violations" { diff --git a/Tests/Rules/AvoidUsingPlainTextForPassword.ps1 b/Tests/Rules/AvoidUsingPlainTextForPassword.ps1 index 872324967..147e3b52d 100644 --- a/Tests/Rules/AvoidUsingPlainTextForPassword.ps1 +++ b/Tests/Rules/AvoidUsingPlainTextForPassword.ps1 @@ -38,4 +38,25 @@ } function TestFunction($password, [System.Security.SecureString[]]$passphrases, [string]$passThru){ -} \ No newline at end of file +} + +function TestFunction2 +{ + Param( + [string] <#some dumb comment#> $password + ) +} + +function TestFunction3 +{ + Param( + [string[]] $password + ) +} + +function TestFunction4 +{ + Param( + [string] [Parameter(ValueFromPipeline)] $Password + ) +} diff --git a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 index 945c1bf9b..45c90167c 100644 --- a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 +++ b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 @@ -1,17 +1,31 @@ Import-Module PSScriptAnalyzer - $violationMessage = [regex]::Escape("Parameter '`$password' should use SecureString, otherwise this will expose sensitive information. See ConvertTo-SecureString for more information.") $violationName = "PSAvoidUsingPlainTextForPassword" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violations = Invoke-ScriptAnalyzer $directory\AvoidUsingPlainTextForPassword.ps1 | Where-Object {$_.RuleName -eq $violationName} +$violationFilepath = Join-Path $directory 'AvoidUsingPlainTextForPassword.ps1' +$violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidUsingPlainTextForPasswordNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} +Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Describe "AvoidUsingPlainTextForPassword" { Context "When there are violations" { - It "has 3 avoid using plain text for password violations" { - $violations.Count | Should Be 4 + $expectedNumViolations = 7 + It "has $expectedNumViolations violations" { + $violations.Count | Should Be $expectedNumViolations } + It "suggests corrections" { + Test-CorrectionExtent $violationFilepath $violations[0] 1 '$passphrases' '[SecureString] $passphrases' + $violations[0].SuggestedCorrections[0].Description | Should Be 'Set $passphrases type to SecureString' + + Test-CorrectionExtent $violationFilepath $violations[1] 1 '$passwordparam' '[SecureString] $passwordparam' + Test-CorrectionExtent $violationFilepath $violations[2] 1 '$credential' '[SecureString] $credential' + Test-CorrectionExtent $violationFilepath $violations[3] 1 '$password' '[SecureString] $password' + Test-CorrectionExtent $violationFilepath $violations[4] 1 '[string]' '[SecureString]' + Test-CorrectionExtent $violationFilepath $violations[5] 1 '[string[]]' '[SecureString[]]' + Test-CorrectionExtent $violationFilepath $violations[6] 1 '[string]' '[SecureString]' + } + It "has the correct violation message" { $violations[3].Message | Should Match $violationMessage } diff --git a/Tests/Rules/MisleadingBacktick.tests.ps1 b/Tests/Rules/MisleadingBacktick.tests.ps1 index 659d6fdb3..05f674b39 100644 --- a/Tests/Rules/MisleadingBacktick.tests.ps1 +++ b/Tests/Rules/MisleadingBacktick.tests.ps1 @@ -1,14 +1,24 @@ Import-Module PSScriptAnalyzer $writeHostName = "PSMisleadingBacktick" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violations = Invoke-ScriptAnalyzer $directory\MisleadingBacktick.ps1 | Where-Object {$_.RuleName -eq $writeHostName} +$violationFilepath = Join-Path $directory 'MisleadingBacktick.ps1' +$violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $writeHostName} $noViolations = Invoke-ScriptAnalyzer $directory\NoMisleadingBacktick.ps1 | Where-Object {$_.RuleName -eq $clearHostName} +Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Describe "Avoid Misleading Backticks" { Context "When there are violations" { It "has 5 misleading backtick violations" { $violations.Count | Should Be 5 } + + It "suggests correction" { + Test-CorrectionExtent $violationFilepath $violations[0] 1 ' ' '' + Test-CorrectionExtent $violationFilepath $violations[1] 1 ' ' '' + Test-CorrectionExtent $violationFilepath $violations[2] 1 ' ' '' + Test-CorrectionExtent $violationFilepath $violations[3] 1 ' ' '' + Test-CorrectionExtent $violationFilepath $violations[4] 1 ' ' '' + } } Context "When there are no violations" { diff --git a/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 b/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 new file mode 100644 index 000000000..e9f116408 --- /dev/null +++ b/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 @@ -0,0 +1,32 @@ +Function Get-ExtentText +{ + Param( + [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.CorrectionExtent] $violation, + [string] $scriptPath + ) + $scriptContent = Get-Content -Path $scriptPath + $start = [System.Management.Automation.Language.ScriptPosition]::new($scriptPath, $violation.StartLineNumber, $violation.StartColumnNumber, $scriptContent[$violation.StartLineNumber - 1]) + $end = [System.Management.Automation.Language.ScriptPosition]::new($scriptPath, $violation.EndLineNumber, $violation.EndColumnNumber, $scriptContent[$violation.EndLineNumber - 1]) + $extent = [System.Management.Automation.Language.ScriptExtent]::new($start, $end) + return($extent.Text) +} + +Function Test-CorrectionExtent +{ + Param( + [string] $violationFilepath, + [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord] $diagnosticRecord, + [int] $correctionsCount, + [string] $violationText, + [string] $correctionText + ) + $corrections = $diagnosticRecord.SuggestedCorrections + $corrections.Count | Should Be $correctionsCount + $corrections[0].Text | Should Be $correctionText + Get-ExtentText $corrections[0] $violationFilepath | ` + Should Be $violationText +} + + +Export-ModuleMember -Function Get-ExtentText +Export-ModuleMember -Function Test-CorrectionExtent \ No newline at end of file diff --git a/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 b/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 index bc7994035..ed9f0f640 100644 Binary files a/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 and b/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psm1 b/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psm1 new file mode 100644 index 000000000..d9a448105 --- /dev/null +++ b/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psm1 @@ -0,0 +1,12 @@ +Function Get-Foo +{ + +} + +Function Get-Bar +{ + +} + +Export-ModuleMember -Function Get-Foo -Alias gfoo +Export-ModuleMember -Function Get-Bar -Alias gbar \ No newline at end of file diff --git a/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psd1 b/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psd1 index c8fc4b534..495e105fb 100644 Binary files a/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psd1 and b/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psm1 b/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psm1 new file mode 100644 index 000000000..3908d6537 --- /dev/null +++ b/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psm1 @@ -0,0 +1,50 @@ +Function Get-Foo1 +{ + +} + +Function Get-Foo2 +{ + +} +Function Get-Foo3 +{ + +} +Function Get-Foo4 +{ + +} +Function Get-Foo5 +{ + +} +Function Get-Foo6 +{ + +} +Function Get-Foo7 +{ + +} +Function Get-Foo8 +{ + +} +Function Get-Foo9 +{ + +} +Function Get-Foo10 +{ + +} +Function Get-Foo11 +{ + +} +Function Get-Foo12 +{ + +} + diff --git a/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psd1 b/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psd1 index 3c253844d..c9c363454 100644 Binary files a/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psd1 and b/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psd1 differ diff --git a/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psm1 b/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psm1 new file mode 100644 index 000000000..a56d75565 --- /dev/null +++ b/Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psm1 @@ -0,0 +1,12 @@ +Function Get-Foo +{ + +} + +Function Get-Bar +{ + +} + +Export-ModuleMember -Function Get-Foo +Export-ModuleMember -Function Get-Bar diff --git a/Tests/Rules/TestManifest/ManifestBadVariablesWildcard.psd1 b/Tests/Rules/TestManifest/ManifestBadVariablesWildcard.psd1 index 946701bc3..18c876e00 100644 Binary files a/Tests/Rules/TestManifest/ManifestBadVariablesWildcard.psd1 and b/Tests/Rules/TestManifest/ManifestBadVariablesWildcard.psd1 differ diff --git a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 index 50255b5dc..2829de4bc 100644 --- a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 +++ b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 @@ -10,6 +10,7 @@ $testManifestBadVariablesWildcardPath = "ManifestBadVariablesWildcard.psd1" $testManifestBadAllPath = "ManifestBadAll.psd1" $testManifestGoodPath = "ManifestGood.psd1" $testManifestInvalidPath = "ManifestInvalid.psd1" +Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Function Run-PSScriptAnalyzerRule { @@ -39,20 +40,31 @@ Describe "UseManifestExportFields" { $results[0].Extent.Text | Should be "'*'" } + It "suggests corrections for FunctionsToExport with wildcard" { + $violations = Run-PSScriptAnalyzerRule $testManifestBadFunctionsWildcardPath + $violationFilepath = Join-path $testManifestPath $testManifestBadFunctionsWildcardPath + Test-CorrectionExtent $violationFilepath $violations[0] 1 "'*'" "@('Get-Foo', 'Get-Bar')" + $violations[0].SuggestedCorrections[0].Description | Should Be "Replace '*' with @('Get-Foo', 'Get-Bar')" + } + It "detects FunctionsToExport with null" { $results = Run-PSScriptAnalyzerRule $testManifestBadFunctionsNullPath $results.Count | Should be 1 $results[0].Extent.Text | Should be '$null' } + It "suggests corrections for FunctionsToExport with null and line wrapping" { + $violations = Run-PSScriptAnalyzerRule $testManifestBadFunctionsNullPath + $violationFilepath = Join-path $testManifestPath $testManifestBadFunctionsNullPath + Test-CorrectionExtent $violationFilepath $violations[0] 1 '$null' "@('Get-Foo1', 'Get-Foo2', 'Get-Foo3', 'Get-Foo4', 'Get-Foo5', 'Get-Foo6', `r`n`t`t'Get-Foo7', 'Get-Foo8', 'Get-Foo9', 'Get-Foo10', 'Get-Foo11', `r`n`t`t'Get-Foo12')" + } + It "detects array element containing wildcard" { + # if more than two elements contain wildcard we can show only the first one as of now. $results = Run-PSScriptAnalyzerRule $testManifestBadFunctionsWildcardInArrayPath - $results.Count | Should be 3 + $results.Count | Should be 2 $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*'" } @@ -68,15 +80,15 @@ Describe "UseManifestExportFields" { $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 "suggests corrections for AliasesToExport with wildcard" { + $violations = Run-PSScriptAnalyzerRule $testManifestBadAliasesWildcardPath + $violationFilepath = Join-path $testManifestPath $testManifestBadAliasesWildcardPath + Test-CorrectionExtent $violationFilepath $violations[0] 1 "'*'" "@('gfoo', 'gbar')" + } It "detects all the *ToExport violations" { $results = Run-PSScriptAnalyzerRule $testManifestBadAllPath - $results.Count | Should be 4 + $results.Count | Should be 3 } }