Skip to content

Commit a42ab5f

Browse files
authored
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
1 parent 8db488d commit a42ab5f

File tree

5 files changed

+136
-28
lines changed

5 files changed

+136
-28
lines changed

Engine/Extensions.cs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,5 +178,19 @@ public static bool GetValue(this NamedAttributeArgumentAst attrAst, out Expressi
178178

179179
return false;
180180
}
181+
182+
internal static bool TryGetPropertyValue(this PSObject psObject, string propertyName, out object value)
183+
{
184+
PSMemberInfo property = psObject.Properties[propertyName];
185+
186+
if (property is null)
187+
{
188+
value = default;
189+
return false;
190+
}
191+
192+
value = property.Value;
193+
return true;
194+
}
181195
}
182196
}

Engine/ScriptAnalyzer.cs

Lines changed: 54 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
using System.Collections;
2323
using System.Diagnostics;
2424
using System.Text;
25+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Extensions;
2526

2627
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
2728
{
@@ -1263,13 +1264,6 @@ internal IEnumerable<DiagnosticRecord> GetExternalRecord(Ast ast, Token[] token,
12631264

12641265
foreach (var psobject in psobjects)
12651266
{
1266-
DiagnosticSeverity severity;
1267-
IScriptExtent extent;
1268-
string message = string.Empty;
1269-
string ruleName = string.Empty;
1270-
string ruleSuppressionID = string.Empty;
1271-
IEnumerable<CorrectionExtent> suggestedCorrections;
1272-
12731267
if (psobject != null && psobject.ImmediateBaseObject != null)
12741268
{
12751269
// Because error stream is merged to output stream,
@@ -1282,28 +1276,9 @@ internal IEnumerable<DiagnosticRecord> GetExternalRecord(Ast ast, Token[] token,
12821276
}
12831277

12841278
// DiagnosticRecord may not be correctly returned from external rule.
1285-
try
1279+
if (TryConvertPSObjectToDiagnostic(psobject, filePath, out DiagnosticRecord diagnostic))
12861280
{
1287-
severity = (DiagnosticSeverity)Enum.Parse(typeof(DiagnosticSeverity), psobject.Properties["Severity"].Value.ToString());
1288-
message = psobject.Properties["Message"].Value.ToString();
1289-
extent = (IScriptExtent)psobject.Properties["Extent"].Value;
1290-
ruleName = psobject.Properties["RuleName"].Value.ToString();
1291-
ruleSuppressionID = psobject.Properties["RuleSuppressionID"].Value?.ToString();
1292-
suggestedCorrections = (IEnumerable<CorrectionExtent>)psobject.Properties["SuggestedCorrections"].Value;
1293-
}
1294-
catch (Exception ex)
1295-
{
1296-
this.outputWriter.WriteError(new ErrorRecord(ex, ex.HResult.ToString("X"), ErrorCategory.NotSpecified, this));
1297-
continue;
1298-
}
1299-
1300-
if (!string.IsNullOrEmpty(message))
1301-
{
1302-
diagnostics.Add(new DiagnosticRecord(message, extent, ruleName, severity, filePath)
1303-
{
1304-
SuggestedCorrections = suggestedCorrections,
1305-
RuleSuppressionID = ruleSuppressionID,
1306-
});
1281+
diagnostics.Add(diagnostic);
13071282
}
13081283
}
13091284
}
@@ -1660,6 +1635,57 @@ public EditableText Fix(EditableText text, Range range, bool skipParsing, out Ra
16601635
return text;
16611636
}
16621637

1638+
private bool TryConvertPSObjectToDiagnostic(PSObject psObject, string filePath, out DiagnosticRecord diagnostic)
1639+
{
1640+
string message = psObject.Properties["Message"]?.Value?.ToString();
1641+
object extentValue = psObject.Properties["Extent"]?.Value;
1642+
string ruleName = psObject.Properties["RuleName"]?.Value?.ToString();
1643+
string ruleSuppressionID = psObject.Properties["RuleSuppressionID"]?.Value?.ToString();
1644+
CorrectionExtent[] suggestedCorrections = psObject.TryGetPropertyValue("SuggestedCorrections", out object correctionsValue)
1645+
? LanguagePrimitives.ConvertTo<CorrectionExtent[]>(correctionsValue)
1646+
: null;
1647+
DiagnosticSeverity severity = psObject.TryGetPropertyValue("Severity", out object severityValue)
1648+
? LanguagePrimitives.ConvertTo<DiagnosticSeverity>(severityValue)
1649+
: DiagnosticSeverity.Warning;
1650+
1651+
bool isValid = true;
1652+
isValid &= CheckHasRequiredProperty("Message", message);
1653+
isValid &= CheckHasRequiredProperty("RuleName", ruleName);
1654+
1655+
if (extentValue is not null && extentValue is not IScriptExtent)
1656+
{
1657+
this.outputWriter.WriteError(
1658+
new ErrorRecord(
1659+
new ArgumentException($"Property 'Extent' is expected to be of type '{typeof(IScriptExtent)}' but was instead of type '{extentValue.GetType()}'"),
1660+
"CustomRuleDiagnosticPropertyInvalidType",
1661+
ErrorCategory.InvalidArgument,
1662+
this));
1663+
isValid = false;
1664+
}
1665+
1666+
if (!isValid)
1667+
{
1668+
diagnostic = null;
1669+
return false;
1670+
}
1671+
1672+
diagnostic = new DiagnosticRecord(message, (IScriptExtent)extentValue, ruleName, severity, filePath, ruleSuppressionID, suggestedCorrections);
1673+
return true;
1674+
}
1675+
1676+
private bool CheckHasRequiredProperty(string propertyName, object propertyValue)
1677+
{
1678+
if (propertyValue is null)
1679+
{
1680+
var exception = new ArgumentNullException(propertyName, $"The '{propertyName}' property is required on custom rule diagnostics");
1681+
this.outputWriter.WriteError(new ErrorRecord(exception, "CustomRuleDiagnosticPropertyMissing", ErrorCategory.InvalidArgument, this));
1682+
return false;
1683+
}
1684+
1685+
return true;
1686+
}
1687+
1688+
16631689
private static Encoding GetFileEncoding(string path)
16641690
{
16651691
using (var stream = new FileStream(path, FileMode.Open))
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
write-host "test"
2+
$a = "asdf"
3+
$a = $a.replace('s','ssssssss')
4+
[math]::abs(-1)
5+
Function ASDF1234{
6+
"asdf"
7+
}
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<#
2+
.SYNOPSIS
3+
Static methods are not allowed in constrained language mode.
4+
.DESCRIPTION
5+
Static methods are not allowed in constrained language mode.
6+
To fix a violation of this rule, use a cmdlet or function instead of a static method.
7+
.EXAMPLE
8+
Test-StaticMethod -CommandAst $CommandAst
9+
.INPUTS
10+
[System.Management.Automation.Language.ScriptBlockAst]
11+
.OUTPUTS
12+
[PSCustomObject[]]
13+
.NOTES
14+
Reference: Output, CLM info.
15+
#>
16+
function Test-StaticMethod
17+
{
18+
[CmdletBinding()]
19+
[OutputType([PSCustomObject[]])]
20+
Param
21+
(
22+
[Parameter(Mandatory = $true)]
23+
[ValidateNotNullOrEmpty()]
24+
[System.Management.Automation.Language.ScriptBlockAst]
25+
$ScriptBlockAst
26+
)
27+
28+
Process
29+
{
30+
try
31+
{
32+
# Gets methods
33+
34+
$invokedMethods = $ScriptBlockAst.FindAll({$args[0] -is [System.Management.Automation.Language.CommandExpressionAst] -and $args[0].Expression -match "^\[.*\]::" },$true)
35+
foreach ($invokedMethod in $invokedMethods)
36+
{
37+
[PSCustomObject]@{Message = "Avoid Using Static Methods";
38+
Extent = $invokedMethod.Extent;
39+
RuleName = $PSCmdlet.MyInvocation.InvocationName;
40+
Severity = "Warning"}
41+
}
42+
}
43+
catch
44+
{
45+
$PSCmdlet.ThrowTerminatingError($PSItem)
46+
}
47+
}
48+
}

Tests/Engine/CustomizedRule.tests.ps1

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,19 @@ Describe "Test importing correct customized rules" {
199199
$violations[0].RuleSuppressionID | Should -Be "MyRuleSuppressionID"
200200
}
201201

202+
It "Does not throw an exception when optional diagnostic properties are not present" {
203+
$violations = Invoke-ScriptAnalyzer -Path "$PSScriptRoot\CustomRuleNREAssets\CLMTest.ps1" -CustomizedRulePath "$PSScriptRoot\CustomRuleNREAssets\MyCustom.psm1"
204+
205+
$violations.Count | Should -Be 1
206+
$violations[0].RuleName | Should -BeExactly 'MyCustom\Test-StaticMethod'
207+
$violations[0].Severity | Should -Be 'Warning'
208+
$violations[0].ScriptName | Should -BeExactly 'CLMTest.ps1'
209+
$violations[0].Extent.StartLineNumber | Should -Be 4
210+
$violations[0].Message | Should -BeExactly 'Avoid Using Static Methods'
211+
$violations[0].RuleSuppressionID | Should -BeNullOrEmpty
212+
$violations[0].SuggestedCorrections | Should -BeNullOrEmpty
213+
}
214+
202215
Context "Test Invoke-ScriptAnalyzer with customized rules - Advanced test cases" -Skip:$testingLibraryUsage {
203216
It "will show the custom rule in the results when given a rule folder path with trailing backslash" {
204217
# needs fixing for Linux

0 commit comments

Comments
 (0)