Updating SARIF2004

[eddynaka]

Fixes #1994

[ghost]

ToString() [](start = 54, length = 10)

I'm not sure whether ToString() or OriginalString is better. #Closed

[eddynaka]

Changed to OriginalString

---

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

[ghost]

p [](start = 8, length = 1)

Restore the blank line. (Our coding standard is "blank line after closing brace".) #Closed

[eddynaka]

Oops

---

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

[ghost]

Don't specify ruleIndex here. The default is -1, so if it it missing, it will be set to -1 when we deserialize the log file. #Closed

[ghost]

artifacts array provides additional useful information [](start = 45, length = 54)

The artifacts array doesn't contain additional information. All it has is a location. (I understand that Line 62 doesn't have uriBaseId, and the artifacts array does, but still, the intent of the rule is that the artifacts array should provide something besides location information. In any case, why are we adding this test? Don't we already have tests for non-useful artifacts arrays? #Closed

[eddynaka]

Besides the point that i added a non-absoluteuri, this testing is a good one saying that we have a location.physicallocation.artifactlocation.uri != analysisTarget. The message is wrong and i will solve it

---

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

[ghost]

results [](start = 7, length = 7)

The "expected output" from a "valid" test shouldn't have any results. Something must be wrong in the logic. #Closed

[eddynaka]

The problem here is that the rule 1010 and 2004 overlap in one scenario. Should i remove that scenario (when result.ruleId exists and result.rule.Id exists)

---

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

[ghost]

No, just disable 2004 for this test.

---

In reply to: 454713672 [](ancestors = 454713672,454707521)

[ghost]

AnalyzeResults [](start = 16, length = 14)

You don't need to write AnalyzeResults and call it from here. Just override Analyze(Result result, string resultPointer), and call your helpers ReportUnnecessaryAnalysisTarget and ReportRuleDuplication from there.

This works because you really want to analyze every result. This is unlike the cases where you only want to analyze certain physical locations, or certain URIs -- and so you have to navigate down from the parent to make sure that you only analyze the ones you care about.

Make sense? #Closed

[eddynaka]

Yes!

---

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

[ghost]

result [](start = 28, length = 6)

I would break after logging the first one. If you discover that "analysisTarget isn't necessary because it's the same as location 0", there's no need to also report "analysisTarget isn't necessary because it's the same as location 1". #Closed

[ghost]

This was marked Resolved but it wasn't actually fixed. I think you tried to fix it with the return statements above, but that's not what I meant.

---

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

[eddynaka]

i see! i though it was in another place.

---

In reply to: 455344867 [](ancestors = 455344867,455252796)

[ghost]

Good result: artifacts array provides additional useful information [](start = 32, length = 67)

This comment isn't right. The reason this is valid is that rule.toolComponent is present. #Closed

[ghost]

artifacts array does not provide any additional useful information [](start = 44, length = 66)

Comment is wrong. This result is bad because analysisTarget is the same as one of the locations. #Closed

[ghost]

Do not include "ruleIndex": -1 #Closed

[ghost]

artifacts array does not provide any additional useful information [](start = 44, length = 66)

Comment is wrong: This is invalid because ruleIndex and rule are both present. #Closed

[ghost]

1. It is legal to "not have enough information to locate the rule metadata". Rule metadata is entirely optional in SARIF.
2. But in any case, "not enough information to locate the rule metadata" is not one of the cases we are testing for with this new rule. So I don't understand why this test case is here.

---

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

[eddynaka]

for this case, we are analyzing the rule where we don't have the ruleIndex AND we don't have the toolComponent. So thats a bad case. With that, i changed to not enough information. Isn't that right?

---

In reply to: 455342702 [](ancestors = 455342702,455257253)

[ghost]

It is legal not to have rule index. I see now that this test case is "You should use result.ruleId instead of result.rule.id"

---

In reply to: 455382915 [](ancestors = 455382915,455342702,455257253)

[ghost]

This comment still doesn't describe this test case. In this test case, toolComponent does not refer to the driver, so result.rule is necessary. But since result.rule is present, result.ruleId should not be present. So the message might be "Bad result: ruleId duplicates information in rule."

---

In reply to: 455389470 [](ancestors = 455389470,455382915,455342702,455257253)

[ghost]

Bad [](start = 32, length = 3)

Same here. #Closed

[ghost]

SARIF2004 [](start = 21, length = 9)

Disable 2004 for this test, too. #Closed

[ghost]

SARIF1009 [](start = 21, length = 9)

Why did this result disappear? #Closed

[eddynaka]

i rollback my changes and added to ignore the rule 2004 as well.

---

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

[ghost]

🕐

[ghost]

return [](start = 20, length = 6)

There is no need for either of these return statements. There is no loop here. No matter which branch the code takes, control will pass out of the end of the if statement and out of this function. I think you were trying to respond to my comment below... #Closed

[eddynaka]

updated the wrong place. removing that

---

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

[ghost]

You can shorten this:

if (result.Rule.ToolComponent?.RefersToDriver(this.driverGuid) == true)

