Update workflows external pr

[fredvd]

No description provided.

[netlify]

✅ Deploy Preview for volto canceled.

Name	Link
🔨 Latest commit	90b97ab
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/6426eb25fb647a0007b89523

[cypress]

Passing run #4746 ↗︎

0

489

20

0

0

Details:

Merge branch 'master' into update_workflows_external_pr
Project: Volto	Commit: 90b97ab23f
Status: Passed	Duration: 14:21 💡
Started: Mar 31, 2023 2:19 PM	Ended: Mar 31, 2023 2:33 PM

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[sneridagh]

@fredvd tell me when this is ready, @stevepiercy maybe you can take a look too? Thanks!

[mauritsvanrees]

This is wrong. Do NOT merge.

Or the docs are wrong or I misinterpret them, that is also possible.

Read this: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

1. This is insecure, as it gives external forks write access to the target repository. I am not exactly sure how to exploit this though.
2. This gives the external authors access to the secrets.
3. But that does not matter, as it does not actually work. :-) Read the comments around the last example of the blog post, with text "The workflow is broken. DO NOT use it in production." The checkout action actually checks out the target repo, so the master branch of plone/volto. It does nothing with the code in the PR itself.

In other words, there are use cases for the pull_request_target workflow, but no use case should actually do anything with the code of the PR. It is only useful as one part of multiple workflows, where it does something before or after other workflows.

Let's take a step back. What are we trying to achieve here? My guess is at least to allow to create the docs, which likely needs access to secrets. Maybe something similar in acceptance tests. Is that the idea?

[fredvd]

@mauritsvanrees

Wath you might be missing, because it is configured on the repo settings, is when/if the workflows for those newly created external pull requests are executed. If they would be executed always uncondtionally it is indeed insecure.

But we're not.

The question is now: how much and how fast are you going to trust outside these fork pull request contributors? And who is going to check those contributions/changed files?

As we discussed: the workflow files are protected, because the workflow files from the target repo will be used, and not possibly pwned changed workflows in the pull request.

We could also decide to run on: pull_request and not pull_request_target. But then the workflow will not be able to push previews/deployments to netlify or other things that require the secrets. So how are you going to handle 'verifying' those pull requests.

Accepting contributions from developers we don't just fully trust is difficult to manage. And the tools github gives all have trade offs in security and or convenience or work required.

cc: @davisagli

[davisagli]

@fredvd In general I would feel more comfortable using the pull_request event and making sure the required workflows don't need secrets, instead of using the pull_request_target event. My understanding is that if a secret is provided to the workflow as an environment variable and the workflow steps are running code from the external PR branch, then the untrusted code could steal the secret. (In this case the executed steps depends not only on the workflow file which is protected when using pull_request_target, but also on the code in the branch that was checked out.)

Docs builds on Netlify are kind of a separate question. It uses a GitHub app, not a GitHub Actions workflow. Based on reading their docs I think that it can be configured to allow builds from external PRs, and I haven't thought of a reason why that is a security issue.

The main change I'm aware of that we need to avoid relying on secrets is to not send Cypress results to the Cypress dashboard. I will try later today to see if there is a way to do this conditionally.

[davisagli]

@fredvd I made a branch which adds the pull_request event as a trigger for the workflows: https://github.com/plone/volto/compare/workflow-pr-trigger?expand=1

And I am trying it out by submitting a pull request from my impostor davisagli2: #4628

It seems to be working well. The Netlify build succeeded and the Cypress builds are succeeding even though they don't have the secret to send results to the Cypress dashboard.

So, I think probably the only change we need is what I already have in the workflow-pr-trigger branch, and this would be my preference since it is safer.

[fredvd]

In general I would feel more comfortable using the pull_request event and making sure the required workflows don't need secrets, instead of using the pull_request_target event

yes that's also my understanding from letting this settle in for a week, gathering comments and multiple people reading the documentation and thinking through the consequences.

Also, we're doing this for a minority of all use cases and developer activity, when onboarding new devs. so it doesn't have to be perfect to the expense of security. security first. And larger change sets where collaboration is necessary, should be developed on a branch on the main repo anyway for best dx.