Fix false positives for 'SARIF1002.UrisMustBeValid' due to Uri.IsWellFormedUriString bug #2501

marmegh · 2022-07-06T16:40:48Z

Uri.IsWellFormedUriString returns false for some valid URIs. This is resulting in some tools rejecting valid SARIF results files that conform to requirements but not all recommendations.

This fix implements a IsWellFormedUri method as part of the SarifValidationSkimmerBase class and replaces all Uri.IsWellFormedUriString() api calls with this method.

…medUri, plus added example.

eddynaka · 2022-07-11T17:26:50Z

src/Sarif.Multitool.Library/Rules/SarifValidationSkimmerBase.cs

@@ -246,6 +246,18 @@ internal static string JsonPointerToJavaScript(string pointerString)
            return sb.ToString();
        }

+        internal static bool UriIsWellFormedUriString(string uriString, UriKind uriKind)


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

EasyRhinoMSFT · 2022-07-11T17:49:28Z

src/Sarif.Multitool.Library/Rules/SarifValidationSkimmerBase.cs

@@ -246,6 +246,18 @@ internal static string JsonPointerToJavaScript(string pointerString)
            return sb.ToString();
        }

+        internal static bool UriIsWellFormedUriString(string uriString, UriKind uriKind)


Suggest naming IsWellFormedUriString per C# style guidelines #Closed

src/Sarif/Comparers/ComparerHelpers.cs

michaelcfanning · 2022-07-11T19:41:55Z

src/ReleaseHistory.md

@@ -3,6 +3,7 @@
 ## Unreleased

 * FEATURE: Add `max-file-size-in-kb` argument that allows filtering scan targets by file size. [#2494](https://github.com/microsoft/sarif-sdk/pull/2494)
+* BUGFIX: Fix false positive for `SARIF1002.UrisMustBeValid` for URIs with the format `file:/c:/location/filename.txt`. [#]()


format

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

michaelcfanning · 2022-07-11T19:44:35Z

src/Sarif.Multitool.Library/Rules/SARIF1002.UrisMustBeValid.cs

@@ -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))


UriIsWellFormedUriString

IsWellFormedUriString (add a great summary XML comment).

UriIsWellFormedHelper

ReplacementUriIsWellFormed... #Pending

michaelcfanning · 2022-07-11T19:46:27Z

src/Sarif.Multitool.Library/Rules/SarifValidationSkimmerBase.cs

