Skip to content

feat: add npm safeinstall command - #10046

Open
mohamed-dev2 wants to merge 1 commit into
npm:latestfrom
mohamed-dev2:feat/safeinstall
Open

mohamed-dev2 wants to merge 1 commit into
npm:latestfrom
mohamed-dev2:feat/safeinstall

Conversation

@mohamed-dev2

Copy link
Copy Markdown

What this adds

A standalone npm safeinstall: same arguments, same configuration, and the same install as npm install, but it stops and asks first on the two things that are easy to miss on a fast read of a command line.

  • a name that is one or two characters away from a well known package (expres for express), which is exactly what a typosquatter registers
  • a package that runs code on your machine while it is being installed

It is a separate command rather than a flag on npm install so the checks stay opt in. Nothing on the ordinary install path changes.

Design notes for review

Why extend Install instead of BaseCommand. SafeInstall extends Install and hands off with super.exec(args). Root lifecycle scripts, resolveAllowScripts, strictAllowScriptsPreflight, Arborist.reify and reifyFinish all run exactly as they do today, and the whole install parameter set, including workspaces, is inherited rather than duplicated. npm install itself is unchanged.

Why a constant list instead of a registry query. The name check compares against a fixed list of widely used packages. It is a heuristic and a judgement call about which names are worth protecting, and it costs no network round trip. A data-driven list of top-level downloads would need a source and a refresh story, which felt like a larger change than this PR. Happy to be told otherwise.

Why CONFIRM and not y. The one key confirmation is easy to send without reading the package name that is about to be fetched. The answer is trimmed before comparison, so a stray space is not a refusal, but the word has to be typed out.

Why --check-privileges is a real config definition. base-cmd.js rejects any unrecognised flag with EUNKNOWNCONFIG, and a command's own params list is not an allowlist for that check, so the flag has to be defined globally. The consequence is that the key is accepted, and settable in .npmrc, for every command while only safeinstall reads it. The definition description and the docs page both state that explicitly. It has no flatten, so the value stays out of flatOptions and is never handed to pacote or Arborist.

Granting is not approving. --check-privileges reports, it does not record. allowScripts in package.json remains the durable policy and npm approve-scripts remains the way to manage it. Documented on the page rather than left implied.

Only the requested packages are inspected. Transitive dependencies are not resolved at check time, since that is the install's job.

request is in the list on purpose. It is deprecated, and it is still the name a typosquatter most wants to sit next to.

Trade-offs a reviewer may push back on

  • A new top level command is a permanent surface area. A flag on npm install would be cheaper, at the cost of a prompt standing in front of every install.
  • The privilege check prompts once per package, so a long list with several native modules means several prompts. Batching them would need a different interaction shape.
  • The command cannot be used in CI. It throws ESAFEINSTALLNOTTY rather than hanging when a prompt is needed and stdin is not a TTY, and the page points at npm ci plus npm approve-scripts for that case.
  • Anything with install scripts is refused by default under the flag, which will be the wrong answer for some packages. That is the safe direction to be wrong in, but it is a policy call.

Verification

  • node . run test on the new file: 79 of 79 assertions pass.
  • 100% statement, branch, function and line coverage of lib/commands/safeinstall.js, which is the gate this repo enforces.
  • node . run lint passes, including the template-oss-check postlint step.
  • node . run build -w docs writes the man page and website output, and nav.yml came out of the generator byte for byte, so the nav entry is not hand written.
  • Snapshots regenerated for the three files that enumerate commands or config keys: npm.js, docs.js and commands/config.js. Two other snapshot files are rewritten by the generator with no content change and were reverted to keep the diff honest.
  • Driven by hand as well as in tests: --usage lists the flag, an unknown flag still fails with EUNKNOWNCONFIG, a typo prints the warning, and a piped typo ends in ESAFEINSTALLNOTTY instead of hanging.

Note on the environment: this was developed on Windows, where 9 assertions in publish.js, stage/index.js, cache.js and patch-diff.js fail. They reproduce identically on a clean tree with these changes stashed, so they are pre-existing and unrelated.

Notes for maintainers

test/lib/commands/safeinstall.js avoids test/fixtures/mock-npm.js on purpose. That helper threads its mocks through require('lib/npm.js'), so a stub for proc-log also reaches lib/npm.js and everything below it, which breaks lib/utils/timers.js and leaves handles open that make tap's temp dir cleanup fail with EBUSY on Windows. A plain npm object with tmock keeps each mock scoped to the module under test. Worth knowing if anyone reaches for the fixture here and hits the same wall.

Adds a standalone `npm safeinstall` that runs the same install as
`npm install` but stops and asks first on the two things that are easy to
miss on a fast read of a command line: a name that is one or two characters
away from a well known package, and a package that runs code while it is
being installed.

It is a separate command rather than a flag on `npm install` so the checks
are opt in. The install path is untouched: `SafeInstall` extends `Install`
and hands the specs to `super.exec(args)`, so root lifecycle scripts,
`resolveAllowScripts`, `Arborist.reify` and `reifyFinish` all run exactly as
they do today.

Name check, always on:

- Each spec is reduced to its registry name with `npm-package-arg`. Local
  paths, tarballs, git urls and aliases have no registry name and are
  skipped, and an unparseable spec is left for `npm install` to report.
- Names are compared against a constant list of widely used packages with
  `distance` from `fastest-levenshtein`, already a direct dependency via
  `lib/utils/did-you-mean.js`. No new dependency and no network call.
- An exact match is never a typo. Otherwise a distance of 1 or 2 is a
  suspect, and names under four characters are held to a distance of 1.
- Suspects are collected for every spec, printed together with the distance
  and any other close names, and gated on a single prompt. Only the literal
  word `CONFIRM` continues; `y`, `yes`, an empty line and `confirm` cancel
  with `ESAFEINSTALLCONFIRM`.

Privilege check, opt in via `--check-privileges`:

- `pacote.manifest` reads the manifest out of the packument and stops, so
  rejecting a package costs one metadata request and no tarball download.
- `preinstall`, `install` and `postinstall` are printed with their commands
  and nothing is installed unless `y` is answered, otherwise
  `ESAFEINSTALLPRIVILEGES`. A package declaring none of them is not asked
  about.
- This reports, it does not record. `allowScripts` remains the durable
  policy and `npm approve-scripts` remains how it is managed.

Both prompts go through `proc-log`'s `input.read`, the path the display
layer already handles, and write to stderr where npm already prompts. When a
prompt is needed and stdin is not a TTY the command throws
`ESAFEINSTALLNOTTY` rather than waiting on input that will never arrive, so a
cancelled or piped run never hangs and `package.json`,
`package-lock.json` and `node_modules` are left untouched.

`check-privileges` is a `Boolean` defaulting to `false` in the config
definitions with no `flatten`, so the value stays out of `flatOptions` and is
never handed to pacote or Arborist. It has to be defined globally because
`base-cmd.js` rejects any unrecognised flag with `EUNKNOWNCONFIG`, which means
the key is accepted by every command even though only `safeinstall` reads
it; the definition and the docs page both say so.

Tests: 79 assertions in `test/lib/commands/safeinstall.js`, 100% statement,
branch, function and line coverage of the new command. The tests deliberately
avoid `test/fixtures/mock-npm.js`, which threads its mocks through
`lib/npm.js` and would replace `proc-log` for npm's own internals as well;
a plain npm object with `tmock` keeps each mock scoped to the module under
test.
@mohamed-dev2
mohamed-dev2 requested a review from a team as a code owner September 26, 2026 22:47
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.

1 participant