Skip to content

Do not fail upon SARIF 'toolExecutionNotifications' of level 'error'

Dinesh Bolkensteyn requested to merge nofail into main

What does this MR do?

This MR fixes a Semgrep analyzer failure reported by a customer/prospect in this Slack thread: https://gitlab.slack.com/archives/CLA54H7PY/p1669087664596579

Our Semgrep analyzer already tests for fatal errors based on exit codes: https://gitlab.com/gitlab-org/security-products/analyzers/semgrep/-/blob/43ec59b7aaaf347b62927b7f925b6965fffe75ed/analyze.go#L41

In the code above, we determined that exit code 2 is not fatal, so we proceed with converting the report. That however immediately fails due to the presence of an toolExecutionNotifications at level error.

Quoting SARIF specification Appendix I. (Informative) Detecting incomplete result sets:

  1. If any invocation object (§3.20) in theRun.invocations (§3.14.11) has a value of false for its executionSuccessful property (§3.20.14), the tool either failed to start, terminated with an exit code that denotes failure, or terminated with an unhandled exception or signal..
  2. If any notification object (§3.58) in invocation.toolExecutionNotifications (§3.20.21) or toolConfigurationNotifications (§3.20.22) has a value of "error" for its level property (§3.58.6), it is possible that the tool was unable to execute every analysis rule on every analysis target. Therefore, the results cannot be assumed to be complete.

This prospect/customer is interested in seeing partial results as only a single file within their project is causing the error: https://github.com/espressif/esp-idf/blob/release/v4.4/components/driver/gpio.c

The SARIF specification also state:

At a minimum, viewers SHOULD make users aware of tool notifications whose level property is "error".

This MR therefore logs such notifications instead of considering them as fatal.

Note that we currently do not test executionSuccessful, but could do so. This MR removes it from the SARIF structure as it is presently unused.

Unit tests covering this scenario all relate to Semgrep. I do not know how this change could impact other analyzers.

What are the relevant issue numbers?

Does this MR meet the acceptance criteria?

Edited by Lucas Charles

Merge request reports