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 #2003: Don't report missing snippet if file content is present. #2019

Merged
merged 7 commits into from
Jul 29, 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
84 changes: 72 additions & 12 deletions src/Sarif.Multitool/Rules/SARIF2010.ProvideCodeSnippets.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
// 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.Linq;
using Microsoft.Json.Pointer;

namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
Expand All @@ -26,6 +27,15 @@ public class ProvideCodeSnippets : SarifValidationSkimmerBase

public override FailureLevel DefaultLevel => FailureLevel.Note;

private IList<Artifact> artifacts;
private IDictionary<string, ArtifactLocation> originalUriBaseIds;

protected override void Analyze(Run run, string runPointer)
{
this.artifacts = run.Artifacts;
this.originalUriBaseIds = run.OriginalUriBaseIds;
}

protected override void Analyze(Result result, string resultPointer)
{
if (result.Locations != null)
Expand All @@ -40,17 +50,20 @@ protected override void Analyze(Result result, string resultPointer)

private void AnalyzeResultLocation(Location location, string locationPointer)
{
AnalyzeRegion(
location.PhysicalLocation?.Region,
locationPointer
.AtProperty(SarifPropertyName.PhysicalLocation)
.AtProperty(SarifPropertyName.Region));

AnalyzeRegion(
location.PhysicalLocation?.ContextRegion,
locationPointer
.AtProperty(SarifPropertyName.PhysicalLocation)
.AtProperty(SarifPropertyName.ContextRegion));
if (AnalyzeArtifactLocation(location.PhysicalLocation?.ArtifactLocation))
{
AnalyzeRegion(
location.PhysicalLocation?.Region,
locationPointer
.AtProperty(SarifPropertyName.PhysicalLocation)
.AtProperty(SarifPropertyName.Region));

AnalyzeRegion(
location.PhysicalLocation?.ContextRegion,
locationPointer
.AtProperty(SarifPropertyName.PhysicalLocation)
.AtProperty(SarifPropertyName.ContextRegion));
}
}

private void AnalyzeRegion(Region region, string regionPointer)
Expand All @@ -65,5 +78,52 @@ private void AnalyzeRegion(Region region, string regionPointer)
nameof(RuleResources.SARIF2010_ProvideCodeSnippets_Note_Default_Text));
}
}

