Fix rewriting files with no issues to suppress (#632)

* 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.

* Respect original newline characters when adding suppression comments

* 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.

* Add tests cases for fixes

* Fix readme typo from #628

Author: gfs

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. #631
+
 ## [1.0.41] - 2024-08-23
 ## Rules
 Extend the false positive fix for the issue reported in #548 to Sdk-style msbuild projects.

DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs

@@ -1,6 +1,7 @@
 // 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;
@@ -61,6 +62,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);
 
@@ -75,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[] theContent = File.ReadAllLines(potentialPath);
+                    string content = File.ReadAllText(potentialPath);
+                    string[] theContent = SplitStringByLinesWithNewLines(content);
                     int currLine = 0;
                     StringBuilder sb = new StringBuilder();
 
@@ -87,18 +95,36 @@ 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}{Environment.NewLine}");
+                                    sb.Append($"{line}");
+                                }
+                                
+                                if (forceMultiLine)
+                                {
+                                    sb.Append($"{ignoreComment} {originalLine}");
+                                }
+                                else
+                                {
+                                    // 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}");
+                                    }
                                 }
-
-                                string suppressionComment = isMultiline ? $"{ignoreComment}{theContent[zeroBasedStartLine]}{Environment.NewLine}" :
-                                    $"{theContent[zeroBasedStartLine]} {ignoreComment}{Environment.NewLine}";
-                                sb.Append(suppressionComment);
                             }
 
                             currLine = zeroBasedStartLine + 1;
@@ -109,7 +135,7 @@ public int Run()
                     {
                         foreach (string line in theContent[currLine..^1])
                         {
-                            sb.Append($"{line}{Environment.NewLine}");
+                            sb.Append($"{line}");
                         }
                         sb.Append($"{theContent.Last()}");
                     }
@@ -127,5 +153,49 @@ public int Run()
 
             return (int)ExitCode.NoIssues;
         }
+
+        /// <summary>
+        /// Find the first location of a newline (\n or \r\n) in a string
+        /// </summary>
+        /// <param name="originalLine"></param>
+        /// <returns>Character index of the first newline sequence or -1 if none found</returns>
+        private int FindNewLine(string originalLine)
+        {
+            int indexOfNewLine = originalLine.IndexOf('\n');
+            if (indexOfNewLine >= 1)
+            {
+                if (originalLine[indexOfNewLine - 1] == '\r')
+                {
+                    indexOfNewLine = indexOfNewLine - 1;
+                }
+            }
+
+            return indexOfNewLine;
+        }
+
+        /// <summary>
+        /// Split string into lines including the newline characters
+        /// </summary>
+        /// <param name="content"></param>
+        /// <returns>Array of strings, each containing the content of one line from the input string including any newline characters from that line</returns>
+        private string[] SplitStringByLinesWithNewLines(string content)
+        {
+            List<string> 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();
+        }
     }
 }

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs

@@ -286,5 +286,63 @@ public void ExecuteSuppresionsForMultilineFormattedFiles(bool preferMultiLine)
             new AnalyzeCommand(opts).Run();
             return (basePath, tempFileName, Path.Combine(basePath, outFileName));
         }
+        
+        /// <summary>
+        /// Test that suppressing an issue doesn't change the line break characters
+        /// </summary>
+        /// <param name="lineBreakSequence">Character sequence used for line breaks</param>
+        /// <param name="preferMultiLine">Use multiline comments or not</param>
+        [DataTestMethod]
+        [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");
+
+            SuppressionCommandOptions opts = new SuppressionCommandOptions
+            {
+                Path = basePath,
+                SarifInput = sarifPath,
+                ApplyAllSuppression = true,
+                PreferMultiline = preferMultiLine
+            };
+
+            int resultCode = new SuppressionCommand(opts).Run();
+            Assert.AreEqual(0, resultCode);
+            string result = File.ReadAllText(sourceFile);
+            Assert.AreEqual(lineBreakSequence, result[^lineBreakSequence.Length..]);
+        }
+        
+        /// <summary>
+        /// Test that files don't change at all when they have findings but those rule ids are not selected for suppression
+        /// </summary>
+        /// <param name="lineBreakSequence">Character sequence used for line breaks</param>
+        /// <param name="preferMultiLine">Use multiline comments or not</param>
+        [DataTestMethod]
+        [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");
+
+            SuppressionCommandOptions opts = new SuppressionCommandOptions
+            {
+                Path = basePath,
+                SarifInput = sarifPath,
+                ApplyAllSuppression = false,
+                PreferMultiline = preferMultiline,
+                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);
+        }
     }
 }

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).