Skip to content

Commit

Permalink
Fix #1485: SARIF1015 mishandles run.originalUriBaseIds (@kupsch) (#1644)
Browse files Browse the repository at this point in the history
SARIF1015 complained if a `uri` property in one of the `artifactLocation` objects in `run.originalUriBaseIds` was relative. But that is allowed as long as `artifactLocation.uriBaseId` is also present.

The actual requirement is:

1. If `uriBaseId` is absent, then _either_ `uri` must be absolute or it must be absent.
2. If `uriBaseId` is present, the `uri` must be relative.

But <span>#</span>2 is true for _all_ `artifactLocation` objects, not just those in `run.originalUriBaseIds`. Therefore, in this PR:

- We remove the logic dealing with `run.originalUriBaseIds` from SARIF1015.
- We introduce a new rule SARIF1018 that enforces <span>#</span>1 in `run.originalUriBaseIds`.
- We file issue #1643, "New rule: If artifactLocation.uriBaseId is present, uri must be relative" to enforce <span>#</span>2 for all `artifactLocation` objects. But that is beyond the scope of this PR.

Also:
- DRY out the construction of the names of the validation rule test files.
- Upgrade the test files for SARIF1015 (which are affected by this change) to the final SARIF version, to make it easier to reliably make the required changes in `run.originalUriBaseIds`.

NOTE: #1485 also reported that the web site's validation page did not accept files with the (valid) filename extension .sarif.json. I filed microsoft/sarif-website#104 for that, and labeled it `m156` because _this_ bug is `m156`. I'll do that one next.
  • Loading branch information
Larry Golding committed Aug 15, 2019
1 parent 1db9484 commit a1e0f26
Show file tree
Hide file tree
Showing 16 changed files with 345 additions and 100 deletions.
2 changes: 2 additions & 0 deletions src/ReleaseHistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,9 @@
* FEATURE: Add validation rule to ensure that all array-index-valued properties are consistent with their respective arrays.

## **v2.1.15** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.15) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.15) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.15) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.15)
* BUGFIX: Validation rule `SARIF1015` incorrectly required `originalUriBaseIds` to be contain URIs. https://github.com/microsoft/sarif-sdk/issues/1485
* BUGFIX: Multitool transform mishandled dottedQuadFileVersion. https://github.com/microsoft/sarif-sdk/issues/1532
* BUGFIX: Restore missing FxCop converter unit test. https://github.com/microsoft/sarif-sdk/issues/1575
* BUGFIX: FortifyFpr converter produced invalid SARIF. https://github.com/microsoft/sarif-sdk/issues/1593
* BUGFIX: FxCop converter produced empty `result.message` objects. https://github.com/microsoft/sarif-sdk/issues/1594
* FEATURE: Add validation rule to ensure correctness of `originalUriBaseIds` entries.
1 change: 1 addition & 0 deletions src/Sarif.Multitool/Rules/RuleId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,6 @@ public static class RuleId
public const string UriMustBeAbsolute = "SARIF1015";
public const string ContextRegionRequiresRegion = "SARIF1016";
public const string InvalidIndex = "SARIF1017";
public const string InvalidUriInOriginalUriBaseIds = "SARIF1018";
}
}
18 changes: 18 additions & 0 deletions src/Sarif.Multitool/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/Rules/RuleResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -192,4 +192,10 @@
<data name="SARIF1017_InvalidIndex" xml:space="preserve">
<value>If an object contains a property that is used as an array index, then that array must be present and must contain at least "index + 1" elements.</value>
</data>
<data name="SARIF1018_Default" xml:space="preserve">
<value>In this artifactLocation object contained in run.originalUriBaseIds, uriBaseId is absent, but uri is '{0}', which is a relative URI.</value>
</data>
<data name="SARIF1018_InvalidUriInOriginalUriBaseIds" xml:space="preserve">
<value>In the artifactLocation objects contained in run.originalUriBaseIds, if uriBaseId is absent, then uri must either be an absolute URI or it must be absent.</value>
</data>
</root>
15 changes: 1 addition & 14 deletions src/Sarif.Multitool/Rules/SARIF1015.UriMustBeAbsolute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,19 +53,6 @@ protected override void Analyze(ReportingDescriptor reportingDescriptor, string
AnalyzeUri(reportingDescriptor.HelpUri, reportingDescriptorPointer.AtProperty(SarifPropertyName.HelpUri));
}

protected override void Analyze(Run run, string runPointer)
{
if (run.OriginalUriBaseIds != null)
{
string originalUriBaseIdsPointer = runPointer.AtProperty(SarifPropertyName.OriginalUriBaseIds);

foreach (string key in run.OriginalUriBaseIds.Keys)
{
AnalyzeUri(run.OriginalUriBaseIds[key].Uri, originalUriBaseIdsPointer.AtProperty(key));
}
}
}

