Skip to content

fix: recognize allowScripts for local link targets#9490

Open
cyphercodes wants to merge 1 commit into
npm:latestfrom
cyphercodes:fix/9488-allow-scripts-file-deps
Open

fix: recognize allowScripts for local link targets#9490
cyphercodes wants to merge 1 commit into
npm:latestfrom
cyphercodes:fix/9488-allow-scripts-file-deps

Conversation

@cyphercodes
Copy link
Copy Markdown
Contributor

Summary

  • Recognize local directory link targets by their incoming link source when matching allowScripts policy entries.
  • Reuse that source identity when approve-scripts/deny-scripts derive file dependency policy keys.
  • Add coverage for reviewed local file: dependency link targets.

Fixes #9488

Testing

  • node node_modules/tap/bin/run.js --no-coverage workspaces/arborist/test/script-allowed.js workspaces/arborist/test/unreviewed-scripts.js test/lib/utils/allow-scripts-writer.js test/lib/utils/check-allow-scripts.js test/lib/utils/resolve-allow-scripts.js
  • node node_modules/eslint/bin/eslint.js lib/utils/allow-scripts-writer.js test/lib/utils/allow-scripts-writer.js workspaces/arborist/lib/script-allowed.js workspaces/arborist/test/script-allowed.js workspaces/arborist/test/unreviewed-scripts.js
  • git diff --check
  • Manual repro: local file: dependency with allowScripts: { "file:../testdep": false } no longer emits an allow-scripts warning; npm approve-scripts --all writes file:../testdep.

@cyphercodes cyphercodes requested review from a team as code owners June 4, 2026 15:58
@owlstronaut
Copy link
Copy Markdown
Contributor

@JamieMagee So this looks valid to me for v11, if the user has added file: as an approve script we shouldn't warn.

One thing I'd like your read on for v12 before this lands there: in rebuild.js the scriptsAllowed check short-circuits on node.isLink, with the comment "Bypasses: --dangerously-allow-all-scripts, links and workspaces (the owner is responsible)." I confirmed that means "file:../dep": false doesn't actually block the script, it runs in all cases (true, false, absent) on latest. Before this PR you'd at least see a warning. With it merged the warning is gone too, so the user gets total silence on top of a script that still runs. Is the isLink bypass the intended v12 behavior (file: deps treated like workspaces, owner-managed, allowScripts entries are warning-control only), or a gap that should be closed before v12 ships?

Copy link
Copy Markdown
Contributor

@owlstronaut owlstronaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds node.realpath and node.path to the spec list for every node. A registry-resolved package can't match a file policy key because matchFileOrDir short circuits. With this change a registry package like sharp@0.33.0 installed at proj/node_modules/sharp will match a k ey like file:/proj/node_modules/sharp": true.

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.

[BUG] npm i shows allow-scripts uncovered dependency warning even if dependency was already approved

2 participants