Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding rule SARIF2001 #1929

Merged
6 commits merged into from
Jun 24, 2020
Merged

Adding rule SARIF2001 #1929

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e32333a
Adding rule SARIF2001
eddynaka Jun 24, 2020
4e6c09a
code review - 1
eddynaka Jun 24, 2020
31d2b52
Merge branch 'features/sarif-validation-rules' into users/ednakamu/sa…
eddynaka Jun 24, 2020
881f1b7
code review - 2
eddynaka Jun 24, 2020
e8d2b45
Merge branch 'features/sarif-validation-rules' into users/ednakamu/sa…
eddynaka Jun 24, 2020
10819c3
merge
eddynaka Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/Sarif.Multitool/Rules/RuleResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion src/Sarif.Multitool/Rules/RuleResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@
<value>The end time of a run must not precede the start time. To allow for the possibility that the duration of the run is less than the resolution of the string representation of the time, the start time and the end time may be equal.</value>
</data>
<data name="SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text" xml:space="preserve">
<value>{0}: The message "{1}" does not end with a period.</value>
<value>{0}: Placeholder '{1}'</value>
</data>
<data name="SARIF2001_AuthorHighQualityMessages_FullDescription_Text" xml:space="preserve">
<value>Messages should consist of one or more complete sentences, ending with a period.</value>
Expand Down Expand Up @@ -216,4 +216,10 @@
<data name="SARIF2005_ProvideHelpfulToolInformation_Warning_UseNumericToolVersions_Text" xml:space="preserve">
<value>{0}: Placeholder '{1}'</value>
</data>
<data name="SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text" xml:space="preserve">
<value>{0}: Placeholder '{1}'</value>
</data>
<data name="SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text" xml:space="preserve">
<value>{0}: Placeholder '{1}'</value>
</data>
</root>
105 changes: 63 additions & 42 deletions src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,85 +3,106 @@

using System;
using System.Collections.Generic;
using System.Text.RegularExpressions;

using Microsoft.Json.Pointer;

namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
{
public class AuthorHighQualityMessages : SarifValidationSkimmerBase
{
public override MultiformatMessageString FullDescription => new MultiformatMessageString
{
Text = RuleResources.SARIF2001_AuthorHighQualityMessages_FullDescription_Text
/// <summary>
/// SARIF2001
/// </summary>
public override string Id => RuleId.AuthorHighQualityMessages;

/// <summary>
/// Placeholder (full description).
/// </summary>
public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF2001_AuthorHighQualityMessages_FullDescription_Text };

protected override IEnumerable<string> MessageResourceNames => new string[] {
nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text),
nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text),
nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text)
};

public override FailureLevel DefaultLevel => FailureLevel.Warning;

public override string Id => RuleId.AuthorHighQualityMessages;
private static readonly Regex s_dynamicContentRegex = new Regex(@"{[0-9]+}", RegexOptions.Compiled | RegexOptions.CultureInvariant);
Copy link

@ghost ghost Jun 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ [](start = 74, length = 1)

Curly braces { and } are regular expression metacharacters, so you must escape them: @"\{[0-9]+\}" #Closed

private static readonly Regex s_enquoteDynamicContentRegex = new Regex(@"'{[0-9]+}'", RegexOptions.Compiled | RegexOptions.CultureInvariant);
Copy link

@ghost ghost Jun 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s_enquoteDynamicContentRegex [](start = 38, length = 28)

How about this:

private static readonly Regex s_nonEnquotedDynamicContextRegex = new Regex(@"(^|[^'])\{[0-9]+\}", RegexOptions.Compiled | RegexOptions.CultureInvariant);

