diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index 2c1e2f3f4..16c2911ff 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -129,8 +129,8 @@ public SwitchParameter SuppressedOnly #region Private Members Dictionary> validationResults = new Dictionary>(); - private ScriptBlockAst ast = null; - private IEnumerable rules = null; + private ScriptBlockAst ast = null; + private IEnumerable rules = null; #endregion @@ -163,9 +163,9 @@ protected override void BeginProcessing() } else { - validationResults.Add("InvalidPaths", new List()); + validationResults.Add("InvalidPaths", new List()); validationResults.Add("ValidModPaths", new List()); - validationResults.Add("ValidDllPaths", new List()); + validationResults.Add("ValidDllPaths", new List()); } #endregion @@ -174,7 +174,7 @@ protected override void BeginProcessing() try { - if (validationResults["ValidDllPaths"].Count == 0 && + if (validationResults["ValidDllPaths"].Count == 0 && validationResults["ValidModPaths"].Count == 0) { ScriptAnalyzer.Instance.Initialize(); @@ -185,7 +185,7 @@ protected override void BeginProcessing() } } catch (Exception ex) - { + { ThrowTerminatingError(new ErrorRecord(ex, ex.HResult.ToString("X", CultureInfo.CurrentCulture), ErrorCategory.NotSpecified, this)); } @@ -228,8 +228,8 @@ private void ProcessPath(string path) if (path == null) { - ThrowTerminatingError(new ErrorRecord(new FileNotFoundException(), - string.Format(CultureInfo.CurrentCulture, Strings.FileNotFound, path), + ThrowTerminatingError(new ErrorRecord(new FileNotFoundException(), + string.Format(CultureInfo.CurrentCulture, Strings.FileNotFound, path), ErrorCategory.InvalidArgument, this)); } @@ -315,11 +315,11 @@ private void AnalyzeFile(string filePath) else { ThrowTerminatingError(new ErrorRecord(new FileNotFoundException(), - string.Format(CultureInfo.CurrentCulture, Strings.InvalidPath, filePath), + string.Format(CultureInfo.CurrentCulture, Strings.InvalidPath, filePath), ErrorCategory.InvalidArgument, filePath)); } - if (errors != null && errors.Length > 0) + if (errors != null && errors.Length > 0) { foreach (ParseError error in errors) { @@ -362,7 +362,7 @@ private void AnalyzeFile(string filePath) #region Run ScriptRules //Trim down to the leaf element of the filePath and pass it to Diagnostic Record string fileName = System.IO.Path.GetFileName(filePath); - + if (ScriptAnalyzer.Instance.ScriptRules != null) { foreach (IScriptRule scriptRule in ScriptAnalyzer.Instance.ScriptRules) @@ -433,7 +433,7 @@ private void AnalyzeFile(string filePath) break; } } - if ((includeRule == null || includeRegexMatch) && (excludeRule == null || !excludeRegexMatch)) + if ((includeRule == null || includeRegexMatch) && (excludeRule == null || !excludeRegexMatch)) { WriteVerbose(string.Format(CultureInfo.CurrentCulture, Strings.VerboseRunningMessage, tokenRule.GetName())); @@ -448,7 +448,7 @@ private void AnalyzeFile(string filePath) catch (Exception tokenRuleException) { WriteError(new ErrorRecord(tokenRuleException, Strings.RuleErrorMessage, ErrorCategory.InvalidOperation, fileName)); - } + } } } } @@ -458,46 +458,50 @@ private void AnalyzeFile(string filePath) #region DSC Resource Rules if (ScriptAnalyzer.Instance.DSCResourceRules != null) { - // Run DSC Class rule - foreach (IDSCResourceRule dscResourceRule in ScriptAnalyzer.Instance.DSCResourceRules) + // Invoke AnalyzeDSCClass only if the ast is a class based resource + if (Helper.Instance.IsDscResourceClassBased(ast)) { - bool includeRegexMatch = false; - bool excludeRegexMatch = false; - - foreach (Regex include in includeRegexList) + // Run DSC Class rule + foreach (IDSCResourceRule dscResourceRule in ScriptAnalyzer.Instance.DSCResourceRules) { - if (include.IsMatch(dscResourceRule.GetName())) + bool includeRegexMatch = false; + bool excludeRegexMatch = false; + + foreach (Regex include in includeRegexList) { - includeRegexMatch = true; - break; + if (include.IsMatch(dscResourceRule.GetName())) + { + includeRegexMatch = true; + break; + } } - } - foreach (Regex exclude in excludeRegexList) - { - if (exclude.IsMatch(dscResourceRule.GetName())) + foreach (Regex exclude in excludeRegexList) { - excludeRegexMatch = true; - break; + if (exclude.IsMatch(dscResourceRule.GetName())) + { + excludeRegexMatch = true; + break; + } } - } - - 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 + if ((includeRule == null || includeRegexMatch) && (excludeRule == null || excludeRegexMatch)) { - var records = Helper.Instance.SuppressRule(dscResourceRule.GetName(), ruleSuppressions, dscResourceRule.AnalyzeDSCClass(ast, filePath).ToList()); - diagnostics.AddRange(records.Item2); - suppressed.AddRange(records.Item1); + 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.AnalyzeDSCClass(ast, filePath).ToList()); + diagnostics.AddRange(records.Item2); + suppressed.AddRange(records.Item1); + } + catch (Exception dscResourceRuleException) + { + WriteError(new ErrorRecord(dscResourceRuleException, Strings.RuleErrorMessage, ErrorCategory.InvalidOperation, filePath)); + } } - catch (Exception dscResourceRuleException) - { - WriteError(new ErrorRecord(dscResourceRuleException, Strings.RuleErrorMessage, ErrorCategory.InvalidOperation, filePath)); - } } } @@ -543,7 +547,7 @@ private void AnalyzeFile(string filePath) } } - } + } } #endregion @@ -607,4 +611,4 @@ private void AnalyzeFile(string filePath) #endregion } -} +} \ No newline at end of file diff --git a/Engine/Helper.cs b/Engine/Helper.cs index c603b20ce..8864ac136 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -191,6 +191,35 @@ public bool IsDscResourceModule(string filePath) return false; } + /// + /// Given an AST, checks whether dsc resource is class based or not + /// + /// + /// + public bool IsDscResourceClassBased(ScriptBlockAst ast) + { + if (null == ast) + { + return false; + } + + List dscResourceFunctionNames = new List(new string[] { "Test", "Get", "Set" }); + + IEnumerable dscClasses = ast.FindAll(item => + item is TypeDefinitionAst + && ((item as TypeDefinitionAst).IsClass) + && (item as TypeDefinitionAst).Attributes.Any(attr => String.Equals("DSCResource", attr.TypeName.FullName, StringComparison.OrdinalIgnoreCase)), true); + + // Found one or more classes marked with DscResource attribute + // So this might be a DscResource. Further validation will be performed by the individual rules + if (null != dscClasses && 0 < dscClasses.Count()) + { + return true; + } + + return false; + } + /// /// Given a commandast, checks whether positional parameters are used or not. /// @@ -280,7 +309,7 @@ public bool PositionalParameterUsed(CommandAst cmdAst) /// /// /// - public CommandInfo GetCommandInfo(string name, CommandTypes commandType=CommandTypes.All) + public CommandInfo GetCommandInfo(string name, CommandTypes commandType = CommandTypes.All) { return Helper.Instance.MyCmdlet.InvokeCommand.GetCommand(name, commandType); } @@ -294,7 +323,7 @@ public IEnumerable DscResourceFunctions(Ast ast) { List resourceFunctionNames = new List(new string[] { "Set-TargetResource", "Get-TargetResource", "Test-TargetResource" }); return ast.FindAll(item => item is FunctionDefinitionAst - && resourceFunctionNames.Contains((item as FunctionDefinitionAst).Name, StringComparer.OrdinalIgnoreCase), true); + && resourceFunctionNames.Contains((item as FunctionDefinitionAst).Name, StringComparer.OrdinalIgnoreCase), true); } /// @@ -481,7 +510,7 @@ public bool AllCodePathReturns(Ast ast) var analysis = VariableAnalysisDictionary[ast]; return analysis.Exit._predecessors.All(block => block._returns || block._unreachable || block._throws); } - + /// /// Initialize variable analysis on the script ast /// @@ -701,7 +730,7 @@ public Dictionary> GetRuleSuppression(Ast ast) { ruleSuppressionList.AddRange(RuleSuppression.GetSuppressions(sbAst.ParamBlock.Attributes, sbAst.Extent.StartOffset, sbAst.Extent.EndOffset, sbAst)); } - + // Get rule suppression from functions IEnumerable funcAsts = ast.FindAll(item => item is FunctionDefinitionAst, true).Cast(); @@ -727,13 +756,13 @@ public Dictionary> GetRuleSuppression(Ast ast) List ruleSuppressions = new List(); results.Add(ruleSuppression.RuleName, ruleSuppressions); } - + results[ruleSuppression.RuleName].Add(ruleSuppression); } return results; } - + /// /// Returns a list of rule suppressions from the function /// @@ -943,11 +972,11 @@ public int Compare(Tuple t1, Tuple t2) } } } - + /// /// Class used to do variable analysis on the whole script /// - public class ScriptAnalysis: ICustomAstVisitor + public class ScriptAnalysis : ICustomAstVisitor { private VariableAnalysis OuterAnalysis; @@ -1148,7 +1177,7 @@ public object VisitBreakStatement(BreakStatementAst breakStatementAst) { return null; } - + /// /// Visits body /// @@ -2396,7 +2425,7 @@ public object VisitArrayExpression(ArrayExpressionAst arrayExprAst) { return typeof(System.Array).FullName; } - + /// /// Returns type of array /// @@ -2499,7 +2528,7 @@ public object VisitIndexExpression(IndexExpressionAst indexAst) return null; } - + /// /// Only returns boolean type for unary operator that returns boolean /// @@ -2540,4 +2569,4 @@ public object VisitUsingExpression(UsingExpressionAst usingExpressionAst) return null; } } -} +} \ No newline at end of file diff --git a/Engine/VariableAnalysis.cs b/Engine/VariableAnalysis.cs index 5445df4bc..7c29c170f 100644 --- a/Engine/VariableAnalysis.cs +++ b/Engine/VariableAnalysis.cs @@ -86,7 +86,7 @@ private void ProcessParameters(IEnumerable parameters) type = paramAst.TypeName.GetReflectionType(); } - if (String.Equals(paramAst.TypeName.FullName, "switch", StringComparison.OrdinalIgnoreCase)) + if (paramAst.TypeName.GetReflectionType() == typeof(System.Management.Automation.SwitchParameter)) { isSwitchOrMandatory = true; } diff --git a/Rules/AvoidUsingInternalURLs.cs b/Rules/AvoidUsingInternalURLs.cs index 095cd5412..42f1692f8 100644 --- a/Rules/AvoidUsingInternalURLs.cs +++ b/Rules/AvoidUsingInternalURLs.cs @@ -44,6 +44,26 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { foreach (StringConstantExpressionAst expressionAst in expressionAsts) { + //Check if XPath is used. If XPath is used, then we don't throw warnings. + Ast parentAst = expressionAst.Parent; + if (parentAst is InvokeMemberExpressionAst) + { + InvokeMemberExpressionAst invocation = parentAst as InvokeMemberExpressionAst; + if (invocation != null) + { + if (String.Equals(invocation.Member.ToString(), "SelectSingleNode",StringComparison.OrdinalIgnoreCase) || + String.Equals(invocation.Member.ToString(), "SelectNodes",StringComparison.OrdinalIgnoreCase) || + String.Equals(invocation.Member.ToString(), "Select", StringComparison.OrdinalIgnoreCase) || + String.Equals(invocation.Member.ToString(), "Evaluate",StringComparison.OrdinalIgnoreCase) || + String.Equals(invocation.Member.ToString(), "Matches",StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + } + } + + bool isPathValid = false; bool isInternalURL = false; //make sure there is no path @@ -128,7 +148,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) new DiagnosticRecord( String.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingInternalURLsError, expressionAst.Value), expressionAst.Extent, - GetName(), DiagnosticSeverity.Warning, fileName); + GetName(), DiagnosticSeverity.Information, fileName); } } @@ -177,7 +197,7 @@ public SourceType GetSourceType() /// public RuleSeverity GetSeverity() { - return RuleSeverity.Warning; + return RuleSeverity.Information; } /// diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index e299b45e2..fa5346b6a 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -118,7 +118,7 @@ Describe "TestSeverity" { It "filters rules based on multiple severity inputs"{ $rules = Get-ScriptAnalyzerRule -Severity Error,Information - $rules.Count | Should be 12 + $rules.Count | Should be 13 } It "takes lower case inputs" { @@ -137,4 +137,4 @@ Describe "TestWildCard" { $rules = Get-ScriptAnalyzerRule -Name PSDSC*　-Severity Information $rules.Count | Should be 3 } -} \ No newline at end of file +} diff --git a/Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1 b/Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1 index 432dd1a91..ea436bb4f 100644 --- a/Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1 +++ b/Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1 @@ -31,6 +31,13 @@ $proc[0] function Test-PreferenceVariable { + Param( + [switch] + $a, + + [System.Management.Automation.SwitchParameter] + $b + ) if (-not $PSBoundParameters.ContainsKey('Verbose')) { $VerbosePreference = $PSCmdlet.GetVariableValue('VerbosePreference') -as @@ -38,4 +45,4 @@ function Test-PreferenceVariable } $VerbosePreference - } \ No newline at end of file +} \ No newline at end of file diff --git a/Tests/Rules/AvoidUsingInternalURLs.ps1 b/Tests/Rules/AvoidUsingInternalURLs.ps1 index da001d85c..2cbd676f0 100644 --- a/Tests/Rules/AvoidUsingInternalURLs.ps1 +++ b/Tests/Rules/AvoidUsingInternalURLs.ps1 @@ -3,4 +3,4 @@ $internalSite = "//msw" $externalSite = "http:\\msw" if (-not $scratch.EndsWith("/")) { $scratch += "/"; -} \ No newline at end of file +} diff --git a/Tests/Rules/AvoidUsingInternalURLsNoViolations.ps1 b/Tests/Rules/AvoidUsingInternalURLsNoViolations.ps1 index cdd8b7cc5..337bb0832 100644 --- a/Tests/Rules/AvoidUsingInternalURLsNoViolations.ps1 +++ b/Tests/Rules/AvoidUsingInternalURLsNoViolations.ps1 @@ -1,4 +1,8 @@ $correctPath = "www.bing.com" $externalSite = "//outside.co/test" rmdir /s /q ".\Directory" +function Test +{ + $filesNode = $infoXml.SelectSingleNode("//files") +} $sd = "O:BAG:BAD:(A;;0x800;;;WD)(A;;0x120fff;;;SY)(A;;0x120fff;;;LS)(A;;0x120fff;;;NS)(A;;0x120fff;;;BA)(A;;0xee5;;;LU)(A;;LC;;;MU)(A;;0x800;;;AG)" \ No newline at end of file