codeql-action icon indicating copy to clipboard operation
codeql-action copied to clipboard

Fix `upload-sarif` potentially initialising CodeQL twice

Open mbg opened this issue 4 months ago • 2 comments

The upload-sarif action can be used in workflows without an init step. This means that it is possible for the upload-sarif action to be used correctly even though the CodeQL CLI has not yet been initialised. However, if we are uploading multiple SARIF files to a given endpoint, then these are first combined using the CodeQL CLI. To allow this, we currently initialise the CodeQL CLI on the fly in combineSarifFilesUsingCLI if needed for this.

This was fine until #2935 where we started uploading two separate SARIF files and combineSarifFilesUsingCLI could appear more than once in the call path. Since the CodeQL instance was shared, this could result in the CLI being initialised twice.

This PR changes the upload-sarif action to initialise the CLI if needed earlier on in the flow and then propagates the instance down the call path so that it can be shared between multiple calls to combineSarifFilesUsingCLI.

Merge / deployment checklist

  • [ ] Confirm this change is backwards compatible with existing workflows.
  • [ ] Confirm the readme has been updated if necessary.
  • [ ] Confirm the changelog has been updated if necessary.

mbg avatar Aug 06 '25 14:08 mbg

@henrymercer I rebased to resolve merge conflicts and added https://github.com/github/codeql-action/pull/3006/commits/483613152e5f7147dbb891be42c168136269ccc2 since the init logic in uploadSpecifiedFiles shouldn't be needed anymore. It would probably be nicer if we split up that function into a variant that requires the CodeQL instance and uploads multiple files and one that doesn't and uploads a single file, but I am not sure it's worth the effort. What do you think?

mbg avatar Aug 07 '25 11:08 mbg

Not a bad idea. It would probably mean throwing most of the changes in this PR out, but it would be a simpler overall change and not require us to split up uploadSpecifiedFiles or deal with the current awkwardness.

mbg avatar Aug 07 '25 11:08 mbg