Add some clarification around accidental publishing - #762

I wonder if we should mention that this strategy is only safe and valid when working in a closed environment without forks. The problem with forks is that we can't really give our secrets, that are required for publishing, to the fork as that would be a security concern.

That's a good point I hadn't considered. Would adding a repo check (like how Formik does it, for example) be acceptable in a non-private repo?

Also just so I'm fully understanding (am a bit new to publishing in general) - wouldn't the action fail in the forked repo since the secrets wouldn't have gotten added there? Or is it an issue if they go and add their own GitHub token/NPM token and attempt to publish under the same package name (if the package isn't a scoped one)?

Mainly wondering because I'm a bit confused how automatic snapshots would be different from regular publishing in terms of the forks/security concerns. I guess that's why Formik has that repo check in their workflow?

Looks like you can turn off workflow runs for forks, and require approval for all outside collaborator PRs. Should we recommend that here in addition to the repo check in the action itself?

Would adding a repo check (like how Formik does it, for example) be acceptable in a non-private repo?

I think that the answer is yes but I'm not completely sure. I'm not 100% sure about this though. I think I have understood this at one point in time but I completely lost this knowledge since then and it was quite fuzzy to me back then anyway. We have both pull_request and pull_request_target events and I don't recall exactly how they are supposed to be used. I think this might be a somewhat good explainer: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Also just so I'm fully understanding (am a bit new to publishing in general) - wouldn't the action fail in the forked repo since the secrets wouldn't have gotten added there?

I think that ye - it would fail. On the other hand - if we know that a particular action doesn't have a chance of succeeding then we shouldn't start it at all as it will only bring noise to the logs etc.

Mainly wondering because I'm a bit confused how automatic snapshots would be different from regular publishing in terms of the forks/security concerns.

Regular publishes are usually done from the base branch of the repo and as such they are almost always executed from within that repo's context - so they have access to those secrets and since new commits can arrive on the base branch only because somebody with write permissions has pushed/merged them then it's fine. After all they could steal those secret one way or another anyway by pushing out some malicious action or something.

From what I can piece together from the article, workflows triggered via pull_request in external forks do not have access to repository secrets and is read-only. Seems that a combination of turning off workflow runs for forks (described above) and adding a repo check (don't even run if the github.repository doesn't match) would be sufficient, as a note to have in the docs.

However, now I'm thinking it would probably just be simpler to remove the GitHub Action stuff entirely from this PR and not include it in the docs. I believe accidental publishing would still occur if a person attempts to generate a snapshot manually on a brand new package without an added changeset, is that correct? So it would still be valuable information to have in the docs without all the (maybe unnecessary?) extra context around automatic snapshots.

What do you think?

You are probably right that a repository check should be enough to avoid unwanted consequences here. I think this part of the PR is good to go and that including this workflow example can be quite handy for people

Added to the docs :) Let me know if there's anything else about it you'd want me to include.

This section is great but it's explained from the perspective of snapshots whereas this is just an implication of the design.

The primary rule that should be explained first is that changeset publish just doesn't actually care about changeset files at all. The only thing that it does is:

1. it queries current versions in the repository
2. it compares with published versions
3. it tries to publish what has not been published yet

So this accidental publish can even happen if you have some unpublished package (or just unpublished version of a package), you start using Changesets and use our changesets/action. If you don't immediately add a changeset file then the first run of our action just calls changeset publish and it publishes what has not been published yet (as described above).

I think that we should start with explaining this principle, then mention this implication for starting using Changesets and changeset/action (later we'd link to here from changeset/action's README) and then also mention how it affects snapshot releases.

Snapshot releases are just always prone to this behavior because when creating them on CI we usually run a script like changeset version && changeset publish, without checking if changeset version even has done anything (checking that would usually complicate stuff beyond what is necessary). So since such action always ends up calling changeset publish we are always facing this "risk".

Sorry for the delay here, I've re-worded everything as you suggested! I've removed the section I added previously ("Unintended first time publish") and just worked everything into the main section for the command.

Does it read any better?