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 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
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": "MessageMustBeFlattened",
"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.MessageMustBeFlattened.Options'>
<Property Key='RuleEnabled' Value='Error' />
</Properties>
</Properties>
43 changes: 43 additions & 0 deletions src/Sarif.Multitool.Library/Rules/GH1007.MessageMustBeFlattened.cs
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 MessageMustBeFlattened : SarifValidationSkimmerBase
{
public MessageMustBeFlattened()
{
this.DefaultConfiguration.Level = FailureLevel.Error;
Fixed Show fixed Hide fixed
}

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

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

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

protected override void Analyze(Result result, string resultPointer)
{
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_MessageMustBeFlattened_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 MessageMustBeFlattened = "GH1007";
Copy link
Collaborator

Choose a reason for hiding this comment

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

MessageMustBeFlattened

This rule will tell user the other direction right:
public const string ProvideMessageArguments = "SARIF2002";

will the user get confused

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the other names start with the verb when possible. So maybe this on could be "FlattenMessage"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to "FlattenResultMessage"

Copy link
Collaborator Author

@yongyan-gh yongyan-gh Dec 8, 2022

Choose a reason for hiding this comment

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

right these 2 rules are opposite to each other.
SARIF validation rules are common for all SARIF files.
GH rules only for SARIF files need to be ingested by GHAS, should be fine if the GH rule conflicts with SARIF rules. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

I know that 'flatten' is our special jargon but I wonder if something like 'ProvideFullyFormattedMessageStrings' is more descriptive.


// 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_MessageMustBeFlattened_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_MessageMustBeFlattened_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_MessageMustBeFlattened_Valid()
=> RunValidTestForRule(RuleId.MessageMustBeFlattened);

[Fact]
public void GH1007_MessageMustBeFlattened_Invalid()
=> RunInvalidTestForRule(RuleId.MessageMustBeFlattened);

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,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": "MessageMustBeFlattened",
"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.MessageMustBeFlattened_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"
}
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Dec 6, 2022

Choose a reason for hiding this comment

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

I think the _Invalid test has the not flatten message,
the _Valid should have the flatten message.

or, a valid test with results with flatten message,
another valid test that has no result at all like this one.
#Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is output SARIF file generated by test, do you mean the input test SARIF file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you are right!

},
"invocations": [
{
"executionSuccessful": true
}
],
"artifacts": [
{
"location": {
"uri": "FunctionalTestOutput.ValidateCommand/GH1007.MessageMustBeFlattened_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"
}
]
}