+        internal static bool UriIsWellFormedUriString(string uriString, UriKind uriKind)
+        {
+            bool isWellFormed = Uri.IsWellFormedUriString(uriString, uriKind);
+            bool csBug = (uriString.StartsWith("file:/") && Uri.TryCreate(uriString, uriKind, out Uri result));


csBug

subjectToWellknownClrBug?

add a nice comment?
#Pending

michaelcfanning · 2022-07-11T19:48:01Z

src/Sarif.Multitool.Library/Rules/SARIF1002.UrisMustBeValid.cs

@@ -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))
                {
                    // {0}: The string '{1}' is not a valid URI reference. URIs must conform to
                    // [RFC 3986](https://tools.ietf.org/html/rfc3986).


Put this information in the helper.
#Pending

michaelcfanning · 2022-07-11T19:48:39Z

src/Sarif.Multitool.Library/Rules/SarifValidationSkimmerBase.cs

+        internal static bool UriIsWellFormedUriString(string uriString, UriKind uriKind)
+        {
+            bool isWellFormed = Uri.IsWellFormedUriString(uriString, uriKind);
+            bool csBug = (uriString.StartsWith("file:/") && Uri.TryCreate(uriString, uriKind, out Uri result));


StartsWith

this should be OrdinalIgnoreCase #Pending

michaelcfanning · 2022-07-11T19:48:54Z

src/Test.UnitTests.Sarif.Multitool.Library/Rules/SarifValidationSkimmerBaseTests.cs

+                {
+                    expectedIsWellFormedUri = true,
+                    uriKind = UriKind.Absolute,
+                    uriString = "file:///c:/Code/sarif-sdk/src/"


file

add tests with upper case!

michaelcfanning · 2022-07-11T19:50:55Z

src/Test.UnitTests.Sarif.Multitool.Library/Rules/SarifValidationSkimmerBaseTests.cs

+                {
+                    expectedIsWellFormedUri = true,
+                    uriKind = UriKind.RelativeOrAbsolute,
+                    uriString = "file:///c:/Code/sarif-sdk/src/"


file

test the network path case?

michaelcfanning · 2022-07-11T19:53:48Z

src/ReleaseHistory.md

@@ -3,6 +3,7 @@
 ## Unreleased

 * FEATURE: Add `max-file-size-in-kb` argument that allows filtering scan targets by file size. [#2494](https://github.com/microsoft/sarif-sdk/pull/2494)
+* BUGFIX: Fix false positive for `SARIF1002.UrisMustBeValid` for URIs with the format `file:/c:/location/filename.txt`. [#]()


#

? #Pending

michaelcfanning

src/Test.UnitTests.Sarif.Multitool.Library/Rules/SARIF1002.UrisMustBeValidTests.cs

EasyRhinoMSFT

…ning BuildAndTest

marmegh added 2 commits July 6, 2022 09:34

Replace Uri.IsWellFormedUri with SarifValidationSkimmerBase.IsWellFor…

60bb414

…medUri, plus added example.

Updated method name to standard

4bb9461

eddynaka reviewed Jul 11, 2022

View reviewed changes

EasyRhinoMSFT reviewed Jul 11, 2022

View reviewed changes

marmegh added 2 commits July 11, 2022 11:13

Fixed unhandled exception.

ebc734c

Resolve merge conflict.

83c2349

marmegh marked this pull request as ready for review July 11, 2022 18:17

marmegh requested a review from michaelcfanning as a code owner July 11, 2022 18:17

github-advanced-security bot found potential problems Jul 11, 2022

View reviewed changes

src/Sarif/Comparers/ComparerHelpers.cs Fixed Show fixed Hide fixed

Added unit test for UriIsWellFormedUriString

f288136

michaelcfanning reviewed Jul 11, 2022

View reviewed changes

michaelcfanning approved these changes Jul 11, 2022

View reviewed changes

marmegh added 2 commits July 12, 2022 11:42

Applied PR feedback.

04fe163

Fixed missing whitespace in test.

6c908c3

github-advanced-security bot found potential problems Jul 12, 2022

View reviewed changes

src/Test.UnitTests.Sarif.Multitool.Library/Rules/SARIF1002.UrisMustBeValidTests.cs Fixed Show fixed Hide fixed

EasyRhinoMSFT approved these changes Jul 12, 2022

View reviewed changes

Remove unneeded file and automatically generated code update from run…

976f073

…ning BuildAndTest

marmegh merged commit 64a4211 into main Jul 13, 2022

marmegh deleted the users/marmegh/validatorFix branch July 13, 2022 21:36

marmegh mentioned this pull request Jul 21, 2022

Creating release v.2.4.16 #2507

Merged

marmegh mentioned this pull request Aug 4, 2022

Newtonsoft and Layout Changes microsoft/sarif-website#142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix false positives for 'SARIF1002.UrisMustBeValid' due to Uri.IsWellFormedUriString bug #2501

Fix false positives for 'SARIF1002.UrisMustBeValid' due to Uri.IsWellFormedUriString bug #2501

marmegh commented Jul 6, 2022

eddynaka Jul 11, 2022 •

edited by marmegh

EasyRhinoMSFT Jul 11, 2022 •

edited

michaelcfanning Jul 11, 2022

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022

michaelcfanning Jul 11, 2022

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning left a comment

EasyRhinoMSFT left a comment

Fix false positives for 'SARIF1002.UrisMustBeValid' due to Uri.IsWellFormedUriString bug #2501

Fix false positives for 'SARIF1002.UrisMustBeValid' due to Uri.IsWellFormedUriString bug #2501

Conversation

marmegh commented Jul 6, 2022

eddynaka Jul 11, 2022 • edited by marmegh

Choose a reason for hiding this comment

EasyRhinoMSFT Jul 11, 2022 • edited

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022 • edited by marmegh

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022 • edited by marmegh

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022 • edited by marmegh

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022 • edited by marmegh

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022 • edited by marmegh

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

michaelcfanning Jul 11, 2022 • edited by marmegh

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

EasyRhinoMSFT left a comment

Choose a reason for hiding this comment

eddynaka Jul 11, 2022 •

edited by marmegh

EasyRhinoMSFT Jul 11, 2022 •

edited

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh

michaelcfanning Jul 11, 2022 •

edited by marmegh