Scorecards.yml: do not upload the result in the security dashboard if it is a pull request

[joycebrum]

When the action is configured to run in pull request, although it do not try to publish results (since 2.0.4), it tries to upload the results to Github's code scanning dashboard and returns an error (https://github.com/systemd/systemd/actions/runs/3276042271/jobs/5391618343).

I've manually disable it using the following configuration:

 # Upload the results to GitHub's code scanning dashboard.
      - name: "Upload to code-scanning"
        if: ${{ github.event_name != 'pull_request' }}
        uses: github/codeql-action/upload-sarif@5f532563584d71fdef14ee64d17bafb34f751ce5 # tag=v1.0.26
        with:
          sarif_file: results.sarif

I think the best solution is to avoid uploading the results to the security dashboard if it is a pull request, what do you think? I can suggest the PR with the changes if so.

[Phantsure]

This feels more of an issue with codeql action
I found two issues which are related to this in codeql action:
github/codeql-action#390
github/codeql-action#1061 (comment)

[Phantsure]

Does this problem also occurs with v2? That template has older version

[joycebrum]

I've tested using codeql-action 2.1.31 and I've got the same error https://github.com/joycebrum/SQLGame/actions/runs/3489036386/jobs/5838617362

Seems to be really related to the issues but it does not seemed solved.

[Phantsure]

Yeah not solved yet. I feel adding an optional tigger on condition and having an optional condition to run when triggered in pull request does seem correct. And this would be failing for all the code-scanning template.

[Phantsure]

Does "results.sarif" not exist for that case?

[joycebrum]

Actually we don't have a published sarif in scorecard action (see https://github.com/ossf/scorecard-action/pull/935/files) when running on pull request, because pull_request do not have the necessary token-id: write permissions.

[Phantsure]

I guess pull_request_target should get through

[joycebrum]

I've testes using pull_request_target instead of pull_request and, for some reason, the scorecard does not run. I think that's because pull_request_target runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does.

[Phantsure]

Yes, for pull_request_target to work it should be already present in base branch while raising pull request. I guessed it would work as sometimes token permission differs with ref when specifying no explicit permissions.

[laurentsimon]

pull_request_target should not be used, because it would expose PAT tokens, if users have configured one.

[Phantsure]

Actually we don't have a published sarif in scorecard action (see https://github.com/ossf/scorecard-action/pull/935/files) when running on pull request, because pull_request do not have the necessary token-id: write permissions.

I feel it is by design of scorecard action. Right @laurentsimon?
I don't see an action item on us here if this is a design choice on scorecard.

[joycebrum]

@Phantsure I think the only action item we could do here is the one I suggested in the PR (#1821) to avoid the upload if it is a PR, which seemed to be enough to fix the run on the test cases I've tested (like systemd)

[laurentsimon]

@joycebrum Can you file an issue on scorecard-action? It seems like in your run, the sarif file only contains:

   "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
   "version": "2.1.0",
   "runs": []
}

In a test PR I sent https://github.com/ossf/scorecard/actions/runs/3561057797/jobs/5981621304, you can see the SARIF contained information.

Maybe that is where the problem comes from, given that empty SARIF files seem to trigger github/codeql-action#390

Enabling SARIF on pull requests lets users see which lines are changing the scorecard results, so it'd be nice to keep it.

[joycebrum]

Sure, I'll do it.

[Phantsure]

@Phantsure I think the only action item we could do here is the one I suggested in the PR (#1821) to avoid the upload if it is a PR, which seemed to be enough to fix the run on the test cases I've tested (like systemd)

@laurentsimon Do you also want this change in starter workflow or want to keep track in new issue here created by @joycebrum ?

[laurentsimon]

Let's close this issue and find out what's going on in scorecard. This may be a bug there. We'll re-open here if needed. @joycebrum ok with this?

[joycebrum]

Sure, it makes sense. Thanks for the support @Phantsure and @laurentsimon