Skip to content

Commit

Permalink
Fix #2090: Validator should warn of leading / in relative artifact lo…
Browse files Browse the repository at this point in the history 
…cation URIs (#2095)

* Fix #2090: Validator should warn of leading / in relative artifact location URIs

* Update release history.

* Remove unnecessary "== true".

Co-authored-by: Larry Golding <lgolding@comcast.net>
Co-authored-by: Michael C. Fanning <michael.fanning@microsoft.com>
  • Loading branch information
@michaelcfanning
3 people committed Oct 15, 2020
1 parent 39f6131 commit 53b3c0c
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 15 deletions.
1 change: 1 addition & 0 deletions src/ReleaseHistory.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## **v2.3.7** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.3.7) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.3.7) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.3.7) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.3.7) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.3.7)
* BUGFIX: GitHub policy should not turn off any note level rules. [#2089](https://github.com/microsoft/sarif-sdk/issues/2089)
* FEATURE: Add a converter for FlawFinder's CSV output format.
* FEATURE: The validation rule `SARIF1004.ExpressUriBaseIdsCorrectly` now verifies that if an `artifactLocation.uri` is a relative reference, it does not begin with a slash.
* DEPENDENCY BREAKING: SARIF now requires Newtonsoft.JSON 11.0.2 (rather than 10.0.3)
* DEPENDENCY: SARIF TypeScript package now requires minimist 1.2.3 or later (rather than >=1.2.0)
* FEATURE: Add a setter to `GitHelper.GitExePath`. [#2110](https://github.com/microsoft/sarif-sdk/pull/2110)
Expand Down
10 changes: 10 additions & 0 deletions src/Sarif.Multitool.Library/Rules/RuleResources.Designer.cs
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Sarif.Multitool.Library/Rules/RuleResources.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,9 @@ This is part of a set of authoring practices that make your rule messages more r

If an 'artifactLocation' object has a 'uriBaseId' property, its 'uri' property must be a relative reference, because if 'uri' is an absolute URI then 'uriBaseId' serves no purpose.

Every URI reference in 'originalUriBaseIds' must resolve to an absolute URI in the manner described in the SARIF specification [3.14.14](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317498).</value>
Every URI reference in 'originalUriBaseIds' must resolve to an absolute URI in the manner described in the SARIF specification [3.14.14](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317498).

Finally, a relative reference in 'artifactLocation.uri' must not begin with a slash, because that prevents it from combining properly with the absolute URI specified by a 'uriBaseId'.</value>
</data>
<data name="SARIF1005_UriMustBeAbsolute_Error_Default_Text" xml:space="preserve">
<value>{0}: The value of this property is required to be an absolute URI, but '{1}' is a relative URI reference.</value>
Expand Down Expand Up @@ -455,4 +457,7 @@ Semantics: Assuming the reader of the log file (an end user or another tool) has
<data name="SARIF2012_ProvideRuleProperties_Note_FriendlyNameNotAPascalIdentifier_Text" xml:space="preserve">
<value>{0}: '{1}' is not a Pascal identifier. For uniformity of experience across all tools that produce SARIF, the friendly name should be a single Pascal identifier, for example, 'ProvideRuleFriendlyName'.</value>
</data>
<data name="SARIF1004_ExpressUriBaseIdsCorrectly_Error_RelativeReferenceMustNotBeginWithSlash_Text" xml:space="preserve">
<value>The relative reference '{0}' begins with a slash, which will prevent it from combining properly with the absolute URI specified by a 'uriBaseId'.</value>
</data>
</root>
21 changes: 19 additions & 2 deletions src/Sarif.Multitool.Library/Rules/SARIF1004.ExpressUriBaseIdsCorrectly.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ public class ExpressUriBaseIdsCorrectly : SarifValidationSkimmerBase
/// Every URI reference in 'originalUriBaseIds' must resolve to an absolute URI in the manner
/// described in the SARIF specification
/// [3.14.14] (https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317498).
///
/// Finally, a relative reference in 'artifactLocation.uri' must not begin with a slash, because
/// that prevents it from combining properly with the absolute URI specified by a 'uriBaseId'.
/// </summary>
public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF1004_ExpressUriBaseIdsCorrectly_FullDescription_Text };

Expand All @@ -36,14 +39,18 @@ public class ExpressUriBaseIdsCorrectly : SarifValidationSkimmerBase
nameof(RuleResources.SARIF1004_ExpressUriBaseIdsCorrectly_Error_TopLevelUriBaseIdMustBeAbsolute_Text),
nameof(RuleResources.SARIF1004_ExpressUriBaseIdsCorrectly_Error_UriBaseIdValueMustEndWithSlash_Text),
nameof(RuleResources.SARIF1004_ExpressUriBaseIdsCorrectly_Error_UriBaseIdValueMustNotContainDotDotSegment_Text),
nameof(RuleResources.SARIF1004_ExpressUriBaseIdsCorrectly_Error_UriBaseIdValueMustNotContainQueryOrFragment_Text)
nameof(RuleResources.SARIF1004_ExpressUriBaseIdsCorrectly_Error_UriBaseIdValueMustNotContainQueryOrFragment_Text),
nameof(RuleResources.SARIF1004_ExpressUriBaseIdsCorrectly_Error_RelativeReferenceMustNotBeginWithSlash_Text)
};

public override FailureLevel DefaultLevel => FailureLevel.Error;

protected override void Analyze(ArtifactLocation artifactLocation, string artifactLocationPointer)
{
if (artifactLocation.UriBaseId != null && artifactLocation.Uri?.IsAbsoluteUri == true)
Uri artifactLocationUri = artifactLocation.Uri;
if (artifactLocationUri == null) { return; }

if (artifactLocation.UriBaseId != null && artifactLocationUri.IsAbsoluteUri)
{
// {0}: This 'artifactLocation' object has a 'uriBaseId' property '{1}', but its
// 'uri' property '{2}' is an absolute URI. Since the purpose of 'uriBaseId' is
Expand All @@ -55,6 +62,16 @@ protected override void Analyze(ArtifactLocation artifactLocation, string artifa
artifactLocation.UriBaseId,
artifactLocation.Uri.OriginalString);
}

if (!artifactLocationUri.IsAbsoluteUri && artifactLocationUri.OriginalString.StartsWith("/"))
{
// The relative reference '{0}' begins with a slash, which will prevent it from combining properly
// with the absolute URI specified by a 'uriBaseId'.
LogResult(
artifactLocationPointer.AtProperty(SarifPropertyName.Uri),
nameof(RuleResources.SARIF1004_ExpressUriBaseIdsCorrectly_Error_RelativeReferenceMustNotBeginWithSlash_Text),
artifactLocation.Uri.OriginalString);
}
}

protected override void Analyze(Run run, string runPointer)
Expand Down
42 changes: 35 additions & 7 deletions ...titool/ValidateCommand/ExpectedOutputs/SARIF1004.ExpressUriBaseIdsCorrectly_Invalid.sarif
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"text": "When using the 'uriBaseId' property, obey the requirements in the SARIF specification [3.4.4](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317431) that enable it to fulfill its purpose of resolving relative references to absolute locations."
},
"fullDescription": {
"text": "When using the 'uriBaseId' property, obey the requirements in the SARIF specification [3.4.4](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317431) that enable it to fulfill its purpose of resolving relative references to absolute locations. In particular:\r\n\r\nIf an 'artifactLocation' object has a 'uriBaseId' property, its 'uri' property must be a relative reference, because if 'uri' is an absolute URI then 'uriBaseId' serves no purpose.\r\n\r\nEvery URI reference in 'originalUriBaseIds' must resolve to an absolute URI in the manner described in the SARIF specification [3.14.14](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317498)."
"text": "When using the 'uriBaseId' property, obey the requirements in the SARIF specification [3.4.4](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317431) that enable it to fulfill its purpose of resolving relative references to absolute locations. In particular:\r\n\r\nIf an 'artifactLocation' object has a 'uriBaseId' property, its 'uri' property must be a relative reference, because if 'uri' is an absolute URI then 'uriBaseId' serves no purpose.\r\n\r\nEvery URI reference in 'originalUriBaseIds' must resolve to an absolute URI in the manner described in the SARIF specification [3.14.14](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317498).\r\n\r\nFinally, a relative reference in 'artifactLocation.uri' must not begin with a slash, because that prevents it from combining properly with the absolute URI specified by a 'uriBaseId'."
},
"messageStrings": {
"Error_UriBaseIdRequiresRelativeUri": {
Expand All @@ -31,6 +31,9 @@
},
"Error_UriBaseIdValueMustNotContainQueryOrFragment": {
"text": "{0}: The '{1}' element of 'originalUriBaseIds' has a 'uri' property '{2}' that contains a query or a fragment. This is not valid because the purpose of the 'uriBaseId' property is to help resolve a relative reference to an absolute URI by concatenating the relative reference to the absolute base URI. This won't work if the base URI contains a query or a fragment."
},
"Error_RelativeReferenceMustNotBeginWithSlash": {
"text": "The relative reference '{0}' begins with a slash, which will prevent it from combining properly with the absolute URI specified by a 'uriBaseId'."
}
},
"defaultConfiguration": {
Expand Down Expand Up @@ -334,7 +337,7 @@
"index": 0
},
"region": {
"startLine": 217,
"startLine": 224,
"startColumn": 35
}
}
Expand Down Expand Up @@ -367,6 +370,31 @@
}
]
},
{
"ruleId": "SARIF1004",
"ruleIndex": 0,
"level": "error",
"message": {
"id": "Error_RelativeReferenceMustNotBeginWithSlash",
"arguments": [
"runs[0].results[0].locations[1].physicalLocation.artifactLocation.uri",
"/leading_slash.c"
]
},
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"index": 0
},
"region": {
"startLine": 171,
"startColumn": 43
}
}
}
]
},
{
"ruleId": "SARIF1004",
"ruleIndex": 0,
Expand All @@ -386,7 +414,7 @@
"index": 0
},
"region": {
"startLine": 193,
"startLine": 200,
"startColumn": 47
}
}
Expand All @@ -412,7 +440,7 @@
"index": 0
},
"region": {
"startLine": 226,
"startLine": 233,
"startColumn": 37
}
}
Expand All @@ -438,7 +466,7 @@
"index": 0
},
"region": {
"startLine": 175,
"startLine": 182,
"startColumn": 43
}
}
Expand All @@ -464,7 +492,7 @@
"index": 0
},
"region": {
"startLine": 208,
"startLine": 215,
"startColumn": 37
}
}
Expand All @@ -490,7 +518,7 @@
"index": 0
},
"region": {
"startLine": 237,
"startLine": 244,
"startColumn": 39
}
}
Expand Down
8 changes: 8 additions & 0 deletions ...tData/Multitool/ValidateCommand/Inputs/SARIF1004.ExpressUriBaseIdsCorrectly_Invalid.sarif
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,14 @@
"uriBaseId": "%SRCROOT%"
}
}
},
{
"physicalLocation": {
"artifactLocation": {
"uri": "/leading_slash.c"

}
}
}
],
"stacks": [
Expand Down
10 changes: 5 additions & 5 deletions ...estData/Multitool/ValidateCommand/Inputs/SARIF1004.ExpressUriBaseIdsCorrectly_Valid.sarif
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,19 +77,19 @@
"uriBaseId": "%SRCROOT%"
},
"stdin": {
"uri": "/c:/log/in.txt",
"uri": "log/in.txt",
"uriBaseId": "%SRCROOT%"
},
"stdout": {
"uri": "/c:/log/out.txt",
"uri": "log/out.txt",
"uriBaseId": "%SRCROOT%"
},
"stderr": {
"uri": "/c:/log/err.txt",
"uri": "log/err.txt",
"uriBaseId": "%SRCROOT%"
},
"stdoutStderr": {
"uri": "/c:/log/out-err.txt",
"uri": "log/out-err.txt",
"uriBaseId": "%SRCROOT%"
}
}
Expand All @@ -112,7 +112,7 @@
"repositoryUri": "https://github.com/example-corp/browser",
"revisionId": "de67ef7",
"mappedTo": {
"uri": "/browser",
"uri": "browser",
"uriBaseId": "%SRCROOT%"
}
}
Expand Down

0 comments on commit 53b3c0c

Please sign in to comment.