protected override void Analyze(ToolComponent toolComponent, string toolComponentPointer)
{
AnalyzeUri(toolComponent.DownloadUri, toolComponentPointer.AtProperty(SarifPropertyName.DownloadUri));
Expand All @@ -90,7 +77,7 @@ private void AnalyzeUri(string uriString, string pointer)
// even for a malformed URI string.
if (uriString != null && Uri.IsWellFormedUriString(uriString, UriKind.RelativeOrAbsolute))
{
// Ok, it's a well-formed URI of some kind. But if it's not absolute, _now_ we
// Ok, it's a well-formed URI of some kind. If it's not absolute, _now_ we
// can report it.
Uri uri = new Uri(uriString, UriKind.RelativeOrAbsolute);
if (!uri.IsAbsoluteUri)
Expand Down
1 change: 0 additions & 1 deletion src/Sarif.Multitool/Rules/SARIF1017.InvalidIndex.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Collections.Generic;
using System.Runtime.InteropServices.WindowsRuntime;

namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// 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 Microsoft.Json.Pointer;

namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
{
public class InvalidUriInOriginalUriBaseIds : SarifValidationSkimmerBase
{
private readonly MultiformatMessageString _fullDescription = new MultiformatMessageString
{
Text = RuleResources.SARIF1018_InvalidUriInOriginalUriBaseIds
};

public override MultiformatMessageString FullDescription => _fullDescription;

public override FailureLevel DefaultLevel => FailureLevel.Error;

/// <summary>
/// SARIF1018
/// </summary>
public override string Id => RuleId.InvalidUriInOriginalUriBaseIds;

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

protected override void Analyze(Run run, string runPointer)
{
if (run.OriginalUriBaseIds != null)
{
string originalUriBaseIdsPointer = runPointer.AtProperty(SarifPropertyName.OriginalUriBaseIds);

foreach (string key in run.OriginalUriBaseIds.Keys)
{
AnalyzeOriginalUriBaseIdsEntry(run.OriginalUriBaseIds[key], originalUriBaseIdsPointer.AtProperty(key));
}
}
}

private void AnalyzeOriginalUriBaseIdsEntry(ArtifactLocation artifactLocation, string pointer)
{
// If uriBaseId is present, the uri must be relative. But this is true for _all_
// artifactLocation objects, not just the ones in run.originalUriBaseIds, so we
// will not verify it here. There will be a separate validation rule to enforce
// this condition. See https://github.com/microsoft/sarif-sdk/issues/1643.
if (artifactLocation.UriBaseId != null) { return; }

// We know that uriBaseId is absent. In this case, uri must _either_ be an absolute
// URI, or it must be absent.
if (artifactLocation.Uri == null) { return; }

// We know that uri is present, so now we can verify that it's an absolute URI.

// If it's not a well-formed URI of _any_ kind, then don't bother triggering this rule.
// Rule SARIF1003, UrisMustBeValid, will catch it.
// Check for well-formedness first, before attempting to create a Uri object, to
// avoid having to do a try/catch. Unfortunately Uri.TryCreate will return true
// even for a malformed URI string.
string uriString = artifactLocation.Uri.OriginalString;
if (uriString != null && Uri.IsWellFormedUriString(uriString, UriKind.RelativeOrAbsolute))
{
// Ok, it's a well-formed URI of some kind. If it's not absolute, _now_ we
// can report it.
Uri uri = new Uri(uriString, UriKind.RelativeOrAbsolute);
if (!uri.IsAbsoluteUri)
{
LogResult(pointer, nameof(RuleResources.SARIF1018_Default), uriString);
}
}
}
}
}
57 changes: 37 additions & 20 deletions src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,79 +32,96 @@ public void JSON1002_DeserializationError()

[Fact]
public void SARIF1001_DoNotUseFriendlyNameAsRuleId_Valid()
=> RunTest(RuleId.DoNotUseFriendlyNameAsRuleId + "." + nameof(RuleId.DoNotUseFriendlyNameAsRuleId) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.DoNotUseFriendlyNameAsRuleId, nameof(RuleId.DoNotUseFriendlyNameAsRuleId)));

[Fact]
public void SARIF1001_DoNotUseFriendlyNameAsRuleId_Invalid()
=> RunTest(RuleId.DoNotUseFriendlyNameAsRuleId + "." + nameof(RuleId.DoNotUseFriendlyNameAsRuleId) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.DoNotUseFriendlyNameAsRuleId, nameof(RuleId.DoNotUseFriendlyNameAsRuleId)));

[Fact]
public void SARIF1003_UrisMustBeValid_Valid()
=> RunTest(RuleId.UrisMustBeValid + "." + nameof(RuleId.UrisMustBeValid) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.UrisMustBeValid, nameof(RuleId.UrisMustBeValid)));