private bool AnalyzeArtifactLocation(ArtifactLocation artifactLocation)
{
// No artifactLocation / no artifacts, so we should look for the snippet.
if (artifactLocation == null || this.artifacts == null)
{
return true;
}

// Checking if we can reconstruct uri from artifactLocation
// If we can't, we still need to validate, since neither originalUriBaseIds
// nor artifactLocation.UriBaseId is required.
artifactLocation.TryReconstructAbsoluteUri(this.originalUriBaseIds, out Uri resolvedUri);
Copy link

@ghost ghost Jul 29, 2020

Choose a reason for hiding this comment

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

TryReconstructAbsoluteUri [](start = 29, length = 25)

If a SARIF viewer can't resolve the URI, it won't be able to find the embedded content, and so it will need the snippet to be present. So:

if (!artifactLocation.TryReconstructAbsoluteUri(this.originalUriBaseIds, out Uri resolvedUri))
{
    return true;
}

We need a test case for this, so please take a look at TryReconstructAbsoluteUri to see how you can make it fail. Probably using an unknown uriBaseId will do it. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes and no...if we couldn't resolve doesn't mean that we need to look for snippet. Imagine the situation where we don't have the uriBaseId but we have the file. That file can be presented in the artifacts list, right?


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

Copy link

Choose a reason for hiding this comment

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

You're right. Please add a comment, because people reading the code will be surprised (as I was) that you call a Try API without checking for success.


In reply to: 462546597 [](ancestors = 462546597,462538548)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added some comments, let me know what do you think


In reply to: 462575560 [](ancestors = 462575560,462546597,462538548)

Copy link

Choose a reason for hiding this comment

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

Good. Wording: "... since neither originalUriBaseIds nor artifactLocation.UriBaseId is required."


In reply to: 462582648 [](ancestors = 462582648,462575560,462546597,462538548)


foreach (Artifact artifact in this.artifacts)
{
// Content/text doesn't exist, continue to next
if (string.IsNullOrEmpty(artifact.Contents?.Text))
{
continue;
}

// Checking if we can reconstruct uri from artifact
// If we can't, we still need to validate, since originalUriBaseIds aren't
// required nether artifactLocation.UriBaseId.
artifact.Location.TryReconstructAbsoluteUri(this.originalUriBaseIds, out Uri artifactUri);
Copy link

@ghost ghost Jul 29, 2020

Choose a reason for hiding this comment

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

TryReconstructAbsoluteUri [](start = 34, length = 25)

if (!artifact.Location.TryReconstructAbsoluteUri(this.originalUriBaseIds, out Uri artifactUri)
{
    continue;
}

Need a test case for this too (an artifact that does have content but that cannot be resolved. #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.

Same thing as before, what if we don't have the uribaseid but the path is right, so we have to continue validating. Right?


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

Copy link

Choose a reason for hiding this comment

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

You are right. Please do add the test case.


In reply to: 462547813 [](ancestors = 462547813,462543051)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some comments in the sarif, so we can understand what i'm validating. Let me know what do you think


In reply to: 462576825 [](ancestors = 462576825,462547813,462543051)


if (resolvedUri != null && artifactUri != null)
Copy link

@ghost ghost Jul 29, 2020

Choose a reason for hiding this comment

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

if (resolvedUri != null && artifactUri != null) [](start = 16, length = 47)

Since you will have previously checked the return values from the two calls to TryReconstructAbsoluteUri, you won't need this test. #Closed

Copy link

Choose a reason for hiding this comment

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

By the argument you made above, we do need these tests.


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

{
if (artifactUri.AbsolutePath.Equals(resolvedUri.AbsolutePath))
{
return false;
}
}
else
{
// Couldn't generate the absoluteUris, so let's compare everything.
if (this.artifacts.Any(a => a.Location?.Uri.OriginalString == artifactLocation.Uri.OriginalString
&& a.Location?.UriBaseId == artifactLocation.UriBaseId))
{
return false;
}
}
}

return true;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,10 @@ public void SARIF2009_ConsiderConventionalIdentifierValues_Invalid()
public void SARIF2010_ProvideCodeSnippets_Valid()
=> RunValidTestForRule(RuleId.ProvideCodeSnippets);

[Fact]
public void SARIF2010_ProvideCodeSnippets_WithEmbeddedContent_Valid()
=> RunTest("SARIF2010.ProvideCodeSnippets_WithEmbeddedContent.sarif");

[Fact]
public void SARIF2010_ProvideCodeSnippets_Invalid()
=> RunInvalidTestForRule(RuleId.ProvideCodeSnippets);
Expand Down
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.5.json",
"version": "2.1.0",
"runs": [
{
"tool": {
"driver": {
"name": "SARIF Functional Testing"
}
},
"invocations": [
{
"executionSuccessful": true
}
],
"artifacts": [
{
"location": {
"uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF2010.ProvideCodeSnippets_WithArtifacts_Valid.sarif",
"uriBaseId": "TEST_DIR"
}
}
],
"results": [],
"columnKind": "utf16CodeUnits"
}
]
}
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.5.json",
"version": "2.1.0",
"runs": [
{
"tool": {
"driver": {
"name": "CodeScanner",
"version": "1.0"
}
},
"originalUriBaseIds": {
"TEST_ROOT": {
"uri": "file://c:/test/"
}
},
"artifacts": [
{
"contents": {
"text": "File Source"
},
"location": {
"uri": "src/test.c"
}
},
{
"contents": {
"text": "File Source"
},
"location": {
"uri": "test2.c",
"uriBaseId": "TEST_ROOT"
}
}
],
"results": [
{
"ruleId": "TST0001",
"level": "error",
"message": {
"text": "Some testing occurred."
},
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "src/test.c"
},
"properties": {
"comment": "This test will NOT generate a valid AbsoluteUri, but it will find in artifacts. So it doesn't need a snippet."
}
}
},
{
"physicalLocation": {
"artifactLocation": {
"uri": "test2.c",
"uriBaseId": "TEST_ROOT"
},
"properties": {
"comment": "This test will generate a valid AbsoluteUri and it will find in artifacts. So it doesn't need a snippet."
}
}
}
]
}
],
"columnKind": "utf16CodeUnits"
}
]
}