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

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

Closed
Tracked by #711
darcyclarke opened this issue Jul 18, 2023 · 12 comments
Closed
Tracked by #711

Comments

@darcyclarke
Copy link
Contributor

darcyclarke commented Jul 18, 2023

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. npm/statusboard#487 / npm/cli#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
@darcyclarke darcyclarke changed the title [RRFC] all commands & dependencies (without context) should respect --ignore-scripts [RRFC] all commands (without context) should respect --ignore-scripts Jul 18, 2023
@ljharb
Copy link
Contributor

ljharb commented Jul 18, 2023

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
Copy link
Contributor Author

darcyclarke commented Jul 19, 2023

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. #437) which is why I thought this would be a smaller/more realistic scope of work.

@ljharb
Copy link
Contributor

ljharb commented Jul 19, 2023

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

@darcyclarke
Copy link
Contributor Author

darcyclarke commented Jul 20, 2023

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
Copy link
Contributor

ljharb commented Jul 20, 2023

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

@darcyclarke
Copy link
Contributor Author

darcyclarke commented Jul 20, 2023

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
Copy link
Contributor

ljharb commented Jul 20, 2023

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
Copy link

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-release1 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).

Footnotes

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

@ljharb
Copy link
Contributor

ljharb commented Sep 11, 2023

@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
Copy link

dominykas commented Sep 12, 2023

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
Copy link

trusktr commented Oct 15, 2023

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

In

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
Copy link

trusktr commented Oct 21, 2023

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 [email protected]: 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 [email protected]: The package has been renamed to `open`
npm WARN deprecated [email protected]: Chokidar 2 does not receive security updates since 2019. Upgrade to chokidar 3 with 15x fewer dependencies      
npm WARN deprecated [email protected]: Use your platform's native performance.now() and performance.timeOrigin.
npm WARN deprecated [email protected]: 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! > [email protected] 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! > [email protected] 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! > [email protected] build:css
npm ERR! > mkdirp themes && npm run css -- -o themes
npm ERR!
npm ERR!
npm ERR! > [email protected] 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: [email protected]
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: [email protected]
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: [email protected]
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).

@darcyclarke darcyclarke closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2023
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

No branches or pull requests

4 participants