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 false positives for 'SARIF1002.UrisMustBeValid' due to Uri.IsWellFormedUriString bug #2501

Merged
merged 8 commits into from
Jul 13, 2022
4 changes: 4 additions & 0 deletions src/ReleaseHistory.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# SARIF Package Release History (SDK, Driver, Converters, and Multitool)

## Unreleased

* BUGFIX: Fix false positive for `SARIF1002.UrisMustBeValid` for URIs with the format `file:/c:/location/filename.txt`. [#]()
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

format

'for file URIs that omit the 'authority' #Pending

Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

#

? #Pending


## **v2.4.15** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.15) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.15) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.15) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.15) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.15)

* BUGFIX: Fix `ArgumentNullException` when `PropertiesDictionary` is instantiated with a null comparer. [#2482](https://github.com/microsoft/sarif-sdk/pull/2482)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ private void AnalyzeUri(Uri uri, string pointer)
string uriString = uri?.OriginalString;
if (uriString != null)
{
if (!Uri.IsWellFormedUriString(uriString, UriKind.RelativeOrAbsolute))
if (!UriIsWellFormedUriString(uriString, UriKind.RelativeOrAbsolute))
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

UriIsWellFormedUriString

IsWellFormedUriString (add a great summary XML comment).

UriIsWellFormedHelper

ReplacementUriIsWellFormed... #Pending

{
// {0}: The string '{1}' is not a valid URI reference. URIs must conform to
// [RFC 3986](https://tools.ietf.org/html/rfc3986).
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

Put this information in the helper.
#Pending

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ private void AnalyzeOriginalUriBaseIdsEntry(string uriBaseId, ArtifactLocation a
// 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))
if (uriString != null && UriIsWellFormedUriString(uriString, UriKind.RelativeOrAbsolute))
{
var uri = new Uri(uriString, UriKind.RelativeOrAbsolute);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ private void AnalyzeUri(string uriString, string pointer)
// 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.
if (uriString != null && Uri.IsWellFormedUriString(uriString, UriKind.RelativeOrAbsolute))
if (uriString != null && UriIsWellFormedUriString(uriString, UriKind.RelativeOrAbsolute))
{
// Ok, it's a well-formed URI of some kind. If it's not absolute, _now_ we
// can report it.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ private void AnalyzeUri(string uriString, string pointer)
// 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.
if (uriString != null && Uri.IsWellFormedUriString(uriString, UriKind.Absolute))
if (uriString != null && UriIsWellFormedUriString(uriString, UriKind.Absolute))
{
// Ok, it's a well-formed absolute URI. If it's not reachable, _now_ we can report it.
Uri uri = new Uri(uriString, UriKind.Absolute);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,14 @@ internal static string JsonPointerToJavaScript(string pointerString)
return sb.ToString();
}

internal static bool UriIsWellFormedUriString(string uriString, UriKind uriKind)
Copy link
Collaborator

@eddynaka eddynaka Jul 11, 2022

Choose a reason for hiding this comment

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

UriIsWellFormedUriString

create a unit test that checks for the previous behavior and the new behavior.

This would guarantee that even if we remove the E2E tests, we would still be able to validate it. #Pending

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest naming IsWellFormedUriString per C# style guidelines #Closed

Copy link
Member

Choose a reason for hiding this comment

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

+1

{
bool isWellFormed = Uri.IsWellFormedUriString(uriString, uriKind);
bool csBug = (uriString.StartsWith("file:/") && Uri.TryCreate(uriString, uriKind, out Uri result));
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

csBug

subjectToWellknownClrBug?

add a nice comment?
#Pending

Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

StartsWith

this should be OrdinalIgnoreCase #Pending


return isWellFormed || csBug;
}

private static readonly string s_javaScriptIdentifierPattern = @"^[$_\p{L}][$_\p{L}0-9]*$";
private static readonly Regex s_javaScriptIdentifierRegex = new Regex(s_javaScriptIdentifierPattern, RegexOptions.Compiled);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@
"uriBaseId": "SRCROOT",
"index": 1
}
},
{
"location": {
"uri": "file:/c:/src/file.c",
"index": 2
}
}
],
"results": [
Expand Down