Adding Rule SARIF2009 #1938

eddynaka · 2020-06-25T00:55:17Z

No description provided.

Adding tests

michaelcfanning · 2020-06-25T13:16:02Z

src/Sarif.Multitool/Rules/SARIF2009.UseConventionalSymbolicNames.cs

+                    // and 'BINROOT' for the root of the directory containing build output (if your project places all build output in a common directory).
+                    LogResult(
+                        originalUriBaseIdsPointer.AtProperty(originalUriBaseId.Key),
+                        nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text),


SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text [](start = 45, length = 70)

resource name is wrong? #Closed

just updated! thanks!

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

michaelcfanning · 2020-06-25T13:16:12Z

src/Sarif.Multitool/Rules/SARIF2009.UseConventionalSymbolicNames.cs

+                // Using a conventional format for the rule id provides a more uniform experience across tools.
+                LogResult(
+                    reportingDescriptorPointer.AtProperty(SarifPropertyName.Id),
+                    nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text),


SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text [](start = 41, length = 70)

resource name wrong? #Closed

just updated! thanks!

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

michaelcfanning · 2020-06-25T13:17:22Z

src/Sarif.Multitool/Rules/SARIF2009.UseConventionalSymbolicNames.cs

+            nameof(RuleResources.SARIF2009_UseConventionalSymbolicNames_Warning_UseConventionalUriBaseIdNames_Text)
+        };
+
+        public override FailureLevel DefaultLevel => FailureLevel.Warning;


Warning [](start = 66, length = 7)

Default level probably too high for this rule governing name conventions #Closed

Just updated to Note.

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

michaelcfanning · 2020-06-25T13:18:56Z

src/Sarif.Multitool/Rules/SARIF2009.UseConventionalSymbolicNames.cs

+        /// <summary>
+        /// SARIF2009
+        /// </summary>
+        public override string Id => RuleId.UseConventionalSymbolicNames;


UseConventionalSymbolicNames [](start = 44, length = 28)

We should drop this rule to a note and change the name to 'ConsiderUsingConventionalNames'. 'Symbolic' isn't buying us much as an adjective (and symbol is a highly used term in analysis itself, so it's a little confusing. We can use 'Consider' for rules that truly are for nice-to-have conventions, as this one is. #Resolved

Just updated! Thank you!

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

michaelcfanning · 2020-06-25T13:20:26Z

src/Sarif.Multitool/Rules/SARIF2009.UseConventionalSymbolicNames.cs

+
+        public override FailureLevel DefaultLevel => FailureLevel.Warning;
+
+        private static readonly string[] s_conventionalSymbols = new string[] { "REPOROOT", "SRCROOT", "TESTROOT", "BINROOT" };


REPOROOT [](start = 81, length = 8)

Why are we using these terms? Is there precedent in existing tools? I don't see the component governance coinage, which is SCANROOT. Why does testroot exist? Why do we permit two locations for source roots, effectively? In short, @lgolding, maybe puzzle on this a bit more.

michaelcfanning · 2020-06-25T13:21:55Z

src/Sarif.Multitool/Rules/SARIF2009.UseConventionalSymbolicNames.cs

+        /// Many tools follow a conventional format for the 'reportingDescriptor.id' property: a short string identifying the tool concatenated with a numeric rule number,
+        /// for example, 'CS2001' for a diagnostic from the Roslyn C# compiler. For uniformity of experience across tools, we recommend this format.
+        /// 
+        /// Many tool use similar names for 'uriBaseId' symbols.We suggest 'REPOROOT' for the root of a repository, 'SRCROOT' for the root of the directory containing all source code, 'TESTROOT' for the root of the directory containing all test code(if your repository is organized in that way), and 'BINROOT' for the root of the directory containing build output(if your project places all build output in a common directory).


the [](start = 130, length = 3)

this comment line is too long and has spacing issues. #Closed

just updated! Can you check again? Thank you

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

Better! My rule of thumb for this is to establish the right hand line length, typically from the first line. In your case, is is on line 23, the line ending in 'property:'. Then, I make sure and run all possible text up to this column. Basically attempt to right justify as far as possible. Following my convention, your comment could be more compact. The term 'for' at the beginning of line 25, for example, could be placed at the end of line 24 without exceeding the established line length. Make sense?

In reply to: 445569155 [](ancestors = 445569155,445553061)

just updated! i think it is more justified and almost all lines are following the same length. Let me know what do you think!

In reply to: 445574287 [](ancestors = 445574287,445569155,445553061)

michaelcfanning · 2020-06-25T13:22:26Z

src/Sarif.Multitool/Rules/SARIF2009.UseConventionalSymbolicNames.cs

