diff --git a/.idea/.gitignore b/.idea/.gitignore new file mode 100644 index 000000000..13566b81b --- /dev/null +++ b/.idea/.gitignore @@ -0,0 +1,8 @@ +# Default ignored files +/shelf/ +/workspace.xml +# Editor-based HTTP Client requests +/httpRequests/ +# Datasource local storage ignored files +/dataSources/ +/dataSources.local.xml diff --git a/.idea/PSScriptAnalyzer.iml b/.idea/PSScriptAnalyzer.iml new file mode 100644 index 000000000..bc2cd8740 --- /dev/null +++ b/.idea/PSScriptAnalyzer.iml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml new file mode 100644 index 000000000..e3dd726b6 --- /dev/null +++ b/.idea/modules.xml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml new file mode 100644 index 000000000..94a25f7f4 --- /dev/null +++ b/.idea/vcs.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/Rules/AvoidGeneralCatch.cs b/Rules/AvoidGeneralCatch.cs new file mode 100644 index 000000000..c5e27119a --- /dev/null +++ b/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 +{ + /// + /// AvoidGeneralCatch: Check if catch clause type is RuntimeException and caution against using it + /// +#if !CORECLR +[Export(typeof(IScriptRule))] +#endif + public class AvoidGeneralCatch : IScriptRule + { + /// + /// AnalyzeScript: Analyze the script to check if any empty catch block is used. + /// + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + + // Finds all CommandAsts. + IEnumerable 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); + + } + } + } + **/ + } + } + + /// + /// GetName: Retrieves the name of this rule. + /// + /// The name of this rule + public string GetName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidGeneralCatchName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidGeneralCatchCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidGeneralCatchDescription); + } + + /// + /// 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/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 30d0a7321..ee252f196 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -284,6 +284,24 @@ internal static string AvoidEmptyCatchBlockError { return ResourceManager.GetString("AvoidEmptyCatchBlockError", resourceCulture); } } + + /// + /// Looks up a localized string similar to General catch block is used. + /// + internal static string AvoidGeneralCatchError { + get { + return ResourceManager.GetString("AvoidGeneralCatchError", resourceCulture); + } + } + + /// + /// Looks up a localized string when no specific exception is caught. + /// + internal static string AvoidGeneralCatchErrorEmpty { + get { + return ResourceManager.GetString("AvoidGeneralCatchErrorEmpty", resourceCulture); + } + } /// /// Looks up a localized string similar to Avoid global aliases.. @@ -1013,6 +1031,15 @@ internal static string AvoidUsingEmptyCatchBlockCommonName { return ResourceManager.GetString("AvoidUsingEmptyCatchBlockCommonName", resourceCulture); } } + + /// + /// Looks up a localized string similar to Avoid Using catch block of type RuntimeException. + /// + internal static string AvoidGeneralCatchCommonName { + get { + return ResourceManager.GetString("AvoidGeneralCatchCommonName", resourceCulture); + } + } /// /// Looks up a localized string similar to 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.. @@ -1022,6 +1049,15 @@ internal static string AvoidUsingEmptyCatchBlockDescription { return ResourceManager.GetString("AvoidUsingEmptyCatchBlockDescription", resourceCulture); } } + + /// + /// Looks up a localized string similar to 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.. + /// + internal static string AvoidGeneralCatchDescription { + get { + return ResourceManager.GetString("AvoidGeneralCatchDescription", resourceCulture); + } + } /// /// Looks up a localized string similar to AvoidUsingEmptyCatchBlock. @@ -1031,6 +1067,15 @@ internal static string AvoidUsingEmptyCatchBlockName { return ResourceManager.GetString("AvoidUsingEmptyCatchBlockName", resourceCulture); } } + + /// + /// Looks up a localized string similar to AvoidGeneralCatchBlock. + /// + internal static string AvoidGeneralCatchName { + get { + return ResourceManager.GetString("AvoidUsingEmptyCatchName", resourceCulture); + } + } /// /// Looks up a localized string similar to Avoid Using Internal URLs. diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 20b04712d..723dab010 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -126,9 +126,15 @@ 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. + + 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 + Avoid Using Empty Catch Block + + Avoid using General Catch type + 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. @@ -378,6 +384,9 @@ AvoidUsingEmptyCatchBlock + + AvoidGeneralCatch + AvoidUsingInvokeExpression @@ -513,6 +522,12 @@ Empty catch block is used. Please use Write-Error or throw statements in catch blocks. + + Runtime Exception is used in catch block. Avoid using this as it will catch all exceptions. + + + No specific exception is being caught. + '{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. diff --git a/Tests/Rules/AvoidGeneralCatch.ps1 b/Tests/Rules/AvoidGeneralCatch.ps1 new file mode 100644 index 000000000..b2f72d753 --- /dev/null +++ b/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 ..." +} \ No newline at end of file diff --git a/Tests/Rules/AvoidGeneralCatch.tests.ps1 b/Tests/Rules/AvoidGeneralCatch.tests.ps1 new file mode 100644 index 000000000..8c7781e00 --- /dev/null +++ b/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 + } +}