Note that you need the explicit comparison == true because the null-coalescing operator ?. causes the left-hand side to have the type Nullable<bool> -- and you can't do an implicit logical test on a Nullable<bool>. #Closed

[ghost]

LogResult [](start = 24, length = 9)

This logic is backwards. Take a look at the notes I attached to the chat. The idea is:

If toolComponent is present, and if it does not refer to the driver -- which means that it has to be present! -- then result.ruleId and result.ruleIndex should not also be present.

Here's what the notes said:

  if (result.Rule.ToolComponent != null && !result.Rule.ToolComponent.RefersToDriver(this.currentDriverGuid))
  {
    // We HAD to use result.Rule (because otherwise we couldn't have specified the extension component).
    if either result.ruleId or result.ruleIndex is present
        // Complain, because we should have put them in one place: under result.Rule along with the toolComponent.
        "This result provides the 'rule' property, but it also provides either or both of 'ruleId' and 'ruleIndex'. If 'rule' is present, then use 'rule.id' instead of 'ruleId', and use 'rule.index' instead of 'ruleIndex'."
  }
``` #Closed

[ghost]

InlineData [](start = 9, length = 10)

Need a test where guid is null. That is actually the more likely case -- the SARIF file doesn't even mention guid. It is less likely that guid is present and its value is the empty string. Keep the "empty string" test, though, by all means. #Closed

[ghost]

Theory [](start = 9, length = 6)

We don't use Theory in this code base because we had some bad perf experience with it some years ago. It might be much better now. We use another technique for data-driven tests. I'll go find an example and add it to this comment. Stand by... #Closed

[ghost]

For an example of our pattern, see src\Test.UnitTests.Sarif.WorkItems\SarifWorkItemExtensionsTests.cs, Lines 118-186.

---

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

[ghost]

The set of test cases is fine. Please use the pattern in src\Test.UnitTests.Sarif.WorkItems\SarifWorkItemExtensionsTests.cs, Lines 118-186:

• This pattern accumulates failures across all test cases, and then displays them at the end.
• Use FluentAssertions.
• Define a struct to define the shape of the test data, so you can use object initializer syntax, and so you can refer to the test data with readable member names instead of "Item1", "Item2"...

---

In reply to: 455441780 [](ancestors = 455441780,455431411)

[ghost]

Ok, we are now defining the test data according to the pattern in src\Test.UnitTests.Sarif.WorkItems\SarifWorkItemExtensionsTests.cs, Lines 118-186. We still need to do the following:

• Accumulate the failing test cases. See how SarifWorkItemExtensions_PhraseToolNames_ConstructPhraseCorrectly accumulates a list of the failed test cases in a StringBuilder.
• Use FluentAssertions. See how that test method uses sb.Length.Should().Be(0, "... message listing the failed test cases ...")

---

In reply to: 455806704 [](ancestors = 455806704,455441780,455431411)

[ghost]

The reason for accumulating the test cases is that you can report all the failures at once, instead of stopping at the first failure.

---

In reply to: 455835254 [](ancestors = 455835254,455806704,455441780,455431411)

[ghost]

I will provide new strings. #Closed

[ghost]

New message:

The result at '{0}' uses the 'rule' property to specify the violated rule, so it is not necessary also to specify 'ruleId' or 'ruleIndex'. Remove the 'ruleId' and 'ruleIndex' properties.

---

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

[ghost]

default tool [](start = 95, length = 12)

"default tool" => "tool.driver" #Closed

[ghost]

IsNullOrWhiteSpace [](start = 27, length = 18)

Just null. We are checking if toolComponent.guid is absent from the SARIF file. If it's absent from the SARIF file, toolComponent.guid will be null. #Closed

[eddynaka]

ok. i will remove the test cases where we test for "", because that won't exist

---

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

[ghost]

New message:

The result at '{0}' uses the 'rule' property to specify the violated rule, but this is not necessary because the rule is defined by 'tool.driver'. Use the 'ruleId' and 'ruleIndex' instead, because they are shorter and just as clear. #Closed

[ghost]

Same message as on Line 94 above ("The result at '{0}' uses the 'rule' property to specify the violated rule, but this is not necessary because ..."). #Closed

[ghost]

{0}: [](start = 11, length = 5)

"In the result at '{0}', the 'analysisTarget' property '{1}' is unnecessary..." #Closed

[ghost]

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

The 'analysisTarget' property '{1}' at '{0}' is unnecessary because it is the same as the result location. Remove the 'analysisTarget' property. #Closed

[ghost]

                firstArtifactPointer,

I want the new message to point to the artifacts array, not to the first element of the array. So define artifactPointer = runPointer.AtProperty(SarifPropertyName.Artifacts) and use it here. I'll provide the new message shortly. #Closed

---

Refers to: src/Sarif.Multitool/Rules/SARIF2004.OptimizeFileSize.cs:178 in 84bde22. [](commit_id = 84bde22, deletion_comment = False)

[ghost]

                firstArtifactPointer,

Here you go:

The 'artifacts' array at '{0}' contains no information..."

---

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

---

Refers to: src/Sarif.Multitool/Rules/SARIF2004.OptimizeFileSize.cs:178 in 84bde22. [](commit_id = 84bde22, deletion_comment = False)

[ghost]

        if (HasIdOnlyRules(firstRulePointer))

Similarly here: define rulesPointer to point to the array (and calculate firstRulePointer from rulesPointer, and use rulesPointer in the new message:

"The 'rules' array at '{0}' contains no information..." #Closed

---

Refers to: src/Sarif.Multitool/Rules/SARIF2004.OptimizeFileSize.cs:219 in 84bde22. [](commit_id = 84bde22, deletion_comment = False)

[ghost]

        // null check in not required.

"in" =>"is" #Closed

---

Refers to: src/Sarif.Multitool/Rules/SARIF2004.OptimizeFileSize.cs:205 in 84bde22. [](commit_id = 84bde22, deletion_comment = False)

[ghost]

FluentAssertions [](start = 6, length = 16)

System usings come first. #Closed

[ghost]

[michaelcfanning]

We should have code that auto-emits this content, rather than maintaining it in three places (resource strings, as comments in rule source, and here). Action item for later. #Pending

[michaelcfanning]

unnecessary [](start = 48, length = 11)

Please add something like: 'The 'analysisTarget' property is used to distinguish cases when a result fires in a file (such as an included header) that is different than the file that was scanned (such as a .cpp file that included the header). This C++ example example is the canonical scenario that led to this design pattern.

Also, though it is implied, the reason we recommend removing it is to optimize file size.

The 'analysisTarget' property '{1}' at '{0}' can be removed because it is the same as the result location. This unnecessarily increases log file size. 'The 'analysisTarget' property is used to distinguish cases when a result fires in a file (such as an included header) that is different than the file that was scanned (such as a .cpp file that included the header).
#Closed

[ghost]

@eddynaka, +1 on Michael's rephrasing (the very last paragraph in his comment above).

---

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

[ghost]

... except that instead of "when a result fires" I suggest "when a tool detects a result".

---

In reply to: 455907085 [](ancestors = 455907085,455904829)

[michaelcfanning]

I didn't intend to take over reauthoring the words, just point you in the right direction. :) feel free to send Eddy updates after it goes through your wordsmithing process. Every string needs a pass in my estimation. I need the team to send me the logs you generated in testing so that I can see what your new output looks like in bulk. If we aren't yet doing that testing, it needs to be included in the formalized process.

---

In reply to: 455907934 [](ancestors = 455907934,455907085,455904829)

[eddynaka]

just updated this as well!

---

In reply to: 455913218 [](ancestors = 455913218,455907934,455907085,455904829)

[michaelcfanning]

You have a tendency to introduce descriptive prefixes in rules that I find distracting. The '{0}' value tends to be sufficiently descriptive to illuminate what's under discussion. This is most easily seen when actually reviewing output of the tool (which I encourage you to do), particularly real world quantities of output, not just a small # of test messages. In this second example. we see that we're trying to evolve to a common output format string for duplicative information (and we should converge on a standard).

'{0}' uses the 'rule' property to specify the violated rule, so it is not necessary also to specify 'ruleId' or 'ruleIndex'. This unnecessarily increases log file size. Remove the 'ruleId' and 'ruleIndex' properties. #Closed

[ghost]

I'm open to this but I ask you to consider the potential value of the descriptive prefixes. The problem is that the JSON paths can be very long, and without the prefix, the first thing that meets your eye is this long string. Consider an example where it's really long:

'/runs/0/results/0/locations/0/physicalLocation/artifactLocation/uri' contains an invalid URI 'fi:le:///C:/dev/'

I think this provides the reader with an easier entry to the message:

The URI 'fi:le:///C:/dev/' at '/runs/0/results/0/locations/0/physicalLocation/artifactLocation/uri' is invalid.

... because it says "I'm about to tell you something about a URI". Without the prefix, you have to scan to the end of the path string to learn that it's about a URI.

---

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

[michaelcfanning]

Your example doesn't correspond to my feedback, which notes that describing the SARIF JSON type is generally not required because the type information tends to be encoded in the JSON path. The term 'URI' isn't typically encoded in the URI and so it is useful to identify the string literal as one.

---

In reply to: 455936290 [](ancestors = 455936290,455907068)

[ghost]

Ok, I understand the distinction.

---

In reply to: 455941928 [](ancestors = 455941928,455936290,455907068)

[eddynaka]

Just updated

---

In reply to: 455982124 [](ancestors = 455982124,455941928,455936290,455907068)

[michaelcfanning]

The 'rules' array [](start = 0, length = 17)

I'd like to see actual instances of this rule output before approving. Can you provide some from your testing? Note that you have some language here 'Removing this array might reduce log file size' that should converge on a standard message. Also, why do we have the 'might'? This analysis s/be very clear whether our suggestion can be taken without implying information loss. If not, let's keep working on the analysis. #Resolved

[ghost]

I've just sent you the output from running the validator on a SARIF file produced from scanning a significant repo.

---

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