github / codeql-action

Actions for running CodeQL analysis
MIT License
1.13k stars 313 forks source link

Source root for conversion from absolute to relative with `invocation.workingDirectory.uri` does not work #2460

Open vivaat opened 1 week ago

vivaat commented 1 week ago

Summary

Specifying 'workingDirectory' in 'invocation' property does not convert absolute paths in result object.

Details

After specifying the option, the paths remain absolute and the preview to the file is thus absent. изображение

PoC

In the below example, invocation property has no effect.

{
  "version": "2.1.0",
  "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
  "runs": [
    {
      "conversion": {
        "tool": {
          "driver": {
            "name": "PVS-Studio"
          }
        },
        "invocation": {
          "workingDirectory" : {
            "uri": "file:///C%3A"
          },
          "executionSuccessful": true
        }
      },

      "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": [
                {
                  "physicalLocation": {
                    "artifactLocation": {
                      "uri": "file:///C%3A/include/geometry/BoundingBox.h"
                    },
                    "region": {
                      "startLine": 1,
                      "endLine": 1
                    }
                  }
                }
              ]
            }
          ]
        }
      }
    }
  ]
}

There is another way to specify it. Doesn't work either.

{
  "version": "2.1.0",
  "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
  "runs": [
    {
     "invocations" : [
        {
          "workingDirectory" : {
            "uri": "file:///C%3A"
          },
          "executionSuccessful": true
        }
      ],

      "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": [
                {
                  "physicalLocation": {
                    "artifactLocation": {
                      "uri": "file:///C%3A/include/geometry/BoundingBox.h"
                    },
                    "region": {
                      "startLine": 1,
                      "endLine": 1
                    }
                  }
                }
              ]
            }
          ]
        }
      }
    }
  ]
}
hvitved commented 1 week ago

@shaikhul I believe this is one for your team?

cannist commented 1 week ago

I can confirm this is a bug. The sarif example given in the OP does not reproduce it because it contains no alerts (the results are in the wrong place) but this one does:

{
    "version": "2.1.0",
    "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
    "runs": [
        {
            "conversion": {
                "tool": {
                    "driver": {
                        "name": "PVS-Studio"
                    }
                },
                "invocation": {
                    "workingDirectory": {
                        "uri": "file:///C%3A"
                    },
                    "executionSuccessful": true
                }
            },
            "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": [
                        {
                            "physicalLocation": {
                                "artifactLocation": {
                                    "uri": "file:///C%3A/include/geometry/BoundingBox.h"
                                },
                                "region": {
                                    "startLine": 1,
                                    "endLine": 1
                                }
                            }
                        }
                    ]
                }
            ]
        }
    ]
}

I'll look more into it.

cannist commented 1 week ago

On further investigation, it turned out that there was another problem with the input that I had missed. It does not look like there is a bug on our side.

The invocation object in the above sarif file is contained in runs.conversion. The working directory of a sarif converter has no relation to result locations. To specify the working directory for the alert producer the location object needs to be put into runs.invocations, more specifically we are looking at the first element in the list.

This sarif file works as expected:

{
    "version": "2.1.0",
    "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
    "runs": [
        {
            "invocations": [
                {
                    "workingDirectory": {
                        "uri": "file:///C%3A"
                    },
                    "executionSuccessful": true
                }
            ],
            "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": [
                        {
                            "physicalLocation": {
                                "artifactLocation": {
                                    "uri": "file:///C%3A/include/geometry/BoundingBox.h"
                                },
                                "region": {
                                    "startLine": 1,
                                    "endLine": 1
                                }
                            }
                        }
                    ]
                }
            ]
        }
    ]
}

So, while there is no bug on our side I think this identifies a flaw in our documentation here which just says to use the "invocation.workingDirectory.uri property in the SARIF file" without saying where the invocation object needs to be located. I'll create an issue with the docs team to get this improved.

vivaat commented 1 week ago

Hey @cannist!

Please note that I provided two variations of the sarif report: one with conversion object and one with invocations list. Both of them does not work in my case.

After running exactly the file that you've attached, the preview still does not appear: изображение

cannist commented 1 week ago

Hmm, weird. I admit I had not tested end-to-end and only observed that the filepath was getting correctly relativized in the db in local dev.

Are you sure you're not still looking at the page for the old alert? The alert from the new sarif upload would now be considered a new one, since it has a different location. What repo are you testing in?

vivaat commented 1 week ago

Yes, 100% positive. Changing uri of the result to relative path shows the preview. Please check the example here -- https://github.com/vivaat/covid-sim/blob/master/out.sarif.

cannist commented 1 week ago

Ok... I have a test that reproduces it and I know why it is happening and why I could initially not reproduce it in local dev.

There are two ways to specify the source root: in the sarif file or via API parameter. Our server code currently prefers the API one over the sarif one. The upload-sarif action seems to always set the parameter to the checkout directory on the worker if it is not explicitly set as input to the action.

We'll have to fix this but there are multiple solutions and all of them have the potential to affect existing setups. I'll have to do some data gathering before I make any changes, possibly implement some metrics on Monday and have them collect data for at least a week.

vivaat commented 1 week ago

Sounds wonderful, thank you! Please let me know if I can help somehow.