diff --git a/Engine/Generic/RuleSuppression.cs b/Engine/Generic/RuleSuppression.cs index d8a41f974..d912eee0c 100644 --- a/Engine/Generic/RuleSuppression.cs +++ b/Engine/Generic/RuleSuppression.cs @@ -308,7 +308,7 @@ public static List GetSuppressions(IEnumerable at } IEnumerable suppressionAttribute = attrAsts.Where( - item => item.TypeName.GetReflectionType() == typeof(System.Diagnostics.CodeAnalysis.SuppressMessageAttribute)); + item => item.TypeName.GetReflectionAttributeType() == typeof(System.Diagnostics.CodeAnalysis.SuppressMessageAttribute)); foreach (var attributeAst in suppressionAttribute) { diff --git a/Engine/Generic/SuppressedRecord.cs b/Engine/Generic/SuppressedRecord.cs index ee50ea48b..57ca1a24a 100644 --- a/Engine/Generic/SuppressedRecord.cs +++ b/Engine/Generic/SuppressedRecord.cs @@ -1,6 +1,9 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System.Collections.Generic; +using System.Collections.ObjectModel; + namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic { /// @@ -9,22 +12,18 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic public class SuppressedRecord : DiagnosticRecord { /// - /// The rule suppression of this record + /// The rule suppressions applied to this record. /// - public RuleSuppression Suppression - { - get; - set; - } + public IReadOnlyList Suppression { get; set; } /// /// Creates a suppressed record based on a diagnostic record and the rule suppression /// /// /// - public SuppressedRecord(DiagnosticRecord record, RuleSuppression suppression) + public SuppressedRecord(DiagnosticRecord record, IReadOnlyList suppressions) { - Suppression = suppression; + Suppression = new ReadOnlyCollection(new List(suppressions)); if (record != null) { RuleName = record.RuleName; diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 743cb4d68..feea8a5ec 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -1323,156 +1323,177 @@ internal List GetSuppressionsConfiguration(ConfigurationDefinit /// Suppress the rules from the diagnostic records list. /// Returns a list of suppressed records as well as the ones that are not suppressed /// - /// - /// + /// The name of the rule possibly being suppressed. + /// A lookup table from rule name to suppressions of that rule. + /// The list of all diagnostics emitted by the given rule. + /// Any errors that arise due to misconfigured suppression settings. + /// A pair, with the first item being the list of suppressed diagnostics, and the second the list of unsuppressed diagnostics. public Tuple, List> SuppressRule( string ruleName, Dictionary> ruleSuppressionsDict, List diagnostics, - out List errorRecords) + out List errors) { - List suppressedRecords = new List(); - List unSuppressedRecords = new List(); - Tuple, List> result = Tuple.Create(suppressedRecords, unSuppressedRecords); - errorRecords = new List(); - if (diagnostics == null || diagnostics.Count == 0) + var suppressedRecords = new List(); + var unsuppressedRecords = new List(); + errors = new List(); + var result = new Tuple, List>(suppressedRecords, unsuppressedRecords); + + if (diagnostics is null || diagnostics.Count == 0) { return result; } - if (ruleSuppressionsDict == null || !ruleSuppressionsDict.ContainsKey(ruleName) - || ruleSuppressionsDict[ruleName].Count == 0) + if (ruleSuppressionsDict is null + || !ruleSuppressionsDict.TryGetValue(ruleName, out List ruleSuppressions) + || ruleSuppressions.Count == 0) { - unSuppressedRecords.AddRange(diagnostics); + unsuppressedRecords.AddRange(diagnostics); return result; } - List ruleSuppressions = ruleSuppressionsDict[ruleName]; - var offsetArr = GetOffsetArray(diagnostics); - int recordIndex = 0; - int startRecord = 0; - bool[] suppressed = new bool[diagnostics.Count]; - foreach (RuleSuppression ruleSuppression in ruleSuppressions) + // We need to report errors for any rule suppressions with IDs that are not applied to anything + // Start by assuming all suppressions are unapplied and then we'll remove them as we apply them later + var unappliedSuppressions = new HashSet(); + foreach (RuleSuppression suppression in ruleSuppressions) { - int suppressionCount = 0; - while (startRecord < diagnostics.Count - // && diagnostics[startRecord].Extent.StartOffset < ruleSuppression.StartOffset) - // && diagnostics[startRecord].Extent.StartLineNumber < ruleSuppression.st) - && offsetArr[startRecord] != null && offsetArr[startRecord].Item1 < ruleSuppression.StartOffset) + if (!string.IsNullOrWhiteSpace(suppression.RuleSuppressionID)) { - startRecord += 1; + unappliedSuppressions.Add(suppression); } - - // at this point, start offset of startRecord is greater or equals to rulesuppression.startoffset - recordIndex = startRecord; - - while (recordIndex < diagnostics.Count) + } + + // We have suppressions and diagnostics + // For each diagnostic, collect all the suppressions that apply to it + // and if there are any, record the diagnostic as suppressed + foreach (DiagnosticRecord diagnostic in diagnostics) + { + Tuple offsetPair = GetOffset(diagnostic); + + var appliedSuppressions = new List(); + foreach (RuleSuppression suppression in ruleSuppressions) { - DiagnosticRecord record = diagnostics[recordIndex]; - var curOffset = offsetArr[recordIndex]; - - //if (record.Extent.EndOffset > ruleSuppression.EndOffset) - if (curOffset != null && curOffset.Item2 > ruleSuppression.EndOffset) + // Check that the diagnostic is within the suppressed extent, if we have such an extent + if (!(offsetPair is null) + && (offsetPair.Item1 < suppression.StartOffset || offsetPair.Item2 > suppression.EndOffset)) { - break; + continue; } - - // we suppress if there is no suppression id or if there is suppression id and it matches - if (string.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID) - || (!String.IsNullOrWhiteSpace(record.RuleSuppressionID) && - string.Equals(ruleSuppression.RuleSuppressionID, record.RuleSuppressionID, StringComparison.OrdinalIgnoreCase))) + + // If there's a rule suppression ID, check that it matches the diagnostic + if (!string.IsNullOrWhiteSpace(suppression.RuleSuppressionID) + && !string.Equals(diagnostic.RuleSuppressionID, suppression.RuleSuppressionID, StringComparison.OrdinalIgnoreCase)) { - suppressed[recordIndex] = true; - suppressedRecords.Add(new SuppressedRecord(record, ruleSuppression)); - suppressionCount += 1; + continue; } - - recordIndex += 1; + + unappliedSuppressions.Remove(suppression); + appliedSuppressions.Add(suppression); } - - // If we cannot found any error but the rulesuppression has a rulesuppressionid then it must be used wrongly - if (!String.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID) && suppressionCount == 0) + + if (appliedSuppressions.Count > 0) { - // checks whether are given a string or a file path - if (String.IsNullOrWhiteSpace(diagnostics.First().Extent.File)) - { - ruleSuppression.Error = String.Format(CultureInfo.CurrentCulture, Strings.RuleSuppressionErrorFormatScriptDefinition, ruleSuppression.StartAttributeLine, - String.Format(Strings.RuleSuppressionIDError, ruleSuppression.RuleSuppressionID)); - } - else - { - ruleSuppression.Error = String.Format(CultureInfo.CurrentCulture, Strings.RuleSuppressionErrorFormat, ruleSuppression.StartAttributeLine, - System.IO.Path.GetFileName(diagnostics.First().Extent.File), String.Format(Strings.RuleSuppressionIDError, ruleSuppression.RuleSuppressionID)); - } - errorRecords.Add(new ErrorRecord(new ArgumentException(ruleSuppression.Error), ruleSuppression.Error, ErrorCategory.InvalidArgument, ruleSuppression)); - //this.outputWriter.WriteError(new ErrorRecord(new ArgumentException(ruleSuppression.Error), ruleSuppression.Error, ErrorCategory.InvalidArgument, ruleSuppression)); + suppressedRecords.Add(new SuppressedRecord(diagnostic, appliedSuppressions)); + } + else + { + unsuppressedRecords.Add(diagnostic); } } - - for (int i = 0; i < suppressed.Length; i += 1) + + // Do any error reporting for misused RuleSuppressionIDs here. + // If the user applied a suppression qualified by a suppression ID, + // but that suppression didn't apply only because the suppression ID didn't match + // we let the user know in the form of an error. + string analyzedFile = diagnostics[0]?.Extent?.File; + bool appliedToFile = !string.IsNullOrEmpty(analyzedFile); + string analyzedFileName = appliedToFile ? Path.GetFileName(analyzedFile) : null; + foreach (RuleSuppression unappliedSuppression in unappliedSuppressions) { - if (!suppressed[i]) + string suppressionIDError = string.Format(Strings.RuleSuppressionIDError, unappliedSuppression.RuleSuppressionID); + + if (appliedToFile) { - unSuppressedRecords.Add(diagnostics[i]); + unappliedSuppression.Error = string.Format( + CultureInfo.CurrentCulture, + Strings.RuleSuppressionErrorFormat, + unappliedSuppression.StartAttributeLine, + analyzedFileName, + suppressionIDError); } + else + { + unappliedSuppression.Error = string.Format( + CultureInfo.CurrentCulture, + Strings.RuleSuppressionErrorFormatScriptDefinition, + unappliedSuppression.StartAttributeLine, + suppressionIDError); + } + + errors.Add( + new ErrorRecord( + new ArgumentException(unappliedSuppression.Error), + unappliedSuppression.Error, + ErrorCategory.InvalidArgument, + unappliedSuppression)); } return result; } - private Tuple[] GetOffsetArray(List diagnostics) + private Tuple GetOffset(DiagnosticRecord diagnostic) { - Func> GetTuple = (x, y) => new Tuple(x, y); - Func> GetDefaultTuple = () => GetTuple(0, 0); - var offsets = new Tuple[diagnostics.Count]; - for (int k = 0; k < diagnostics.Count; k++) + IScriptExtent extent = diagnostic.Extent; + + if (extent is null) { - var ext = diagnostics[k].Extent; - if (ext == null) - { - continue; - } - if (ext.StartOffset == 0 && ext.EndOffset == 0) + return null; + } + + if (extent.StartOffset != 0 && extent.EndOffset != 0) + { + return Tuple.Create(extent.StartOffset, extent.EndOffset); + } + + // We're forced to determine the offset ourselves from the lines and columns now + // Rather than counting every line in the file, we scan through the tokens looking for ones matching the offset + + Token startToken = null; + int i = 0; + for (; i < Tokens.Length; i++) + { + Token curr = Tokens[i]; + if (curr.Extent.StartLineNumber == extent.StartLineNumber + && curr.Extent.StartColumnNumber == extent.StartColumnNumber) { - // check if line and column number correspond to 0 offsets - if (ext.StartLineNumber == 1 - && ext.StartColumnNumber == 1 - && ext.EndLineNumber == 1 - && ext.EndColumnNumber == 1) - { - offsets[k] = GetDefaultTuple(); - continue; - } - // created using the ScriptExtent constructor, which sets - // StartOffset and EndOffset to 0 - // find the token the corresponding start line and column number - var startToken = Tokens.Where(x - => x.Extent.StartLineNumber == ext.StartLineNumber - && x.Extent.StartColumnNumber == ext.StartColumnNumber) - .FirstOrDefault(); - if (startToken == null) - { - offsets[k] = GetDefaultTuple(); - continue; - } - var endToken = Tokens.Where(x - => x.Extent.EndLineNumber == ext.EndLineNumber - && x.Extent.EndColumnNumber == ext.EndColumnNumber) - .FirstOrDefault(); - if (endToken == null) - { - offsets[k] = GetDefaultTuple(); - continue; - } - offsets[k] = GetTuple(startToken.Extent.StartOffset, endToken.Extent.EndOffset); + startToken = curr; + break; } - else + } + + if (startToken is null) + { + return Tuple.Create(0, 0); + } + + Token endToken = null; + for (; i < Tokens.Length; i++) + { + Token curr = Tokens[i]; + if (curr.Extent.EndLineNumber == extent.EndLineNumber + && curr.Extent.EndColumnNumber == extent.EndColumnNumber) { - // Extent has valid offsets - offsets[k] = GetTuple(ext.StartOffset, ext.EndOffset); + endToken = curr; + break; } } - return offsets; + + if (endToken is null) + { + return Tuple.Create(0, 0); + } + + return Tuple.Create(startToken.Extent.StartOffset, endToken.Extent.EndOffset); } public static string[] ProcessCustomRulePaths(string[] rulePaths, SessionState sessionState, bool recurse = false) diff --git a/Tests/Engine/RuleSuppression.tests.ps1 b/Tests/Engine/RuleSuppression.tests.ps1 index 49e2570db..891675b3b 100644 --- a/Tests/Engine/RuleSuppression.tests.ps1 +++ b/Tests/Engine/RuleSuppression.tests.ps1 @@ -62,7 +62,6 @@ Describe "RuleSuppressionWithoutScope" { -SuppressedOnly $ruleViolations.Count | Should -Be 1 } - } Context "Script" { @@ -100,6 +99,121 @@ function SuppressPwdParam() -SuppressedOnly $ruleViolations.Count | Should -Be 1 } + + It "Records multiple suppressions applied to a single diagnostic" { + $script = @' +function MyFunc +{ + [System.Diagnostics.CodeAnalysis.SuppressMessage("PSAvoidUsingPlainTextForPassword", "password1", Justification='a')] + [System.Diagnostics.CodeAnalysis.SuppressMessage("PSAvoidUsingPlainTextForPassword", "password1", Justification='b')] + param( + [string]$password1, + [string]$password2 + ) +} +'@ + + $diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $script -IncludeRule 'PSAvoidUsingPlainTextForPassword' + $suppressions = Invoke-ScriptAnalyzer -ScriptDefinition $script -SuppressedOnly -IncludeRule 'PSAvoidUsingPlainTextForPassword' + + $diagnostics | Should -HaveCount 1 + $diagnostics[0].RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $diagnostics[0].RuleSuppressionID | Should -BeExactly "password2" + + $suppressions | Should -HaveCount 1 + $suppressions[0].RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $suppressions[0].RuleSuppressionID | Should -BeExactly "password1" + $suppressions[0].Suppression | Should -HaveCount 2 + $suppressions[0].Suppression.Justification | Sort-Object | Should -Be @('a', 'b') + } + + It "Records multiple suppressions applied to a single diagnostic when they have identical justifications" { + $script = @' +function MyFunc +{ + [System.Diagnostics.CodeAnalysis.SuppressMessage("PSAvoidUsingPlainTextForPassword", "password1", Justification='a')] + [System.Diagnostics.CodeAnalysis.SuppressMessage("PSAvoidUsingPlainTextForPassword", "password1", Justification='a')] + param( + [string]$password1, + [string]$password2 + ) +} +'@ + + $diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $script -IncludeRule 'PSAvoidUsingPlainTextForPassword' + $suppressions = Invoke-ScriptAnalyzer -ScriptDefinition $script -SuppressedOnly -IncludeRule 'PSAvoidUsingPlainTextForPassword' + + $diagnostics | Should -HaveCount 1 + $diagnostics[0].RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $diagnostics[0].RuleSuppressionID | Should -BeExactly "password2" + + $suppressions | Should -HaveCount 1 + $suppressions[0].RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $suppressions[0].RuleSuppressionID | Should -BeExactly "password1" + $suppressions[0].Suppression | Should -HaveCount 2 + $suppressions[0].Suppression.Justification | Sort-Object | Should -Be @('a', 'a') + } + + It "Records no suppressions for a different rule" { + $script = @' +function MyFunc +{ + [System.Diagnostics.CodeAnalysis.SuppressMessage("DifferentRule", "password1", Justification='a')] + [System.Diagnostics.CodeAnalysis.SuppressMessage("DifferentRule", "password1", Justification='b')] + param( + [string]$password1, + [string]$password2 + ) +} +'@ + + $diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $script -IncludeRule 'PSAvoidUsingPlainTextForPassword' + $suppressions = Invoke-ScriptAnalyzer -ScriptDefinition $script -SuppressedOnly -IncludeRule 'PSAvoidUsingPlainTextForPassword' + + $diagnostics | Should -HaveCount 2 + $diagnostics[0].RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $diagnostics[0].RuleSuppressionID | Should -BeExactly "password1" + $diagnostics[1].RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $diagnostics[1].RuleSuppressionID | Should -BeExactly "password2" + + $suppressions | Should -BeNullOrEmpty + } + + It "Issues an error for a unapplied suppression with a suppression ID" { + $script = @' +function MyFunc +{ + [System.Diagnostics.CodeAnalysis.SuppressMessage("PSAvoidUsingPlainTextForPassword", "banana")] + param( + [string]$password1, + [string]$password2 + ) +} +'@ + + $diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $script -IncludeRule 'PSAvoidUsingPlainTextForPassword' -ErrorVariable diagErr -ErrorAction SilentlyContinue + $suppressions = Invoke-ScriptAnalyzer -ScriptDefinition $script -SuppressedOnly -IncludeRule 'PSAvoidUsingPlainTextForPassword' -ErrorVariable suppErr -ErrorAction SilentlyContinue + + $diagnostics | Should -HaveCount 2 + $diagnostics[0].RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $diagnostics[0].RuleSuppressionID | Should -BeExactly "password1" + $diagnostics[1].RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $diagnostics[1].RuleSuppressionID | Should -BeExactly "password2" + + $suppressions | Should -BeNullOrEmpty + + # For some reason these tests fail in WinPS, but the actual output is as expected + if ($PSEdition -eq 'Core') + { + $diagErr | Should -HaveCount 1 + $diagErr.TargetObject.RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $diagErr.TargetObject.RuleSuppressionID | Should -BeExactly "banana" + + $suppErr | Should -HaveCount 1 + $suppErr.TargetObject.RuleName | Should -BeExactly "PSAvoidUsingPlainTextForPassword" + $suppErr.TargetObject.RuleSuppressionID | Should -BeExactly "banana" + } + } } Context "Rule suppression within DSC Configuration definition" {