Adding rule SARIF1012 #1944
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // Copyright (c) Microsoft. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text.RegularExpressions; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules | ||
| { | ||
| public class MessageArgumentsMustBeConsistentWithRule : SarifValidationSkimmerBase | ||
| { | ||
| /// <summary> | ||
| /// SARIF1012 | ||
| /// </summary> | ||
| public override string Id => RuleId.MessageArgumentsMustBeConsistentWithRule; | ||
|
|
||
| /// <summary> | ||
| /// Placeholder | ||
| /// </summary> | ||
| public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_FullDescription_Text }; | ||
|
|
||
| protected override IEnumerable<string> MessageResourceNames => new string[] { | ||
| nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_MessageIdMustExist_Text), | ||
| nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_SupplyEnoughMessageArguments_Text) | ||
| }; | ||
|
|
||
| public override FailureLevel DefaultLevel => FailureLevel.Error; | ||
|
|
||
| private IList<ReportingDescriptor> currentRules; | ||
|
|
||
| protected override void Analyze(Run run, string runPointer) | ||
| { | ||
| this.currentRules = run?.Tool?.Driver?.Rules; | ||
| } | ||
|
|
||
| protected override void Analyze(Result result, string resultPointer) | ||
| { | ||
| // if message.id is not null, we have to check if that exists in rules | ||
|
There was a problem hiding this comment.
Use full sentences for comments (capitalize first letter, end with period). Also I suggest this phrasing:
|
||
| if (!string.IsNullOrEmpty(result.Message.Id)) | ||
| { | ||
| ReportingDescriptor rule = result.RuleIndex >= 0 | ||
|
There was a problem hiding this comment.
The SARIF SDK has a great helper for this:
There was a problem hiding this comment. Ok. With that in mind, we will probably change the following condition to this: if (this.currentRules == null
|| rule.MessageStrings == null
|| !rule.MessageStrings.ContainsKey(result.Message.Id))
{Because we dont initialize MessageStrings in the last scenario. What do you think? In reply to: 447332733 [](ancestors = 447332733) There was a problem hiding this comment. This looks good. You could simplify to You need the explicit comparison to In reply to: 447336153 [](ancestors = 447336153,447332733) There was a problem hiding this comment. |
||
| ? (this.currentRules?[result.RuleIndex]) | ||
| : (this.currentRules?.FirstOrDefault(r => r.Id == (result.RuleId ?? result.Rule?.Id))); | ||
|
|
||
| if (this.currentRules == null | ||
| || rule == null | ||
| || !rule.MessageStrings.ContainsKey(result.Message.Id)) | ||
| { | ||
| // {0}: Placeholder {1} {2} | ||
| LogResult( | ||
| resultPointer, | ||
| nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_MessageIdMustExist_Text), | ||
| result.Message.Id, | ||
| (result.RuleIndex >= 0 | ||
| ? result.RuleIndex.ToString() | ||
| : (result.RuleId ?? result.Rule?.Id ?? "null"))); | ||
|
There was a problem hiding this comment. Here you want to display the rule id, and the SARIF SDK has a helper for that too: There was a problem hiding this comment. |
||
| return; | ||
| } | ||
|
|
||
| // we have rules, we find it but, the count of parameters are invalid | ||
|
There was a problem hiding this comment.
"A message with the specified key is present in the rule. Check if the result supplied enough arguments." #Closed |
||
| string messageText = rule.MessageStrings[result.Message.Id].Text; | ||
| int countOfPlaceholders = CountOfPlaceholders(messageText); | ||
|
There was a problem hiding this comment.
It's not the number of placeholders that matters. It's the highest numbered placeholder. If the highest numbered placeholder is There was a problem hiding this comment. |
||
| if (countOfPlaceholders > (result.Message.Arguments?.Count ?? 0)) | ||
| { | ||
| // {0}: Placeholder {1} {2} {3} {4} {5} | ||
| LogResult( | ||
| resultPointer, | ||
| nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_SupplyEnoughMessageArguments_Text), | ||
| result.Message.Arguments.Count.ToString(), | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. There was a problem hiding this comment. |
||
| result.Message.Id, | ||
| (result.RuleIndex >= 0 | ||
| ? result.RuleIndex.ToString() | ||
| : (result.RuleId ?? result.Rule?.Id ?? "null")), | ||
| countOfPlaceholders.ToString(), | ||
| messageText); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private int CountOfPlaceholders(string text) | ||
| { | ||
| MatchCollection matchCollection = Regex.Matches(text, "{\\d+}"); | ||
|
There was a problem hiding this comment.
You can use a regex "capture" to get just the integer part: There was a problem hiding this comment. Also, In reply to: 447722378 [](ancestors = 447722378) There was a problem hiding this comment. Finally, note that "verbatim strings" ( In reply to: 447723010 [](ancestors = 447723010,447722378) |
||
| HashSet<string> matches = new HashSet<string>(); | ||
| foreach (Match match in matchCollection) | ||
| { | ||
| matches.Add(match.Value); | ||
| } | ||
|
|
||
| return matches.Count; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| { | ||
| "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json", | ||
| "version": "2.1.0", | ||
| "runs": [ | ||
| { | ||
| "tool": { | ||
| "driver": { | ||
| "name": "SARIF Functional Testing", | ||
| "rules": [ | ||
| { | ||
| "id": "SARIF1012", | ||
| "name": "MessageArgumentsMustBeConsistentWithRule", | ||
| "shortDescription": { | ||
| "text": "Placeholder." | ||
| }, | ||
| "fullDescription": { | ||
| "text": "Placeholder" | ||
| }, | ||
| "messageStrings": { | ||
| "Error_MessageIdMustExist": { | ||
| "text": "{0}: Placeholder '{1}' '{2}'" | ||
| }, | ||
| "Error_SupplyEnoughMessageArguments": { | ||
| "text": "{0}: Placeholder '{1}' '{2}' '{3}' '{4}' '{5}'" | ||
| } | ||
| }, | ||
| "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" | ||
| }, | ||
| { | ||
| "id": "SARIF2001", | ||
| "name": "AuthorHighQualityMessages", | ||
| "shortDescription": { | ||
| "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable." | ||
| }, | ||
| "fullDescription": { | ||
| "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable.\r\n\r\nIncluding \"dynamic content\" (information that varies among results from the same rule) makes your messages more specific. It avoids the \"wall of bugs\" phenomenon, where hundreds of occurrences of the same message appear unapproachable.\r\n\r\nPlacing dynamic content in quotes sets it off from the static text, making it easier to spot. It's especially helpful when the dynamic content is a string that might contain spaces, and most especially when the string might be empty (and so would be invisible if it weren't for the quotes). We recommend single quotes for a less cluttered appearance, even though English usage would require double quotes.\r\n\r\nFinally, write in complete sentences and end each sentence with a period. This guidance does not apply to Markdown messages, which might include formatting that makes the punctuation unnecessary." | ||
| }, | ||
| "messageStrings": { | ||
| "Warning_EnquoteDynamicContent": { | ||
| "text": "{0}: In rule '{1}', the message with id '{2}' includes dynamic content that is not enclosed in single quotes. Enquoting dynamic content makes it easier to spot, and single quotes give a less cluttered appearance." | ||
| }, | ||
| "Warning_IncludeDynamicContent": { | ||
| "text": "{0}: In rule '{1}', the message with id '{2}' does not include any dynamic content. Dynamic content makes your messages more specific and avoids the \"wall of bugs\" phenomenon." | ||
| }, | ||
| "Warning_TerminateWithPeriod": { | ||
| "text": "{0}: In rule '{1}', the message with id '{2}' does not end in a period. Write rule messages as complete sentences." | ||
| } | ||
| }, | ||
| "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| "invocations": [ | ||
| { | ||
| "executionSuccessful": true | ||
| } | ||
| ], | ||
| "artifacts": [ | ||
| { | ||
| "location": { | ||
| "uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif", | ||
| "uriBaseId": "TEST_DIR" | ||
| } | ||
| } | ||
| ], | ||
| "results": [ | ||
| { | ||
| "ruleId": "SARIF1012", | ||
| "ruleIndex": 0, | ||
| "level": "error", | ||
| "message": { | ||
| "id": "Error_MessageIdMustExist", | ||
| "arguments": [ | ||
| "runs[0].results[0]", | ||
| "DoesNotExist", | ||
| "0" | ||
| ] | ||
| }, | ||
| "locations": [ | ||
| { | ||
| "physicalLocation": { | ||
| "artifactLocation": { | ||
| "index": 0 | ||
| }, | ||
| "region": { | ||
| "startLine": 44, | ||
| "startColumn": 9 | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "ruleId": "SARIF1012", | ||
| "ruleIndex": 0, | ||
| "level": "error", | ||
| "message": { | ||
| "id": "Error_SupplyEnoughMessageArguments", | ||
| "arguments": [ | ||
| "runs[0].results[1]", | ||
| "1", | ||
| "DoesExist", | ||
| "0", | ||
| "3", | ||
| "{0}: Placeholder {1} {2}." | ||
| ] | ||
| }, | ||
| "locations": [ | ||
| { | ||
| "physicalLocation": { | ||
| "artifactLocation": { | ||
| "index": 0 | ||
| }, | ||
| "region": { | ||
| "startLine": 69, | ||
| "startColumn": 9 | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "ruleId": "SARIF2001", | ||
| "ruleIndex": 1, | ||
| "message": { | ||
| "id": "Warning_EnquoteDynamicContent", | ||
| "arguments": [ | ||
| "runs[0].tool.driver.rules[0].messageStrings.DoesExist.text", | ||
| "TEST1001", | ||
| "DoesExist" | ||
| ] | ||
| }, | ||
| "locations": [ | ||
| { | ||
| "physicalLocation": { | ||
| "artifactLocation": { | ||
| "index": 0 | ||
| }, | ||
| "region": { | ||
| "startLine": 22, | ||
| "startColumn": 53 | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| ], | ||
| "columnKind": "utf16CodeUnits" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually:
Analyzewon't be called ifrunisnull.Toolis a required property ofRun, so if it's missing, the schema validation (which takes place before any validation rules are run) will catch it, and the MultiTool will stop.Driveris a required property ofTool.So really all you need is
run.Tool.Driver?.Rules. #ClosedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will push soon!
In reply to: 447331417 [](ancestors = 447331417)