Skip to content

Commit e671a8b

Browse files
committed
feat(install): walk tree and emit advisory warning for unreviewed install scripts
The Phase 1 advisory warning. No scripts are blocked. After arb.reify() completes, reify-finish walks the actual tree for dependencies whose install-relevant lifecycle scripts are not yet covered by the allowScripts policy. The result is appended to the install output as one grouped block, not one log line per package. - workspaces/arborist/lib/install-scripts.js: per-node helper that returns the install-relevant lifecycle scripts. Covers preinstall, install, postinstall, prepare (non-registry sources only), and the synthetic 'node-gyp rebuild' detected by isNodeGypPackage from @npmcli/node-gyp. The runtime fs check is needed because the lockfile's hasInstallScript field misses packages whose only install-time work is binding.gyp. - lib/utils/check-allow-scripts.js: walks arb.actualTree.inventory and filters to unreviewed nodes. Honours --ignore-scripts and --dangerously-allow-all-scripts as full opt-outs. Treats explicit deny entries as reviewed (no warning). - lib/utils/reify-finish.js: runs the walker and passes results to reify-output as an extras payload. - lib/utils/reify-output.js: prints the grouped summary after the funding and audit messages. JSON output puts the same data on summary.unreviewedScripts. Refs: npm/rfcs#868
1 parent efe4b9e commit e671a8b

6 files changed

Lines changed: 589 additions & 3 deletions

File tree

lib/utils/check-allow-scripts.js

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
const isScriptAllowed = require('@npmcli/arborist/lib/script-allowed.js')
2+
const getInstallScripts = require('@npmcli/arborist/lib/install-scripts.js')
3+
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.
7+
//
8+
// Returns an array of `{ node, scripts }` entries. `scripts` is an object
9+
// describing the relevant lifecycle scripts that would run.
10+
11+
const checkAllowScripts = async ({ arb, npm }) => {
12+
const ignoreScripts = !!arb.options?.ignoreScripts
13+
const dangerouslyAllowAll = !!npm?.flatOptions?.dangerouslyAllowAllScripts
14+
15+
if (ignoreScripts || dangerouslyAllowAll) {
16+
return []
17+
}
18+
19+
const tree = arb.actualTree
20+
if (!tree?.inventory) {
21+
return []
22+
}
23+
24+
const policy = arb.options?.allowScripts || null
25+
26+
const unreviewed = []
27+
for (const node of tree.inventory.values()) {
28+
if (node.isProjectRoot || node.isWorkspace) {
29+
continue
30+
}
31+
if (node.isLink) {
32+
// Linked workspace dependencies are managed by the workspace owner.
33+
continue
34+
}
35+
36+
const verdict = isScriptAllowed(node, policy)
37+
if (verdict === true || verdict === false) {
38+
continue
39+
}
40+
41+
const scripts = await getInstallScripts(node)
42+
if (Object.keys(scripts).length === 0) {
43+
continue
44+
}
45+
46+
unreviewed.push({ node, scripts })
47+
}
48+
49+
return unreviewed
50+
}
51+
52+
module.exports = checkAllowScripts

lib/utils/reify-finish.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const reifyOutput = require('./reify-output.js')
2+
const checkAllowScripts = require('./check-allow-scripts.js')
23
const ini = require('ini')
34
const { writeFile } = require('node:fs/promises')
45
const { resolve } = require('node:path')
@@ -15,7 +16,8 @@ const reifyFinish = async (npm, arb) => {
1516
}
1617
}
1718
}
18-
reifyOutput(npm, arb)
19+
const unreviewedScripts = await checkAllowScripts({ arb, npm })
20+
reifyOutput(npm, arb, { unreviewedScripts })
1921
}
2022

2123
module.exports = reifyFinish

lib/utils/reify-output.js

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,25 @@ const { depth } = require('treeverse')
1414
const ms = require('ms')
1515
const npmAuditReport = require('npm-audit-report')
1616
const { readTree: getFundingInfo } = require('libnpmfund')
17+
const { getTrustedRegistryIdentity } = require('@npmcli/arborist/lib/script-allowed.js')
1718
const auditError = require('./audit-error.js')
1819

19-
// TODO: output JSON if flatOptions.json is true
20-
const reifyOutput = (npm, arb) => {
20+
// Trusted display identity for the install-script advisory. Same idea as
21+
// the matcher: prefer the URL-derived name. For DISPLAY purposes only
22+
// (not policy matching), version falls back to node.version when the
23+
// URL doesn't carry one — the user still benefits from seeing what the
24+
// tarball claims to be, even when we cannot trust it for matching.
25+
const trustedDisplay = (node) => {
26+
const trusted = getTrustedRegistryIdentity(node)
27+
/* istanbul ignore next: defensive fallbacks for nodes without name/version */
28+
return {
29+
name: (trusted && trusted.name) || node.name || null,
30+
version: (trusted && trusted.version) || node.version || null,
31+
}
32+
}
33+
const reifyOutput = (npm, arb, extras = {}) => {
2134
const { diff, actualTree } = arb
35+
const unreviewedScripts = extras.unreviewedScripts || []
2236

2337
// note: fails and crashes if we're running audit fix and there was an error which is a good thing, because there's no point printing all this other stuff in that case!
2438
const auditReport = auditError(npm, arb.auditReport) ? null : arb.auditReport
@@ -113,11 +127,23 @@ const reifyOutput = (npm, arb) => {
113127
summary.audit = npm.command === 'audit' ? auditReport
114128
: auditReport.toJSON().metadata
115129
}
130+
if (unreviewedScripts.length) {
131+
summary.unreviewedScripts = unreviewedScripts.map(({ node, scripts }) => {
132+
const { name, version } = trustedDisplay(node)
133+
return {
134+
name,
135+
version,
136+
path: node.path,
137+
scripts,
138+
}
139+
})
140+
}
116141
output.buffer(summary)
117142
} else {
118143
packagesChangedMessage(npm, summary)
119144
packagesFundingMessage(npm, summary)
120145
printAuditReport(npm, auditReport)
146+
unreviewedScriptsMessage(npm, unreviewedScripts)
121147
}
122148
}
123149

@@ -217,4 +243,29 @@ const packagesFundingMessage = (npm, { funding }) => {
217243
output.standard(' run `npm fund` for details')
218244
}
219245

246+
const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
247+
if (!unreviewedScripts.length) {
248+
return
249+
}
250+
251+
output.standard()
252+
const count = unreviewedScripts.length
253+
const pkg = count === 1 ? 'package has' : 'packages have'
254+
output.standard(`${count} ${pkg} install scripts not yet covered by allowScripts:`)
255+
256+
for (const { node, scripts } of unreviewedScripts) {
257+
const { name, version } = trustedDisplay(node)
258+
/* istanbul ignore next: every test node has a name */
259+
const display = name || '<unknown>'
260+
const ver = version ? `@${version}` : ''
261+
const events = Object.entries(scripts)
262+
.map(([event, cmd]) => `${event}: ${cmd}`)
263+
.join('; ')
264+
output.standard(` ${display}${ver} (${events})`)
265+
}
266+
267+
output.standard()
268+
output.standard('Run `npm approve-scripts --pending` to review, or `npm approve-scripts <pkg>` to allow.')
269+
}
270+
220271
module.exports = reifyOutput

0 commit comments

Comments
 (0)