[Fact]
public void SARIF1003_UrisMustBeValid_Invalid()
=> RunTest(RuleId.UrisMustBeValid + "." + nameof(RuleId.UrisMustBeValid) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.UrisMustBeValid, nameof(RuleId.UrisMustBeValid)));

[Fact]
public void SARIF1007_EndTimeMustNotBeBeforeStartTime_Valid()
=> RunTest(RuleId.EndTimeMustNotBeBeforeStartTime + "." + nameof(RuleId.EndTimeMustNotBeBeforeStartTime) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.EndTimeMustNotBeBeforeStartTime, nameof(RuleId.EndTimeMustNotBeBeforeStartTime)));

[Fact]
public void SARIF1007_EndTimeMustNotBeBeforeStartTime_Invalid()
=> RunTest(RuleId.EndTimeMustNotBeBeforeStartTime + "." + nameof(RuleId.EndTimeMustNotBeBeforeStartTime) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.EndTimeMustNotBeBeforeStartTime, nameof(RuleId.EndTimeMustNotBeBeforeStartTime)));
[Fact]
public void SARIF1008_MessagesShouldEndWithPeriod_Valid()
=> RunTest(RuleId.MessagesShouldEndWithPeriod + "." + nameof(RuleId.MessagesShouldEndWithPeriod) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.MessagesShouldEndWithPeriod, nameof(RuleId.MessagesShouldEndWithPeriod)));

[Fact]
public void SARIF1008_MessagesShouldEndWithPeriod_Invalid()
=> RunTest(RuleId.MessagesShouldEndWithPeriod + "." + nameof(RuleId.MessagesShouldEndWithPeriod) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.MessagesShouldEndWithPeriod, nameof(RuleId.MessagesShouldEndWithPeriod)));
[Fact]
public void SARIF1012_EndLineMustNotBeLessThanStartLine_Valid()
=> RunTest(RuleId.EndLineMustNotBeLessThanStartLine + "." + nameof(RuleId.EndLineMustNotBeLessThanStartLine) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.EndLineMustNotBeLessThanStartLine, nameof(RuleId.EndLineMustNotBeLessThanStartLine)));

[Fact]
public void SARIF1012_EndLineMustNotBeLessThanStartLine_Invalid()
=> RunTest(RuleId.EndLineMustNotBeLessThanStartLine + "." + nameof(RuleId.EndLineMustNotBeLessThanStartLine) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.EndLineMustNotBeLessThanStartLine, nameof(RuleId.EndLineMustNotBeLessThanStartLine)));
[Fact]
public void SARIF1013_EndColumnMustNotBeLessThanStartColumn_Valid()
=> RunTest(RuleId.EndColumnMustNotBeLessThanStartColumn + "." + nameof(RuleId.EndColumnMustNotBeLessThanStartColumn) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.EndColumnMustNotBeLessThanStartColumn, nameof(RuleId.EndColumnMustNotBeLessThanStartColumn)));

[Fact]
public void SARIF1013_EndColumnMustNotBeLessThanStartColumn_Invalid()
=> RunTest(RuleId.EndColumnMustNotBeLessThanStartColumn + "." + nameof(RuleId.EndColumnMustNotBeLessThanStartColumn) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.EndColumnMustNotBeLessThanStartColumn, nameof(RuleId.EndColumnMustNotBeLessThanStartColumn)));
[Fact]
public void SARIF1014_UriBaseIdRequiresRelativeUri_Valid()
=> RunTest(RuleId.UriBaseIdRequiresRelativeUri + "." + nameof(RuleId.UriBaseIdRequiresRelativeUri) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.UriBaseIdRequiresRelativeUri, nameof(RuleId.UriBaseIdRequiresRelativeUri)));

[Fact]
public void SARIF1014_UriBaseIdRequiresRelativeUri_Invalid()
=> RunTest(RuleId.UriBaseIdRequiresRelativeUri + "." + nameof(RuleId.UriBaseIdRequiresRelativeUri) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.UriBaseIdRequiresRelativeUri, nameof(RuleId.UriBaseIdRequiresRelativeUri)));

[Fact]
public void SARIF1015_UriMustBeAbsolute_Valid()
=> RunTest(RuleId.UriMustBeAbsolute + "." + nameof(RuleId.UriMustBeAbsolute) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.UriMustBeAbsolute, nameof(RuleId.UriMustBeAbsolute)));

[Fact]
public void SARIF1015_UriMustBeAbsolute_Invalid()
=> RunTest(RuleId.UriMustBeAbsolute + "." + nameof(RuleId.UriMustBeAbsolute) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.UriMustBeAbsolute, nameof(RuleId.UriMustBeAbsolute)));

