From d8058b90b79ad499478401a24900a9b15a3c65f2 Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:31:06 -0700 Subject: [PATCH 1/8] Fix rewriting files with no issues to suppress Fixes an issue where the Suppress command would rewrite a file essentially as is (but potentially change whitespace) even if all the detected issues in the file were filtered out by the rule filter. --- .../Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs index 1efb1eb1..b3541c94 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs @@ -61,6 +61,12 @@ public int Run() issueRecords = issueRecords.Where(x => _opts.RulesToApplyFrom.Any(y => y == x.RuleId)).ToList(); } + // No issues remain for file after filtering to specified rule ids, skip file + if (!issueRecords.Any()) + { + continue; + } + issueRecords .Sort((a, b) => a.PhysicalLocation.Region.StartLine - b.PhysicalLocation.Region.StartLine); From e7308ca61d5aaa6744b2a1223b20d2f61a6d7b8b Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Mon, 26 Aug 2024 10:38:09 -0700 Subject: [PATCH 2/8] Respect original newline characters when adding suppression comments --- .../Commands/SuppressionCommand.cs | 72 +++++++++++++++++-- 1 file changed, 65 insertions(+), 7 deletions(-) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs index b3541c94..0d9031cb 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs @@ -1,9 +1,11 @@ // Copyright (C) Microsoft. All rights reserved. Licensed under the MIT License. using System; +using System.Collections.Generic; using System.IO; using System.Linq; using System.Text; +using System.Text.RegularExpressions; using Microsoft.ApplicationInspector.RulesEngine; using Microsoft.CodeAnalysis.Sarif; using Microsoft.DevSkim.CLI.Options; @@ -82,8 +84,8 @@ public int Run() { _logger.LogError($"{potentialPath} specified in sarif does not appear to exist on disk."); } - - string[] theContent = File.ReadAllLines(potentialPath); + string content = File.ReadAllText(potentialPath); + string[] theContent = SplitMyStringIntoPieces(content); int currLine = 0; StringBuilder sb = new StringBuilder(); @@ -99,12 +101,24 @@ public int Run() { foreach (string line in theContent[currLine..zeroBasedStartLine]) { - sb.Append($"{line}{Environment.NewLine}"); + sb.Append($"{line}"); } - string suppressionComment = isMultiline ? $"{ignoreComment}{theContent[zeroBasedStartLine]}{Environment.NewLine}" : - $"{theContent[zeroBasedStartLine]} {ignoreComment}{Environment.NewLine}"; - sb.Append(suppressionComment); + string originalLine = theContent[zeroBasedStartLine]; + int lineEndPosition = FindNewLine(originalLine); + // Use the content then the ignore comment then the original newline characters from the extra array + if (lineEndPosition != -1) + { + sb.Append(isMultiline + ? $"{ignoreComment}{theContent[zeroBasedStartLine]}" + : $"{originalLine[0..lineEndPosition]} {ignoreComment}{originalLine[lineEndPosition..]}"); + } + else + { + sb.Append(isMultiline + ? $"{ignoreComment}{theContent[zeroBasedStartLine]}" + : $"{theContent[zeroBasedStartLine]} {ignoreComment}"); + } } currLine = zeroBasedStartLine + 1; @@ -115,7 +129,7 @@ public int Run() { foreach (string line in theContent[currLine..^1]) { - sb.Append($"{line}{Environment.NewLine}"); + sb.Append($"{line}"); } sb.Append($"{theContent.Last()}"); } @@ -133,5 +147,49 @@ public int Run() return (int)ExitCode.NoIssues; } + + /// + /// Find the first location of a newline (\n or \r\n) in a string + /// + /// + /// Character index of the first newline sequence or -1 if none found + private int FindNewLine(string originalLine) + { + int indexOfNewLine = originalLine.IndexOf('\n'); + if (indexOfNewLine >= 1) + { + if (originalLine[indexOfNewLine - 1] == '\r') + { + indexOfNewLine = indexOfNewLine - 1; + } + } + + return indexOfNewLine; + } + + /// + /// Split string into lines including the newline characters + /// + /// + /// Array of strings, each containing the content of one line from the input string including any newline characters from that line + private string[] SplitMyStringIntoPieces(string content) + { + List lines = new(); + int curPos = 0; + for (int i = 0; i < content.Length; i++) + { + if (content[i] == '\n') + { + lines.Add(content[curPos..(i+1)]); + curPos = i + 1; + } + + if (i == content.Length - 1) + { + lines.Add(content[curPos..(i+1)]); + } + } + return lines.ToArray(); + } } } From 91ab99613fba2176e452a92c6949e1f012897c2f Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Mon, 26 Aug 2024 10:53:08 -0700 Subject: [PATCH 3/8] Add tests cases for fixes --- .../SuppressionsTest.cs | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs index b7822b14..92b6a49a 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs @@ -286,5 +286,55 @@ public void ExecuteSuppresionsForMultilineFormattedFiles(bool preferMultiLine) new AnalyzeCommand(opts).Run(); return (basePath, tempFileName, Path.Combine(basePath, outFileName)); } + + /// + /// Test that suppressing an issue doesn't change the line break characters + /// + /// + [DataTestMethod] + [DataRow("\r\n")] + [DataRow("\n")] + public void DontPerturbExtantLineBreaks(string lineBreakSequence) + { + (string basePath, string sourceFile, string sarifPath) = runAnalysis($"MD5{lineBreakSequence}http://contoso.com{lineBreakSequence}", "c"); + + SuppressionCommandOptions opts = new SuppressionCommandOptions + { + Path = basePath, + SarifInput = sarifPath, + ApplyAllSuppression = true + }; + + int resultCode = new SuppressionCommand(opts).Run(); + Assert.AreEqual(0, resultCode); + string result = File.ReadAllText(sourceFile); + Assert.AreEqual(lineBreakSequence, result[^lineBreakSequence.Length..]); + } + + /// + /// Test that files don't change at all when they have findings but those rule ids are not selected for suppression + /// + /// + [DataTestMethod] + [DataRow("\r\n")] + [DataRow("\n")] + public void DontChangeFilesWithoutSelectedFindings(string lineBreakSequence) + { + string originalContent = $"MD5{lineBreakSequence}http://contoso.com{lineBreakSequence}"; + (string basePath, string sourceFile, string sarifPath) = runAnalysis(originalContent, "c"); + + SuppressionCommandOptions opts = new SuppressionCommandOptions + { + Path = basePath, + SarifInput = sarifPath, + ApplyAllSuppression = false, + RulesToApplyFrom = new string[] { "NotAValidRuleId" } // Don't apply any rules + }; + + int resultCode = new SuppressionCommand(opts).Run(); + Assert.AreEqual(0, resultCode); + string result = File.ReadAllText(sourceFile); + Assert.AreEqual(originalContent, result); + } } } \ No newline at end of file From dbca276315aacb89a8a20039720d0c1e14743d6d Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Mon, 26 Aug 2024 10:54:34 -0700 Subject: [PATCH 4/8] Merge change from #628 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 28971027..c04b2da7 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ The Visual Studio extension is available in the [Visual Studio Marketplace](http The Visual Studio Code plugin is available in the [Visual Studio Code Marketplace](https://marketplace.visualstudio.com/items?itemName=MS-CST-E.vscode-devskim). -DevSkim is also available as a [GitHub Action](https://github.com/microsoft/DevSkim-Action) to itegrate with the GitHub Security Issues pane. +DevSkim is also available as a [GitHub Action](https://github.com/microsoft/DevSkim-Action) to integrate with the GitHub Security Issues pane. Platform specific binaries of the DevSkim CLI are also available on our GitHub [releases page](https://github.com/microsoft/DevSkim/releases). From f9c55d39c5915d61549334994c9e43ecbf4a6f5d Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Mon, 26 Aug 2024 10:56:12 -0700 Subject: [PATCH 5/8] Update Changelog.md --- Changelog.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Changelog.md b/Changelog.md index 09a37381..a76e9f6a 100644 --- a/Changelog.md +++ b/Changelog.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.0.42] - 2024-08-26 +## Fix +Fixes suppression command to not perturb line breaks, particularly when a file has findings which are not selected for suppression. + ## [1.0.41] - 2024-08-23 ## Rules Extend the false positive fix for the issue reported in #548 to Sdk-style msbuild projects. From d6dd113efb818cb983b78ea165572f97f142989d Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Mon, 26 Aug 2024 15:44:08 -0700 Subject: [PATCH 6/8] Update Changelog.md --- Changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index a76e9f6a..9f5066e2 100644 --- a/Changelog.md +++ b/Changelog.md @@ -6,7 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [1.0.42] - 2024-08-26 ## Fix -Fixes suppression command to not perturb line breaks, particularly when a file has findings which are not selected for suppression. +Fixes suppression command to not perturb line breaks, particularly when a file has findings which are not selected for suppression. #631 ## [1.0.41] - 2024-08-23 ## Rules From 52cf205c6f6a7d1a71356031f330e22e6e010d49 Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Mon, 26 Aug 2024 16:44:32 -0700 Subject: [PATCH 7/8] Clean up Rewrite edge case handling for placing multi line comments at the start of the line when the line ends with `\` and might be a multi-line continuation to avoid putting the comment inside the multiline string. --- .../Commands/SuppressionCommand.cs | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs index 0d9031cb..7d03287c 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs @@ -5,7 +5,6 @@ using System.IO; using System.Linq; using System.Text; -using System.Text.RegularExpressions; using Microsoft.ApplicationInspector.RulesEngine; using Microsoft.CodeAnalysis.Sarif; using Microsoft.DevSkim.CLI.Options; @@ -83,9 +82,10 @@ public int Run() if (!File.Exists(potentialPath)) { _logger.LogError($"{potentialPath} specified in sarif does not appear to exist on disk."); + continue; } string content = File.ReadAllText(potentialPath); - string[] theContent = SplitMyStringIntoPieces(content); + string[] theContent = SplitStringByLinesWithNewLines(content); int currLine = 0; StringBuilder sb = new StringBuilder(); @@ -95,29 +95,35 @@ public int Run() { Region region = issueRecord.PhysicalLocation.Region; int zeroBasedStartLine = region.StartLine - 1; - bool isMultiline = theContent[zeroBasedStartLine].EndsWith(@"\"); - string ignoreComment = DevSkimRuleProcessor.GenerateSuppressionByLanguage(region.SourceLanguage, issueRecord.RulesId, _opts.PreferMultiline || isMultiline, _opts.Duration, _opts.Reviewer, devSkimLanguages); + string originalLine = theContent[zeroBasedStartLine]; + int lineEndPosition = FindNewLine(originalLine); + // If line ends with `\` it may have continuation on the next line, + // so put the comment at the line start and use multiline format + bool forceMultiLine = lineEndPosition >= 0 && originalLine[..lineEndPosition].EndsWith(@"\"); + string ignoreComment = DevSkimRuleProcessor.GenerateSuppressionByLanguage(region.SourceLanguage, issueRecord.RulesId, _opts.PreferMultiline || forceMultiLine, _opts.Duration, _opts.Reviewer, devSkimLanguages); if (!string.IsNullOrEmpty(ignoreComment)) { foreach (string line in theContent[currLine..zeroBasedStartLine]) { sb.Append($"{line}"); } - - string originalLine = theContent[zeroBasedStartLine]; - int lineEndPosition = FindNewLine(originalLine); - // Use the content then the ignore comment then the original newline characters from the extra array - if (lineEndPosition != -1) + + if (forceMultiLine) { - sb.Append(isMultiline - ? $"{ignoreComment}{theContent[zeroBasedStartLine]}" - : $"{originalLine[0..lineEndPosition]} {ignoreComment}{originalLine[lineEndPosition..]}"); + sb.Append($"{ignoreComment} {originalLine}"); } else { - sb.Append(isMultiline - ? $"{ignoreComment}{theContent[zeroBasedStartLine]}" - : $"{theContent[zeroBasedStartLine]} {ignoreComment}"); + // Use the content then the ignore comment then the original newline characters from the extra array + if (lineEndPosition != -1) + { + sb.Append($"{originalLine[0..lineEndPosition]} {ignoreComment}{originalLine[lineEndPosition..]}"); + } + // No new line so we can just use the line as is + else + { + sb.Append($"{originalLine} {ignoreComment}"); + } } } @@ -172,7 +178,7 @@ private int FindNewLine(string originalLine) /// /// /// Array of strings, each containing the content of one line from the input string including any newline characters from that line - private string[] SplitMyStringIntoPieces(string content) + private string[] SplitStringByLinesWithNewLines(string content) { List lines = new(); int curPos = 0; From bc46274bc3a34840905d686db0b9ab2d192b7fc5 Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Mon, 26 Aug 2024 16:44:44 -0700 Subject: [PATCH 8/8] Also test multiline case --- .../SuppressionsTest.cs | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs index 92b6a49a..608b2ce1 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs @@ -290,19 +290,23 @@ public void ExecuteSuppresionsForMultilineFormattedFiles(bool preferMultiLine) /// /// Test that suppressing an issue doesn't change the line break characters /// - /// + /// Character sequence used for line breaks + /// Use multiline comments or not [DataTestMethod] - [DataRow("\r\n")] - [DataRow("\n")] - public void DontPerturbExtantLineBreaks(string lineBreakSequence) + [DataRow("\r\n", true)] + [DataRow("\r\n", false)] + [DataRow("\n", true)] + [DataRow("\n", false)] + public void DontPerturbExtantLineBreaks(string lineBreakSequence, bool preferMultiLine) { - (string basePath, string sourceFile, string sarifPath) = runAnalysis($"MD5{lineBreakSequence}http://contoso.com{lineBreakSequence}", "c"); + (string basePath, string sourceFile, string sarifPath) = runAnalysis($"MD5\\{lineBreakSequence}http://contoso.com{lineBreakSequence}", "c"); SuppressionCommandOptions opts = new SuppressionCommandOptions { Path = basePath, SarifInput = sarifPath, - ApplyAllSuppression = true + ApplyAllSuppression = true, + PreferMultiline = preferMultiLine }; int resultCode = new SuppressionCommand(opts).Run(); @@ -314,11 +318,14 @@ public void DontPerturbExtantLineBreaks(string lineBreakSequence) /// /// Test that files don't change at all when they have findings but those rule ids are not selected for suppression /// - /// + /// Character sequence used for line breaks + /// Use multiline comments or not [DataTestMethod] - [DataRow("\r\n")] - [DataRow("\n")] - public void DontChangeFilesWithoutSelectedFindings(string lineBreakSequence) + [DataRow("\r\n", true)] + [DataRow("\r\n", false)] + [DataRow("\n", true)] + [DataRow("\n", false)] + public void DontChangeFilesWithoutSelectedFindings(string lineBreakSequence, bool preferMultiline) { string originalContent = $"MD5{lineBreakSequence}http://contoso.com{lineBreakSequence}"; (string basePath, string sourceFile, string sarifPath) = runAnalysis(originalContent, "c"); @@ -328,6 +335,7 @@ public void DontChangeFilesWithoutSelectedFindings(string lineBreakSequence) Path = basePath, SarifInput = sarifPath, ApplyAllSuppression = false, + PreferMultiline = preferMultiline, RulesToApplyFrom = new string[] { "NotAValidRuleId" } // Don't apply any rules };