[RRFC] all commands (without context) should respect `--ignore-scripts`

darcyclarke commented 1 year ago

Motivation ("The Why")

Today, the ecosystem has been incorrectly trained to believe that --ignore-scripts ensures no "scripts" are executed & the nuance of the various situations where that is true has historically been hard to document & educate (ex. bug bounty submissions based on the GitHub program/scope encompassing "Arbitrary script execution upon package install with the --ignore-scripts flag" https://bounty.github.com/targets/npm-cli.html). For v10 (ref. https://github.com/npm/statusboard/issues/487 / https://github.com/npm/cli/pull/6641), I'm recommending the team makes all commands respect this flag properly (no matter how silly the situation).

How

Current Behaviour

npm makes nuanced decisions about which scripts to run even when --ignore-scripts is defined (ex. git deps run prepare, prepack, npm test will run scripts.test etc.)

Desired Behaviour

running install, publish, pack & more will ignore all defined scripts when --ignore-scripts is set (ex. git repos would no longer be treated uniquely during install & prepare/prepack etc. wouldn't be executed)
running npm test --ignore-scripts or npm run foo --ignore-scripts executes nothing (seems weird, but the result would be consistent)

References

Bikeshedding

you may want to look at grouping scripts & providing net-new flags to provide nuanced behavior some have asked for (ex. --ignore-pre-scripts / --ignore-post-scripts / --ignore-lifecycle-scripts or something similar) but --ignore-scripts should be the most broad in its enforcement & is the most requested

ljharb commented 1 year ago

I think the challenge is that it affects both the current project's scripts and also dependencies' scripts.

I always would probably want to ignore by default the scripts of dependencies - but i would never want to ignore my own scripts.

darcyclarke commented 1 year ago

Trying to accommodate for that kind of nuance is how we got into the current state (ie. there are too many variants for a single flag to navigate). That said, it should still be possible to fully disable scripts in a single flag & this is what people are expecting but is not what happens.

Previous work/efforts to advance scripts more holistically stalled & I'm guessing are unlikely to pick up any time soon (ref. https://github.com/npm/rfcs/pull/437) which is why I thought this would be a smaller/more realistic scope of work.

ljharb commented 1 year ago

I'm still unclear why anyone would ever want to ignore first-party scripts?

darcyclarke commented 1 year ago

The distinction isn't clear to anyone without in-depth knowledge/experience (ie. has read the code). There is no documentation delineating "first-party" scripts today. Would a workspace's scripts be "first-party", even though it can be defined as a dependency? What about linked deps? The flag's name is "ignore-scripts". It should do what its named & nothing more/less. If the ecosystem desires nuance, that's a separate feature/flag.

To be clear, I won't die on this hill as I'm no longer actively involved in npm's development. The fact that the work for v10 is being planned/scoped is what made me think this change should be discussed/considered given that there has historically been security concerns & confusion around script execution during installation (ie. I'd like to protect people & make things clearer). If the base-case of just ignoring lifecycle scripts of git deps when --ignore-scripts is set gets resolved then I'd be fine with that as an outcome.

ljharb commented 1 year ago

First-party would mean only the scripts you could access yourself with npm run.

darcyclarke commented 1 year ago

npm run foo -ws or npm run foo --prefix=./node_modules/any-package-at-all-anywhere-on-the-system/ <- so these would be "first-party"?

Either way, there's no mention of/delineation of "first-party" or "third-party" anywhere in the docs. And the argument here has nothing to do with that; it's literally whether or not --ignore-scripts should do as it says without nuance. There's plenty of reasons for why you'd want to ignore scripts which you may/may not have defined (ex. environment-specific needs, debugging - & in the case of "third-party" scripts - security & immutability)

The current docs note:

Note that commands explicitly intended to run a particular script, such as npm start, npm stop, npm restart, npm test, and npm run-script will still run their intended script if ignore-scripts is set, but they will not run any pre- or post-scripts.

But there's even more nuance then that, npm install --ignore-scripts will still run lifecycle scripts for git deps (because it is running pack without passing along the option/config). There's no way to prevent this today. The proposal is to make --ignore-scripts truly ignore all scripts. If there's a want/need for ignoring some but not all scripts, then that flag (or flags) should be created. Right now - for those without context - --ignore-scripts is actually --ignore-some-scripts-some-times & continues to be broadly misinterpreted.

ljharb commented 1 year ago

I totally agree that it's a confusing option right now and doesn't match people's intuitions. (and --prefix is a terrible option that shouldn't exist, so i wasn't including that in my definition :-p )

The biggest desire I'm aware of is to avoid running untrusted scripts during install - ie, those of installed dependencies. What other use cases are there?

dominykas commented 1 year ago

The biggest desire I'm aware of is to avoid running untrusted scripts during install - ie, those of installed dependencies. What other use cases are there?

This isn't about trust, but more about efficiency - the fact that prepare runs again during pack is pretty annoying too.

Use case: you're trying publish a pre-release[^1] with git based TS dependencies. It's reasonable for them to execute tsc during install, but there's no point in re-executing it during pack/[pre]publish (no caching, because rm -rf dist is also a sane thing to do before tsc).

[^1]: it's less likely to happen for final releases - git based deps for production are generally a no no

ljharb commented 1 year ago

@dominykas i'd need more info to understand, but it sounds like you're describing a new lifecycle script rather than the need to manually remember that you need to disable extra scripts while running pack/publish?

dominykas commented 1 year ago

Yes, quite possibly a new life cycle script can also solve this - something that runs when you npm install, but only for git based dependencies. But there's so many lifecycle scripts, with so many not-exactly-what-it-says-on-the-tin behaviors, that maybe a new one will not necessarily make things better, given we can just respect --ignore-scripts.

trusktr commented 1 year ago

I'm still unclear why anyone would ever want to ignore first-party scripts?

In

https://github.com/npm/cli/issues/4202

I wanted to use Lerna to boostrap (link) packages (submodules in the repo) together, without running any scripts at all. That way, during the bootstrap process, packages would not try to build themselves without all packages first being in place, otherwise a build could fail midway through bootstrap.

Then, after bootstrap, I intended to run lerna run build to build everything with all packages being in place and linked.

I solved my problem more recently by removing prepare scripts, and committing build outputs to all repos to keep them installable from git without a build, with a commit hook that ensures output is up to date.

trusktr commented 1 year ago

This just bit me again today. It is very annoying!

I ran the following in the Docsify repo in Windows, trying to avoid issues during install by using --ignore-scripts, but now npm simply won't let me install because it runs scripts and it fails:

PS D:\src\lume+lume\packages\docsifyjs+docsify> npm i --ignore-scripts
npm WARN deprecated stable@0.1.8: Modern JS already guarantees Array#sort() is a stable sort, so this library is deprecated. See the compatibility table on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility
npm WARN deprecated opn@6.0.0: The package has been renamed to `open`
npm WARN deprecated chokidar@2.1.8: Chokidar 2 does not receive security updates since 2019. Upgrade to chokidar 3 with 15x fewer dependencies      
npm WARN deprecated w3c-hr-time@1.0.2: Use your platform's native performance.now() and performance.timeOrigin.
npm WARN deprecated svgo@1.3.2: This SVGO version is no longer supported. Upgrade to v2.x.x.
npm ERR! code 1
npm ERR! path D:\src\lume+lume\packages\docsifyjs+docsify
npm ERR! command failed
npm ERR! command C:\WINDOWS\system32\cmd.exe /d /s /c npm run build
npm ERR! > docsify@4.13.0 build
npm ERR! > rimraf lib themes && run-s build:js build:css build:css:min build:cover build:emoji
npm ERR!
npm ERR!
npm ERR! > docsify@4.13.0 build:js
npm ERR! > cross-env NODE_ENV=production node build/build.js
npm ERR!
npm ERR! lib/plugins/ga.js
npm ERR! lib/plugins/gtag.min.js
npm ERR! lib/plugins/gtag.js
npm ERR! lib/plugins/ga.min.js
npm ERR! lib/plugins/external-script.js
npm ERR! lib/plugins/gitalk.min.js
npm ERR! lib/plugins/matomo.js
npm ERR! lib/plugins/matomo.min.js
npm ERR! lib/plugins/disqus.js
npm ERR! lib/plugins/gitalk.js
npm ERR! lib/plugins/external-script.min.js
npm ERR! lib/plugins/disqus.min.js
npm ERR! lib/plugins/emoji.min.js
npm ERR! lib/plugins/emoji.js
npm ERR! lib/plugins/search.js
npm ERR! lib/plugins/search.min.js
npm ERR! lib/plugins/front-matter.min.js
npm ERR! lib/plugins/front-matter.js
npm ERR! lib/plugins/zoom-image.min.js
npm ERR! lib/plugins/zoom-image.js
npm ERR! lib/docsify.js
npm ERR! lib/docsify.min.js
npm ERR!
npm ERR! > docsify@4.13.0 build:css
npm ERR! > mkdirp themes && npm run css -- -o themes
npm ERR!
npm ERR!
npm ERR! > docsify@4.13.0 css
npm ERR! > node build/css -o themes
npm ERR! [Stylus Build ] stderr: node:internal/modules/cjs/loader:1051
npm ERR!   throw err;
npm ERR!   ^
npm ERR!
npm ERR! Error: Cannot find module 'D:\src\lume+lume\packages\docsifyjs+docsify\node_modules\stylus\bin\stylus'
npm ERR!     at Module._resolveFilename (node:internal/modules/cjs/loader:1048:15)
npm ERR!     at Module._load (node:internal/modules/cjs/loader:901:27)
npm ERR!     at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
npm ERR!     at node:internal/main/run_main_module:23:47 {
npm ERR!   code: 'MODULE_NOT_FOUND',
npm ERR!   requireStack: []
npm ERR! }
npm ERR!
npm ERR! Node.js v20.5.0
npm ERR!
npm ERR! [Stylus Build ] child process exited with code 1
npm ERR! npm ERR! Lifecycle script `css` failed with error:
npm ERR! npm ERR! Error: command failed
npm ERR! npm ERR!   in workspace: docsify@4.13.0
npm ERR! npm ERR!   at location: D:\src\lume+lume\packages\docsifyjs+docsify
npm ERR! npm ERR! Lifecycle script `build:css` failed with error:
npm ERR! npm ERR! Error: command failed
npm ERR! npm ERR!   in workspace: docsify@4.13.0
npm ERR! npm ERR!   at location: D:\src\lume+lume\packages\docsifyjs+docsify
npm ERR! ERROR: "build:css" exited with 1.
npm ERR! npm ERR! Lifecycle script `build` failed with error:
npm ERR! npm ERR! Error: command failed
npm ERR! npm ERR!   in workspace: docsify@4.13.0
npm ERR! npm ERR!   at location: D:\src\lume+lume\packages\docsifyjs+docsify

npm ERR! A complete log of this run can be found in: C:\Users\trusktr\AppData\Local\npm-cache\_logs\2023-10-21T21_35_44_896Z-debug-0.log

I'm guessing it is trying to run the prepare script, which is trying to run npm run build. EDIT: Yep, that was it.

I just want npm to put dependencies in place, nothing more (it should show a message if it is not able to do that, f.e. a git dependency that requires prepare).

npm / rfcs