[Fact]
public void SARIF1016_ContextRegionRequiresRegion_Valid()
=> RunTest(RuleId.ContextRegionRequiresRegion + "." + nameof(RuleId.ContextRegionRequiresRegion) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.ContextRegionRequiresRegion, nameof(RuleId.ContextRegionRequiresRegion)));

[Fact]
public void SARIF1016_ContextRegionRequiresRegion_Invalid()
=> RunTest(RuleId.ContextRegionRequiresRegion + "." + nameof(RuleId.ContextRegionRequiresRegion) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.ContextRegionRequiresRegion, nameof(RuleId.ContextRegionRequiresRegion)));

[Fact]
public void SARIF1017_InvalidIndex_Valid()
=> RunTest(RuleId.InvalidIndex + "." + nameof(RuleId.InvalidIndex) + "_Valid.sarif");
=> RunTest(MakeValidTestFileName(RuleId.InvalidIndex, nameof(RuleId.InvalidIndex)));

[Fact]
public void SARIF1017_InvalidIndex_Invalid()
=> RunTest(RuleId.InvalidIndex + "." + nameof(RuleId.InvalidIndex) + "_Invalid.sarif");
=> RunTest(MakeInvalidTestFileName(RuleId.InvalidIndex, nameof(RuleId.InvalidIndex)));

[Fact]
public void SARIF1018_InvalidUriInOriginalUriBaseIds_Valid()
=> RunTest(MakeValidTestFileName(RuleId.InvalidUriInOriginalUriBaseIds, nameof(RuleId.InvalidUriInOriginalUriBaseIds)));

[Fact]
public void SARIF1018_InvalidUriInOriginalUriBaseIds_Invalid()
=> RunTest(MakeInvalidTestFileName(RuleId.InvalidUriInOriginalUriBaseIds, nameof(RuleId.InvalidUriInOriginalUriBaseIds)));

private const string ValidTestFileNameSuffix = "_Valid.sarif";
private const string InvalidTestFileNameSuffix = "_Invalid.sarif";

private string MakeValidTestFileName(string ruleId, string ruleName)
=> $"{ruleId}.{ruleName}{ValidTestFileNameSuffix}";

private string MakeInvalidTestFileName(string ruleId, string ruleName)
=> $"{ruleId}.{ruleName}{InvalidTestFileNameSuffix}";

protected override string ConstructTestOutputFromInputResource(string inputResourceName)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@
<None Remove="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1016.ContextRegionRequiresRegion_Valid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1017.InvalidIndex_Invalid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1017.InvalidIndex_Valid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1018.InvalidUriInOriginalUriBaseIds_Invalid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1018.InvalidUriInOriginalUriBaseIds_Valid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\Inputs\JSON1001.SyntaxError.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\Inputs\JSON1002.DeserializationError.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\Inputs\SARIF1001.DoNotUseFriendlyNameAsRuleId_Invalid.sarif" />
Expand All @@ -79,6 +81,8 @@
<None Remove="TestData\Multitool\ValidateCommand\Inputs\SARIF1016.ContextRegionRequiresRegion_Valid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\Inputs\SARIF1017.InvalidIndex_Invalid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\Inputs\SARIF1017.InvalidIndex_Valid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\Inputs\SARIF1018.InvalidUriInOriginalUriBaseIds_Invalid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\Inputs\SARIF1018.InvalidUriInOriginalUriBaseIds_Valid.sarif" />
<None Remove="TestData\Multitool\ValidateCommand\Inputs\SimpleExample.sarif-external-properties.json" />
</ItemGroup>

Expand Down Expand Up @@ -3059,6 +3063,8 @@
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1016.ContextRegionRequiresRegion_Valid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1017.InvalidIndex_Invalid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1017.InvalidIndex_Valid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1018.InvalidUriInOriginalUriBaseIds_Invalid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\ExpectedOutputs\SARIF1018.InvalidUriInOriginalUriBaseIds_Valid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\JSON1001.SyntaxError.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\JSON1002.DeserializationError.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SARIF1001.DoNotUseFriendlyNameAsRuleId_Invalid.sarif" />
Expand All @@ -3079,6 +3085,8 @@
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SARIF1015.UriMustBeAbsolute_Valid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SARIF1016.ContextRegionRequiresRegion_Invalid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SARIF1016.ContextRegionRequiresRegion_Valid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SARIF1018.InvalidUriInOriginalUriBaseIds_Invalid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SARIF1018.InvalidUriInOriginalUriBaseIds_Valid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SimpleExample.sarif-external-properties.json" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SARIF1017.InvalidIndex_Invalid.sarif" />
<EmbeddedResource Include="TestData\Multitool\ValidateCommand\Inputs\SARIF1017.InvalidIndex_Valid.sarif" />
Expand Down
Loading

0 comments on commit a1e0f26

Please sign in to comment.