+                {
+                    // {0}: The 'originalUriBaseIds' symbol '{1}' is not one of the conventional symbols. 
+                    // We suggest 'REPOROOT' for the root of a repository, 'SRCROOT' for the root of the directory containing all source code,
+                    // 'TESTROOT' for the root of the directory containing all test code (if your repository is organized in that way),


the lines of this comment are too long and need to be truncated. #Closed

just updated! Can you check again? Thank you

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

Improved! My rule of thumb, though, is that there should never be complete words that are to the right margin of the shortest preceding line. On line 100, for example, 'directory' should have been dropped to the next line (as it is entirely right of the margin established on line 99 by 'symbols.' It is fine for a word to 'poke' past the end of the margin, as long as it starts before that rightmost margin. Make sense? Sorry to nit-pick so much on the comments, but rules source code is authored once and truly read many many times as people use a tool, are confused by results, want to understand more, etc. Analysis rules like this also are effectively a set of guidelines encoded in C#. At some point, these rules might be reauthored in another analysis environment.

In reply to: 445569283 [](ancestors = 445569283,445553388)

i will keep your guidelines in mind! Thanks for sharing and reviewing. just updated!

In reply to: 445585630 [](ancestors = 445585630,445569283,445553388)

+1 on Michael's formatting guidelines.

In reply to: 445591404 [](ancestors = 445591404,445585630,445569283,445553388)

michaelcfanning · 2020-06-25T13:23:44Z

Has anyone run this rule yet against test SARIF in the repo? #Resolved

eddynaka · 2020-06-25T13:27:14Z

Hi @michaelcfanning , i have ran yesterday. I will double check. Thanks for reviewing. Checking everything right now.

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

eddynaka · 2020-06-25T13:46:01Z

Just ran again and i got some questions. So, i changed the error level from warning to note. With that, the output sarif isn't showing anymore. Is that expected?

In reply to: 649540515 [](ancestors = 649540515,649538563)

michaelcfanning · 2020-06-25T13:48:43Z

Thanks, Eddy! By the way, this is very nice work. My questions here are mostly along whether this rule is something reasonable to put on users. :S We will sort that out, though. 'note' levels require passing a --verbose flag on the command-line (not actually sure whether the validation rules support this yet).

In reply to: 649551076 [](ancestors = 649551076,649540515,649538563)

eddynaka · 2020-06-25T14:01:53Z

Thank you for reviewing! i will update those tests that are failing. Later we can check it out how can we validate the output

In reply to: 649552588 [](ancestors = 649552588,649551076,649540515,649538563)

michaelcfanning · 2020-06-25T14:09:07Z

...TestData/Multitool/ValidateCommand/Inputs/SARIF1004.ExpressUriBaseIdsCorrectly_Invalid.sarif

          "uri": "project",
          "description": {
            "text": "This artifactLocation has no uriBaseId, so its uri, if present, must be absolute. But it isn't. It also doesn't end with a slash."
          }
        },
-        "SOURCE_ROOT": {
+        "SRCROOT": {
          "uri": "src",
          "uriBaseId": "PROJECT_ROOT",


"PROJECT_ROOT [](start = 23, length = 13)

PROJECT_ROOT was renamed, yes? #Resolved

since your first comment about the terms, i will rollback to source_root and wait for larry's comment, ok?

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

It's fine to bring this change into conformance with Larry's original proposal. To be consistent, we would rename this to 'REPOROOT' am I correct?

In reply to: 445589112 [](ancestors = 445589112,445586890)

yes, it would. i will check with him, because, we have many keys that would alert in this scenario and if that is expected.

In reply to: 445594014 [](ancestors = 445594014,445589112,445586890)

I rollback to the initial version and enabled the verbose by passing true as parameter. What do you think?

In reply to: 445595450 [](ancestors = 445595450,445594014,445589112,445586890)

ghost · 2020-06-25T16:55:22Z

src/Sarif.Multitool/Rules/SARIF2009.UseConventionalSymbolicNames.cs

+                // uniform experience across tools.
+                LogResult(
+                    reportingDescriptorPointer.AtProperty(SarifPropertyName.Id),
+                    nameof(RuleResources.SARIF2009_UseConventionalSymbolicNames_Warning_UseConventionalRuleIds_Text),


Warning [](start = 80, length = 7)

But now it needs to be "Note" instead of "Warning". This is why I don't agree with encoding the level in the resource name. #Resolved

Just updated! Thank you!

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

eddynaka · 2020-06-25T17:48:32Z

Just updated again. Added true as parameter and used that to enable the verbose option.

In reply to: 649561880 [](ancestors = 649561880,649552588,649551076,649540515,649538563)

ghost · 2020-06-25T21:54:27Z

src/Sarif.Multitool/Rules/SARIF2009.ConsiderConventionalIdentifierValues.cs

+                LogResult(
+                    reportingDescriptorPointer.AtProperty(SarifPropertyName.Id),
+                    nameof(RuleResources.SARIF2009_ConsiderConventionalIdentifierValues_Note_UseConventionalRuleIds_Text),
+                    reportingDescriptor.Name);


Name [](start = 40, length = 4)

The message should mention the id, not the name. #Resolved

ghost · 2020-06-25T21:58:38Z

...f/TestData/Multitool/ValidateCommand/Inputs/SARIF1004.ExpressUriBaseIdsCorrectly_Valid.sarif

@@ -10,18 +10,18 @@
        }
      },
      "originalUriBaseIds": {
-        "PROJECT_ROOT": {
+        "REPOROOT": {


REPOROOT [](start = 9, length = 8)

For now, let's use these:

The old PROJECT_ROOT becomes REPO_ROOT (still contains an underscore).

The old SOURCE_ROOT is unchanged (do not change it to SRCROOT).

You're right, RULES_ROOT is not in our current set. Changing to TEST_ROOT in this test file is fine -- but do use the underscore. #Resolved

ghost · 2020-06-25T21:59:17Z

...f/TestData/Multitool/ValidateCommand/Inputs/SARIF1004.ExpressUriBaseIdsCorrectly_Valid.sarif

          "description": {
            "text": "This artifactLocation has neither a uri nor a uriBaseId. This is fine."
          }
        },
-        "RULES_ROOT": {
+        "TESTROOT": {
          "uri": "file:///C:/rules/",


rules [](start = 29, length = 5)

Let's change rules to tests here for consistency with the key TEST_ROOT. #Resolved

ghost

🕐

ghost

Adding Rule SARIF2009

0d6d466

Adding tests

eddynaka self-assigned this Jun 25, 2020

eddynaka requested a review from a user June 25, 2020 00:56

michaelcfanning reviewed Jun 25, 2020

View reviewed changes

code review - 1

735f4f6

code review - 2

85801c3

michaelcfanning reviewed Jun 25, 2020

View reviewed changes

code review - 3

729449e

ghost reviewed Jun 25, 2020

View reviewed changes

code review - 4

59bb5c4

code review - 5

c8da712

ghost reviewed Jun 25, 2020

View reviewed changes

ghost suggested changes Jun 25, 2020

View reviewed changes

ghost approved these changes Jun 25, 2020

View reviewed changes

code review - 6

1baafcf

eddynaka marked this pull request as ready for review June 25, 2020 22:21

eddynaka merged commit 2d8b655 into features/sarif-validation-rules Jun 25, 2020

eddynaka deleted the users/ednakamu/sarif-2009 branch June 25, 2020 22:22


		public override FailureLevel DefaultLevel => FailureLevel.Warning;

		private static readonly string[] s_conventionalSymbols = new string[] { "REPOROOT", "SRCROOT", "TESTROOT", "BINROOT" };

Adding Rule SARIF2009 #1938

Adding Rule SARIF2009 #1938

Conversation

eddynaka commented Jun 25, 2020

michaelcfanning Jun 25, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jun 25, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jun 25, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jun 25, 2020 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jun 25, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jun 25, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Jun 25, 2020 • edited by eddynaka Loading

eddynaka commented Jun 25, 2020 • edited Loading

eddynaka commented Jun 25, 2020

michaelcfanning commented Jun 25, 2020

eddynaka commented Jun 25, 2020

michaelcfanning Jun 25, 2020 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jun 25, 2020 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka commented Jun 25, 2020

ghost Jun 25, 2020 • edited by eddynaka Loading

Choose a reason for hiding this comment

ghost Jun 25, 2020 • edited by eddynaka Loading

Choose a reason for hiding this comment

ghost Jun 25, 2020 • edited by eddynaka Loading

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

michaelcfanning Jun 25, 2020 •

edited by ghost

Loading

michaelcfanning Jun 25, 2020 •

edited by ghost

Loading

michaelcfanning Jun 25, 2020 •

edited by ghost

Loading

michaelcfanning Jun 25, 2020 •

edited by eddynaka

Loading

michaelcfanning Jun 25, 2020 •

edited by ghost

Loading

michaelcfanning Jun 25, 2020 •

edited by ghost

Loading

michaelcfanning commented Jun 25, 2020 •

edited by eddynaka

Loading

eddynaka commented Jun 25, 2020 •

edited

Loading

michaelcfanning Jun 25, 2020 •

edited by eddynaka

Loading

ghost Jun 25, 2020 •

edited by eddynaka

Loading

ghost Jun 25, 2020 •

edited by eddynaka

Loading

ghost Jun 25, 2020 •

edited by eddynaka

Loading

ghost Jun 25, 2020 •

edited by eddynaka

Loading