Issue #1744 Add a rule to alert catch [RuntimeException] { ... }Initial branch

Rules/AvoidGeneralCatch.cs

@@ -0,0 +1,131 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.Management.Automation.Language;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+#if !CORECLR
+using System.ComponentModel.Composition;
+#endif
+using System.Globalization;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
+{
+    /// <summary>
+    /// AvoidGeneralCatch: Check if catch clause type is RuntimeException and caution against using it  
+    /// </summary>
+#if !CORECLR
+[Export(typeof(IScriptRule))]
+#endif
+    public class AvoidGeneralCatch : IScriptRule
+    {
+        /// <summary>
+        /// AnalyzeScript: Analyze the script to check if any empty catch block is used.
+        /// </summary>
+        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
+
+            // Finds all CommandAsts.
+            IEnumerable<Ast> foundAsts = ast.FindAll(testAst => testAst is CatchClauseAst, true);
+
+            // Iterates all CatchClauseAst and check the statements count.
+            foreach (Ast foundAst in foundAsts)
+            {
+                CatchClauseAst catchAst = (CatchClauseAst)foundAst;
+
+                if (catchAst.CatchTypes.Count == 0) 
+                {
+                    yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidGeneralCatchErrorEmpty),
+                        catchAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName);
+                }
+                else
+                {
+
+                    foreach (TypeConstraintAst caughtAst in catchAst.CatchTypes) {
+                        if (string.Equals(caughtAst.TypeName.FullName, "RuntimeException", StringComparison.CurrentCultureIgnoreCase) || string.Equals(caughtAst.TypeName.FullName, "System.Management.Automation.RuntimeException", StringComparison.CurrentCultureIgnoreCase)) {
+
+                        yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidGeneralCatchError),
+                            caughtAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName);
+
+                        }
+                    }
+                }
+
+/**
+                if (catchAst.CatchTypes.Count != 0)
+                {
+
+                    foreach (TypeConstraintAst caughtAst in catchAst.CatchTypes) {
+                        if (string.Equals(caughtAst.TypeName.FullName, "RuntimeException", StringComparison.CurrentCultureIgnoreCase) | string.Equals(caughtAst.TypeName.FullName, "System.Management.Automation.RuntimeException", StringComparison.CurrentCultureIgnoreCase)); {
+
+                        yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidGeneralCatchError),
+                            caughtAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName);
+
+                        }
+                    }
+                }
+                **/
+            }
+        }
+
+        /// <summary>
+        /// GetName: Retrieves the name of this rule.
+        /// </summary>
+        /// <returns>The name of this rule</returns>
+        public string GetName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidGeneralCatchName);
+        }
+
+        /// <summary>
+        /// GetCommonName: Retrieves the common name of this rule.
+        /// </summary>
+        /// <returns>The common name of this rule</returns>
+        public string GetCommonName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidGeneralCatchCommonName);
+        }
+
+        /// <summary>
+        /// GetDescription: Retrieves the description of this rule.
+        /// </summary>
+        /// <returns>The description of this rule</returns>
+        public string GetDescription()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidGeneralCatchDescription);
+        }
+
+        /// <summary>
+        /// Method: Retrieves the type of the rule: builtin, managed or module.
+        /// </summary>
+        public SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+
+        /// <summary>
+        /// GetSeverity: Retrieves the severity of the rule: error, warning of information.
+        /// </summary>
+        /// <returns></returns>
+        public RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Warning;
+        }
+
+        /// <summary>
+        /// Method: Retrieves the module/assembly name the rule is from.
+        /// </summary>
+        public string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+    }
+}
+
+
+
+
+
+

Rules/Strings.resx

@@ -126,9 +126,15 @@
   <data name="AvoidUsingEmptyCatchBlockDescription" xml:space="preserve">
     <value>Empty catch blocks are considered poor design decisions because if an error occurs in the try block, this error is simply swallowed and not acted upon. While this does not inherently lead to bad things. It can and this should be avoided if possible. To fix a violation of this rule, using Write-Error or throw statements in catch blocks.</value>
   </data>
+  <data name="AvoidGeneralCatchDescription" xml:space="preserve">
+    <value>A catch block with the type system.management.automation.runtimeexception should not be used in a catch block as it will incorrectly catch all exceptions </value>
+  </data>
   <data name="AvoidUsingEmptyCatchBlockCommonName" xml:space="preserve">
     <value>Avoid Using Empty Catch Block</value>
   </data>
+  <data name="AvoidGeneralCatchCommonName" xml:space="preserve">
+    <value>Avoid using General Catch type</value>
+  </data>
   <data name="AvoidUsingInvokeExpressionRuleDescription" xml:space="preserve">
     <value>The Invoke-Expression cmdlet evaluates or runs a specified string as a command and returns the results of the expression or command. It can be extraordinarily powerful so it is not that you want to never use it but you need to be very careful about using it.  In particular, you are probably on safe ground if the data only comes from the program itself.  If you include any data provided from the user - you need to protect yourself from Code Injection. To fix a violation of this rule, please remove Invoke-Expression from script and find other options instead.</value>
   </data>
@@ -378,6 +384,9 @@
   <data name="AvoidUsingEmptyCatchBlockName" xml:space="preserve">
     <value>AvoidUsingEmptyCatchBlock</value>
   </data>
+  <data name="AvoidGeneralCatchName" xml:space="preserve">
+    <value>AvoidGeneralCatch</value>
+  </data>
   <data name="AvoidUsingInvokeExpressionRuleName" xml:space="preserve">
     <value>AvoidUsingInvokeExpression</value>
   </data>
@@ -513,6 +522,12 @@
   <data name="AvoidEmptyCatchBlockError" xml:space="preserve">
     <value>Empty catch block is used. Please use Write-Error or throw statements in catch blocks.</value>
   </data>
+  <data name="AvoidGeneralCatchError" xml:space="preserve">
+    <value>Runtime Exception is used in catch block. Avoid using this as it will catch all exceptions.</value>
+  </data>
+  <data name="AvoidGeneralCatchErrorEmpty" xml:space="preserve">
+    <value>No specific exception is being caught.</value>
+  </data>
   <data name="AvoidUsingCmdletAliasesError" xml:space="preserve">
     <value>'{0}' is an alias of '{1}'. Alias can introduce possible problems and make scripts hard to maintain. Please consider changing alias to its full content.</value>
   </data>

Tests/Rules/AvoidGeneralCatch.ps1

@@ -0,0 +1,23 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+try
+{
+    1/0
+}
+catch [DivideByZeroException]
+{
+    "catch divide by zero exception"
+}
+catch [System.Management.Automation.RuntimeException]
+{
+    "catch RuntimeException"
+}
+catch
+{
+    "No exception"
+}
+finally
+{
+    "cleaning up ..."
+}

Tests/Rules/AvoidGeneralCatch.tests.ps1

@@ -0,0 +1,18 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+BeforeAll {
+    $violationMessage = "Runtime Exception as catch block type is used. Please use Write-Error or throw statements in catch blocks."
+    $violationName = "PSAvoidGeneralCatch"
+    $violations = Invoke-ScriptAnalyzer $PSScriptRoot\AvoidGeneralCatch.ps1 | Where-Object {$_.RuleName -eq $violationName}
+}
+
+Describe "UseDeclaredVarsMoreThanAssignments" {
+    It "has 2 violations satisfying AvoidGeneralCatch rule" {
+        $violations.Count | Should -Be 2
+    }
+
+    It "has the correct description message" {
+        $violations[0].Message | Should -Match $violationMessage
+    }
+}