Skip to content

Commit

Permalink
Fix rewriting files with no issues to suppress (#632)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@gfs
gfs authored Aug 27, 2024
1 parent 7e08341 commit fb82d09
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 11 deletions.
4 changes: 4 additions & 0 deletions Changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
90 changes: 80 additions & 10 deletions DevSkim-DotNet/Microsoft.DevSkim.CLI/Commands/SuppressionCommand.cs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -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();

Expand All @@ -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;
Expand All @@ -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()}");
}
Expand All @@ -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();
}
}
}
58 changes: 58 additions & 0 deletions DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down

0 comments on commit fb82d09

Please sign in to comment.