tomasbjerre / violations-lib

Java library for parsing report files from static code analysis.
Apache License 2.0
148 stars 39 forks source link

SARIF 2.1.0 toolExecutionNotifications, toolConfigurationNotifications #154

Closed KalleOlaviNiemitalo closed 2 years ago

KalleOlaviNiemitalo commented 2 years ago

I'd like SarifParser to convert also the notification objects (SARIF-v2.1.0 §3.58) in the invocation.toolExecutionNotifications property (SARIF-v2.1.0 §3.20.21) and the invocation.toolConfigurationNotifications property (SARIF-v2.1.0 §3.20.22). Currently, it converts only the result objects (SARIF-v2.1.0 §3.27) in the run.results property (SARIF-v2.1.0 §3.14.23) and ignores the notification objects.

This would be particularly useful for invocation.toolConfigurationNotifications caused by an incorrect setting in a configuration file, as the Warnings Next Generation plugin in Jenkins would then be able to to display the configuration file and pinpoint the error, if the SARIF log contains this information.

SarifParser currently reads the following properties of result objects: ruleId, message, level, and locations. Almost all of these correspond to identically-named properties in notification objects. However, the result.ruleId property has no direct equivalent. Instead, SarifParser would have to read a reportingDescriptorReference object from the notification.descriptor property and look up the rule based on that. After this has been implemented, it would be easy to add support for the result.rule property as well.

Sample SARIF 2.1.0 log with toolConfigurationNotifications ```JSON { "$schema": "https://json.schemastore.org/sarif-2.1.0.json", "version": "2.1.0", "runs": [ { "tool": { "driver": { "name": "demo", "version": "1.0.42", "rules": [], "notifications": [ { "id": "DE0019", "name": "CannotCopyFromNonFileUri", "defaultConfiguration": { "level": "warning" } } ] } }, "invocations": [ { "toolConfigurationNotifications": [ { "descriptor": { "index": 0 }, "locations": [ { "physicalLocation": { "artifactLocation": { "uri": "config.xml", "uriBaseId": "CWD" }, "region": { "startLine": 290, "startColumn": 17 } } }, { "physicalLocation": { "artifactLocation": { "uri": "http://example.org/image.png" } } } ], "message": { "text": "Cannot copy from non-file URI: http://example.org/image.png" }, "level": "warning", "timeUtc": "2022-07-18T17:54:13.412202Z" } ], "executionSuccessful": true } ], "originalUriBaseIds": { "CWD": { "uri": "file:///home/demo/" } }, "specialLocations": { "displayBase": { "uri": "", "uriBaseId": "CWD" } }, "results": [], "columnKind": "utf16CodeUnits" } ] } ```

_edit: in toolExecutionNotifications and toolConfigurationNotifications, the notification.descriptor property refers to toolComponent.notifications rather than toolComponent.rules, according to SARIF-v2.1.0 §3.52.3_

tomasbjerre commented 2 years ago

Putting this on my todo.

tomasbjerre commented 2 years ago

I am releasing this now. Open issue again if any problems!

KalleOlaviNiemitalo commented 2 years ago

In SARIF-v2.1.0, §3.52.3 (reportingDescriptor lookup) references §3.54.2 (toolComponent lookup). It seems you did not implement that part, and parseNotifications considers only the reporting descriptors of the driver, rather than any other tool components. OK, I'll file a new issue if I come across a SARIF log that requires the toolComponent lookup.

From §3.52.1 (General) and §3.52.4 (id property), it seems to me that SARIF consumers should not use the result.ruleId or reportingDescriptorReference.id property for locating the reportingDescriptor to which a result object refers; they should instead use result.ruleIndex or reportingDescriptorReference.index. §3.52.5 (index property) has an example in which the toolComponent.rules array contains two reportingDescriptor objects with the same reportingDescriptor.id value. As of tag 1.152.0, the parseResults method still uses result.ruleId for this purpose and does not consider that result.ruleId may end with a hierarchical component that is not in any reportingDescriptor.id. It would be good to fix that, but the flaw applies only to result objects and so is out of scope for this issue.

tomasbjerre commented 2 years ago

I think I have this fixed now. I tried to implement the reportingDescriptor lookup, but without example reports it is likely to contain bugs.

Should be no problem adding things from the toolComponent now, just open an issue with what you need.