4 ms·

Note that you need to be very careful about this if you have a public repository that accepts PR's from third parties. There is nothing stopping someone from ad

Note that you need to be very careful about this if you have a public repository that accepts PR's from third parties. There is nothing stopping someone from adding this via a PR: - name: Give me this person's NPM token run: cat ~/.npmrc Even if you have it locked down so they can't see the build output, they could just add a curl command to post the contents of your .npmrc file to their server. A number of open source projects have been hacked this way in the past. This is why I keep my NPM publish entirely isolated from the Github repo. I review every PR that is submitted to me carefully, but don't want any chance of accidentally merging in a malicious PR that will compromise my NPM packages.

pafo 7y ago
Thank you for this comment! It is a very good point.
tracker1 7y ago
absolutely... I think Github should probably put a warning statement on any PRs that include changes to .github/
- deleted 7y ago
  [deleted]
- judge2020 7y ago
  That would be good for the UI, but PRs that modify .github/workflows already never run the checks. I guess they could also modify your npm `scripts` to `cat .npmrc` but that's another issue.
robrtsql 7y ago
Does this rely on the `.npmrc` file being cached from a previous run (which will only happen if you use a self-hosted runner)? Or does this vulnerability mean that PR contributors basically have direct access to all of the secrets that you put on your GitHub repo?
- NathanKP 7y ago
  Yeah in the authors example they are writing a .npmrc file but it could also just as easily be outputting the env variable of the secret. Fundamentally if NPM can access the secret in the build that means anyone who can modify the build process can also access the secret. There are so many vectors of vulnerability: from modifying NPM package.json scripts, to modifying makefiles, to modifying the build scripts, etc that I find the only true solution is to have one public repo that accepts PR's from the public and only does checks like linting and running the tests, but does not have any access to secrets for publish. Ideally the public repo also does not contain your actual publish scripts either, only you as the package owner have access to them, as they are stored in your own private repo. Then have another private repo that you sync into or an external, fully private build system that keeps the secrets, and does the real NPM publish. I think it is just far too dangerous to do the NPM publish from the public facing repo. I assume the author is well aware of this too, but just wanted to include a warning in case anyone else hasn't run into this or thought of it yet.
xPaw 7y ago
How would the workflow run if they specify `on: release: types: [published]`?
amenod 7y ago
I like the way GitLab solves this. You mark your secrets ("CI/CD env vars") as protected, then only protected branches and tags can use them. When someone has right to merge or push to a protected branch, thay of course gain access to these vars, but if you don't allow anyone to do this, you are pretty safe.
mscccc 7y ago
> With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository. https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets#using-encrypted-secrets-in-a-workflow https://help.github.com/en/actions/automating-your-workflow-...
- NathanKP 7y ago
  Yeah but it only takes accidentally merging in something malicious once to have it running in the main repository now
est31 7y ago
Yes this is a problem in Rust-land too. crates.io doesn't support user tokens to be limited to a crate only [1], but gives access to all the crates an user has access to. So if the token leaks, attackers have access to all your crates. For the cpal crate we worked around the issue by creating a new github user that has only access to the cpal crate (and maybe in the future a few other crates in the same org) but nothing more [2]. This solution isn't really good tho. [1]: https://github.com/rust-lang/crates.io/issues/849 https://github.com/rust-lang/crates.io/issues/849 [2]: https://github.com/RustAudio/cpal/pull/337 https://github.com/RustAudio/cpal/pull/337

pafo 7y ago

Thank you for this comment! It is a very good point.

tracker1 7y ago

absolutely... I think Github should probably put a warning statement on any PRs that include changes to .github/

deleted 7y ago
[deleted]
judge2020 7y ago
That would be good for the UI, but PRs that modify .github/workflows already never run the checks. I guess they could also modify your npm `scripts` to `cat .npmrc` but that's another issue.

robrtsql 7y ago

Does this rely on the `.npmrc` file being cached from a previous run (which will only happen if you use a self-hosted runner)? Or does this vulnerability mean that PR contributors basically have direct access to all of the secrets that you put on your GitHub repo?

NathanKP 7y ago
Yeah in the authors example they are writing a .npmrc file but it could also just as easily be outputting the env variable of the secret. Fundamentally if NPM can access the secret in the build that means anyone who can modify the build process can also access the secret. There are so many vectors of vulnerability: from modifying NPM package.json scripts, to modifying makefiles, to modifying the build scripts, etc that I find the only true solution is to have one public repo that accepts PR's from the public and only does checks like linting and running the tests, but does not have any access to secrets for publish. Ideally the public repo also does not contain your actual publish scripts either, only you as the package owner have access to them, as they are stored in your own private repo. Then have another private repo that you sync into or an external, fully private build system that keeps the secrets, and does the real NPM publish. I think it is just far too dangerous to do the NPM publish from the public facing repo. I assume the author is well aware of this too, but just wanted to include a warning in case anyone else hasn't run into this or thought of it yet.

xPaw 7y ago

How would the workflow run if they specify `on: release: types: [published]`?

amenod 7y ago

I like the way GitLab solves this. You mark your secrets ("CI/CD env vars") as protected, then only protected branches and tags can use them. When someone has right to merge or push to a protected branch, thay of course gain access to these vars, but if you don't allow anyone to do this, you are pretty safe.

mscccc 7y ago

> With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository. https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets#using-encrypted-secrets-in-a-workflow https://help.github.com/en/actions/automating-your-workflow-...

NathanKP 7y ago
Yeah but it only takes accidentally merging in something malicious once to have it running in the main repository now

est31 7y ago

Yes this is a problem in Rust-land too. crates.io doesn't support user tokens to be limited to a crate only [1], but gives access to all the crates an user has access to. So if the token leaks, attackers have access to all your crates. For the cpal crate we worked around the issue by creating a new github user that has only access to the cpal crate (and maybe in the future a few other crates in the same org) but nothing more [2]. This solution isn't really good tho. [1]: https://github.com/rust-lang/crates.io/issues/849 https://github.com/rust-lang/crates.io/issues/849 [2]: https://github.com/RustAudio/cpal/pull/337 https://github.com/RustAudio/cpal/pull/337