-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] npm pack and publish should not run hooks, or hooks need to have opt-out #7211
Comments
While I do believe this is a security bug because of the implications on GitHub, it's not technically a security bug as I read it because of this text,
GitHub doesn't differentiate between a code-contributor and repo-owner for the purposes of secrets. This is silly and unfortunate. Outside of GitHub, it's quite possible for CI to be controlled and to only inject secrets into a specific job that only runs trusted code. For example, to only inject the |
husky (and any tool that automatically installs git hooks) is definitely a security attack vector (there's a reason git chooses not to support that), and should be avoided in all cases for this reason. Does I use the |
No, it doesn't work. =( But we're in agreement, it should.
Doesn't that seem like a build-stage thing? I wouldn't put that in a pack or ignore stage. You're generating files. By any means, I don't want that functionality and |
Maybe as a side note, if we're all in agreement that Husky is crazy in even suggesting this we can at least send them an issue to remove this from their home page.
According to Husky they have "over 1.3M projects on GitHub". Which is insane |
pack is the build stage - it's when you're making the tarball, and is the only place the build step should run. That kind of logic should never have been in prepublish or "prepare" in the first place. The confusion is only because people wrongly believe that it's reasonable to install from a git repo. |
I've never seen
I think everything creates a new script called "build" which is the build script. In Angular, |
oh totally, i use |
I don't understand why you'd want that. As compared to just running By secure, I mean to say, I wouldn't have to worry about a change to package.json injecting code that leaks my NPM secrets when I'm just running |
Because I do, actually, run Why should they have that option? Unlike I'm not convinced "what if something on my filesystem is changed without me knowing it" is a realistically defensible attack vector - if they can do that they could change |
The point is that "in there", can usually be narrowed down to in your CI. That is to say, CI changes can be placed under ownership of a different team (such as a release team which owns the token). This token can be restricted such that it's only injected into the publication job. This isn't possible when the publication job (which requires Does that answer your concern? And to go back to your initial statement,
I agree with that. Why is desirable that |
I'm still very unclear on why this is a concern - if you don't want non-default behavior, you just don't put it in package.json. If another team at your company is modifying your files in a way you don't like, lobby them to stop doing that. No need to keep going back and forth here, tho, maybe the npm folks will understand your concern better than I. |
Because security isn't about "lobbying" actors to be good. It's about protecting against innocent mistakes, and malice. Developers need to be able to modify the package.json -- that's how they add dependencies to the project. They don't need to be able to inject code into the script that pushes a tarball up to npm, because that gives them the ability to leak the npm token in CI. I want to stop them from doing this. I want two classes of users, "developers" who can edit code anywhere. And "publishers" who have access to the NPM_TOKEN, and can control the publication step which calls |
To shine a light on what is currently happening with regards to
I think this is probably an oversight. The only functional difference in those scripts is that
This leads me to believe it's an oversight because So I think the following could all help with this issue:
|
Doing some more digging, I think making I do think documentation could be improved on the current specifics of prepare as well as linking to the |
Is there an existing issue for this?
This issue exists in the latest npm version
Current Behavior
Currently
npm pack
andnpm publish
run aprepare
hook. This presents a problem in practice because the prepare script as in the case of Husky is used to install githooks which is almost certainly going to break CI..I would also argue this is a security problem. The prepare script gives an area for code injection. By placing code in the prepare hook you can leaks secrets with
npm pack
andnpm publish
(like theNPM_TOKEN
they're often exposed to). This is a bigger problem too because codeowners is ineffective at mitigating this. But all of this raises a few questions,npm pack
andnpm publish
need to run any hooks? One of them is effectively a fancy tar alternative, and the other one just does an HTTP push with credentials sending the tar to npmjs. Why does either one of these things need to be hookable.npm pack
andnpm publish
have an--ignore-scripts
option likenpm install
. Especially for the purposes of CI!npm pack
andnpm publish
document these hooks in the help pages?npm install --ignore-scripts
works, I would think that the functionality ofnpm pack --ignore-scripts
should be to error if that functionality isn't supported. Is there any reason why npm cli supports sending unsupported options to subcommands?Expected Behavior
A
package.json
withprepare: "exit 1"
should permitnpm pack
. I can't see the utility in having a hook for annpm pack
.Steps To Reproduce
Workaround
The only workaround that I know of here to securely run
npm pack
andnpm prepare
in CI without risking code execution and secret leaking is to trim these lifecycle hooks out of the package.json,Environment
The text was updated successfully, but these errors were encountered: