Feature request: add official support for `on: pull_request`

[pnacht]

As described in #109, the Scorecard Action already works experimentally on PRs. However, upgrading this to official support would significantly improve the Action's value proposition (see #1017), by turning it into a preemptive check rather than a reactive one (stopping PRs that reduce security before they land on main).

My understanding is that the Action is currently designed to always "pass" for PRs, and it only runs checks that look at code, to ensure that only the relevant scores are included (and no settings-based checks, for example).

This is already useful in that it is something maintainers can glance at for each PR to see the impact on the project's score. However, I believe it would be better if the Action "failed" a PR that reduced the repo's score. The workflow would likely not be registered as "required", so maintainers could still merge an "unsafe" PR if they believed it was worth it, but it would at least serve as a strong "look here" signal.

Looking at a random PR of a project that already uses the Action on PRs and the output of the Scorecards run, I am unclear whether results can be more easily parsed. Are PR results also sent to the Security Dashboard? Or must maintainers look at the SARIF file or Action logs?

None of these solutions seem very user-friendly: would it be possible to present these results in a simple table within the PR "environment" (without having to leave to the Security Dashboard), even if within the logs?

[laurentsimon]

On a PR, the difference between the dashboard results and the PR results will show automatically in the PR by GitHub. GitHub does this automatically when the SARIF file is uploaded. A PR results won't update the dashboard. Have you tried it to see whether it's user-friendly enough? We can explore tables, log presented as .md (+link in comments), comments on the PR, etc. if it's not good enough.

This is already useful in that it is something maintainers can glance at for each PR to see the impact on the project's score. However, I believe it would be better if the Action "failed" a PR that reduced the repo's score. The workflow would likely not be registered as "required", so maintainers could still merge an "unsafe" PR if they believed it was worth it, but it would at least serve as a strong "look here" signal.

Do-able, but maybe provide an option to let users configure it. Definitely worth thinking about. Thanks for the idea.

[pnacht]

I wasn't aware that PR results appear automatically in the PR's page. I haven't found an example where this happens (the example above didn't have one, maybe because it didn't change any of the scores). Do you know one I can look at?

[laurentsimon]

To tests, create a PR with a workflow that does not have permissions set at the top. I don't remember where it shows, and I have a feeling that you're correct and that it does not show in a place that's obvious to developers :/

[laurentsimon]

We could use the annotation / check APIs to provide inline results in the PR https://docs.github.com/en/rest/checks#update-a-check-run--parameters

[laurentsimon]

SARIF builtin feature seems to show the diff using the annotation API. See example in https://github.com/ossf/scorecard/pull/2485 -> https://github.com/ossf/scorecard/actions/runs/3561057797/jobs/5981621304 As a maintainer, I see a button to dismiss / ignore the alter: do you see it too? (I'm hoping you don't)

[pnacht]

SARIF builtin feature seems to show the diff using the annotation API. See example in https://github.com/ossf/scorecard/pull/2485/files As a maintainer, I see a button to dismiss / ignore the alter: do you see it too? (I'm hoping you don't)

Oh, that's really cool. Yeah, that's an excellent solution. And I see the check already shows itself as having failed, too, which should let maintainers know they have something to look into.

And no, I don't see anything to dismiss/ignore the change, which is also very good. What happens if you dismiss it? Does the check's status change to green as well? Because if that's the case, then the check could even be registered as "Required" (preferably at the maintainer's choosing), so that maintainers have to manually go into the code and approve any "bad" changes.

[laurentsimon]

I have not tried clicking "dismiss" to see if the check passes (Need to try on another repo). If I do, it will also ignore this results in the next PRs and in the daily scorecard runs.

[pnacht]

Ah, that's a bummer, far less interesting in that case. I thought it'd be a dismiss just for the PR, but it'd still appear in the Security Dashboard, basically a way of saying "this is okay for now, we can fix that issue later." (And then just dismiss in the Security Dashboard as well if they're fine with the status quo)

Still worth doing, but maintainers have to be made aware that clicking "Dismiss" means they'll never see that warning again.

[laurentsimon]

yep, this is a GitHub limitation. We did ask them in the past to update this behavior the way you suggest.

[laurentsimon]

Something to test before releasing this feature is on first installation of Scorecard - via a PR. Since there are no existing SARIF results to compare against, we need to be sure the user is not overwhelmed with alert annotations on the PR.

[evverx]

Given that scorecard has no idea how to, say, convert versions into hashes and shows promotional links instead I'm not sure how running it on PRs like https://github.com/systemd/systemd/pull/27071 can help to actually fix stuff. In that particular case it would have just complained about the unpinned dependencies and the permissions and I don't think it's particularly helpful. Those promotional links aren't very convenient either because stuff should be copy-pasted back and forth. Anyway, I think until scorecard can actually show how to fix issues it isn't exactly useful on PRs because it just complains without showing how to fix anything right in PRs.

Since there are no existing SARIF results to compare against, we need to be sure the user is not overwhelmed with alert annotations on the PR

I wouldn't worry about that. GitHub introduced a feature that should address that: https://github.com/redhat-plumbers-in-action/differential-shellcheck/issues/215.

[evverx]

By the way that new systemd action doesn't really work because all the actions have read-only access by default and it would be really cool if scorecard could detect that. That setting isn't reachable via the GitHub API though as far as I can remember.

[evverx]

One last thing. Looking at, for example, https://github.com/systemd/systemd/pull/26928#issuecomment-1488300416 I don't think that generally scorecard is taken seriously due to its false positives and it isn't clear why would anyone want to run it on PRs to get "high severity" alerts like that on PRs too. It would be great if scorecard could start fixing stuff like that. Those issues have been known for more than a year and I'm not sure why they are ignored.

[evverx]

I wonder if there are any updates on this? I'm not sure what the point of the scorecard action is given that it can't help with reviewing PRs like https://github.com/systemd/systemd/pull/27071.

(it's related to https://github.com/systemd/systemd/pull/27530)

[evverx]

Here's an in-flight PR where scorecard should have reviewed its supply-chain stuff automatically: https://github.com/systemd/systemd/pull/27501.

Maintainers and contributors shouldn't spend their time on that with the scorecard action enabled.

Anyway I hope those examples should help to make it clear that in its current form the action isn't exactly useful.

[diogoteles08]

Closely related to #3204

[evverx]

Looking at https://github.com/marketplace/actions/openssf-scorecard-monitor I'm not sure it can be used to compare diffs and emit SARIF to analyze PRs. As far as I can tell it can be run on push/cron events but that's not what I'm looking for. Other than that I wouldn't even consider it because it still includes promotional links.