Fix #1138: Add validation rule: contextRegion requires region

[ghost]

Also:

• Enhance the "new-style" verification so that we no longer require the file "Invalid_Expected.sarif". Each file can now contain a property that specifies the expected locations of all the validation results.

[ghost]

@michaelcfanning is added to the review. #Closed

[ghost]

@cfaucon is added to the review. #Closed

[ghost]

@rtaket is added to the review. #Closed

[ghost]

@EasyRhinoMSFT is added to the review. #Closed

[ghost]

@vScottLouvau is added to the review. #Closed

[michaelcfanning]

physicalLocation.ContextRegion [](start = 16, length = 30)

What if someone provides an entirely empty region? A region has no required items. What if someone populated the context region with line/other information, but the Region object only includes a 'Message' string? Etc.

[ghost]

The schema doesn't declare any required properties, but a region object with no properties is invalid, because:

• §3.25.5: When a region object represents a text region specified by line/column properties, it SHALL contain a property named startLine...
• §3.25.9: When a region object represents a text region specified by offset/length properties, it SHALL contain a property named charOffset...
• §3.25.11: When a region object represents a binary region, it SHALL contain a property named byteOffset...

I filed #1143: "Add validation rule: at least one of the region "start" properties must be present."

---

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

[michaelcfanning]

You should satisfy that work item by updating the schema itself, rather than authoring a validator rule. An 'anyOf' rule should cover this, e.g.:

"anyOf": [
{"required" : ["startLine"]},
{"required" : ["charOffset"]},
{"required" : ["byteOffset"]}
]

IOW, we should favor schema-driven solutions wherever possible. Of course, this may prompt us to author improved validation within jschema itself...

---

In reply to: 238504114 [](ancestors = 238504114,238499344)

[ghost]

I 100% agree that we should favor schema-driven solutions.

• Remember that my code generator doesn't know how to deal with "anyOf" (we know we have to extend it for the case where one of the alternatives is "null", but this here is a different case).
• What you're suggesting doesn't seem to work, even though I agree it's plausible -- or, at least, VS doesn't interpret it correctly.

I'll send you a couple of sample files to show you what I mean.

---

In reply to: 238504940 [](ancestors = 238504940,238504114,238499344)

[ghost]

In any case, I'm going to merge this now.

---

In reply to: 238510661 [](ancestors = 238510661,238504940,238504114,238499344)

[michaelcfanning]

properties [](start = 7, length = 10)

Ok. I still have concerns about the fragility of this mechanism (for example, the need to change the index). The implied path changes seem trivial to update, however. And I get that you are providing data that points to to primitive values. I have had issues with this sort of approach in the past where I have attempted to constrain the reporting to a single check. Sometimes it is impossible (and perhaps not even desirable) to limit things in this way. In which chase, it is helpful to be able to comprehensively describe the rule ids that are fired for certain patterns.

[michaelcfanning]

detectedResultLocations [](start = 21, length = 23)

ok, my other comment notwithstanding, I agree with your approach, simple and clean. we'll see where things go moving forward. i suspect your big challenge will be trying to define approach granularity for analysis. to date, the checks have been a bit narrow. it might be that we should organize checks either by the primitives that are examined in a comprehensive way ('DoNotScrewUpEmittingUriValuesPlease') as well as, where appropriate, by SARIF object type 'EmitContextRegionsProperly'). This default organizational scheme is likely to carry us a fair distance. Note as well that this approach will help us (in the case of SARIF object types) in that we can type the output/other help very closely to the spec section that documents the type.

[ghost]

Yes, I suspected you'd agree that this approach is a fair compromise between simplicity, ease of update, and expressive power. Thanks for looking.

[michaelcfanning]

SARIF1016 [](start = 59, length = 9)

I have a funky idea for you which is to at least consider the possibility that the relevant SARIF spec section comprise the rule id. So SARIF.3.24.5 for this case (the analysis that addresses subtleties in the physicalLocation.contextRegion object. NOW: of course, the spec is malleable at this point in time. so this may simply be a lousy idea. Or, we might proceed with writing validators until the spec is finalized, then do a scrub to convert to final section numbers. Don't forget, we have the hierarchical string to allow some flexibility as well in creating sub-checks that roll up into a master rule id. SARIF.3.24/5, for example, would point to a master rule, dedicated to validating the physicalLocation object, the sub-rule of which validates the plc.contextRegion is handled properly. Something to ponder.

[ghost]

I worry about those ids breaking in the next version of the spec. Even an editorial section reordering would break them, let alone new content ("3.25 metrics object").

---

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

[michaelcfanning]