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

GHAS validation rule - Message must be flattened #2580

Merged
merged 8 commits into from
Dec 28, 2022
Merged
Show file tree
Hide file tree
Changes from 5 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
7 changes: 7 additions & 0 deletions policies/github.config.sarif-external-properties
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@
"defaultConfiguration": {
"enabled": true
}
},
{
"id": "GH1007",
"name": "FlattenResultMessage",
"defaultConfiguration": {
"enabled": true
}
}
]
}
Expand Down
3 changes: 3 additions & 0 deletions policies/github.config.xml
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,7 @@
<Properties Key='GH1006.ProvideCheckoutPath.Options'>
<Property Key='RuleEnabled' Value='Error' />
</Properties>
<Properties Key='GH1007.FlattenResultMessage.Options'>
<Property Key='RuleEnabled' Value='Error' />
</Properties>
</Properties>
1 change: 1 addition & 0 deletions src/ReleaseHistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* BUGFIX: Eliminate `IOException` and `DirectoryNotFoundException` exceptions thrown by `merge` command when splitting by rule (due to invalid file characters in rule ids). [#2513](https://github.com/microsoft/sarif-sdk/pull/2513)
* BUGFIX: Fix classes inside NotYetAutoGenerated folder missing `virtual` keyword for public methods and properties, by regenerate and manually sync the changes. [#2537](https://github.com/microsoft/sarif-sdk/pull/2537)
* FEATURE: Allow external set of `MaxFileSizeInKilobytes`, which will allow SDK users to change the value. (Default value is 1024) [#2578](https://github.com/microsoft/sarif-sdk/pull/2578)
* FEATURE: Add a Github validation rule `GH1007`, which requires flattened result message so GHAS code scanning can ingest the log. [#2580](https://github.com/microsoft/sarif-sdk/issues/2580)

## **v3.1.0** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/3.1.0) | [Driver](https://www.nuget.org/packages/Sarif.Driver/3.1.0) | [Converters](https://www.nuget.org/packages/Sarif.Converters/3.1.0) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/3.1.0) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/3.1.0)

Expand Down
44 changes: 44 additions & 0 deletions src/Sarif.Multitool.Library/Rules/GH1007.FlattenResultMessage.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// 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 FlattenResultMessage : SarifValidationSkimmerBase
{
public override ReportingConfiguration DefaultConfiguration => new ReportingConfiguration
{
Level = FailureLevel.Error,
Enabled = this.EnabledByDefault
};

/// <summary>
/// GH1007
/// </summary>
public override string Id => RuleId.FlattenResultMessage;

public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.GH1007_FlattenResultMessage_FullDescription_Text };

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

public override bool EnabledByDefault => false;

protected override void Analyze(Result result, string resultPointer)
Copy link
Member

Choose a reason for hiding this comment

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

Analyze

So now we can't run an analysis without producing errors? Either we recommend using format string args or we object to the absence of flattened messages?

Ideally, the conflicting rules would turn off if the other one is being enforced.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how about adding a Set property to Skimmer class, e.g. ConflictSkimmerSet. In AnalyzeCommandBase.AnalyzeTarget() method, iterates available skimmer and add the ConflictSkimmerSet value to disabledSkimmers set if has value.

this way the rule author can override existing old rule conflicts with the new rule.

what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting! And so the skimmers just need a dependency on rule ids it replaces. Yes, I think this could work.

Copy link
Member

@michaelcfanning michaelcfanning Dec 13, 2022

Choose a reason for hiding this comment

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

OK, so I thought about your idea a bit more.

Rules should be able to declare an 'IncompatibleRuleIds' set or something similar, All the rule does is to publish this data.

Then we update the analysis engine, which, when instantiating skimmers, makes sure that no new enabled skimmers have a rule id that conflicts with the set of loaded skimmers so far. The analysis engine could raise an error-level configuration notification and exit.

This approach would require users to properly configure analysis to account for the configuration problem. I think everything would drop in pretty cleanly by doing this, we need a low-level update to rules metadata and a simple update to the default skimmer loading behavior.

btw - I'm approving this change, we can take on the above or continue to refine the idea in further work items/PRs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks created the issue #2591

regarding "The analysis engine could raise an error-level configuration notification and exit.", should we raise an configuration notification error and disable the incompatible rules then continue the analysis instead exiting?

am thinking about the web validator scenario, user can run only SARIF validator rules, or all SARIF + GH validator rules, if adding a new GH rule which is not compatible with a SARIF rule, we may raise the configuration error and continue analysis without a config file change. If exits right away, the web validator stops getting results until config file is fixed.

{
if (string.IsNullOrEmpty(result.Message.Text))
{
// {0}: The 'text' property of this result message is absent. GitHub Advanced Security code
// scanning will reject this file because it does not support the argumented message now.
// Try to populate the flattened message text in 'message.text' property.
LogResult(
resultPointer.AtProperty(SarifPropertyName.Message),
nameof(RuleResources.GH1007_FlattenResultMessage_Error_Default_Text));
}
}
}
}
1 change: 1 addition & 0 deletions src/Sarif.Multitool.Library/Rules/RuleId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public static class RuleId
public const string ReviewArraysThatExceedConfigurableDefaults = "GH1004";
public const string LocationsMustBeRelativeUrisOrFilePaths = "GH1005";
public const string ProvideCheckoutPath = "GH1006";
public const string FlattenResultMessage = "GH1007";

// TEMPLATE:
// public const string RuleFriendlyName = "SARIFnnnn";
Expand Down
18 changes: 18 additions & 0 deletions src/Sarif.Multitool.Library/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.Library/Rules/RuleResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -450,4 +450,10 @@ Semantics: Assuming the reader of the log file (an end user or another tool) has
<data name="SARIF2016_FileUrisShouldBeRelative_Note_ShouldNotContainBackSlash_Text" xml:space="preserve">
<value>{0}: The relative file URL '{1}' contains one or more backslashes, which will be preserved when concatenating to an absolute URL. This can result in inconsistent representations, compared to URLs created from an absolute file path, which may be regarded as not equivalent. Replace all backslashes with forward slashes.</value>
</data>
<data name="GH1007_FlattenResultMessage_Error_Default_Text" xml:space="preserve">
<value>{0}: The 'text' property of this result message is absent. GitHub Advanced Security code scanning will reject this file because it does not support the argumented message now. Try to populate the flattened message text in 'message.text' property.</value>
</data>
<data name="GH1007_FlattenResultMessage_FullDescription_Text" xml:space="preserve">
<value>GitHub Advanced Security code scanning will reject a SARIF file that express result messages with 'message.id' and 'message.arguments' but without the 'message.text' property since the arugmented message format is not supported yet. Please populate the flattened message text in 'message.text' property.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,14 @@ public void GH1006_ProvideCheckoutPath_Valid()
public void GH1006_ProvideCheckoutPath_Invalid()
=> RunInvalidTestForRule(RuleId.ProvideCheckoutPath);

[Fact]
public void GH1007_FlattenResultMessage_Valid()
=> RunValidTestForRule(RuleId.FlattenResultMessage);

[Fact]
public void GH1007_FlattenResultMessage_Invalid()
=> RunInvalidTestForRule(RuleId.FlattenResultMessage);

private void RunArrayLimitTest(string testFileNameSuffix)
{
// Some of the actual limits are impractically large for testing purposes,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
{
"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.6.json",
"version": "2.1.0",
"runs": [
{
"tool": {
"driver": {
"name": "SARIF Functional Testing",
"rules": [
{
"id": "GH1007",
"name": "FlattenResultMessage",
"fullDescription": {
"text": "GitHub Advanced Security code scanning will reject a SARIF file that express result messages with 'message.id' and 'message.arguments' but without the 'message.text' property since the arugmented message format is not supported yet. Please populate the flattened message text in 'message.text' property."
},
"messageStrings": {
"Error_Default": {
"text": "{0}: The 'text' property of this result message is absent. GitHub Advanced Security code scanning will reject this file because it does not support the argumented message now. Try to populate the flattened message text in 'message.text' property."
}
},
"defaultConfiguration": {
"enabled": false,
"level": "error"
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
]
}
},
"invocations": [
{
"executionSuccessful": true
}
],
"artifacts": [
{
"location": {
"uri": "FunctionalTestOutput.ValidateCommand/GH1007.FlattenResultMessage_Invalid.sarif",
"uriBaseId": "TEST_DIR"
}
}
],
"results": [
{
"ruleId": "GH1007",
"ruleIndex": 0,
"level": "error",
"message": {
"id": "Error_Default",
"arguments": [
"runs[0].results[0].message"
]
},
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"index": 0
},
"region": {
"startLine": 29,
"startColumn": 22
}
}
}
]
}
],
"columnKind": "utf16CodeUnits"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.6.json",
"version": "2.1.0",
"runs": [
{
"tool": {
"driver": {
"name": "SARIF Functional Testing"
}
},
"invocations": [
{
"executionSuccessful": true
}
],
"artifacts": [
{
"location": {
"uri": "FunctionalTestOutput.ValidateCommand/GH1007.FlattenResultMessage_Valid.sarif",
"uriBaseId": "TEST_DIR"
}
}
],
"results": [],
"columnKind": "utf16CodeUnits"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.6.json",
"version": "2.1.0",
"runs": [
{
"tool": {
"driver": {
"name": "SARIF Functional Testing",
"version": "1.2.3",
"rules": [
{
"id": "TEST1001",
"fullDescription": {
"text": "Argumented message."
},
"messageStrings": {
"DoesExist": {
"text": "'{0}' is an apparent access token of '{1}'."
}
}
}
]
}
},
"results": [
{
"ruleId": "TEST1001",
"ruleIndex": 0,
"message": {
"id": "DoesExist",
"arguments": [
"123456789",
"Alibaba Cloud service"
]
}
}
],
"columnKind": "utf16CodeUnits"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{
"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.6.json",
"version": "2.1.0",
"runs": [
{
"tool": {
"driver": {
"name": "SARIF Functional Testing",
"version": "1.2.3",
"rules": [
{
"id": "TEST1001",
"fullDescription": {
"text": "Argumented message."
}
}
]
}
},
"results": [
{
"ruleId": "TEST1001",
"message": {
"text": "'123456789' is an apparent access token of 'Alibaba Cloud service'."
}
}
],
"columnKind": "utf16CodeUnits"
}
]
}