Merge pull request #185 from PowerShell/BugFixes

Bug fixes

Author: yutingc

Engine/Commands/InvokeScriptAnalyzerCommand.cs

@@ -129,8 +129,8 @@ public SwitchParameter SuppressedOnly
         #region Private Members
 
         Dictionary<string, List<string>> validationResults = new Dictionary<string, List<string>>();
-        private ScriptBlockAst ast                         = null;
-        private IEnumerable<IRule> rules                   = null;
+        private ScriptBlockAst ast = null;
+        private IEnumerable<IRule> rules = null;
 
         #endregion
 
@@ -163,9 +163,9 @@ protected override void BeginProcessing()
             }
             else
             {
-                validationResults.Add("InvalidPaths",  new List<string>());
+                validationResults.Add("InvalidPaths", new List<string>());
                 validationResults.Add("ValidModPaths", new List<string>());
-                validationResults.Add("ValidDllPaths", new List<string>());                 
+                validationResults.Add("ValidDllPaths", new List<string>());
             }
 
             #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
     }
-}
+}

Engine/Helper.cs

@@ -191,6 +191,35 @@ public bool IsDscResourceModule(string filePath)
             return false;
         }
 
+        /// <summary>
+        /// Given an AST, checks whether dsc resource is class based or not
+        /// </summary>
+        /// <param name="ast"></param>
+        /// <returns></returns>
+        public bool IsDscResourceClassBased(ScriptBlockAst ast)
+        {
+            if (null == ast)
+            {
+                return false;
+            }
+
+            List<string> dscResourceFunctionNames = new List<string>(new string[] { "Test", "Get", "Set" });
+
+            IEnumerable<Ast> 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;
+        }
+
         /// <summary>
         /// Given a commandast, checks whether positional parameters are used or not.
         /// </summary>
@@ -280,7 +309,7 @@ public bool PositionalParameterUsed(CommandAst cmdAst)
         /// <param name="name"></param>
         /// <param name="commandType"></param>
         /// <returns></returns>
-        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<Ast> DscResourceFunctions(Ast ast)
         {
             List<string> resourceFunctionNames = new List<string>(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);
         }
 
         /// <summary>
@@ -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);
         }
-        
+
         /// <summary>
         /// Initialize variable analysis on the script ast
         /// </summary>
@@ -701,7 +730,7 @@ public Dictionary<string, List<RuleSuppression>> GetRuleSuppression(Ast ast)
             {
                 ruleSuppressionList.AddRange(RuleSuppression.GetSuppressions(sbAst.ParamBlock.Attributes, sbAst.Extent.StartOffset, sbAst.Extent.EndOffset, sbAst));
             }
-            
+
             // Get rule suppression from functions
             IEnumerable<FunctionDefinitionAst> funcAsts = ast.FindAll(item => item is FunctionDefinitionAst, true).Cast<FunctionDefinitionAst>();
 
@@ -727,13 +756,13 @@ public Dictionary<string, List<RuleSuppression>> GetRuleSuppression(Ast ast)
                     List<RuleSuppression> ruleSuppressions = new List<RuleSuppression>();
                     results.Add(ruleSuppression.RuleName, ruleSuppressions);
                 }
-                
+
                 results[ruleSuppression.RuleName].Add(ruleSuppression);
             }
 
             return results;
         }
-        
+
         /// <summary>
         /// Returns a list of rule suppressions from the function
         /// </summary>
@@ -943,11 +972,11 @@ public int Compare(Tuple<int, int> t1, Tuple<int, int> t2)
             }
         }
     }
-    
+
     /// <summary>
     /// Class used to do variable analysis on the whole script
     /// </summary>
-    public class ScriptAnalysis: ICustomAstVisitor
+    public class ScriptAnalysis : ICustomAstVisitor
     {
         private VariableAnalysis OuterAnalysis;
 
@@ -1148,7 +1177,7 @@ public object VisitBreakStatement(BreakStatementAst breakStatementAst)
         {
             return null;
         }
-        
+
         /// <summary>
         /// Visits body
         /// </summary>
@@ -2396,7 +2425,7 @@ public object VisitArrayExpression(ArrayExpressionAst arrayExprAst)
         {
             return typeof(System.Array).FullName;
         }
-        
+
         /// <summary>
         /// Returns type of array
         /// </summary>
@@ -2499,7 +2528,7 @@ public object VisitIndexExpression(IndexExpressionAst indexAst)
 
             return null;
         }
-        
+
         /// <summary>
         /// Only returns boolean type for unary operator that returns boolean
         /// </summary>
@@ -2540,4 +2569,4 @@ public object VisitUsingExpression(UsingExpressionAst usingExpressionAst)
             return null;
         }
     }
-}
+}

Engine/VariableAnalysis.cs

@@ -86,7 +86,7 @@ private void ProcessParameters(IEnumerable<ParameterAst> 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;
                         }

Rules/AvoidUsingInternalURLs.cs

@@ -44,6 +44,26 @@ public IEnumerable<DiagnosticRecord> 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<DiagnosticRecord> 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()
         /// <returns></returns>
         public RuleSeverity GetSeverity()
         {
-            return RuleSeverity.Warning;
+            return RuleSeverity.Information;
         }
 
         /// <summary>

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
     }
-}
+}