✨ Add dangerous workflow check with untrusted code checkout pattern

[asraa]

Signed-off-by: Asra Ali [email protected]

• Please check if the PR fulfills these requirements

• [ x] Tests for the changes have been added (for bug fixes / features)
• [ x] PR title follows the guidelines defined in https://github.com/ossf/scorecard/blob/main/CONTRIBUTING.md#pr-process

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  Adds a new check Dangerous-Workflow that checks if a github workflow uses dangerous coding patterns.
  The first pattern implemented is a check for an untrusted code checkout.

This is described in #426

• What is the current behavior? (You can also link to an open issue here)

• What is the new behavior (if this is a feature change)?

• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  No

• Other information:

[azeemshaikh38]

LGTM. Thanks @asraa !

[naveensrinivasan]

Thanks!

[laurentsimon]

another great PR, thanks @asraa !

[laurentsimon]

@olivekl comments?

[olivekl]

A couple suggestions:

"This check determines whether the project has dangerous code patterns. ADD: Some examples of these patterns are [X] and [Y]."

"The first check is misuse of..." Why the reference to first? Is there a second part to the check that I'm not seeing?

Related: is this check looking for other dangerous patterns aside from the pull_request_target trigger used with an explicit pull request checkout? If not, would this test be better named as something specific to this particular pattern, if it's the only one being detected?

Nit: are we using "PR" or trying to stick to the more platform agnostic "merge request"? Or is this test only applicable to GitHub (in which case, let's add that)?

[asraa]

Yay thanks!!

"This check determines whether the project has dangerous code patterns. ADD: Some examples of these patterns are [X] and [Y]."

added

Is there a second part to the check that I'm not seeing?

Yep, I've only added one so far but the plan is to add a series of checks (#426 (comment)). I tried to rephrase this with The first code pattern checked. So I think it makes sense to keep this more general.

Or is this test only applicable to GitHub (in which case, let's add that)?

The check will have a lot of GitHub Actions dependent style and relies on it's particular default behavior, so I added this clarification Determines if the project's GitHub Action workflows avoid dangerous patterns.

[azeemshaikh38]

Given recent discussion about Scorecard data change affecting clients, let's disable this check from running by default in CLI and cron job for now. If clients specifically want to invoke this check, that's ok.

[laurentsimon]

Given recent discussion about Scorecard data change affecting clients, let's disable this check from running by default in CLI and cron job for now. If clients specifically want to invoke this check, that's ok.

@asraa use an env variable to guard the access to this check. You can update the code in cmd/root.go, function getEnabledChecks()

[azeemshaikh38]

Given recent discussion about Scorecard data change affecting clients, let's disable this check from running by default in CLI and cron job for now. If clients specifically want to invoke this check, that's ok.

@asraa use an env variable to guard the access to this check. You can update the code in cmd/root.go, function getEnabledChecks()

Just delete(enabledchecks, checks.CheckDangerousWorkflow) here should be enough :)
Same for cron job here.

[laurentsimon]

Given recent discussion about Scorecard data change affecting clients, let's disable this check from running by default in CLI and cron job for now. If clients specifically want to invoke this check, that's ok.

@asraa use an env variable to guard the access to this check. You can update the code in cmd/root.go, function getEnabledChecks()

Just delete(enabledchecks, checks.CheckDangerousWorkflow) here should be enough :)

I want to be able to use and test the local repo interface with all checks (without providing a long list of --checks), so I'd prefer an env variable.

Same for cron job here.

[azeemshaikh38]

I want to be able to use and test the local repo interface with all checks (without providing a long list of --checks), so I'd prefer an env variable.

Same for cron job here.

If its only needed for testing, you can simply remove the line locally? ;)

[asraa]

I went with the ENV var solution, but only because I imagine future checks might just want to add to the list? And it would centralize where the code could be removed before the next cut but let me know if I should change that

[laurentsimon]

@asraa heads up. #1195 got merged. You're going to have to update the checks.yaml file and add a repos field with a list of interfaces that are supported by your check. See example checks.yaml#L78.

The doc generation will also validate that your check does not use APIs that are inconsistent with the repos field. If you find a problem with the validation, let me know. (Note: validation happens automatically as you re-generate the .md)

[asraa]

thanks for the review @chrismcgehee!

[asraa]

ping? anything else to do here?

[laurentsimon]

ping? anything else to do here?

license check is failing. apart from that, I think we're good to merge. If anyone disagrees, please reply.

[asraa]

oops, something funky with the merge -- one second will update

[olivekl]

ping? anything else to do here?

license check is failing. apart from that, I think we're good to merge. If anyone disagrees, please reply.

I'd just had a couple comments above on the documentation description. We could also address them after the merge, though.

[laurentsimon]

ping? anything else to do here?

license check is failing. apart from that, I think we're good to merge. If anyone disagrees, please reply.

I'd just had a couple comments above on the documentation description. We could also address them after the merge, though.

I think there's time to add your suggestions in this PR, since the pre-submit tests are still failing.

[asraa]

Yeah! Happy to address more comments -- merge issue fixed

[laurentsimon]

did you update the cron job

scorecard/cron/worker/main.go

Line 196 in 83649a7

delete(checksToRun, checks.CheckContributors)

?

[asraa]

did you update the cron job

Oops -- good catch, I had assumed that it would call the root command, but it doesn't. Updated

[olivekl]

Just a few comments!

[olivekl]

A couple suggestions:

"This check determines whether the project has dangerous code patterns. ADD: Some examples of these patterns are [X] and [Y]."

"The first check is misuse of..." Why the reference to first? Is there a second part to the check that I'm not seeing?

Related: is this check looking for other dangerous patterns aside from the pull_request_target trigger used with an explicit pull request checkout? If not, would this test be better named as something specific to this particular pattern, if it's the only one being detected?

Nit: are we using "PR" or trying to stick to the more platform agnostic "merge request"? Or is this test only applicable to GitHub (in which case, let's add that)?

[laurentsimon]

this check is only supported on GitHub. @olivekl do we need to use merge request instead of pull request in this sentence?

[olivekl]

Since it's only on GitHub I think pull request is fine.

[laurentsimon]

I fixed the pre-submit error

--- FAIL: TestSARIFOutput (0.00s)
    --- FAIL: TestSARIFOutput/check-8 (0.00s)
        sarif_test.go:800: check-8: invalid result: 1

in #1233
Once you push the error should go away

[asraa]

ping? anything else to do here?
(assuming I'll update this to use the github workflow parser next)

[laurentsimon]

ping? anything else to do here? (assuming I'll update this to use the github workflow parser next)

nop, go ahead update with workflow parser. Unless it's a trivial change, do it in a follow-up PR. Let me know which option you prefer.

[azeemshaikh38]

ping? anything else to do here? (assuming I'll update this to use the github workflow parser next)

nop, go ahead update with workflow parser. Unless it's a trivial change, do it in a follow-up PR. Let me know which option you prefer.

+1

[dota17]

It is recommended that the Readme of the project be updated synchronously. Currently, there are 17 check items, whereas the Readme has only 16 check items.
https://github.com/ossf/scorecard/blob/main/README.md

[naveensrinivasan]

Thanks, we would appreciate if you can do PR to fix the issue.