VIolation of SARIF specification par. 3.27.12.

[vivaat]

Summary

github/codeql-action/upload-sarif violates SARIF specification par. 3.27.12.

Details

According to SARIF specification par. 3.27.12, a result object SHOULD contain a property named locations whose value is an array of zero or more location objects. At the same time, starting github/codeql-action/upload-sarif@v3 action on a sarif report that contain result object with empty locations property fails with the error Code Scanning could not process the submitted SARIF file: locationFromSarifResult: expected at least one location.

PoC

Having a report with result object with empty list locations property.

{
  "version": "2.1.0",
  "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
  "runs": [
    {
      "tool" : {
        "driver": {
          "name": "PVS-Studio",
          "semanticVersion": "7.32.0.1",
          "informationUri": "https://pvs-studio.com",
          "rules": [
            {
              "id": "V010",
              "name": "RuleV010",
              "help": {
                "text": "https://pvs-studio.com/en/docs/warnings/v010/"
              },
              "helpUri": "https://pvs-studio.com/en/docs/warnings/v010/"
            }
          ],
          "results": [
            {
              "ruleId": "V010",
              "message": {
                "text": "Analysis of 'Utility' type projects is not supported in this tool. Use direct analyzer integration or compiler monitoring instead."
              },
              "level": "error",
              "locations": []
            }
          ]
        }
      }
    }
  ]
}

[cannist]

GitHub code scanning indeed only supports a subset of the SARIF standard. In particular a location is needed for many features that are core to the experience. For example for showing alert annotations directly in pull requests but also for features that are less obvious like the ability to determine whether an alert (result in SARIF terms) found in two different commits is the same or whether they are two different instances of the same rule.

While we do not support doing anything with results that lack a location we could silently ignore them instead of rejecting the whole sarif file with an error message. However, we think that that would probably be more confusing ("Why do I not see the uploaded results?") than the current behavior. A direct error makes it easier for SARIF producers that are working on integrating with code scanning to find out if we can actually do anything with what they give us.

We have some documentation around our SARIF support here: SARIF support for code scanning. There is also a Microsoft SARIF validator that lets you enable the GitHub ingestion rules and check if we would accept a SARIF file.

[cannist]

As a side note: The example SARIF file you have given looks like it is trying to report an analysis failure in a result. If the tool invocation failed it would be better to report that in an invocation object in runs.invocations by setting executionSuccessful to false. GitHub will then highlight the problem to the user on the tool status page and show the information from the exitCode and exitCodeDescription properties.

[vivaat]

@cannist Hey! Thank you for the answer and the explanation. It seems like silently suppressing the error message may indeed lead to unexpected unpleasantries (as it usually does =) ).

Using an invocation object will not work in all scenarios, unfortunately. This empty locations feature seems to be really handy in our case. Is there any chance this behavior is going to change in future?

[cannist]

I'm sorry, we have no plans for changing this at the moment, as far as I am aware. This could change if we observe customer demand but I'd be surprised. 🤷

@alonahlobina, as one of our product managers you would know better if there were considerations around supporting alerts without locations from the product side. Can you say something about this?

[vivaat]

@cannist Could you advise if you support some other way to display this kind of informational message? invocations is not really an option for a subset of this kind of messages. Empty locations would be really handy for this purpose, but maybe there is some other way that I am missing?

[cannist]

I don't think there is an officially supported way, unfortunately. Currently the exitCodeDescription I mentioned is the only way. But I can see that it would not be an option for certain kinds of messages. Just putting a made-up location into a result will probably also get it displayed as alert but essentially these kind of messages are not alerts and the experience might be confusing to users.

Conceptually, from the SARIF standard, I think these kind of messages should be put into toolExecutionNotifications (spec). We use a very limited part of that internally but the implementation is not ready to be used in general and I don't think it is on the near roadmap. 🙁