Fix NRE when custom rules omit optional properties in diagnostics (#1715

) * Fix NRE when custom rules omit optional properties in diagnostics * Fix script extent type check to prevent throwing * Fix mishandling of null extent
PowerShell · Sep 28, 2021 · a42ab5f · a42ab5f
1 parent 8db488d
commit a42ab5f
Show file tree

Hide file tree

Showing 5 changed files with 136 additions and 28 deletions.
diff --git a/Engine/Extensions.cs b/Engine/Extensions.cs
@@ -178,5 +178,19 @@ public static bool GetValue(this NamedAttributeArgumentAst attrAst, out Expressi
 
             return false;
         }
+
+        internal static bool TryGetPropertyValue(this PSObject psObject, string propertyName, out object value)
+        {
+            PSMemberInfo property = psObject.Properties[propertyName];
+
+            if (property is null)
+            {
+                value = default;
+                return false;
+            }
+
+            value = property.Value;
+            return true;
+        }
     }
 }
diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs
@@ -22,6 +22,7 @@
 using System.Collections;
 using System.Diagnostics;
 using System.Text;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Extensions;
 
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
 {
@@ -1263,13 +1264,6 @@ internal IEnumerable<DiagnosticRecord> GetExternalRecord(Ast ast, Token[] token,
 
                         foreach (var psobject in psobjects)
                         {
-                            DiagnosticSeverity severity;
-                            IScriptExtent extent;
-                            string message = string.Empty;
-                            string ruleName = string.Empty;
-                            string ruleSuppressionID = string.Empty;
-                            IEnumerable<CorrectionExtent> suggestedCorrections;
-
                             if (psobject != null && psobject.ImmediateBaseObject != null)
                             {
                                 // Because error stream is merged to output stream,
@@ -1282,28 +1276,9 @@ internal IEnumerable<DiagnosticRecord> GetExternalRecord(Ast ast, Token[] token,
                                 }
 
                                 // DiagnosticRecord may not be correctly returned from external rule.
-                                try
+                                if (TryConvertPSObjectToDiagnostic(psobject, filePath, out DiagnosticRecord diagnostic))
                                 {
-                                    severity = (DiagnosticSeverity)Enum.Parse(typeof(DiagnosticSeverity), psobject.Properties["Severity"].Value.ToString());
-                                    message = psobject.Properties["Message"].Value.ToString();
-                                    extent = (IScriptExtent)psobject.Properties["Extent"].Value;
-                                    ruleName = psobject.Properties["RuleName"].Value.ToString();
-                                    ruleSuppressionID = psobject.Properties["RuleSuppressionID"].Value?.ToString();
-                                    suggestedCorrections = (IEnumerable<CorrectionExtent>)psobject.Properties["SuggestedCorrections"].Value;
-                                }
-                                catch (Exception ex)
-                                {
-                                    this.outputWriter.WriteError(new ErrorRecord(ex, ex.HResult.ToString("X"), ErrorCategory.NotSpecified, this));
-                                    continue;
-                                }
-
-                                if (!string.IsNullOrEmpty(message))
-                                {
-                                    diagnostics.Add(new DiagnosticRecord(message, extent, ruleName, severity, filePath)
-                                    {
-                                        SuggestedCorrections = suggestedCorrections,
-                                        RuleSuppressionID = ruleSuppressionID,
-                                    });
+                                    diagnostics.Add(diagnostic);
                                 }
                             }
                         }
@@ -1660,6 +1635,57 @@ public EditableText Fix(EditableText text, Range range, bool skipParsing, out Ra
             return text;
         }
 
+        private bool TryConvertPSObjectToDiagnostic(PSObject psObject, string filePath, out DiagnosticRecord diagnostic)
+        {
+            string message = psObject.Properties["Message"]?.Value?.ToString();
+            object extentValue = psObject.Properties["Extent"]?.Value;
+            string ruleName = psObject.Properties["RuleName"]?.Value?.ToString();
+            string ruleSuppressionID = psObject.Properties["RuleSuppressionID"]?.Value?.ToString();
+            CorrectionExtent[] suggestedCorrections = psObject.TryGetPropertyValue("SuggestedCorrections", out object correctionsValue)
+                ? LanguagePrimitives.ConvertTo<CorrectionExtent[]>(correctionsValue)
+                : null;
+            DiagnosticSeverity severity = psObject.TryGetPropertyValue("Severity", out object severityValue)
+                ? LanguagePrimitives.ConvertTo<DiagnosticSeverity>(severityValue)
+                : DiagnosticSeverity.Warning;
+
+            bool isValid = true;
+            isValid &= CheckHasRequiredProperty("Message", message);
+            isValid &= CheckHasRequiredProperty("RuleName", ruleName);
+
+            if (extentValue is not null && extentValue is not IScriptExtent)
+            {
+                this.outputWriter.WriteError(
+                    new ErrorRecord(
+                        new ArgumentException($"Property 'Extent' is expected to be of type '{typeof(IScriptExtent)}' but was instead of type '{extentValue.GetType()}'"),
+                        "CustomRuleDiagnosticPropertyInvalidType",
+                        ErrorCategory.InvalidArgument,
+                        this));
+                isValid = false;
+            }
+
+            if (!isValid)
+            {
+                diagnostic = null;
+                return false;
+            }
+
+            diagnostic = new DiagnosticRecord(message, (IScriptExtent)extentValue, ruleName, severity, filePath, ruleSuppressionID, suggestedCorrections);
+            return true;
+        }
+
+        private bool CheckHasRequiredProperty(string propertyName, object propertyValue)
+        {
+            if (propertyValue is null)
+            {
+                var exception = new ArgumentNullException(propertyName, $"The '{propertyName}' property is required on custom rule diagnostics");
+                this.outputWriter.WriteError(new ErrorRecord(exception, "CustomRuleDiagnosticPropertyMissing", ErrorCategory.InvalidArgument, this));
+                return false;
+            }
+
+            return true;
+        }
+
+
         private static Encoding GetFileEncoding(string path)
         {
             using (var stream = new FileStream(path, FileMode.Open))

diff --git a/Tests/Engine/CustomRuleNREAssets/CLMTest.ps1 b/Tests/Engine/CustomRuleNREAssets/CLMTest.ps1
@@ -0,0 +1,7 @@
+write-host "test"
+$a = "asdf"
+$a = $a.replace('s','ssssssss')
+[math]::abs(-1)
+Function ASDF1234{
+    "asdf"
+}
diff --git a/Tests/Engine/CustomRuleNREAssets/MyCustom.psm1 b/Tests/Engine/CustomRuleNREAssets/MyCustom.psm1
@@ -0,0 +1,48 @@
+<#
+.SYNOPSIS
+    Static methods are not allowed in constrained language mode.
+.DESCRIPTION
+    Static methods are not allowed in constrained language mode.
+    To fix a violation of this rule, use a cmdlet or function instead of a static method.
+.EXAMPLE
+    Test-StaticMethod -CommandAst $CommandAst
+.INPUTS
+    [System.Management.Automation.Language.ScriptBlockAst]
+.OUTPUTS
+    [PSCustomObject[]]
+.NOTES
+    Reference: Output, CLM info.
+#>
+function Test-StaticMethod
+{
+    [CmdletBinding()]
+    [OutputType([PSCustomObject[]])]
+    Param
+    (
+        [Parameter(Mandatory = $true)]
+        [ValidateNotNullOrEmpty()]
+        [System.Management.Automation.Language.ScriptBlockAst]
+        $ScriptBlockAst
+    )
+
+    Process
+    {
+        try
+        {
+            # Gets methods
+
+            $invokedMethods = $ScriptBlockAst.FindAll({$args[0] -is [System.Management.Automation.Language.CommandExpressionAst] -and $args[0].Expression -match "^\[.*\]::" },$true)
+            foreach ($invokedMethod in $invokedMethods)
+            {
+                [PSCustomObject]@{Message  = "Avoid Using Static Methods";
+                                  Extent   = $invokedMethod.Extent;
+                                  RuleName = $PSCmdlet.MyInvocation.InvocationName;
+                                  Severity = "Warning"}
+            }
+        }
+        catch
+        {
+            $PSCmdlet.ThrowTerminatingError($PSItem)
+        }
+    }
+}
diff --git a/Tests/Engine/CustomizedRule.tests.ps1 b/Tests/Engine/CustomizedRule.tests.ps1
@@ -199,6 +199,19 @@ Describe "Test importing correct customized rules" {
             $violations[0].RuleSuppressionID   | Should -Be "MyRuleSuppressionID"
         }
 
+		It "Does not throw an exception when optional diagnostic properties are not present" {
+			$violations = Invoke-ScriptAnalyzer -Path "$PSScriptRoot\CustomRuleNREAssets\CLMTest.ps1" -CustomizedRulePath "$PSScriptRoot\CustomRuleNREAssets\MyCustom.psm1"
+
+			$violations.Count | Should -Be 1
+			$violations[0].RuleName | Should -BeExactly 'MyCustom\Test-StaticMethod'
+			$violations[0].Severity | Should -Be 'Warning'
+			$violations[0].ScriptName | Should -BeExactly 'CLMTest.ps1'
+			$violations[0].Extent.StartLineNumber | Should -Be 4
+			$violations[0].Message | Should -BeExactly 'Avoid Using Static Methods'
+			$violations[0].RuleSuppressionID | Should -BeNullOrEmpty
+			$violations[0].SuggestedCorrections | Should -BeNullOrEmpty
+		}
+
 		Context "Test Invoke-ScriptAnalyzer with customized rules - Advanced test cases" -Skip:$testingLibraryUsage {
             It "will show the custom rule in the results when given a rule folder path with trailing backslash" {
 				# needs fixing for Linux