-
Notifications
You must be signed in to change notification settings - Fork 429
Description
I've just discovered that if github/codeql-action/upload-sarif@v1 fails to get the git commit (relates to #58), it doesn't raise an error.
This is very bad for security scanning tools like static code analysis, because it likely means that nothing was scanned but gives a false positive success status for the workflow.
Output:
Run github/codeql-action/upload-sarif@v1
fatal: not a git repository (or any of the parent directories): .git
Failed to call git to get current commit. Continuing with data from environment or input: Error: The process '/usr/bin/git' failed with exit code 128
Error: The process '/usr/bin/git' failed with exit code 128
at ExecState._setResult (/home/runner/work/_actions/github/codeql-action/v1/node_modules/@actions/exec/lib/toolrunner.js:592:25)
at ExecState.CheckComplete (/home/runner/work/_actions/github/codeql-action/v1/node_modules/@actions/exec/lib/toolrunner.js:575:18)
at ChildProcess.<anonymous> (/home/runner/work/_actions/github/codeql-action/v1/node_modules/@actions/exec/lib/toolrunner.js:469:27)
at ChildProcess.emit (events.js:[21](https://github.com/HariSekhon/Templates/runs/5267697089?check_suite_focus=true#step:6:21)0:5)
at maybeClose (internal/child_process.js:1021:16)
at Socket.<anonymous> (internal/child_process.js:4[30](https://github.com/HariSekhon/Templates/runs/5267697089?check_suite_focus=true#step:6:30):11)
at Socket.emit (events.js:210:5)
at Pipe.<anonymous> (net.js:659:12)
Uploading results
Processing sarif files: ["results.sarif"]
Uploading results
Successfully uploaded results
Here is an example public workflow of mine showing the problem:
https://github.com/HariSekhon/Templates/runs/5267697089?check_suite_focus=true
I've corrected the error in my workflow so newer versions of this workflow do work (I missed the checkout step when porting to a reusable workflow for some reason), but the point is that the github/codeql-action/upload-sarif@v1 action should fail when it detects that it is not in a git repo unless an optional flag is given to permit this condition as it will often be a serious error in the workflow, causing nothing to be scanned.
Any error in determining the git commit should result in an error code being propagated upwards and failing the entire workflow for safety to detect when you are not receiving security alerts due to not being in a checkout.
Update: technically an empty SARIF file is probably being uploaded after a scan of an empty non-checkout dir - some tools may pull docker images and in those cases it's fine to not spend time checking out the repo - but the default behaviour should be to error out on any error such as not being in a git checkout for safety, unless passing a flag to indicate that was intended.