If the start of the line (^) or anything that is not a single quote ([^']) is followed by a replacement sequence (\{[0-9]+\}), that's a bad thing. So you test:

if (s_nonEnquotedDynamicContentRegex.IsMatch(text))
{
    // Report error.
}
``` #Closed


protected override IEnumerable<string> MessageResourceNames
protected override void Analyze(Tool tool, string toolPointer)
{
get
if (tool.Driver != null)
{
return new string[]
{
nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text)
};
AnalyzeToolDriver(tool.Driver, toolPointer.AtProperty(SarifPropertyName.Driver));
}
}

protected override void Analyze(ReportingDescriptor reportingDescriptor, string reportingDescriptorPointer)
private void AnalyzeToolDriver(ToolComponent toolComponent, string toolDriverPointer)
{
AnalyzeMessageStrings(reportingDescriptor.MessageStrings, reportingDescriptorPointer);
if (toolComponent.Rules != null && toolComponent.Rules.Count > 0)
Copy link

@ghost ghost Jun 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& toolComponent.Rules.Count > 0 [](start = 43, length = 33)

You don't need this check because if Rules is empty, then the foreach loop will simply execute 0 times. #Closed

{
foreach (ReportingDescriptor rule in toolComponent.Rules)
{
AnalyzeReportingDescriptor(rule, toolDriverPointer.AtProperty(SarifPropertyName.Rules));
Copy link

@ghost ghost Jun 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toolDriverPointer.AtProperty(SarifPropertyName.Rules) [](start = 53, length = 53)

You actually want to point to the specific rule, say, the rule at index 0. To do that:

  1. Before the foreach loop, define string rulesPointer = toolDriverPointer.AtProperty(SarifPropertyName.Rules;
  2. Inside the foreach loop, keep a counter (or just use a simple for loop. Suppose the counter is index.
  3. Then when you call AnalyzeReportingDescriptor, the pointer argument should be rulesPointer.AtIndex(index). #Closed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result will be /runs/0/tool/driver/rules/0 (for the rule at index 0) and so on.

In reply to: 445100702 [](ancestors = 445100702)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. just changed to a for, so i dont need to work with the int variable.

In reply to: 445101027 [](ancestors = 445101027,445100702)

}
}
}

private void AnalyzeMessageStrings(
IDictionary<string, MultiformatMessageString> messageStrings,
string reportingDescriptorPointer)
private void AnalyzeReportingDescriptor(ReportingDescriptor rule, string reportingDescriptorPointer)
{
if (messageStrings != null)
if (rule.MessageStrings != null && rule.MessageStrings.Count > 0)
Copy link

@ghost ghost Jun 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& rule.MessageStrings.Count > 0 [](start = 43, length = 33)

This check isn't needed. #Closed

{
string messageStringsPointer = reportingDescriptorPointer.AtProperty(SarifPropertyName.MessageStrings);

foreach (string key in messageStrings.Keys)
foreach (KeyValuePair<string, MultiformatMessageString> message in rule.MessageStrings)
{
MultiformatMessageString messageString = messageStrings[key];

string messageStringPointer = messageStringsPointer.AtProperty(key);

AnalyzeMessageString(messageString.Text, messageStringPointer, SarifPropertyName.Text);
AnalyzeMessageString(message.Value.Text, messageStringsPointer.AtProperty(message.Key));
Copy link

@ghost ghost Jun 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

message.Value.Text [](start = 41, length = 18)

You'll also need to pass the message.Key to AnalyzeMessageString because it will surely be part of the message string. For example, "The message string 'x' in rule 'y' has a problem..." #Closed

}
}
}

protected override void Analyze(MultiformatMessageString multiformatMessageString, string multiformatMessageStringPointer)
private void AnalyzeMessageString(string messageString, string messagePointer)
{
AnalyzeMessageString(multiformatMessageString.Text, multiformatMessageStringPointer, SarifPropertyName.Text);
}
if (string.IsNullOrEmpty(messageString))
{
return;
}

protected override void Analyze(Message message, string messagePointer)
{
AnalyzeMessageString(message.Text, messagePointer, SarifPropertyName.Text);
}
string textPointer = messagePointer.AtProperty(SarifPropertyName.Text);

private void AnalyzeMessageString(
string messageString,
string messagePointer,
string propertyName)
{
if (!string.IsNullOrEmpty(messageString) && DoesNotEndWithPeriod(messageString))
// IncludeDynamicContent: check if messageString has dynamic content
Copy link

@ghost ghost Jun 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t [](start = 79, length = 1)

The comment is a complete sentence, so end in a period. (Same for comments below.) #Closed

if (!s_dynamicContentRegex.IsMatch(messageString))
{
// {0}: Placeholder '{1}'
LogResult(
textPointer,
nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text),
messageString
);
}

// EnquoteDynamicContent: check if messageString has enquoted dynamic content
if (!s_enquoteDynamicContentRegex.IsMatch(messageString))
{
string textPointer = messagePointer.AtProperty(propertyName);
// {0}: Placeholder '{1}'
LogResult(
textPointer,
nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text),
messageString
);
Copy link

@ghost ghost Jun 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

); [](start = 20, length = 2)

Put closing paren on same line as the last argument. You did it correctly on Line 104 below. #Closed

}

// TerminateWithPeriod: check if messageString ends with period
if (!messageString.EndsWith(".", StringComparison.Ordinal))
{
// {0}: Placeholder '{1}'
LogResult(
textPointer,
nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text),
messageString);
}
}

private static bool DoesNotEndWithPeriod(string message)
{
return message != null && !message.EndsWith(".", StringComparison.Ordinal);
}
}
}