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

GHAS validation rule - Message must be flattened #2580

Merged
merged 8 commits into from
Dec 28, 2022
+264 −2
Merged

GHAS validation rule - Message must be flattened #2580

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
911db71
Add a GitHub validation rule.
yongyan-gh Dec 6, 2022
7676817
Fix tests
yongyan-gh Dec 6, 2022
babd930
update rule name to FlatternResultMessage
yongyan-gh Dec 7, 2022
48b91d3
Fix CodeQL warning
yongyan-gh Dec 8, 2022
5518206
Update release history
yongyan-gh Dec 8, 2022
1c3f70c
Rename to ProvideFullyFormattedMessageStrings
yongyan-gh Dec 13, 2022
2b37341
rename rule to ProvideFullyFormattedMessageStrings
yongyan-gh Dec 13, 2022
e33751f
Merge branch 'main' into user/yongyan-gh/ghFlatternMessageRule
michaelcfanning Dec 27, 2022
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
View file
Open in desktop
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
View file
Open in desktop
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>
43 changes: 43 additions & 0 deletions src/Sarif.Multitool.Library/Rules/GH1007.FlattenResultMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

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

using Microsoft.Json.Pointer;

namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
{
public class FlattenResultMessage : SarifValidationSkimmerBase
{
public FlattenResultMessage()
{
this.DefaultConfiguration.Level = FailureLevel.Error;
Fixed Show fixed Hide fixed
}

/// <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)
};

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 •
edited
Loading

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
View file
Open in desktop
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
View file
Open in desktop

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
View file
Open in desktop
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>
8 changes: 8 additions & 0 deletions src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs
View file
Open in desktop
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
71 changes: 71 additions & 0 deletions ...tData/Multitool/ValidateCommand/ExpectedOutputs/GH1007.FlattenResultMessage_Invalid.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
{
"$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": {
"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"
}
]
}
28 changes: 28 additions & 0 deletions ...estData/Multitool/ValidateCommand/ExpectedOutputs/GH1007.FlattenResultMessage_Valid.sarif
View file
Open in desktop
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"
}
]
}
41 changes: 41 additions & 0 deletions ...Sarif/TestData/Multitool/ValidateCommand/Inputs/GH1007.FlattenResultMessage_Invalid.sarif
View file
Open in desktop
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"
}
]
}
31 changes: 31 additions & 0 deletions ...s.Sarif/TestData/Multitool/ValidateCommand/Inputs/GH1007.FlattenResultMessage_Valid.sarif
View file
Open in desktop
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"
}
]
}