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

Fix #2084 (GitHub related location message); Fix #2085 (GitHub invocations) #2086

Merged
merged 5 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
16 changes: 1 addition & 15 deletions docs/ValidationRules.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ GitHub Advanced Security code scanning only displays results whose locations are

### Description

GitHub Advanced Security code scanning will reject a SARIF file that expresses result locations as absolute 'file' scheme URIs unless it can determine the URI of the repository root (which GitHub refers to as the "checkout path"). There are three ways to address this issue.
GitHub Advanced Security code scanning will reject a SARIF file that expresses result locations as absolute 'file' scheme URIs unless GitHub can determine the URI of the repository root (which GitHub refers to as the "checkout path"). There are three ways to address this issue.

1. Recommended: Express all result locations as relative URI references with respect to the checkout path.

Expand All @@ -102,20 +102,6 @@ GitHub Advanced Security code scanning will reject a SARIF file that expresses r

---

## Rule `GH1007.ProvideRequiredRelatedLocationProperties`

### Description

GitHub Advanced Security code scanning will reject a SARIF file that includes a "related location" with no 'message' property. This is a bug in code scanning. You can set 'message' to an empty string if you don't have anything else to say about the location.

### Messages

#### `Default`: Error

{0}: This related location does not have a 'message' property, so GitHub Advanced Security code scanning will reject the entire log file. This is a bug in code scanning. You can set 'message' to an empty string if you don't have anything else to say about the location.

---

## Rule `SARIF1001.RuleIdentifiersMustBeValid`

### Description
Expand Down
7 changes: 0 additions & 7 deletions policies/github.config.sarif-external-properties
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,6 @@
"defaultConfiguration": {
"enabled": true
}
},
{
"id": "GH1007",
"name": "ProvideRequiredRelatedLocationProperties",
"defaultConfiguration": {
"enabled": true
}
}
]
}
Expand Down
3 changes: 0 additions & 3 deletions policies/github.config.xml
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,4 @@
<Properties Key='GH1006.ProvideCheckoutPath.Options'>
<Property Key='RuleEnabled' Value='Error' />
</Properties>
<Properties Key='GH1007.ProvideRequiredRelatedLocationProperties.Options'>
<Property Key='RuleEnabled' Value='Error' />
</Properties>
</Properties>

This file was deleted.

1 change: 0 additions & 1 deletion src/Sarif.Multitool.Library/Rules/RuleId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ public static class RuleId
public const string ReviewArraysThatExceedConfigurableDefaults = "GH1004";
public const string LocationsMustBeRelativeUrisOrFilePaths = "GH1005";
public const string ProvideCheckoutPath = "GH1006";
public const string ProvideRequiredRelatedLocationProperties = "GH1007";

// TEMPLATE:
// public const string RuleFriendlyName = "SARIFnnnn";
Expand Down
18 changes: 0 additions & 18 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: 0 additions & 6 deletions src/Sarif.Multitool.Library/Rules/RuleResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -446,12 +446,6 @@ Semantics: Assuming the reader of the log file (an end user or another tool) has
<data name="GH1006_ProvideCheckoutPath_Error_Default_Text" xml:space="preserve">
<value>{0}: This result location is expressed as an absolute 'file' URI. GitHub Advanced Security code scanning will reject this file because it cannot determine the location of the repository root (which it refers to as the "checkout path"). Either express result locations as relative URI references with respect to the checkout path, place the checkout path in 'invocations[].workingDirectory', or place the checkout path in a configuration file at the root of the repository.</value>
</data>
<data name="GH1007_ProvideRequiredRelatedLocationProperties_Error_Default_Text" xml:space="preserve">
<value>{0}: This related location does not have a 'message' property, so GitHub Advanced Security code scanning will reject the entire log file. This is a bug in GitHub. You can set 'message' to an empty string if you don't have anything else to say about the location.</value>
</data>
<data name="GH1007_ProvideRequiredRelatedLocationProperties_FullDescription_Text" xml:space="preserve">
<value>GitHub Advanced Security code scanning will reject a SARIF file that includes a "related location" with no 'message' property. This is a bug in GitHub. You can set 'message' to an empty string if you don't have anything else to say about the location.</value>
</data>
<data name="SARIF2005_ProvideToolProperties_Warning_ProvideToolnformationUri_Text" xml:space="preserve">
<value>{0}: The tool '{1}' does not provide 'informationUri'. This property helps the developer responsible for addessing a result by providing a way to learn more about the tool.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,6 @@ namespace Microsoft.CodeAnalysis.Sarif.Visitors
{
public class GitHubIngestionVisitor : SarifRewritingVisitor
{
// GitHub requires that every related location have a message. It's not clear why this
// requirement exists, as this data is mostly used to build embedded links from
// results (where the link anchor text actually resides).
private const string PlaceholderRelatedLocationMessage = "[No message provided.]";

// GitHub reportedly has an ingestion limit of 500 issues.
// Internal static rather than private const to allow a unit test with a practical limit.
internal static int s_MaxResults = 500;
Expand All @@ -25,14 +20,6 @@ public override Run VisitRun(Run node)
this.artifacts = node.Artifacts;
this.threadFlowLocations = node.ThreadFlowLocations;

// GitHub does not support submitting invocation objects. Invocations
// contains potentially sensitive environment details, such as
// account names embedded in paths. Invocations also store
// notifications of catastrophic tool failures, however, which
// means there is current no mechanism for reporting these to
// GitHub users in context of the security tab.
node.Invocations = null;

if (node.Results != null)
{
int errorsCount = 0;
Expand Down Expand Up @@ -157,20 +144,6 @@ public override ArtifactLocation VisitArtifactLocation(ArtifactLocation node)

public override Result VisitResult(Result node)
{
if (node.RelatedLocations != null)
{
foreach (Location relatedLocation in node.RelatedLocations)
{
if (string.IsNullOrEmpty(relatedLocation.Message?.Text))
{
relatedLocation.Message = new Message
{
Text = PlaceholderRelatedLocationMessage
};
}
}
}

if (node.Fingerprints != null)
{
// GitHub appears to require that fingerprints be emitted to the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,14 +379,6 @@ public void GH1006_ProvideCheckoutPath_Valid()
public void GH1006_ProvideCheckoutPath_Invalid()
=> RunInvalidTestForRule(RuleId.ProvideCheckoutPath);

[Fact]
public void GH1007_ProvideRequiredRelatedLocationProperties_Valid()
=> RunValidTestForRule(RuleId.ProvideRequiredRelatedLocationProperties);

[Fact]
public void GH1007_ProvideRequiredRelatedLocationProperties_Invalid()
=> RunInvalidTestForRule(RuleId.ProvideRequiredRelatedLocationProperties);

private void RunArrayLimitTest(string testFileNameSuffix)
{
// Some of the actual limits are impractically large for testing purposes,
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

Loading