feat: Phase 2 of allowScripts opt-in install-script policy#9424
feat: Phase 2 of allowScripts opt-in install-script policy#9424JamieMagee wants to merge 1 commit into
allowScripts opt-in install-script policy#9424Conversation
| node.isLink || | ||
| node.isWorkspace || | ||
| bypassed?.has(node) || | ||
| isScriptAllowed(node, this.options.allowScripts) === true |
There was a problem hiding this comment.
Matcher gap for --install-strategy=linked: script-allowed.js:34 reads node.inBundle, but IsolatedNode (workspaces/arborist/lib/isolated-classes.js) doesn't define it. A bundled child added via IsolatedReifier.#createBundledTree whose resolved parses as a registry URL can be allowlisted by name here — hoisted mode would correctly return null and block.
Likely low real-world risk (needs an attacker package bundling a child with a forged registry-URL resolved, plus the user's allowScripts listing the colliding name, plus --install-strategy=linked), but worth either defining inBundle on IsolatedNode or covering it in workspaces/arborist/test/script-allowed.js.
| // it. `#addToBuildSet` checks `bypassedAllowScripts`. Log the | ||
| // bypassed names so it shows up in `--loglevel warn`. | ||
| const bypassedAllowScripts = new Set(nodes) | ||
| arb.options.bypassedAllowScripts = bypassedAllowScripts |
There was a problem hiding this comment.
Mutating arb.options works for the CLI's one-shot usage, but leaks the bypass set to any subsequent arb.rebuild() if the same Arborist is reused (programmatic callers). Cleaner to pass via arb.rebuild({ nodes, bypassedAllowScripts }) and read from the call site.
| // silently skips those scripts; this turns the silent skip into a | ||
| // hard failure for CI. Bypassed by `--ignore-scripts` and | ||
| // `--dangerously-allow-all-scripts`. | ||
| const strictAllowScriptsPreflight = async (arb, opts) => { |
There was a problem hiding this comment.
Near-verbatim copy of lib/utils/strict-allow-scripts-preflight.js — same skips (isProjectRoot/isWorkspace/isLink), same ESTRICTALLOWSCRIPTS code, just inlined instead of using checkAllowScripts. The two will drift. Move the shared logic into the arborist workspace so both callers consume one impl?
| // `npm approve-scripts --allow-scripts-pending` still shows them | ||
| // through the unfiltered checkAllowScripts walker. | ||
| const visible = unreviewedScripts.filter( | ||
| ({ node }) => !node.optional && !node.devOptional |
There was a problem hiding this comment.
Filtering optional/devOptional out of the summary makes "why didn't my optional thing build?" harder to debug — the verbose log doesn't distinguish a policy skip from a network/build failure. Include the reason (unreviewed vs denied) in the verbose line, or keep optional skips at info?
| const nodes = tree.inventory.filter(node => this.isNode(specs, node)) | ||
| const nodes = [...tree.inventory.filter(node => this.isNode(specs, node))] | ||
|
|
||
| // Naming a package on `npm rebuild` opts that package's install |
There was a problem hiding this comment.
Why? Also, why is this change going into phase 2? I thought the idea was that phase 2 was just changing the default.
There was a problem hiding this comment.
Thanks for the feedback. I opened this in draft and didn't expect anyone to review this yet 😅 . I'll be rolling this back before I switch the PR to ready for review (and spend a little more time on the semantics of the optional deps handling too).
3 participants
Implements Phase 2 of npm/rfcs#868. Default-deny is on: dependency install scripts are blocked unless
allowScriptslists them.preinstall/install/postinstall/preparequeues on the resolvedallowScriptspolicy. Bin linking is not gated.optional/devOptionaldeps are silently skipped (verbose log only), same shape as a failed optional install.npm rebuild <pkg>opts the named positionals in for that run (with alog.warn); barenpm rebuildstill honours the gate.libnpmexecadds a strict-mode preflight so--strict-allow-scriptsfails fast undernpm exec/npx.strict-allow-scriptsconfig doc updated; "Phase 1" wording removed from comments and docs.Bypasses:
--ignore-scripts(global),--dangerously-allow-all-scripts(per-install), andbypassedAllowScriptsfornpm rebuild <pkg>.Refs: npm/rfcs#868, follows #9360.