feat: add npm safeinstall command - #10046
Open
mohamed-dev2 wants to merge 1 commit into
Open
mohamed-dev2 wants to merge 1 commit into
mohamed-dev2 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
A standalone
npm safeinstall: same arguments, same configuration, and the same install asnpm install, but it stops and asks first on the two things that are easy to miss on a fast read of a command line.expresforexpress), which is exactly what a typosquatter registersIt is a separate command rather than a flag on
npm installso the checks stay opt in. Nothing on the ordinary install path changes.Design notes for review
Why extend
Installinstead ofBaseCommand.SafeInstallextendsInstalland hands off withsuper.exec(args). Root lifecycle scripts,resolveAllowScripts,strictAllowScriptsPreflight,Arborist.reifyandreifyFinishall run exactly as they do today, and the whole install parameter set, including workspaces, is inherited rather than duplicated.npm installitself 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
CONFIRMand noty. 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-privilegesis a real config definition.base-cmd.jsrejects any unrecognised flag withEUNKNOWNCONFIG, and a command's ownparamslist 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 onlysafeinstallreads it. The definition description and the docs page both state that explicitly. It has noflatten, so the value stays out offlatOptionsand is never handed to pacote or Arborist.Granting is not approving.
--check-privilegesreports, it does not record.allowScriptsinpackage.jsonremains the durable policy andnpm approve-scriptsremains 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.
requestis 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
npm installwould be cheaper, at the cost of a prompt standing in front of every install.ESAFEINSTALLNOTTYrather than hanging when a prompt is needed and stdin is not a TTY, and the page points atnpm ciplusnpm approve-scriptsfor that case.Verification
node . run teston the new file: 79 of 79 assertions pass.lib/commands/safeinstall.js, which is the gate this repo enforces.node . run lintpasses, including thetemplate-oss-checkpostlint step.node . run build -w docswrites the man page and website output, andnav.ymlcame out of the generator byte for byte, so the nav entry is not hand written.npm.js,docs.jsandcommands/config.js. Two other snapshot files are rewritten by the generator with no content change and were reverted to keep the diff honest.--usagelists the flag, an unknown flag still fails withEUNKNOWNCONFIG, a typo prints the warning, and a piped typo ends inESAFEINSTALLNOTTYinstead of hanging.Note on the environment: this was developed on Windows, where 9 assertions in
publish.js,stage/index.js,cache.jsandpatch-diff.jsfail. 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.jsavoidstest/fixtures/mock-npm.json purpose. That helper threads itsmocksthroughrequire('lib/npm.js'), so a stub forproc-logalso reacheslib/npm.jsand everything below it, which breakslib/utils/timers.jsand leaves handles open that make tap's temp dir cleanup fail withEBUSYon Windows. A plain npm object withtmockkeeps each mock scoped to the module under test. Worth knowing if anyone reaches for the fixture here and hits the same wall.