Skip to content
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

Respect ignoreScripts config when running npm pack / npm publish #249

Closed
wants to merge 1 commit into from

Conversation

dominykas
Copy link

This should address npm/cli#3707, although now that I'm re-reading npm/cli#3300, I wonder what is the actual intended behavior? It seems arborist is also keen to run prepare regardless of the user setting: https://github.com/npm/cli/blob/latest/workspaces/arborist/lib/arborist/rebuild.js#L180-L182 (while the other scripts it runs above/below do respect the setting).

TBH, my gut feeling is that the --ignore-scripts should take precedence over anything else. I know there's an RFC for command specific config, but we're probably a long way from there and running scripts when people explicitly disable them is a security issue too that needs to be communicated as such at the very least.

If it's the case that prepare takes precedence over --ignore-scripts, then there's probably a need for --ignore-scripts-but-we-really-mean-it-this-time...

@dominykas dominykas requested a review from a team as a code owner November 14, 2022 13:35
@wraithgar
Copy link
Member

prepare is always ran by npm when installing git dependencies, regardless of the flag.

There are in fact two bugs open right now because it is not doing that in certain situations, so removing this altogether would be quite a breaking and unintended change.

cf npm/cli#4148, npm/cli#4027

@wraithgar wraithgar closed this Nov 14, 2022
@dominykas
Copy link
Author

dominykas commented Nov 18, 2022

Is there a way to disable this entirely for the pack (and publish) use cases?

e.g. https://github.com/npm/cli/blob/dc8e6bdd1d9e3416846c4f0624705cb42a7fb067/test/lib/commands/publish.js#L582 disables the publish/prepublish scripts - but not the prepare one?

Not talking about the install ones. But I'm also curious about the statement "always run when installing" - because that would be a major security issue?

@EvanCarroll
Copy link

prepare is always ran by npm when installing git dependencies, regardless of the flag.

There are in fact two bugs open right now because it is not doing that in certain situations, so removing this altogether would be quite a breaking and unintended change.

cf npm/cli#4148, npm/cli#4027

I don't understand why pack or publish would install git dependencies, or why I would want that behavior. One of the commands outputs a tarball, one of them pushes up to npm. Neither of these operations need any dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants