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.

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 SARIF2012 #1949

Merged
merged 3 commits into from
Jun 30, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
18 changes: 18 additions & 0 deletions src/Sarif.Multitool/Rules/RuleResources.Designer.cs

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

6 changes: 6 additions & 0 deletions src/Sarif.Multitool/Rules/RuleResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -334,4 +334,10 @@ Many tool use similar names for 'uriBaseId' symbols. We suggest 'REPOROOT' for t
<data name="SARIF2015_EnquoteDynamicMessageContent_Note_Default_Text" xml:space="preserve">
<value>{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.</value>
</data>
<data name="SARIF2012_ProvideHelpUris_FullDescription_Text" xml:space="preserve">
<value>Placeholder</value>
</data>
<data name="SARIF2012_ProvideHelpUris_Note_Default_Text" xml:space="preserve">
<value>{0}: Placeholder '{1}'</value>
</data>
</root>
66 changes: 66 additions & 0 deletions src/Sarif.Multitool/Rules/SARIF2012.ProvideHelpUris.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// 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 Microsoft.Json.Pointer;

namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
{
public class ProvideHelpUris : SarifValidationSkimmerBase
{
/// <summary>
/// SARIF2012
/// </summary>
public override string Id => RuleId.ProvideHelpUris;

/// <summary>
/// Placeholder
/// </summary>
public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF2012_ProvideHelpUris_FullDescription_Text };

protected override IEnumerable<string> MessageResourceNames => new string[] {
nameof(RuleResources.SARIF2012_ProvideHelpUris_Note_Default_Text)
};

public override FailureLevel DefaultLevel => FailureLevel.Note;

protected override void Analyze(Tool tool, string toolPointer)
Copy link

@ghost ghost Jun 30, 2020

Choose a reason for hiding this comment

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

Analyze [](start = 32, length = 7)

Let's back up one more level, to the run level. There are two Tool objects in SARIF, and one of them is related to conversion from a tool's native output format to SARIF. We're not interested in what the converter has to say. #Closed

Copy link

Choose a reason for hiding this comment

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

We're only interested in run.tool.driver.rules.


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

Copy link

Choose a reason for hiding this comment

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

This is fine. By the way, @hakohli@microsoft.com has a different way of following these nestings. See her most recent PR. Just FYI.


In reply to: 447987255 [](ancestors = 447987255,447985445)

{
if (tool.Driver != null)
{
AnalyzeToolDriver(tool.Driver, toolPointer.AtProperty(SarifPropertyName.Driver));
}
}

private void AnalyzeToolDriver(ToolComponent toolComponent, string toolDriverPointer)
{
if (toolComponent.Rules != null)
{
string rulesPointer = toolDriverPointer.AtProperty(SarifPropertyName.Rules);
for (int i = 0; i < toolComponent.Rules.Count; i++)
{
AnalyzeReportingDescriptor(toolComponent.Rules[i], rulesPointer.AtIndex(i));
}
}
}

private void AnalyzeReportingDescriptor(ReportingDescriptor reportingDescriptor, string reportingDescriptorPointer)
{
if (reportingDescriptor.HelpUri == null)
{
string ruleMoniker = reportingDescriptor.Id;
if (!string.IsNullOrWhiteSpace(reportingDescriptor.Name))
{
ruleMoniker += $".{reportingDescriptor.Name}";
}

// {0}: Placeholder '{1}'
LogResult(
reportingDescriptorPointer,
Copy link

@ghost ghost Jun 30, 2020

Choose a reason for hiding this comment

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

reportingDescriptorPointer [](start = 20, length = 26)

Include the rule's id and, if present, the friendly name. Use one message argument that looks like <id> if there is no name, and <id>.<name> if there is a name. The message will end up being (for example): "The rule 'SARIF1012' does not have a 'helpUri' property. Help URIs help engineers to ... blah blah..." #Closed

nameof(RuleResources.SARIF2012_ProvideHelpUris_Note_Default_Text),
ruleMoniker);
}
}
}
}
15 changes: 12 additions & 3 deletions src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ public ValidateCommandTests(ITestOutputHelper outputHelper, bool testProducesSar
base(outputHelper, testProducesSarifCurrentVersion)
{ }

// Pass this parameter to RunTest for those rules that can produce "pass"-level results.
private const bool Verbose = true;
Copy link

@ghost ghost Jun 30, 2020

Choose a reason for hiding this comment

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

Thanks. #ByDesign


protected override string IntermediateTestFolder => @"Multitool";

private class TestParameters
Expand Down Expand Up @@ -236,6 +233,18 @@ public void SARIF2011_ProvideContextRegion_Invalid()
MakeInvalidTestFileName(RuleId.ProvideContextRegion, nameof(RuleId.ProvideContextRegion)),
parameter: new TestParameters(verbose: true));

[Fact]
public void SARIF2012_ProvideHelpUris_Valid()
=> RunTest(
MakeValidTestFileName(RuleId.ProvideHelpUris, nameof(RuleId.ProvideHelpUris)),
parameter: new TestParameters(verbose: true));

[Fact]
public void SARIF2012_ProvideHelpUris_Invalid()
=> RunTest(
MakeInvalidTestFileName(RuleId.ProvideHelpUris, nameof(RuleId.ProvideHelpUris)),
parameter: new TestParameters(verbose: true));

[Fact]
public void SARIF2014_ProvideDynamicMessageContent_Valid()
=> RunTest(MakeValidTestFileName(RuleId.ProvideDynamicMessageContent, nameof(RuleId.ProvideDynamicMessageContent)),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
{
"$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": "SARIF2012",
"name": "ProvideHelpUris",
"shortDescription": {
"text": "Placeholder."
},
"fullDescription": {
"text": "Placeholder"
},
"messageStrings": {
"Note_Default": {
"text": "{0}: Placeholder '{1}'"
}
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
]
}
},
"invocations": [
{
"toolConfigurationNotifications": [
{
"message": {
"text": "Rule 'SARIF2002' was explicitly disabled by the user. As result, this tool run cannot be used for compliance or other auditing processes that require a comprehensive analysis."
},
"descriptor": {
"id": "WRN999.RuleExplicitlyDisabled"
}
},
{
"message": {
"text": "Rule 'SARIF2006' was explicitly disabled by the user. As result, this tool run cannot be used for compliance or other auditing processes that require a comprehensive analysis."
},
"descriptor": {
"id": "WRN999.RuleExplicitlyDisabled"
}
}
],
"executionSuccessful": true
}
],
"artifacts": [
{
"location": {
"uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF2012.ProvideHelpUris_Invalid.sarif",
"uriBaseId": "TEST_DIR"
}
}
],
"results": [
{
"ruleId": "SARIF2012",
"ruleIndex": 0,
"level": "note",
"message": {
"id": "Note_Default",
"arguments": [
"runs[0].tool.driver.rules[0]",
"SARIF2009.ConsiderConventionalIdentifierValues"
]
},
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"index": 0
},
"region": {
"startLine": 11,
"startColumn": 13
}
}
}
]
}
],
"columnKind": "utf16CodeUnits"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{
"$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"
}
},
"invocations": [
{
"toolConfigurationNotifications": [
{
"message": {
"text": "Rule 'SARIF2002' was explicitly disabled by the user. As result, this tool run cannot be used for compliance or other auditing processes that require a comprehensive analysis."
},
"descriptor": {
"id": "WRN999.RuleExplicitlyDisabled"
}
},
{
"message": {
"text": "Rule 'SARIF2006' was explicitly disabled by the user. As result, this tool run cannot be used for compliance or other auditing processes that require a comprehensive analysis."
},
"descriptor": {
"id": "WRN999.RuleExplicitlyDisabled"
}
}
],
"executionSuccessful": true
}
],
"artifacts": [
{
"location": {
"uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF2012.ProvideHelpUris_Valid.sarif",
"uriBaseId": "TEST_DIR"
}
}
],
"results": [],
"columnKind": "utf16CodeUnits"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"$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",
"version": "1.0.0",
"rules": [
{
"id": "SARIF2009",
"name": "ConsiderConventionalIdentifierValues"
}
]
}
},
"versionControlProvenance": [
{
"repositoryUri": "https://github.com/microsoft/sarif-sdk"
}
],
"columnKind": "utf16CodeUnits"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"$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",
"version": "1.0.0",
"rules": [
{
"id": "SARIF2009",
"name": "ConsiderConventionalIdentifierValues",
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
]
}
},
"versionControlProvenance": [
{
"repositoryUri": "https://github.com/microsoft/sarif-sdk"
}
],
"columnKind": "utf16CodeUnits"
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
"NoPlaceholders": {
"text": "This message does not contain dynamic content."
}
}
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
Copy link
Contributor

@harleenkohli harleenkohli Jun 30, 2020

Choose a reason for hiding this comment

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

we removed these to make the test cases smaller, maybe this got back due to merge. Please remove these (same for other 3 files- 2014, 2015) #ByDesign

Copy link
Contributor

Choose a reason for hiding this comment

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

oh haha! wait a second! now we have a rule which makes this field mandatory!!! lol, now i see why u put it back.


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

Copy link
Collaborator Author

@eddynaka eddynaka Jun 30, 2020

Choose a reason for hiding this comment

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

the new rule developed (2012), will show that as an error, since we are using it as verbose. That's why i added again. #Resolved

]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
"Default": {
"text": "This string has enquoted dynamic content '{0}' and '{1}', and ends with a period."
}
}
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
"NotEnquoted": {
"text": "This message contains dynamic content {0} that is not enquoted."
}
}
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
"Default": {
"text": "This string has enquoted dynamic content '{0}' and '{1}', and ends with a period."
}
}
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
]
}
Expand Down