Github actions: browser tests fail for PR's raised from forked repos

[juergba]

Description

When a PR is created/synchronized from a forked repo, a pull_request event is emitted.

In order to protect public repositories for malicious users we run all pull request workflows raised from forks
with a read-only token and no access to secrets.

Unfortunately we don't have access to secrets and therefore the login to Sauce Labs fails.

There is a new event pull_request_target which gives access to secrets, but also grants a read/write token for this repo. Which is bad and opens door for unfriendly people.

[boneskull]

there has to be some sort of way around this. I don't really want to find out that the browsers break after merge

[boneskull]

cc @christian-bromann; I think this is the problem I mentioned some time ago. do you know of a workaround?

[christian-bromann]

@boneskull forks have to set their own credentials as it doesn't seem to be possible yet to make projects secrets available for them (found a thread here). One way to workaround this would be to have a sub-account that is allowed to run 1 or 2 concurrent sessions and hard code it into the setup so that forks can use these. So check if sauce credentials are available, if not (so we have a PR from a fork) use the credentials of the sub account. I guess you won't get that much PRs from forks and I doubt anyone would make the effort to abuse this.

[christian-bromann]

@boneskull @juergba I think pull_request_target does the trick.

[outsideris]

Sub-account feature is not available under our sauce labs org. It is an enterprise plan only. I think we are not.

[outsideris]

I will test pull_reqquest_target more. I didn't fully understand it.

[outsideris]

See Keeping your GitHub Actions and workflows secure: Preventing pwn requests

You may ask yourself: if the pull_request_target workflow only checks out and builds the PR, i.e. runs untrusted code but doesn’t reference any secrets, is it still vulnerable?

From my understanding for pull_request_target, there is no automatic safe way to run browser tests.

1. We just run browser tests by pull_request_target trigger after other tests run successfully. They can access our secrets even from forked PRs, but malicious attackers can access our secrets as well.
2. After maintainers reviewed the code changed from forked PR, maintainers add a label(e.g 'run-browser-test') on the forked PRs. So, GH actions will be triggered with permission to access secrets like below. This is safer than 1. But we should add a label manually and it is prone to human error.

on:
  pull_request_target:
  types: [labeled]

If we make a sub-account of Sauce Labs, we can run no. 1 more safely. But we couldn't.

Any thought? @mochajs/core

[juergba]

From my understanding for pull_request_target, there is no automatic safe way to run browser tests.: I agree

There is a manual workflow_dispatch event, I'm not sure wether it has access to secrets. It should, since it's triggered only by maintainers on Github. We could:

• skip browser tests for PR from forked repos, they fail/get cancelled anyway.
• add a separate GH actions for browser tests. Triggered manually by maintainers when needed, eg. before merging into master branch.

[outsideris]

If we will trigger browser tests for forked PRs, using a labeled event (e.g run-browser-test) is better than workflow_dispatch because it looks more convenient for us.
Maintainers only can attach labels to PRs.

I will test it.

[juergba]

If the label run-browser-test is set once on the forked PR, the user can push more commits containing whatever code. The tests will run without maintainers' control and the situation is unsafe again. It's not only the browser test, all tests could contain dangerous code when you use pull_request_target.

You also have to test wether [ci skip] is still working, pull_request_target event doesn't seem to be supported, only push and pull_request events.

Edit: a malicious user could even set run-browser-test label by himself, since with pull_request_target he has a read/write token on our repo.

[outsideris]

I believe the labeled event will be triggered when we attached a label. That means we should unlabeled it and then labeled it again which is annoyed. I will test it works I expect. And will check workflow_dispatch as well.

We should use pull_request_target for browser tests. All other tests should be run under pull_request. It is safe.

a malicious user could even set run-browser-test label by himself, since with pull_request_target he has a read/write token on our repo.

So, we should run something with PRs on pull_request_target after we checked if there is no malicious code.

Currently, there is no clear way with a manual trigger to show status properly and rerun it easily when pr author pushes new commits. We need more tests about that.

[outsideris]

With #4616 , we can attach run-browser-test label to forked PRs. It will automatically run browser tests with our secrets and then remove the label. So, we should attach the label after reviewing code carefully.

I've tested it in #4618

By default, browser tests will be skipped.

When we attach labels except run-browser-test, browser tests will also be skipped.

When we attach run-browser-test label, the browser test will be triggered and the label removed after running the workflow.