feat: allowScripts tooling and inBundle hardening (#9480) · npm/cli@64e3f79 · GitHub
Skip to content

Commit 64e3f79

Browse files
JamieMageeJamieMagee
authored
feat: allowScripts tooling and inBundle hardening (#9480)
Pulls the non-behavioral pieces out of #9424 so they can land on v11: the `collectUnreviewedScripts`/`strictAllowScriptsError` helpers, the `inBundle` fixes, and an opt-in libnpmexec preflight. Nothing changes by default here, install scripts still run. The default-deny flip stays in #9424 for v12. ## References #9424
1 parent 6be874b commit 64e3f79

15 files changed

Lines changed: 553 additions & 154 deletions

‎lib/commands/rebuild.js‎

Lines changed: 8 additions & 1 deletion

‎lib/utils/allow-scripts-cmd.js‎

Lines changed: 4 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -110,27 +110,10 @@ class AllowScriptsCmd extends BaseCommand {
110110
output.standard('No packages with unreviewed install scripts.')
111111
return
112112
}
113-
// Bundled dependencies cannot be allowlisted in Phase 1 (RFC defers
114-
// this to a follow-up because matching by name@version from the
115-
// bundled tarball would reintroduce manifest confusion). Exclude
116-
// them from `--all` so we don't silently write a policy entry under
117-
// attacker-controlled identity.
118-
const candidates = unreviewed.filter(({ node }) => !node.inBundle)
119-
const skipped = unreviewed.length - candidates.length
120-
if (skipped > 0) {
121-
/* istanbul ignore next: plural variant covered separately */
122-
const noun = skipped === 1 ? 'dependency' : 'dependencies'
123-
log.warn(
124-
this.logTitle,
125-
`Skipping ${skipped} bundled ${noun}; bundled deps with install ` +
126-
'scripts cannot be allowlisted in this release.'
127-
)
128-
}
129-
if (candidates.length === 0) {
130-
output.standard('No packages eligible for approval.')
131-
return
132-
}
133-
const groups = this.groupByPackage(candidates.map(({ node }) => node))
113+
// Bundled dependencies never appear in `unreviewed` (checkAllowScripts
114+
// skips them because they never run their install scripts and cannot
115+
// be allowlisted), so there is nothing extra to filter here.
116+
const groups = this.groupByPackage(unreviewed.map(({ node }) => node))
134117
await this.writePolicyChanges(groups)
135118
}
136119

‎lib/utils/check-allow-scripts.js‎

Lines changed: 21 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,59 +1,27 @@
1-
const isScriptAllowed = require('@npmcli/arborist/lib/script-allowed.js')
2-
const getInstallScripts = require('@npmcli/arborist/lib/install-scripts.js')
1+
const { collectUnreviewedScripts } = require('@npmcli/arborist/lib/unreviewed-scripts.js')
32

4-
// Walks arb.actualTree.inventory and returns the list of dep nodes that
5-
// have install-relevant lifecycle scripts and are not yet covered (or
6-
// explicitly denied) by the allowScripts policy.
3+
// Walks a tree's inventory and returns the list of dep nodes that have
4+
// install-relevant lifecycle scripts and are not yet covered (or explicitly
5+
// denied) by the allowScripts policy.
6+
//
7+
// Thin wrapper around arborist's shared `collectUnreviewedScripts`, mapping
8+
// the CLI's `({ arb, npm, tree })` shape onto the shared walk. Defaults to
9+
// `arb.actualTree` (post-reify) but accepts an explicit tree so callers can
10+
// pre-flight against the idealTree before scripts run.
711
//
812
// Returns an array of `{ node, scripts }` entries. `scripts` is an object
913
// describing the relevant lifecycle scripts that would run.
10-
11-
const checkAllowScripts = async ({ arb, npm, tree, includeWhenIgnored = false }) => {
12-
const ignoreScripts = !!arb.options?.ignoreScripts
13-
const dangerouslyAllowAll = !!npm?.flatOptions?.dangerouslyAllowAllScripts
14-
15-
// With ignore-scripts set, no scripts run, so execution callers
16-
// (install, rebuild, strict preflight) bail out here. approve/deny pass
17-
// includeWhenIgnored so they keep listing unreviewed packages, which is
18-
// what you need to move from a blanket ignore-scripts to an allowlist.
19-
// Listing never runs anything.
20-
if ((ignoreScripts && !includeWhenIgnored) || dangerouslyAllowAll) {
21-
return []
22-
}
23-
24-
// Defaults to actualTree (post-reify) but accepts an explicit tree so
25-
// callers can pre-flight against the idealTree before scripts run.
26-
const targetTree = tree || arb.actualTree
27-
if (!targetTree?.inventory) {
28-
return []
29-
}
30-
31-
const policy = arb.options?.allowScripts || null
32-
33-
const unreviewed = []
34-
for (const node of targetTree.inventory.values()) {
35-
if (node.isProjectRoot || node.isWorkspace) {
36-
continue
37-
}
38-
if (node.isLink) {
39-
// Linked workspace dependencies are managed by the workspace owner.
40-
continue
41-
}
42-
43-
const verdict = isScriptAllowed(node, policy)
44-
if (verdict === true || verdict === false) {
45-
continue
46-
}
47-
48-
const scripts = await getInstallScripts(node)
49-
if (Object.keys(scripts).length === 0) {
50-
continue
51-
}
52-
53-
unreviewed.push({ node, scripts })
54-
}
55-
56-
return unreviewed
57-
}
14+
//
15+
// `includeWhenIgnored` keeps listing unreviewed packages even when
16+
// ignore-scripts is set, so approve/deny can show what you'd move from a
17+
// blanket ignore-scripts to an allowlist. Execution callers leave it false.
18+
const checkAllowScripts = async ({ arb, npm, tree, includeWhenIgnored = false }) =>
19+
collectUnreviewedScripts({
20+
tree: tree || arb.actualTree,
21+
policy: arb.options?.allowScripts || null,
22+
ignoreScripts: !!arb.options?.ignoreScripts,
23+
dangerouslyAllowAllScripts: !!npm?.flatOptions?.dangerouslyAllowAllScripts,
24+
includeWhenIgnored,
25+
})
5826

5927
module.exports = checkAllowScripts

‎test/lib/commands/approve-scripts.js‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -397,10 +397,10 @@ t.test('approve-scripts --pending lists packages that only have binding.gyp', as
397397
t.match(out, /install: node-gyp rebuild/, 'synthetic node-gyp install is named')
398398
})
399399

