Repository navigation
fix: respect gypfile:false when detecting install scripts - #10061
JamieMagee wants to merge 1 commit into
Conversation
fc8a3a2 to
9e4f5b9
Compare
| // Lockfiles omit scripts and gypfile. Recover both before inferring a | ||
| // native build, including packages without a script-presence flag. | ||
| if (isGyp || !anyScript && node.hasInstallScript === true) { | ||
| const { content } = await PackageJson.normalize(node.path) |
There was a problem hiding this comment.
Under install-strategy=linked, a transitive gypfile: false dep still warns install: (install scripts present) when its lockfile or packument has hasInstallScript: true. This happens on fresh, lockfile-only and repeated installs, the same as on latest. The reason is that arb.actualTree uses the hoisted shape, so node.path (node_modules/<name>) does not exist on disk for a transitive dep under linked. normalize fails, matchesPackage is false, and the sentinel is emitted. Hoisted handles this case correctly, and so does a top-level linked dep (its path is a symlink into .store).
The PR body says the fix covers linked installs. Could this read through the store path for linked nodes, or could the claim be narrowed? A linked test with a transitive dep would help too, because the new linked test in test/arborist/reify.js only covers a top-level dep on a fresh install.
There was a problem hiding this comment.
I think we should cover linked installs too if possible.
Confirmed against both the base helper and this PR: a transitive opted-out dependency with hasInstallScript: true still produces (install scripts present) under linked installs across fresh, clean lockfile-based, and repeated installs. This is pre-existing behavior, but the new top-level linked test does not cover it.
The stale flag also remains in the linked project lockfile, while hoisted installs clear it.
| target.package.scripts = scripts | ||
| target.package.gypfile = pkg.gypfile | ||
| if (pkg.gypfile === false && !scripts.preinstall && !scripts.install && !scripts.postinstall) { | ||
| delete target.package.hasInstallScript |
There was a problem hiding this comment.
Under hoisted, this delete reaches the saved lockfile (npm install drops hasInstallScript: true). Under linked it does not, because the store IsolatedNode has its own package object, so the lockfile keeps true. The result is a lockfile diff that depends on the install strategy. Is rewriting the project lockfile intended here? If so, linked should probably behave the same way.
| * `"scripts":{"install": "node-gyp rebuild"}` | ||
|
|
||
| If there is a `binding.gyp` file in the root of your package and you have not defined an `install` or `preinstall` script, npm will default the `install` command to compile using node-gyp. | ||
| If your package contains `binding.gyp`, npm uses `node-gyp rebuild` as the default `install` script. |
There was a problem hiding this comment.
Nit: this drops "in the root of your package". isNodeGypPackage only checks ${path}/binding.gyp, so the old wording was more accurate.
| // Before extraction, this path can still contain an older package. | ||
| const matchesPackage = content && pkg.name && pkg.version && | ||
| content.name === pkg.name && content.version === pkg.version | ||
| if (matchesPackage) { | ||
| scripts = content.scripts || {} | ||
| gypfile = content.gypfile | ||
| recoveredOptOut = gypfile === false |
There was a problem hiding this comment.
This recovery can replace known incoming lifecycle scripts with metadata from the previously installed package. Matching name/version does not establish that the installed package has the same contents as the incoming source.
I reproduced this with two local tarballs named native-pkg@1.0.0: the installed tarball contains binding.gyp, sets gypfile: false, and has no scripts; the replacement has a different integrity and declares postinstall.
With --strict-allow-scripts and no approval, the base helper rejects before extraction.
With this change, recovery replaces the incoming scripts with the old empty scripts and accepts the old opt-out. Preflight succeeds, the replacement is extracted, and the lockfile changes; only afterward does npm warn that scripts were blocked.
The execution allowlist still blocks the script, so this is not an execution bypass, but it violates strict preflight’s rejection-before-mutation behavior. Could we preserve known incoming scripts and avoid treating matching name/version alone as proof that installed metadata describes the incoming package?
Please add a same-name/version, different-source regression test asserting ESTRICTALLOWSCRIPTS and unchanged installed contents and lockfile.
| * `"scripts":{"install": "node-gyp rebuild"}` | ||
|
|
||
| If there is a `binding.gyp` file in the root of your package and you have not defined an `install` or `preinstall` script, npm will default the `install` command to compile using node-gyp. | ||
| If your package contains `binding.gyp`, npm uses `node-gyp rebuild` as the default `install` script. |
| // Lockfiles omit scripts and gypfile. Recover both before inferring a | ||
| // native build, including packages without a script-presence flag. | ||
| if (isGyp || !anyScript && node.hasInstallScript === true) { | ||
| const { content } = await PackageJson.normalize(node.path) |
There was a problem hiding this comment.
I think we should cover linked installs too if possible.
Confirmed against both the base helper and this PR: a transitive opted-out dependency with hasInstallScript: true still produces (install scripts present) under linked installs across fresh, clean lockfile-based, and repeated installs. This is pre-existing behavior, but the new top-level linked test does not cover it.
The stale flag also remains in the linked project lockfile, while hoisted installs clear it.
4 participants
Arborist reads the installed package before inferring
node-gyp rebuild, so lockfile installs respectgypfile: falseand avoid false blocked-script warnings. The fix covers linked installs andscripts: null, with regression tests for stale script flags. Explicit scripts and the lockfile format are unchanged.References
Fixes #9837