chore: run CI on PRs from forks

[SimenB]

Purpose (TL;DR) - mandatory

CI is not running - see #398

[codecov]

Codecov Report

Merging #399 (df5c157) into master (aaa8153) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #399   +/-   ##
=======================================
  Coverage   93.85%   93.85%           
=======================================
  Files           1        1           
  Lines         586      586           
=======================================
  Hits          550      550           
  Misses         36       36           

Flag	Coverage Δ
unit	93.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaa8153...df5c157. Read the comment docs.

[SimenB]

limiting to only run on pushes to master to avoid duplicate runs when opening a PR from a branch in this repo (like this very PR):

[fatso83]

Weird. We always used to run on pull requests. When did we stop doing that, @mroderick?

I don't claim to remember the specifics, but AFAIK we decided not to run the saucelabs tests on prs to avoid the risk of the credentials being exposed. Someone could do console.log(process.env.SAUCELABS_SECRET) for instance.
Does this change any of that?

P.S. Didn't get the reference to 398.

[SimenB]

P.S. Didn't get the reference to 398.

No CI runs there since it's from a fork

[benjamingr]

I don't claim to remember the specifics, but AFAIK we decided not to run the saucelabs tests on prs to avoid the risk of the credentials being exposed. Someone could do console.log(process.env.SAUCELABS_SECRET) for instance.
Does this change any of that?

Yeah I agree running CI on forks is a bit dangerous - I think PRs whose author has the commit bit or PRs with a specific GitHub label (since only project members can add those) might be a good tradeoff?

[SimenB]

Note that GH by default doesn't run CI for people who are first time contributors (https://github.blog/changelog/2021-04-22-github-actions-maintainers-must-approve-first-time-contributor-workflow-runs/) if that helps.

The current situation is not really tenable as it places the burden of testing everything CI does on the person merging a PR

[fatso83]

OK, if that is the case, then I think this should merge. I think we should do the same with Sinon TBH. Should some pre-approved committer then expose those credentials, it is always possible to create a new token.

[fatso83]

cc @mroderick @mantoni

[SimenB]

Oh, and ref people logging out the secret

https://docs.github.com/en/actions/security-guides/encrypted-secrets#using-encrypted-secrets-in-a-workflow

Note: With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

[benjamingr]

@SimenB the thing is - either browser tests run or don't run on CI.

• If they don't run we haven't actually solved much (running just Node tests is better than nothing but wouldn't catch Safari bugs).
• If they do run then people can log the secret.

Worst case like Carl said - the secret leaks and we'll replace the token - it's just a saucelabs account not the npm publish token :)