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

Should GuardDog Stop Reporting Usage of "prepare" Scripts for NPM Packages? #308

Closed
cedricvanrompay-datadog opened this issue Feb 27, 2024 · 8 comments · Fixed by #313
Closed

Comments

@cedricvanrompay-datadog
Copy link
Contributor

GuardDog flags NPM packages having a prepare script (see source) but it seems that these scripts only get executed if you cd in the repository of the script and run npm install (without any arguments):

prepare:

  • Runs on local npm install without any arguments

source: https://docs.npmjs.com/cli/v10/using-npm/scripts#life-cycle-scripts

This script is for preparing your environment before doing development work on the package.

Said differently, if you do npm install foo and package foo has a prepare script it seems that the script will not be run.

Should we remove this script from the list that get reported by GuardDog?

@cedricvanrompay-datadog
Copy link
Contributor Author

I'll try to find some time validating this with a Proof of Concept

@sobregosodd
Copy link
Contributor

In the doc it reads:

NOTE: If a package being installed through git contains a prepare script, its dependencies and devDependencies will be installed, and the prepare script will be run, before the package is packaged and installed.

And dependencies can be specified as git repo, that means that the prepare will run then.

Can we validate this?

@cedricvanrompay-datadog
Copy link
Contributor Author

cedricvanrompay-datadog commented Mar 6, 2024

prepare does run when doing a npm install in the repository iself. We can see it by using a shim for bash which is invoked as part of the prepare script of https://www.npmjs.com/package/foreground-child:

➜  foreground-child git:(main) cat ~/.local/bin/bash
#!/opt/homebrew/bin/bash
echo HI FROM BASH SHIM
➜  foreground-child git:(main) ✗ npm install
npm WARN cli npm v10.1.0 does not support Node.js v18.12.1. This version of npm supports the following node versions: `^18.17.0 || >=20.5.0`. You can find the latest version at https://nodejs.org/.

> foreground-child@3.1.1 prepare
> tsc -p tsconfig.json && tsc -p tsconfig-esm.json && bash ./scripts/fixup.sh

HI FROM BASH SHIM

up to date, audited 419 packages in 3s

38 packages are looking for funding
  run `npm fund` for details

3 vulnerabilities (2 moderate, 1 critical)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

However it does not run indeed when doing npm install foreground-child from another NPM project:

$ npm install foreground-child
npm WARN cli npm v10.1.0 does not support Node.js v18.12.1. This version of npm supports the following node versions: `^18.17.0 || >=20.5.0`. You can find the latest version at https://nodejs.org/.

added 8 packages, and audited 9 packages in 549ms

2 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

When installing the package using a git URL, the prepare script does not seem to be executed:

$ npm install https://github.com/tapjs/foreground-child.git
npm WARN cli npm v10.1.0 does not support Node.js v18.12.1. This version of npm supports the following node versions: `^18.17.0 || >=20.5.0`. You can find the latest version at https://nodejs.org/.

added 8 packages, and audited 9 packages in 9s

2 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

However, it seems that NPM detected that the package comes from GitHub and used a special format.

➜  npm-prepare-script cat package.json
{
  "name": "npm-prepare-script",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "foreground-child": "github:tapjs/foreground-child"
  }
}

In the documentation, "Git URLs" and "GitHub URLs" are different types of dependencies:

So maybe prepare is run for Git URLs but not for GitHub URLs? This would not make any sense though, as the rationale for running it for Git URLs should still apply for GitHub URLs.

My guess is that when you do npm install git://X, NPM will do a git clone of the repository in some temporary directory and then run npm install in it, which would trigger the prepare script, but I cannot find the place in the source code of the NPM CLI that does that. (edit: confirmed, see https://docs.npmjs.com/cli/v8/configuring-npm/package-json#git-urls-as-dependencies)

Some additional notes:

@cedricvanrompay-datadog
Copy link
Contributor Author

Well, I may have a deeper look at exactly when will NPM scripts be executed, but I still think we should stop reporting NPM packages that have a prepare scripts for the sake of limiting alert fatigue, as it seems that having a prepare script is not a strong security signal.

@cedricvanrompay-datadog
Copy link
Contributor Author

Actually, reading https://docs.npmjs.com/cli/v10/using-npm/scripts#life-cycle-operation-order again, I have the impression that doing npm install [some-package] will never run lifecycle scripts, not even preinstall, install and postinstall. They seem to only be run when doing npm install (without arguments) or doing a global install npm install -g [some-package].

I am not sure how to test this, I am trying to find a package on NPM which install script runs a program I can shim...

@cedricvanrompay-datadog
Copy link
Contributor Author

Well, at least in Yarn it seems quite clear that doing yarn add [some package] will run many scripts, at least preinstall, install and postinstall. See the postinstall section in https://yarnpkg.com/advanced/lifecycle-scripts

@sobregosodd
Copy link
Contributor

Well, I may have a deeper look at exactly when will NPM scripts be executed, but I still think we should stop reporting NPM packages that have a prepare scripts for the sake of limiting alert fatigue, as it seems that having a prepare script is not a strong security signal.

@cedricvanrompay-datadog Based on the evidence you mention, I too consider then that we can stop reporting prepare scripts.

Well, at least in Yarn it seems quite clear that doing yarn add [some package] will run many scripts, at least preinstall, install and postinstall. See the postinstall section in https://yarnpkg.com/advanced/lifecycle-scripts

After reading the yarn lifecycle is clear to me that it does not executes nor support prepare scripts.

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 a pull request may close this issue.

2 participants