400-
t.test('approve-scripts --all skips bundled deps with a notice', async t => {
401-
// Bundled deps cannot be allowlisted in Phase 1 (RFC defers their
402-
// allowlisting to a follow-up). --all must not silently write a key
403-
// derived from the bundled tarball's self-claimed identity.
400+
t.test('approve-scripts --all never approves bundled deps', async t => {
401+
// Bundled deps never run their install scripts and cannot be
402+
// allowlisted. They never reach the unreviewed list, so --all must not
403+
// write a key derived from the bundled tarball's self-claimed identity.
404404
const { npm, logs, prefix } = await _mockNpm(t, {
405405
prefixDir: {
406406
'package.json': JSON.stringify({
@@ -457,7 +457,8 @@ t.test('approve-scripts --all skips bundled deps with a notice', async t => {
457457
'non-bundled parent gets approved')
458458
t.notOk(Object.keys(pkg.allowScripts).some(k => k.startsWith('inner')),
459459
'bundled inner is not approved')
460-
t.match(logs.warn.byTitle('approve-scripts'), [/Skipping 1 bundled dependency/])
460+
t.strictSame(logs.warn.byTitle('approve-scripts'), [],
461+
'no warning; bundled deps are excluded upstream')
461462
})
462463

463464
t.test('approve-scripts <bundled-pkg> positional is ignored', async t => {
@@ -517,7 +518,7 @@ t.test('approve-scripts <bundled-pkg> positional is ignored', async t => {
517518
)
518519
})
519520

520-
t.test('approve-scripts --all with only bundled deps prints "no eligible" notice', async t => {
521+
t.test('approve-scripts --all with only bundled deps has nothing to review', async t => {
521522
const { npm, logs, joinedOutput, prefix } = await _mockNpm(t, {
522523
prefixDir: {
523524
'package.json': JSON.stringify({
@@ -566,8 +567,9 @@ t.test('approve-scripts --all with only bundled deps prints "no eligible" notice
566567
config: { all: true },
567568
})
568569
await npm.exec('approve-scripts', [])
569-
t.match(joinedOutput(), /No packages eligible for approval/)
570-
t.match(logs.warn.byTitle('approve-scripts'), [/Skipping 1 bundled dependency/])
570+
t.match(joinedOutput(), /No packages with unreviewed install scripts/)
571+
t.strictSame(logs.warn.byTitle('approve-scripts'), [],
572+
'no warning; bundled deps are excluded upstream')
571573
// Ensure no policy entry was written.
572574
const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8'))
573575
t.notOk(pkg.allowScripts, 'no allowScripts written')

‎test/lib/commands/rebuild.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,3 +306,49 @@ t.test('no advisory warning when allowScripts covers the package', async t => {
306306
await npm.exec('rebuild', [])
307307
t.strictSame(logs.warn.byTitle('rebuild'), [])
308308
})
309+
310+
t.test('rebuild <name> never targets a bundled dependency', async t => {
311+
const { npm, prefix: path } = await setupMockNpm(t, {
312+
prefixDir: {
313+
'package.json': JSON.stringify({
314+
name: 'host',
315+
version: '1.0.0',
316+
dependencies: { parent: '1.0.0' },
317+
}),
318+
node_modules: {
319+
parent: {
320+
'index.js': '',
321+
'package.json': JSON.stringify({
322+
name: 'parent',
323+
version: '1.0.0',
324+
bundleDependencies: ['bcrypt'],
325+
dependencies: { bcrypt: '1.0.0' },
326+
}),
327+
node_modules: {
328+
bcrypt: {
329+
'index.js': '',
330+
'package.json': JSON.stringify({
331+
name: 'bcrypt',
332+
version: '1.0.0',
333+
bin: 'index.js',
334+
scripts: {
335+
install: "node -e \"require('fs').writeFileSync('ran', '')\"",
336+
},
337+
}),
338+
},
339+
},
340+
},
341+
},
342+
},
343+
})
344+
345+
const ranFile = resolve(path, 'node_modules/parent/node_modules/bcrypt/ran')
346+
t.throws(() => fs.statSync(ranFile))
347+
348+
await npm.exec('rebuild', ['bcrypt'])
349+
350+
t.throws(
351+
() => fs.statSync(ranFile),
352+
'bundled bcrypt install script must not run'
353+
)
354+
})

‎test/lib/utils/check-allow-scripts.js‎

Lines changed: 5 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -149,46 +149,6 @@ t.test('prepare counts for non-registry sources only', async t => {
149149
t.equal(result[0].node.name, 'git-pkg')
150150
})
151151

152-
t.test('detects synthetic node-gyp via binding.gyp runtime check', async t => {
153-
const checkAllowScripts = mockCheck(t, {
154-
'@npmcli/arborist/lib/install-scripts.js': async (n) => {
155-
if (n.path === '/has-bindings') {
156-
return { install: 'node-gyp rebuild' }
157-
}
158-
return {}
159-
},
160-
})
161-
162-
const result = await checkAllowScripts({
163-
arb: arb({
164-
nodes: [
165-
node({ name: 'native', path: '/has-bindings' }),
166-
node({ name: 'pure-js', path: '/no-bindings' }),
167-
],
168-
}),
169-
npm: { flatOptions: {} },
170-
})
171-
t.equal(result.length, 1)
172-
t.equal(result[0].node.name, 'native')
173-
t.strictSame(result[0].scripts, { install: 'node-gyp rebuild' })
174-
})
175-
176-
t.test('skips node-gyp detection when gypfile is explicitly false', async t => {
177-
// Mock returns no scripts to simulate the gypfile:false short-circuit
178-
// inside getInstallScripts.
179-
const checkAllowScripts = mockCheck(t, {
180-
'@npmcli/arborist/lib/install-scripts.js': async () => ({}),
181-
})
182-
183-
const result = await checkAllowScripts({
184-
arb: arb({
185-
nodes: [node({ name: 'opt-out', gypfile: false })],
186-
}),
187-
npm: { flatOptions: {} },
188-
})
189-
t.strictSame(result, [])
190-
})
191-
192152
t.test('skips approved nodes', async t => {
193153
const checkAllowScripts = mockCheck(t)
194154
const result = await checkAllowScripts({
@@ -252,7 +212,7 @@ t.test('survives missing actualTree', async t => {
252212
t.strictSame(result, [])
253213
})
254214

255-
t.test('bundled dep with install scripts is reported as unreviewed regardless of policy', async t => {
215+
t.test('bundled dep with install scripts is never reported (never runs, never pending)', async t => {
256216
const checkAllowScripts = mockCheck(t)
257217
const bundled = node({
258218
name: 'bundled-pkg',
@@ -265,13 +225,12 @@ t.test('bundled dep with install scripts is reported as unreviewed regardless of
265225
const result = await checkAllowScripts({
266226
arb: arb({
267227
nodes: [bundled],
268-
// Policy explicitly allows the bundled name — the matcher should
269-
// still return null and the walker should still flag the bundled
270-
// dep as unreviewed.
228+
// Even with an explicit allow entry, a bundled dep never runs its
229+
// install scripts and is never counted as pending, so the walker
230+
// must not flag it.
271231
allowScripts: { 'bundled-pkg': true },
272232
}),
273233
npm: { flatOptions: {} },
274234
})
275-
t.equal(result.length, 1, 'bundled dep flagged despite explicit allow entry')
276-
t.equal(result[0].node, bundled)
235+
t.strictSame(result, [], 'bundled dep never flagged')
277236
})

‎workspaces/arborist/lib/arborist/isolated-reifier.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,10 @@ module.exports = cls => class IsolatedReifier extends cls {
3838
#processedEdges = new Set()
3939
#workspaceProxies = new Map()
4040

41-
#generateChild (node, location, pkg, isInStore, root) {
41+
#generateChild (node, location, pkg, isInStore, root, inBundle = false) {
4242
const newChild = new IsolatedNode({
4343
isInStore,
44+
inBundle,
4445
location,
4546
name: node.packageName || node.name,
4647
optional: node.optional,
@@ -327,7 +328,7 @@ module.exports = cls => class IsolatedReifier extends cls {
327328
})
328329

329330
bundledTree.nodes.forEach(node => {
330-
this.#generateChild(node, node.location, node.pkg, false, root)
331+
this.#generateChild(node, node.location, node.pkg, false, root, true)
331332
})
332333

333334
bundledTree.edges.forEach(edge => {

‎workspaces/arborist/lib/isolated-classes.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ class IsolatedNode {
1919
integrity = null
2020
inventory = new IsolatedInventory()
2121
isInStore = false
22+
inBundle = false
2223
linksIn = new Set()
2324
meta = { loadedFromDisk: false }
2425
optional = false
@@ -46,6 +47,9 @@ class IsolatedNode {
4647
if (options.isInStore) {
4748
this.isInStore = true
4849
}
50+
if (options.inBundle) {
51+
this.inBundle = true
52+
}
4953
if (options.optional) {
5054
this.optional = true
5155
}

‎workspaces/arborist/lib/script-allowed.js‎

Lines changed: 12 additions & 12 deletions

0 commit comments

Comments
 (0)