From e41c1d9f0ff5ab2359976276abe9f04b00a79901 Mon Sep 17 00:00:00 2001 From: Martin Ruiz Date: Mon, 3 Aug 2026 11:52:28 -0700 Subject: [PATCH] fix(install-scripts): align source approvals with RFC identities Use trusted resolver and lockfile identities for remote, git, file, and linked dependencies. Keep approval, denial, warnings, documentation, and policy matching consistent without treating canonical physical paths as equivalent identities. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9a58801a-1dff-40d8-b74c-b21545984d73 --- .../content/commands/npm-approve-scripts.md | 42 +- docs/lib/content/commands/npm-deny-scripts.md | 26 +- .../content/commands/npm-install-scripts.md | 56 +- lib/utils/allow-scripts-cmd.js | 176 +++++-- lib/utils/allow-scripts-writer.js | 162 ++++-- tap-snapshots/test/lib/docs.js.test.cjs | 6 +- test/lib/commands/approve-scripts.js | 31 +- test/lib/commands/install-scripts.js | 493 +++++++++++++++++- test/lib/utils/allow-scripts-writer.js | 267 +++++++++- workspaces/arborist/lib/script-allowed.js | 97 +++- workspaces/arborist/test/arborist/rebuild.js | 16 +- workspaces/arborist/test/arborist/reify.js | 2 +- workspaces/arborist/test/script-allowed.js | 202 ++++++- .../arborist/test/unreviewed-scripts.js | 25 +- .../config/lib/definitions/definitions.js | 7 +- 15 files changed, 1411 insertions(+), 197 deletions(-) diff --git a/docs/lib/content/commands/npm-approve-scripts.md b/docs/lib/content/commands/npm-approve-scripts.md index 55c892fbf96b1..7698b8554ef22 100644 --- a/docs/lib/content/commands/npm-approve-scripts.md +++ b/docs/lib/content/commands/npm-approve-scripts.md @@ -36,15 +36,44 @@ npm approve-scripts --all npm approve-scripts --allow-scripts-pending ``` -`` matches every installed version of that package. By default the -command writes pinned entries (`pkg@1.2.3`), which keep their approval -narrowed to the specific version you reviewed. Pass `--no-allow-scripts-pin` to write -name-only entries that allow any future version. - -`--all` approves every package with unreviewed install scripts in one go. +For registry dependencies, `` matches every installed version and +`` can narrow the selection. By default the command writes pinned +entries (`pkg@1.2.3`), which keep their approval narrowed to the specific +version you reviewed. Pass `--no-allow-scripts-pin` to write name-only entries +that allow any future version. + +Non-registry package names and versions are not trusted policy identities. +A bare installed name can select non-registry dependencies only when every +match shares one trust anchor. Different remote URLs, file sources, or hosted +repositories are separate trust anchors, while multiple commits from one +hosted repository share an anchor. If a name is ambiguous, approval fails and +lists exact source selectors. Re-run with one of those selectors, also shown +by `--allow-scripts-pending`. Version and range selectors do not select +non-registry dependencies. + +For a direct remote tarball, approval writes the exact `resolved` URL from +`package-lock.json`, not the package name inside the tarball. For example, +approving a dependency installed from `https://example.com/tool.tgz` writes +`"https://example.com/tool.tgz": true`. The URL remains exact even with +`--no-allow-scripts-pin`. + +For file tarballs and linked directories, approval writes the exact resolved +file spec from `package-lock.json` (for example, +`"file:../packages/logger": true`). File identities stay exact with +`--no-allow-scripts-pin`. + +For a hosted git dependency, approval writes the hosted repository shortcut +with the resolved committish (for example, `github:org/repo#abc1234`). +With `--no-allow-scripts-pin`, npm drops the committish and broadens approval +to the hosted repository (`github:org/repo`). + +`--all` explicitly approves every package with unreviewed install scripts, +including distinct sources that share an installed name. `--allow-scripts-pending` is read-only: it lists every package whose install scripts are not yet covered by `allowScripts`, without modifying `package.json`. +Non-registry entries show the installed name followed by the exact source +selector in brackets. `approve-scripts` honours the asymmetric pin rule: if you re-approve a package whose installed version has changed, the existing pin is rewritten @@ -85,6 +114,7 @@ npm approve-scripts --allow-scripts-pending ### See Also * [npm deny-scripts](/commands/npm-deny-scripts) +* [npm install-scripts](/commands/npm-install-scripts) * [npm install](/commands/npm-install) * [npm rebuild](/commands/npm-rebuild) * [package.json](/configuring-npm/package-json) diff --git a/docs/lib/content/commands/npm-deny-scripts.md b/docs/lib/content/commands/npm-deny-scripts.md index 4aee897891c1d..3704d4918fde6 100644 --- a/docs/lib/content/commands/npm-deny-scripts.md +++ b/docs/lib/content/commands/npm-deny-scripts.md @@ -25,16 +25,29 @@ npm deny-scripts [ ...] npm deny-scripts --all ``` -`` matches every installed version of that package. Denies are always -written name-only (`"pkg": false`), regardless of `--allow-scripts-pin`. Pinning a deny -to a specific version would silently re-allow scripts for any other version -of the same package, which defeats the purpose; the command picks the -safer default for you. +`` selects installed dependencies by their displayed package name. +Registry-package denials are written name-only (`"pkg": false`), regardless +of `--allow-scripts-pin`, so a future registry version does not silently +regain script permission. Direct remote tarballs are denied by their exact +`resolved` URL (for example, `"https://registry.example/pkg.tgz": false`), +file dependencies by their resolved file spec (for example, +`"file:../packages/logger": false`), and hosted git dependencies by the hosted +repository shortcut without a committish (for example, +`"github:org/repo": false`). Tarball-reported package names are never used +as policy identities. Linked directories use the exact resolved file spec +from `package-lock.json`. + +A bare installed name denies every matching non-registry source. To select +only one source when several share a name, pass its exact selector from +`npm install-scripts ls`. Remote and file denials stay exact; a selected +hosted-git dependency is denied at the repository level. Non-registry version +and range selectors are not supported because those versions come from the +dependency itself. `--all` denies every package with unreviewed install scripts. If a `true` (pinned or name-only) entry exists for a package and you then -deny it, the existing allow entries are removed so the name-only deny is +deny it, the existing allow entries are removed so the denial entry is unambiguous. ### Examples @@ -54,5 +67,6 @@ npm deny-scripts --all ### See Also * [npm approve-scripts](/commands/npm-approve-scripts) +* [npm install-scripts](/commands/npm-install-scripts) * [npm install](/commands/npm-install) * [package.json](/configuring-npm/package-json) diff --git a/docs/lib/content/commands/npm-install-scripts.md b/docs/lib/content/commands/npm-install-scripts.md index 32f05a8577039..e8cb00049ba72 100644 --- a/docs/lib/content/commands/npm-install-scripts.md +++ b/docs/lib/content/commands/npm-install-scripts.md @@ -39,20 +39,54 @@ npm install-scripts ls npm install-scripts prune ``` -`approve` allows install scripts for the named packages. `` matches -every installed version of that package. By default it writes pinned entries -(`pkg@1.2.3`), which keep their approval narrowed to the specific version you -reviewed. Pass `--no-allow-scripts-pin` to write name-only entries that allow -any future version. `--all` approves every package with unreviewed install -scripts in one go. - -`deny` records an explicit denial for the named packages (a name-only `false` -entry), which survives `npm install-scripts approve --all` and excludes the -package from any future blanket approval. `--all` denies every package with +`approve` allows install scripts for the named packages. For registry +dependencies, `` matches every installed version and `` can +narrow the selection. By default it writes pinned entries (`pkg@1.2.3`), +which keep their approval narrowed to the specific version you reviewed. +Pass `--no-allow-scripts-pin` to write name-only entries that allow any +future version. + +Non-registry package names and versions come from the installed dependency +and are not trusted policy identities. A bare installed name can select +non-registry dependencies only when every match shares one trust anchor. +Different remote URLs, file sources, or hosted repositories are separate +trust anchors, while multiple commits from one hosted repository share an +anchor. If a name is ambiguous, approval fails and lists exact source +selectors. Re-run with one shown by `ls` or the error. Version and range +selectors do not select non-registry dependencies. + +`--all` explicitly approves every package with unreviewed install scripts, +including distinct sources that share an installed name. + +For direct remote tarballs and file dependencies, npm records the trusted +source identity instead of the package name reported by the dependency. +Direct remote tarballs use the exact `resolved` URL from `package-lock.json`, +while file tarballs and linked directories use their exact resolved file +spec. These identities remain exact with +`--no-allow-scripts-pin`, because there is no trusted package-name form to +broaden them to. + +For hosted git dependencies, approval writes the hosted repository shortcut +with the resolved committish (for example, `github:org/repo#abc1234`). +With `--no-allow-scripts-pin`, npm drops the committish and broadens approval +to the hosted repository (`github:org/repo`). + +`deny` records an explicit denial that survives +`npm install-scripts approve --all` and excludes the dependency from any +future blanket approval. Registry dependencies use a name-only `false` entry, +direct remote tarballs use the exact resolved URL, file dependencies use the +resolved file spec, and hosted git dependencies use the hosted repository +shortcut without a committish. These denial identities are independent of +`--allow-scripts-pin`. A bare installed name denies every matching +non-registry source. To select one source when several share a name, pass its +exact selector. Remote and file denials stay exact; hosted-git dependencies +are denied at the repository level. `--all` denies every package with unreviewed install scripts. `ls` is read-only: it lists every package whose install scripts are not yet -covered by `allowScripts`, without modifying `package.json`. +covered by `allowScripts`, without modifying `package.json`. Non-registry +entries show the installed name followed by the exact source selector in +brackets. `prune` removes `allowScripts` entries that no longer match an installed package with an install script, either because the package is no longer diff --git a/lib/utils/allow-scripts-cmd.js b/lib/utils/allow-scripts-cmd.js index f07dc8d1dc504..ec8ef2e42be64 100644 --- a/lib/utils/allow-scripts-cmd.js +++ b/lib/utils/allow-scripts-cmd.js @@ -1,5 +1,4 @@ const { log, output } = require('proc-log') -const npa = require('npm-package-arg') const semver = require('semver') const pkgJson = require('@npmcli/package-json') const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js') @@ -7,35 +6,80 @@ const getInstallScripts = require('@npmcli/arborist/lib/install-scripts.js') const checkAllowScripts = require('./check-allow-scripts.js') const resolveAllowScripts = require('./resolve-allow-scripts.js') const { + parseSpec, applyApprovalForPackage, applyDenyForPackage, nameKeyFor, + versionedKeyFor, } = require('./allow-scripts-writer.js') const { classifyUnusedEntries } = require('./allow-scripts-prune.js') const BaseCommand = require('../base-cmd.js') -// Parse a positional arg into a name and an optional version range. A bare -// name matches every installed version; `pkg@1.2.3` or `pkg@^1` narrows by -// semver. npm-package-arg handles dotted and scoped names; fall back to the -// raw string as the name if it can't be parsed. -const parsePositional = (arg) => { - let parsed - try { - parsed = npa(arg) - } catch { - return { name: arg, range: null } +// Registry selectors may include a version range. Every other parsed spec is +// treated as an exact source selector; non-registry manifest versions are not +// trusted for positional matching. +const parseSelector = (arg) => { + const parsed = parseSpec(arg) + if (!parsed) { + return { + name: arg, + range: null, + exactSource: null, + allowNonRegistryName: false, + } } + const name = parsed.name || arg + const exactSource = (parsed.registry || parsed.type === 'alias') ? null : arg + const allowNonRegistryName = parsed.registry && parsed.raw === parsed.name if (parsed.type === 'version' || parsed.type === 'range') { const spec = parsed.fetchSpec const range = (!spec || spec === '*' || parsed.rawSpec === '' || parsed.rawSpec === '*') ? null : spec - return { name, range } + return { name, range, exactSource, allowNonRegistryName } + } + return { name, range: null, exactSource, allowNonRegistryName } +} + +const selectorDetails = (node) => { + const policyKey = nameKeyFor(node) + const versionedPolicyKey = versionedKeyFor(node) + if (parseSpec(policyKey)?.registry) { + return { + ...trustedDisplay(node), + exactSource: null, + displaySource: versionedPolicyKey || policyKey, + trustAnchor: `registry:${policyKey}`, + isRegistry: true, + policyKey, + } + } + + const exactSource = versionedPolicyKey + let trustAnchor = exactSource ? `source:${exactSource}` : null + if (parseSpec(exactSource)?.type === 'git' && policyKey) { + trustAnchor = `git:${policyKey}` + } + + return { + name: node.name || null, + version: null, + exactSource, + displaySource: exactSource || policyKey, + trustAnchor, + isRegistry: false, + policyKey, } - return { name, range: null } } +const isSelectableNode = (node) => + !node.isProjectRoot && + !node.isWorkspace && + !node.isLink && + !node.inBundle && + !node.inert + // Shared implementation for `npm approve-scripts`, `npm deny-scripts`, and the `npm install-scripts` namespace. // `npm install-scripts` dispatches to `runMode('approve' | 'deny' | 'list', ...)`. // The standalone commands set `static verb` and run through the default `exec`. @@ -145,14 +189,15 @@ class AllowScriptsCmd extends BaseCommand { `${count} ${pkg} ${has} install scripts blocked because they are not covered by allowScripts:` ) for (const { node, scripts } of unreviewed) { - const { name, version } = trustedDisplay(node) + const { name, version, exactSource, isRegistry } = selectorDetails(node) /* istanbul ignore next: every test node has a name */ const display = name || '' - const ver = version ? `@${version}` : '' + const versionSuffix = version ? `@${version}` : '' + const sourceSuffix = isRegistry ? '' : ` [${exactSource || 'untrusted source'}]` const events = Object.entries(scripts) .map(([event, cmd]) => `${event}: ${cmd}`) .join('; ') - output.standard(` ${display}${ver} (${events})`) + output.standard(` ${display}${versionSuffix}${sourceSuffix} (${events})`) } output.standard('') output.standard( @@ -162,15 +207,15 @@ class AllowScriptsCmd extends BaseCommand { } // Build the same `{ name, changes }` shape printSummary uses for writes, - // but tag every entry as `pending` since nothing is written. Names and - // versions are derived exactly like the text listing above. + // but tag every entry as `pending` since nothing is written. Selectors are + // derived exactly like the text listing above. pendingSummary (unreviewed) { const groups = new Map() for (const { node } of unreviewed) { - const { name, version } = trustedDisplay(node) + const { name, version, exactSource } = selectorDetails(node) /* istanbul ignore next: every test node has a name */ const display = name || '' - const key = version ? `${display}@${version}` : display + const key = exactSource || (version ? `${display}@${version}` : display) if (!groups.has(display)) { groups.set(display, []) } @@ -196,7 +241,17 @@ class AllowScriptsCmd extends BaseCommand { } async runPositional (args, arb) { - const { matched, unmatched } = this.findNodesForArgs(args, arb) + const { matched, unmatched, ambiguous } = this.findNodesForArgs(args, arb) + if (ambiguous.length > 0) { + const details = ambiguous.map(({ arg, sources }) => + `Package selector "${arg}" matches multiple sources:\n` + + sources.map(source => ` ${source}`).join('\n') + ).join('\n') + throw Object.assign( + new Error(`${details}\nRe-run with an exact source selector.`), + { code: 'EINSTALLSCRIPTSAMBIGUOUS' } + ) + } if (unmatched.length > 0) { throw Object.assign( new Error(`No installed packages match: ${unmatched.join(', ')}`), @@ -216,36 +271,73 @@ class AllowScriptsCmd extends BaseCommand { } findNodesForArgs (args, arb) { - // Match positional args against each node's trusted name. Registry deps - // use the URL-derived name; non-registry deps fall back to the dependency - // edge name. A version or range on the arg narrows the match to installed - // versions that satisfy it. Bundled deps are excluded for the same reason - // as --all. Args that match nothing are returned in `unmatched`. + // Registry dependencies match their trusted name and may be narrowed by + // version. Non-registry dependencies match their installed name, while an + // exact source selector matches their versioned policy identity. const matched = [] const unmatched = [] + const ambiguous = [] + const candidates = [...arb.actualTree.inventory.values()] + .filter(isSelectableNode) + .map((node, index) => { + const details = selectorDetails(node) + const location = node.location || node.path + return { + node, + ...details, + trustAnchor: details.trustAnchor || + `untrusted:${location || `${node.name || ''}:${index}`}`, + displaySource: details.displaySource || + location || + node.name || + '', + } + }) + for (const arg of args) { - const { name: wantName, range } = parsePositional(arg) - const found = [] - for (const node of arb.actualTree.inventory.values()) { - if (node.isProjectRoot || node.isWorkspace || node.inBundle) { - continue + const { + name: wantedName, + range, + exactSource, + allowNonRegistryName, + } = parseSelector(arg) + const found = candidates.filter(candidate => { + if (exactSource) { + return candidate.exactSource === exactSource } - const { name, version } = trustedDisplay(node) - if (!name || name !== wantName) { - continue + if (!candidate.name || candidate.name !== wantedName) { + return false } - if (range && (!version || !semver.satisfies(version, range, { loose: true }))) { - continue + if (!candidate.isRegistry && !allowNonRegistryName) { + return false } - found.push(node) - } + if (!range) { + return true + } + return candidate.isRegistry && + Boolean(candidate.version) && + semver.satisfies(candidate.version, range, { loose: true }) + }) + if (found.length === 0) { unmatched.push(arg) - } else { - matched.push(...found) + continue } + + if (this.verb === 'approve' && !exactSource) { + const trustAnchors = new Set(found.map(({ trustAnchor }) => trustAnchor)) + if (trustAnchors.size > 1) { + ambiguous.push({ + arg, + sources: [...new Set(found.map(({ displaySource }) => displaySource))], + }) + continue + } + } + + matched.push(...found.map(({ node }) => node)) } - return { matched, unmatched } + return { matched, unmatched, ambiguous } } get logTitle () { @@ -358,7 +450,7 @@ class AllowScriptsCmd extends BaseCommand { // "no longer has scripts". const nodes = [] for (const node of arb.actualTree.inventory.values()) { - if (node.isProjectRoot || node.isWorkspace || node.isLink || node.inBundle || node.inert) { + if (!isSelectableNode(node)) { continue } const scripts = await getInstallScripts(node) diff --git a/lib/utils/allow-scripts-writer.js b/lib/utils/allow-scripts-writer.js index 6964279f2f2e0..910ef94e663f0 100644 --- a/lib/utils/allow-scripts-writer.js +++ b/lib/utils/allow-scripts-writer.js @@ -1,7 +1,9 @@ const npa = require('npm-package-arg') const { log } = require('proc-log') const { + filePolicyIdentity, getTrustedRegistryIdentity, + matches, resolvedSourceSpecs, } = require('@npmcli/arborist/lib/script-allowed.js') @@ -12,36 +14,89 @@ const { // project's `allowScripts` field, depending on `--allow-scripts-pin` and the currently // installed versions. // -// Denying always writes `"": false`, regardless of `--allow-scripts-pin`, per the -// RFC's asymmetric-pin rule. +// Denying writes the widest trusted identity available: registry and hosted-git +// keys use their coarsest trusted identity; file and remote dependencies use +// the exact resolved source because there is no coarser trusted identity. +// `--allow-scripts-pin` does not affect denies. const primaryResolvedSource = (node) => resolvedSourceSpecs(node)[0] || '' +const parseSpec = (spec) => { + if (typeof spec !== 'string' || spec === '') { + return null + } + try { + return npa(spec) + } catch { + return null + } +} +const isRemoteSpec = (spec) => parseSpec(spec)?.type === 'remote' +const isRegistrySpec = (spec) => parseSpec(spec)?.registry === true + +const incomingEdges = function * (node, seen = new Set()) { + if (!node || seen.has(node)) { + return + } + seen.add(node) + + if (node.edgesIn && typeof node.edgesIn[Symbol.iterator] === 'function') { + yield * node.edgesIn + } + if (node.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') { + for (const link of node.linksIn) { + yield * incomingEdges(link, seen) + } + } +} +const hasIncomingSpec = (node, predicate) => { + for (const edge of incomingEdges(node)) { + if (predicate(edge?.spec)) { + return true + } + } + return false +} +const hasRemoteProvenance = (node) => hasIncomingSpec(node, isRemoteSpec) +const hasRegistryProvenance = (node) => + node?.isRegistryDependency === true || hasIncomingSpec(node, isRegistrySpec) + +// `undefined` means the node has no remote provenance. `null` means it does, +// but no exact resolved URL can be matched, so callers must fail closed. +const exactRemoteKeyFor = (node, resolved) => { + if (!hasRemoteProvenance(node)) { + return undefined + } + return isRemoteSpec(resolved) ? resolved : null +} // Convert an arborist Node into the spec string used for a versioned policy // entry. Returns `null` if the node cannot be represented as a versioned key -// derived from trusted sources (lockfile URL for registry, hosted shortcut -// for git, the resolved file path for local installs). Never falls back to -// `node.packageName` / `node.version`, which are tarball-controlled. +// derived from trusted sources (lockfile URL for registry, exact resolved URL +// for direct remote installs, hosted shortcut for git, exact lockfile source +// for file installs). Never falls back to `node.packageName` / `node.version`, +// which are tarball-controlled. const versionedKeyFor = (node) => { if (!node) { return null } const resolved = primaryResolvedSource(node) + const remoteKey = exactRemoteKeyFor(node, resolved) + if (remoteKey !== undefined) { + return remoteKey + } if (resolved.startsWith('git')) { - try { - const parsed = npa(resolved) - if (parsed.hosted) { - const committish = parsed.gitCommittish || parsed.hosted.committish - const base = parsed.hosted.shortcut({ noCommittish: true }) - return committish ? `${base}#${committish}` : base - } - } catch { - /* istanbul ignore next: npa already parsed this string in keyTargetsNode */ + const parsed = parseSpec(resolved) + if (!parsed?.hosted) { return null } - return null + const committish = parsed.gitCommittish || parsed.hosted.committish + const base = parsed.hosted.shortcut({ noCommittish: true }) + return committish ? `${base}#${committish}` : base } - if (/^https?:\/\//.test(resolved)) { + if (isRemoteSpec(resolved)) { + if (!hasRegistryProvenance(node)) { + return null + } const trusted = getTrustedRegistryIdentity(node) if (trusted && trusted.version) { return `${trusted.name}@${trusted.version}` @@ -56,9 +111,9 @@ const versionedKeyFor = (node) => { ) return null } - /* istanbul ignore next: 'file:' and '/' branches are each covered separately */ - if (resolved.startsWith('file:') || resolved.startsWith('/')) { - return resolved + const fileIdentity = filePolicyIdentity(node) + if (fileIdentity !== undefined) { + return fileIdentity } // No trusted source. Refuse to compose a key from attacker-controlled // `node.packageName` / `node.version`. @@ -66,28 +121,31 @@ const versionedKeyFor = (node) => { return null } -// Convert an arborist Node into the spec string used for a name-only policy -// entry. Same trust rules as versionedKeyFor — returns `null` rather than -// falling back to tarball-controlled fields. +// Convert an arborist Node into the spec string used for the widest-trusted +// policy entry. Same trust rules as versionedKeyFor — returns `null` rather +// than falling back to tarball-controlled fields. const nameKeyFor = (node) => { if (!node) { return null } const resolved = primaryResolvedSource(node) + const remoteKey = exactRemoteKeyFor(node, resolved) + if (remoteKey !== undefined) { + return remoteKey + } if (resolved.startsWith('git')) { - try { - const parsed = npa(resolved) - if (parsed.hosted) { - return parsed.hosted.shortcut({ noCommittish: true }) - } - } catch { - /* istanbul ignore next: npa already parsed this string in keyTargetsNode */ + const parsed = parseSpec(resolved) + if (!parsed?.hosted) { return null } - return null + return parsed.hosted.shortcut({ noCommittish: true }) } - if (resolved.startsWith('file:') || resolved.startsWith('/')) { - return resolved + const fileIdentity = filePolicyIdentity(node) + if (fileIdentity !== undefined) { + return fileIdentity + } + if (!hasRegistryProvenance(node)) { + return null } // Registry deps: only the URL-derived (or edges-derived, in the // omit-lockfile case) trusted name is acceptable. @@ -105,13 +163,11 @@ const isSingleVersionPin = (key) => { } // Build the warning string emitted when an existing deny entry blocks -// an approval. Per RFC, a name-only deny ("pkg": false) is widest and -// the only remediation is to remove the entry. A versioned deny -// ("pkg@1.2.3": false or a disjunction) blocks only specific versions; -// the user can either widen it via `npm install-scripts deny ` or -// remove it to approve the currently-installed version only. +// an approval. Only versioned or ranged registry denies can be widened with +// `npm install-scripts deny `; name-only registry denies and git, +// file, or remote denies must be removed to approve the current install. const denyWarning = (key, subject, name) => { - if (isNameOnlyKey(key)) { + if (!isVersionedRegistryKey(key)) { return `${key} is denied; remove the entry from allowScripts to approve ${subject}.` } /* istanbul ignore next: name fallback is defensive; callers pass nameKeyFor(sample) */ @@ -121,20 +177,20 @@ const denyWarning = (key, subject, name) => { `to approve ${subject}.` } -const isNameOnlyKey = (key) => { +const isVersionedRegistryKey = (key) => { try { const parsed = npa(key) - if (parsed.type === 'tag') { + if (parsed.type === 'version') { return true } - if (parsed.type === 'range') { - return parsed.fetchSpec === '*' - || parsed.rawSpec === '' - || parsed.rawSpec === '*' + if (parsed.type !== 'range') { + return false } - return false + return parsed.fetchSpec !== '*' + && parsed.rawSpec !== '' + && parsed.rawSpec !== '*' } catch { - /* istanbul ignore next: keys reaching this helper have already parsed via keyTargetsNode */ + /* istanbul ignore next: keys reaching denyWarning have already parsed in keyTargetsNode */ return false } } @@ -158,6 +214,9 @@ const keyTargetsNode = (key, node) => { case 'tag': case 'range': case 'version': { + if (node?.isRegistryDependency === false) { + return false + } const trusted = getTrustedRegistryIdentity(node) if (!trusted) { return false @@ -180,8 +239,7 @@ const keyTargetsNode = (key, node) => { case 'file': case 'directory': case 'remote': - return resolvedSourceSpecs(node) - .some(resolved => resolved === parsed.saveSpec || resolved === parsed.fetchSpec) + return matches(node, key, false) default: return false } @@ -321,7 +379,8 @@ const applyApprovalForPackage = (existing, nodes, { pin = true } = {}) => { return { allowScripts, changes } } -// Apply a deny for a single package. Always name-only; ignores `--allow-scripts-pin`. +// Apply a deny for a single package. Uses the source-appropriate trusted +// identity and ignores `--allow-scripts-pin`. const applyDenyForPackage = (existing, nodes) => { const allowScripts = { ...existing } const changes = [] @@ -336,8 +395,8 @@ const applyDenyForPackage = (existing, nodes) => { return { allowScripts, changes } } - // Drop any pinned allow entries for this package: the name-only deny - // overrides them anyway, and leaving them in place is confusing. + // Drop narrower allow entries for this dependency; the denial overrides + // them. for (const key of Object.keys(allowScripts)) { if (keyTargetsNode(key, sample) && key !== name) { delete allowScripts[key] @@ -353,6 +412,7 @@ const applyDenyForPackage = (existing, nodes) => { } module.exports = { + parseSpec, applyApprovalForPackage, applyDenyForPackage, versionedKeyFor, diff --git a/tap-snapshots/test/lib/docs.js.test.cjs b/tap-snapshots/test/lib/docs.js.test.cjs index dbfc88ab63abc..8569c9c2de1ba 100644 --- a/tap-snapshots/test/lib/docs.js.test.cjs +++ b/tap-snapshots/test/lib/docs.js.test.cjs @@ -354,8 +354,10 @@ List packages with install scripts that are not yet covered by the Write pinned (\`pkg@version\`) entries when approving install scripts. Set to \`false\` to write name-only entries that allow any version. Has no effect on -\`npm deny-scripts\`, which always writes name-only entries regardless of this -setting. +\`npm deny-scripts\`, which uses a source-appropriate trusted identity +regardless of this setting: a name-only key for registry packages, the exact +source for direct remote and file dependencies, and the hosted repository +shortcut without a committish for hosted git dependencies. diff --git a/test/lib/commands/approve-scripts.js b/test/lib/commands/approve-scripts.js index 449382d19e987..52f57b76baef8 100644 --- a/test/lib/commands/approve-scripts.js +++ b/test/lib/commands/approve-scripts.js @@ -449,7 +449,7 @@ const twoVersionFixture = { 'package.json': JSON.stringify({ name: 'host', version: '1.0.0', - dependencies: { 'top-of-tree': '*' }, + dependencies: { lodash: '4.17.21', 'top-of-tree': '*' }, }), 'package-lock.json': JSON.stringify({ name: 'host', @@ -457,7 +457,11 @@ const twoVersionFixture = { lockfileVersion: 3, requires: true, packages: { - '': { name: 'host', version: '1.0.0', dependencies: { 'top-of-tree': '*' } }, + '': { + name: 'host', + version: '1.0.0', + dependencies: { lodash: '4.17.21', 'top-of-tree': '*' }, + }, 'node_modules/lodash': { version: '4.17.21', resolved: 'https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz', @@ -484,7 +488,11 @@ const twoVersionFixture = { }), }, 'top-of-tree': { - 'package.json': JSON.stringify({ name: 'top-of-tree', version: '1.0.0' }), + 'package.json': JSON.stringify({ + name: 'top-of-tree', + version: '1.0.0', + dependencies: { lodash: '3.10.1' }, + }), node_modules: { lodash: { 'package.json': JSON.stringify({ @@ -553,9 +561,9 @@ t.test('approve-scripts pins only versions satisfying the range', as t.strictSame(pkg.allowScripts, { 'lodash@4.17.21': true }) }) -t.test('approve-scripts --pending handles node with no version', async t => { - // Exercise the ternary's falsy branch in runPending: `node.version ? '@'... : ''` - // when the node has no version field. +t.test('approve-scripts --pending handles node with no version or trusted source', async t => { + // A synthetic node without provenance must not expose its manifest version + // as a trusted selector. const mockSync = await _mockNpm(t, { prefixDir: { 'package.json': JSON.stringify({ name: 'host', version: '1.0.0' }), @@ -578,13 +586,16 @@ t.test('approve-scripts --pending handles node with no version', async t => { }, }) await mockSync.npm.exec('approve-scripts', []) - // Output should mention the package without an @version suffix. - t.match(mockSync.joinedOutput(), / no-version-pkg \(install: do-stuff\)/) + // The installed name remains visible, but the source is explicitly untrusted. + t.match( + mockSync.joinedOutput(), + / no-version-pkg \[untrusted source\] \(install: do-stuff\)/ + ) }) t.test('approve-scripts --pending --json handles node with no version', async t => { - // Exercise pendingSummary's `version ? ... : display` falsy branch: the - // key is the bare name when the node has no version field. + // Without a trusted source or registry version, JSON falls back to the + // installed name rather than composing a selector from manifest data. const { npm, joinedOutput } = await _mockNpm(t, { prefixDir: { 'package.json': JSON.stringify({ name: 'host', version: '1.0.0' }), diff --git a/test/lib/commands/install-scripts.js b/test/lib/commands/install-scripts.js index cf7063b595715..ee859ce354a2d 100644 --- a/test/lib/commands/install-scripts.js +++ b/test/lib/commands/install-scripts.js @@ -8,11 +8,27 @@ const mockNpm = async (t, opts = {}) => { return _mockNpm(t, opts) } -const setupProject = ({ allowScripts, withScripts = ['canvas'], noScripts = [] } = {}) => { +const remoteCypressUrl = + 'https://cdn.example.test/releases/cypress.tgz' +const registryShapedRemoteCypressUrl = + 'https://cdn.example.test/artifact/-/artifact-1.0.0.tgz' +const topLevelToolUrl = + 'https://good.example.test/releases/tool.tgz' +const nestedToolUrl = + 'https://evil.example.test/decoy/-/decoy-7.0.0.tgz' + +const setupProject = ({ + allowScripts, + withScripts = ['canvas'], + noScripts = [], + remoteUrls = {}, +} = {}) => { const pkg = { name: 'host', version: '1.0.0', - dependencies: Object.fromEntries([...withScripts, ...noScripts].map((n) => [n, '*'])), + dependencies: Object.fromEntries( + [...withScripts, ...noScripts].map((name) => [name, remoteUrls[name] ?? '*']) + ), } if (allowScripts !== undefined) { pkg.allowScripts = allowScripts @@ -31,7 +47,8 @@ const setupProject = ({ allowScripts, withScripts = ['canvas'], noScripts = [] } lockPackages[`node_modules/${name}`] = { version: '1.0.0', hasInstallScript: true, - resolved: `https://registry.npmjs.org/${name}/-/${name}-1.0.0.tgz`, + resolved: remoteUrls[name] ?? + `https://registry.npmjs.org/${name}/-/${name}-1.0.0.tgz`, } } for (const name of noScripts) { @@ -40,7 +57,8 @@ const setupProject = ({ allowScripts, withScripts = ['canvas'], noScripts = [] } } lockPackages[`node_modules/${name}`] = { version: '1.0.0', - resolved: `https://registry.npmjs.org/${name}/-/${name}-1.0.0.tgz`, + resolved: remoteUrls[name] ?? + `https://registry.npmjs.org/${name}/-/${name}-1.0.0.tgz`, } } @@ -57,6 +75,133 @@ const setupProject = ({ allowScripts, withScripts = ['canvas'], noScripts = [] } } } +const setupDistinctRemoteSourcesProject = () => { + const pkg = { + name: 'host', + version: '1.0.0', + dependencies: { + tool: topLevelToolUrl, + parent: '1.0.0', + }, + } + + return { + 'package.json': JSON.stringify(pkg, null, 2), + 'package-lock.json': JSON.stringify({ + name: pkg.name, + version: pkg.version, + lockfileVersion: 3, + requires: true, + packages: { + '': pkg, + 'node_modules/tool': { + version: '1.0.0', + hasInstallScript: true, + resolved: topLevelToolUrl, + }, + 'node_modules/parent': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/parent/-/parent-1.0.0.tgz', + dependencies: { tool: nestedToolUrl }, + }, + 'node_modules/parent/node_modules/tool': { + version: '1.0.0', + hasInstallScript: true, + resolved: nestedToolUrl, + }, + }, + }), + node_modules: { + tool: { + 'package.json': JSON.stringify({ + name: 'tool', + version: '1.0.0', + scripts: { install: 'echo install' }, + }), + }, + parent: { + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + dependencies: { tool: nestedToolUrl }, + }), + node_modules: { + tool: { + 'package.json': JSON.stringify({ + name: 'tool', + version: '1.0.0', + scripts: { install: 'echo install' }, + }), + }, + }, + }, + }, + } +} + +const toolSourceNode = (source, overrides = {}) => ({ + name: 'tool', + version: '1.0.0', + resolved: source, + isRegistryDependency: false, + edgesIn: new Set([{ name: 'tool', spec: source }]), + ...overrides, +}) + +const mockProjectWithInventory = (t, inventory) => { + const FakeArborist = function (options) { + this.options = options + this.actualTree = { inventory: new Map(Object.entries(inventory)) } + } + FakeArborist.prototype.loadActual = async () => {} + + return mockNpm(t, { + prefixDir: { + 'package.json': JSON.stringify({ name: 'host', version: '1.0.0' }), + }, + mocks: { + '@npmcli/arborist': FakeArborist, + '{LIB}/utils/check-allow-scripts.js': async () => [], + }, + }) +} + +const mixedRegistryRemoteInventory = () => ({ + registry: toolSourceNode('https://registry.npmjs.org/tool/-/tool-1.0.0.tgz', { + isRegistryDependency: true, + edgesIn: new Set([{ name: 'tool', spec: '1.0.0' }]), + }), + remote: toolSourceNode(topLevelToolUrl), +}) + +const linkedFileNode = (source, location) => { + const rootPath = resolve('project') + const linkPath = resolve(rootPath, location) + const targetPath = resolve(rootPath, source.slice('file:'.length)) + const root = { + path: rootPath, + meta: { + get: nodePath => nodePath === linkPath + ? { resolved: source, link: true } + : {}, + }, + } + return { + name: 'tool', + version: '1.0.0', + resolved: null, + isRegistryDependency: false, + path: targetPath, + realpath: targetPath, + root, + linksIn: new Set([{ + path: linkPath, + resolved: 'file:../../tool', + root, + }]), + } +} + t.test('completion', async t => { const comp = (argv) => InstallScripts.completion({ conf: { argv: { remain: argv } } }) @@ -81,6 +226,165 @@ t.test('install-scripts approve writes a pinned entry', async t => { t.strictSame(pkg.allowScripts, { 'canvas@1.0.0': true }) }) +t.test('install-scripts approve writes exact URL for a remote tarball', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: remoteCypressUrl }, + }), + }) + await npm.exec('install-scripts', ['approve', 'cypress']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [remoteCypressUrl]: true }) +}) + +t.test('install-scripts approve selects a remote tarball by installed name', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: registryShapedRemoteCypressUrl }, + }), + }) + await npm.exec('install-scripts', ['approve', 'cypress']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [registryShapedRemoteCypressUrl]: true }) +}) + +t.test('install-scripts approve rejects a name shared by distinct remote sources', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupDistinctRemoteSourcesProject(), + }) + + await t.rejects( + npm.exec('install-scripts', ['approve', 'tool']), + { + code: 'EINSTALLSCRIPTSAMBIGUOUS', + message: /tool.*multiple sources/i, + } + ) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.notOk('allowScripts' in pkg) +}) + +t.test('install-scripts does not select unnamed untrusted candidates', async t => { + const { npm, logs, clearLogs } = await mockProjectWithInventory(t, { + located: { + location: 'node_modules/located', + resolved: null, + }, + anonymous: { + resolved: null, + }, + }) + + for (const selector of ['node_modules/located', '']) { + clearLogs() + await t.rejects( + npm.exec('install-scripts', ['approve', selector]), + { code: 'ENOMATCH' } + ) + t.strictSame(logs.warn, [], `${selector} was not selected`) + } +}) + +t.test('install-scripts approve accepts an exact remote source selector', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupDistinctRemoteSourcesProject(), + }) + + await npm.exec('install-scripts', ['approve', topLevelToolUrl]) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [topLevelToolUrl]: true }) +}) + +t.test('install-scripts approve does not trust remote manifest versions as selectors', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: remoteCypressUrl }, + }), + }) + + await t.rejects( + npm.exec('install-scripts', ['approve', 'cypress@1.0.0']), + { code: 'ENOMATCH' } + ) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.notOk('allowScripts' in pkg) +}) + +t.test('install-scripts non-bare registry selectors cannot select remote dependencies', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: remoteCypressUrl }, + }), + }) + + for (const selector of [ + 'cypress@', + 'cypress@*', + 'cypress@latest', + 'cypress@npm:other@1.0.0', + ]) { + await t.rejects( + npm.exec('install-scripts', ['approve', selector]), + { code: 'ENOMATCH' }, + selector + ) + } + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.notOk('allowScripts' in pkg) +}) + +t.test('install-scripts deny blocks every exact source sharing an installed name', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupDistinctRemoteSourcesProject(), + }) + + await npm.exec('install-scripts', ['deny', 'tool']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { + [nestedToolUrl]: false, + [topLevelToolUrl]: false, + }) +}) + +t.test('install-scripts positional selection skips Link wrappers', async t => { + const target = toolSourceNode(topLevelToolUrl) + const link = { + name: 'tool', + version: '1.0.0', + resolved: 'file:.store/tool', + isLink: true, + isRegistryDependency: false, + } + const { npm, prefix } = await mockProjectWithInventory(t, { link, target }) + + await npm.exec('install-scripts', ['approve', 'tool']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [topLevelToolUrl]: true }) +}) + +t.test('install-scripts positional selection skips inert nodes', async t => { + const available = toolSourceNode(topLevelToolUrl) + const inert = toolSourceNode(nestedToolUrl, { inert: true }) + const { npm, prefix } = await mockProjectWithInventory(t, { available, inert }) + + await npm.exec('install-scripts', ['approve', 'tool']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [topLevelToolUrl]: true }) +}) + t.test('install-scripts approve --all approves every unreviewed package', async t => { const { npm, prefix } = await mockNpm(t, { prefixDir: setupProject({ withScripts: ['canvas', 'sharp'] }), @@ -95,6 +399,125 @@ t.test('install-scripts approve --all approves every unreviewed package', async }) }) +t.test('install-scripts approve --all writes exact URL for a remote tarball', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: remoteCypressUrl }, + }), + config: { all: true }, + }) + await npm.exec('install-scripts', ['approve']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [remoteCypressUrl]: true }) +}) + +t.test('install-scripts approve --all allows distinct same-name remote sources explicitly', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupDistinctRemoteSourcesProject(), + config: { all: true }, + }) + + await npm.exec('install-scripts', ['approve']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { + [nestedToolUrl]: true, + [topLevelToolUrl]: true, + }) +}) + +t.test('install-scripts approve allows multiple commits from one hosted repository', async t => { + const firstCommit = 'github:example/tool#deadbeef' + const secondCommit = 'github:example/tool#cafebabe' + const { npm, prefix } = await mockProjectWithInventory(t, { + firstCommit: toolSourceNode(firstCommit), + secondCommit: toolSourceNode(secondCommit), + }) + + await npm.exec('install-scripts', ['approve', 'tool']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [firstCommit]: true, [secondCommit]: true }) +}) + +t.test('install-scripts approve rejects same-name dependencies from different git repositories', async t => { + const firstRepositoryCommit = 'github:example/tool#deadbeef' + const secondRepositoryCommit = 'github:attacker/tool#cafebabe' + const { npm, prefix } = await mockProjectWithInventory(t, { + firstRepositoryCommit: toolSourceNode(firstRepositoryCommit), + secondRepositoryCommit: toolSourceNode(secondRepositoryCommit), + }) + + await t.rejects( + npm.exec('install-scripts', ['approve', 'tool']), + { code: 'EINSTALLSCRIPTSAMBIGUOUS' } + ) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.notOk('allowScripts' in pkg) +}) + +t.test('install-scripts ambiguity lists an exact selector for registry matches', async t => { + const { npm } = await mockProjectWithInventory(t, mixedRegistryRemoteInventory()) + + await t.rejects( + npm.exec('install-scripts', ['approve', 'tool']), + { + code: 'EINSTALLSCRIPTSAMBIGUOUS', + message: new RegExp( + `tool@1\\.0\\.0[\\s\\S]*${topLevelToolUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}` + ), + } + ) +}) + +t.test('install-scripts registry version selector disambiguates a remote name collision', async t => { + const { npm, prefix } = await mockProjectWithInventory(t, mixedRegistryRemoteInventory()) + + await npm.exec('install-scripts', ['approve', 'tool@1.0.0']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'tool@1.0.0': true }) +}) + +t.test('install-scripts approve rejects distinct linked file sources with one name', async t => { + const inventory = { + good: linkedFileNode('file:../good-tool', 'node_modules/tool'), + attacker: linkedFileNode( + 'file:../attacker-tool', + 'node_modules/parent/node_modules/tool' + ), + } + const { npm, prefix } = await mockProjectWithInventory(t, inventory) + + await t.rejects( + npm.exec('install-scripts', ['approve', 'tool']), + { code: 'EINSTALLSCRIPTSAMBIGUOUS' } + ) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.notOk('allowScripts' in pkg) +}) + +t.test('install-scripts exact resolver identity disambiguates linked targets', async t => { + const exactSource = 'file:../good-tool' + const inventory = { + good: linkedFileNode(exactSource, 'node_modules/tool'), + attacker: linkedFileNode( + 'file:../attacker-tool', + 'node_modules/parent/node_modules/tool' + ), + } + const { npm, prefix } = await mockProjectWithInventory(t, inventory) + + await npm.exec('install-scripts', ['approve', exactSource]) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [exactSource]: true }) +}) + t.test('install-scripts deny writes a name-only false entry', async t => { const { npm, prefix } = await mockNpm(t, { prefixDir: setupProject({ withScripts: ['canvas'] }), @@ -105,6 +528,19 @@ t.test('install-scripts deny writes a name-only false entry', async t => { t.strictSame(pkg.allowScripts, { canvas: false }) }) +t.test('install-scripts deny writes exact URL for a remote tarball', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: remoteCypressUrl }, + }), + }) + await npm.exec('install-scripts', ['deny', 'cypress']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [remoteCypressUrl]: false }) +}) + t.test('install-scripts deny --all denies every unreviewed package', async t => { const { npm, prefix } = await mockNpm(t, { prefixDir: setupProject({ withScripts: ['canvas', 'sharp'] }), @@ -116,6 +552,20 @@ t.test('install-scripts deny --all denies every unreviewed package', async t => t.strictSame(pkg.allowScripts, { canvas: false, sharp: false }) }) +t.test('install-scripts deny --all writes exact URL for a remote tarball', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: remoteCypressUrl }, + }), + config: { all: true }, + }) + await npm.exec('install-scripts', ['deny']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { [remoteCypressUrl]: false }) +}) + t.test('install-scripts ignores allow-scripts-pending and still writes', async t => { // The namespace exposes listing through `ls`, so a stray // `allow-scripts-pending` config must not divert approve into list mode. @@ -140,6 +590,41 @@ t.test('install-scripts ls lists unreviewed packages', async t => { t.match(out, /sharp@1\.0\.0/) }) +t.test('install-scripts ls shows installed name and exact source for remote tarballs', async t => { + const { npm, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: registryShapedRemoteCypressUrl }, + }), + }) + + await npm.exec('install-scripts', ['ls']) + + const out = joinedOutput() + t.match(out, /cypress/) + t.match(out, new RegExp(registryShapedRemoteCypressUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))) + t.notMatch(out, /artifact@1\.0\.0/) +}) + +t.test('install-scripts ls --json exposes exact remote source selectors', async t => { + const { npm, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['cypress'], + remoteUrls: { cypress: registryShapedRemoteCypressUrl }, + }), + config: { json: true }, + }) + + await npm.exec('install-scripts', ['ls']) + + t.strictSame(JSON.parse(joinedOutput()), { + allowScripts: [{ + name: 'cypress', + changes: [{ key: registryShapedRemoteCypressUrl, change: 'pending' }], + }], + }) +}) + t.test('install-scripts ls with no unreviewed says so', async t => { const { npm, joinedOutput } = await mockNpm(t, { prefixDir: setupProject({ allowScripts: { canvas: true }, withScripts: ['canvas'] }), diff --git a/test/lib/utils/allow-scripts-writer.js b/test/lib/utils/allow-scripts-writer.js index 8edf25be3079c..c5bfada8bc77f 100644 --- a/test/lib/utils/allow-scripts-writer.js +++ b/test/lib/utils/allow-scripts-writer.js @@ -1,5 +1,6 @@ const t = require('tap') const path = require('node:path') +const isScriptAllowed = require('@npmcli/arborist/lib/script-allowed.js') const { applyApprovalForPackage, applyDenyForPackage, @@ -24,6 +25,39 @@ const node = (overrides = {}) => { } } +const lockfileRoot = (rootPath, entries) => ({ + path: rootPath, + meta: { + get: nodePath => entries[nodePath] || {}, + }, +}) + +const registryShapedRemoteUrl = + // Registry-shaped so versionFromTgz can parse it; isRegistryDependency:false drives identity. + 'https://example.com/cypress/-/cypress-15.18.1.tgz' +const registryShapedRemoteNode = (url = registryShapedRemoteUrl) => ({ + ...node({ + name: 'cypress', + version: '15.18.1', + resolved: url, + isRegistryDependency: false, + }), + edgesIn: new Set([{ spec: url }]), +}) + +const linkedCypressNode = (...specs) => ({ + ...node({ + name: 'cypress', + version: '15.18.1', + resolved: registryShapedRemoteUrl, + isRegistryDependency: false, + }), + edgesIn: new Set(), + linksIn: new Set(specs.map(spec => ({ + edgesIn: new Set([{ name: 'cypress', spec }]), + }))), +}) + // A registry node with no `resolved` URL in the lockfile. Its trusted name // comes from a dependency edge, but its version isn't trustable, so // versionedKeyFor returns null (npm/cli#9558). @@ -47,6 +81,96 @@ t.test('nameKeyFor / versionedKeyFor — registry', async t => { t.equal(versionedKeyFor(n), 'canvas@2.11.0') }) +t.test('nameKeyFor / versionedKeyFor — remote tarball uses exact resolved URL', async t => { + for (const url of [ + registryShapedRemoteUrl, + 'HTTPS://example.com/cypress/-/cypress-15.18.1.tgz', + 'https:/example.com/cypress/-/cypress-15.18.1.tgz', + ]) { + const n = registryShapedRemoteNode(url) + t.equal(nameKeyFor(n), url) + t.equal(versionedKeyFor(n), url) + } +}) + +t.test('nameKeyFor / versionedKeyFor — registry tarball URL without remote edge uses registry identity', async t => { + const n = Object.assign(registryShapedRemoteNode(), { + edgesIn: new Set([{ spec: '^15.18.1' }]), + }) + t.equal(nameKeyFor(n), 'cypress') + t.equal(versionedKeyFor(n), 'cypress@15.18.1') +}) + +t.test('nameKeyFor / versionedKeyFor — linked remote target uses incoming Link provenance', async t => { + const n = linkedCypressNode(registryShapedRemoteUrl) + + t.equal(nameKeyFor(n), registryShapedRemoteUrl) + t.equal(versionedKeyFor(n), registryShapedRemoteUrl) + t.equal(isScriptAllowed(n, { [registryShapedRemoteUrl]: true }), true) +}) + +t.test('nameKeyFor / versionedKeyFor — incoming Link provenance skips cycles and null links', async t => { + const n = linkedCypressNode(registryShapedRemoteUrl) + const [link] = n.linksIn + const provenanceLink = { edgesIn: link.edgesIn } + // Put valid provenance last so traversal must pass the cycle and null entry. + link.edgesIn = new Set() + link.linksIn = new Set([n, null, provenanceLink]) + + t.equal(nameKeyFor(n), registryShapedRemoteUrl) + t.equal(versionedKeyFor(n), registryShapedRemoteUrl) +}) + +t.test('nameKeyFor / versionedKeyFor — linked registry target keeps registry identity', async t => { + const n = linkedCypressNode('^15.18.1') + + t.equal(nameKeyFor(n), 'cypress') + t.equal(versionedKeyFor(n), 'cypress@15.18.1') +}) + +t.test('nameKeyFor / versionedKeyFor — mixed linked provenance prefers exact remote identity', async t => { + const n = linkedCypressNode('^15.18.1', registryShapedRemoteUrl) + + t.equal(nameKeyFor(n), registryShapedRemoteUrl) + t.equal(versionedKeyFor(n), registryShapedRemoteUrl) +}) + +t.test('nameKeyFor / versionedKeyFor — remote provenance without exact resolved URL fails closed', async t => { + const n = linkedCypressNode(registryShapedRemoteUrl) + n.resolved = null + const [link] = n.linksIn + link.resolved = 'file:.store/cypress' + + t.equal(nameKeyFor(n), null) + t.equal(versionedKeyFor(n), null) +}) + +t.test('nameKeyFor / versionedKeyFor — registry-shaped URL without source provenance fails closed', async t => { + const n = linkedCypressNode() + + t.equal(nameKeyFor(n), null) + t.equal(versionedKeyFor(n), null) +}) + +t.test('nameKeyFor / versionedKeyFor — non-registry remote node with no remote edges is not direct-remote', async t => { + // isRegistryDependency:false and a remote resolved URL are not sufficient + // when none of the incoming edge specs is classified as remote. + // Null and malformed specs exercise isRemoteSpec's defensive paths. + const n = Object.assign(node({ + name: 'pkg', + isRegistryDependency: false, + resolved: 'https://cdn.example.com/releases/pkg.tgz', + }), { + edgesIn: new Set([ + { spec: null }, + { spec: 'https://' }, + { spec: 'file:../pkg' }, + ]), + }) + t.equal(nameKeyFor(n), null) + t.equal(versionedKeyFor(n), null) +}) + t.test('nameKeyFor / versionedKeyFor — git', async t => { const n = node({ name: 'bar', @@ -63,7 +187,13 @@ t.test('nameKeyFor / versionedKeyFor — file', async t => { }) t.test('nameKeyFor / versionedKeyFor — local directory link target', async t => { - const targetPath = path.resolve('local') + const rootPath = path.resolve('project') + const targetPath = path.resolve(rootPath, '../local') + const linkPath = path.resolve(rootPath, 'node_modules/local') + const targetKey = 'file:../local' + const root = lockfileRoot(rootPath, { + [linkPath]: { resolved: '../local', link: true }, + }) const n = { name: 'local', packageName: 'local', @@ -71,20 +201,65 @@ t.test('nameKeyFor / versionedKeyFor — local directory link target', async t = resolved: null, path: targetPath, realpath: targetPath, - linksIn: new Set([{ resolved: 'file:../local' }]), + root, + linksIn: new Set([{ + path: linkPath, + resolved: 'file:../../local', + root, + }]), } - t.equal(nameKeyFor(n), 'file:../local') - t.equal(versionedKeyFor(n), 'file:../local') + t.equal(nameKeyFor(n), targetKey) + t.equal(versionedKeyFor(n), targetKey) t.strictSame( applyApprovalForPackage({}, [n], { pin: true }).allowScripts, - { 'file:../local': true } - ) - t.match( - applyApprovalForPackage({ 'file:local': false }, [n], { pin: true }).warning, - /denied|versioned deny/ - ) + { [targetKey]: true } + ) + const blocked = applyApprovalForPackage({ [targetKey]: false }, [n], { pin: true }) + t.strictSame(blocked.allowScripts, { [targetKey]: false }) + t.match(blocked.warning, /denied|versioned deny/) +}) + +t.test('nameKeyFor / versionedKeyFor — linked file targets use exact resolver identities', async t => { + const rootPath = path.resolve('project') + const targets = ['good-tool', 'attacker-tool'].map(name => { + const targetPath = path.resolve(rootPath, '..', name) + const linkPath = path.resolve(rootPath, `node_modules/${name}`) + const root = lockfileRoot(rootPath, { + [linkPath]: { resolved: `../${name}`, link: true }, + }) + return { + name: 'tool', + packageName: 'tool', + version: '1.0.0', + resolved: null, + path: targetPath, + realpath: targetPath, + root, + linksIn: new Set([{ + path: linkPath, + resolved: `file:../../${name}`, + root, + }]), + } + }) + + const keys = targets.map(nameKeyFor) + t.strictSame(keys, ['file:../good-tool', 'file:../attacker-tool']) + t.not(keys[0], keys[1]) + t.strictSame(targets.map(versionedKeyFor), keys) +}) + +t.test('nameKeyFor / versionedKeyFor — unresolved non-file link targets fail closed', async t => { + const n = linkedCypressNode(registryShapedRemoteUrl) + n.resolved = null + const [link] = n.linksIn + link.resolved = registryShapedRemoteUrl + + t.equal(nameKeyFor(n), null) + t.equal(versionedKeyFor(n), null) + t.equal(isScriptAllowed(n, { [registryShapedRemoteUrl]: true }), null) }) t.test('nameKeyFor / versionedKeyFor — empty link target has no portable file key', async t => { @@ -132,6 +307,40 @@ t.test('applyApprovalForPackage — empty allowScripts, --no-pin', async t => { t.strictSame(changes, [{ key: 'canvas', change: 'added' }]) }) +t.test('applyApprovalForPackage — remote tarball writes exact URL with --pin', async t => { + const { allowScripts, changes } = applyApprovalForPackage( + {}, + [registryShapedRemoteNode()], + { pin: true } + ) + t.strictSame(allowScripts, { [registryShapedRemoteUrl]: true }) + t.strictSame(changes, [{ key: registryShapedRemoteUrl, change: 'added' }]) +}) + +t.test('applyApprovalForPackage — remote tarball ignores registry-only deny for same name/version', async t => { + const { allowScripts, changes, warning } = applyApprovalForPackage( + { 'cypress@15.18.1': false }, + [registryShapedRemoteNode()], + { pin: true } + ) + t.strictSame(allowScripts, { + 'cypress@15.18.1': false, + [registryShapedRemoteUrl]: true, + }) + t.strictSame(changes, [{ key: registryShapedRemoteUrl, change: 'added' }]) + t.equal(warning, undefined) +}) + +t.test('applyApprovalForPackage — remote tarball writes exact URL with --no-pin', async t => { + const { allowScripts, changes } = applyApprovalForPackage( + {}, + [registryShapedRemoteNode()], + { pin: false } + ) + t.strictSame(allowScripts, { [registryShapedRemoteUrl]: true }) + t.strictSame(changes, [{ key: registryShapedRemoteUrl, change: 'added' }]) +}) + t.test('applyApprovalForPackage — stale pin rewritten to new installed version', async t => { const { allowScripts, changes } = applyApprovalForPackage( { 'canvas@2.10.0': true }, @@ -307,6 +516,15 @@ t.test('applyDenyForPackage — empty allowScripts adds name-only false', async t.strictSame(changes, [{ key: 'core-js', change: 'added' }]) }) +t.test('applyDenyForPackage — remote tarball writes exact URL', async t => { + const { allowScripts, changes } = applyDenyForPackage( + {}, + [registryShapedRemoteNode()] + ) + t.strictSame(allowScripts, { [registryShapedRemoteUrl]: false }) + t.strictSame(changes, [{ key: registryShapedRemoteUrl, change: 'added' }]) +}) + t.test('applyDenyForPackage — pinned allow is replaced by name-only deny', async t => { const { allowScripts } = applyDenyForPackage( { 'core-js@3.0.0': true }, @@ -493,14 +711,17 @@ t.test('applyApprovalForPackage — file dep with deny entry blocks approval', a t.match(warning, /denied|versioned deny/) }) -t.test('applyApprovalForPackage — remote tarball deny blocks approval', async t => { - const remote = { name: 'pkg', packageName: 'pkg', version: '1.0.0', resolved: 'https://example.com/pkg.tgz' } - const { warning } = applyApprovalForPackage( - { 'https://example.com/pkg.tgz': false }, - [remote], +t.test('applyApprovalForPackage — remote tarball deny requires removal-only guidance', async t => { + const { allowScripts, changes, warning } = applyApprovalForPackage( + { [registryShapedRemoteUrl]: false }, + [registryShapedRemoteNode()], { pin: true } ) - t.match(warning, /denied|versioned deny/) + t.strictSame(allowScripts, { [registryShapedRemoteUrl]: false }) + t.strictSame(changes, []) + t.match(warning, /remove the entry/) + t.notMatch(warning, /versioned deny/) + t.notMatch(warning, /widen the deny/) }) t.test('applyApprovalForPackage — no-pin with no name produces no-op', async t => { @@ -611,10 +832,7 @@ t.test('keyTargetsNode handles file-type tarball key matching saveSpec', async t t.equal(allowScripts['file:pkg.tgz'], false) }) -t.test('keyTargetsNode handles file-type tarball key matching fetchSpec', async t => { - // When node.resolved is an absolute path matching parsed.fetchSpec. - // Use path.resolve so the absolute path is platform-correct (npa - // parses POSIX-style `/abs/...` as a directory on Windows). +t.test('keyTargetsNode does not match path-equivalent file keys', async t => { const absTgz = path.resolve('pkg.tgz') const tarballNode = { name: 'pkg', @@ -628,7 +846,8 @@ t.test('keyTargetsNode handles file-type tarball key matching fetchSpec', async { pin: true } ) t.equal(allowScripts['./pkg.tgz'], false) - t.match(warning, /denied|versioned deny/) + t.equal(allowScripts[absTgz], true) + t.equal(warning, undefined) }) t.test('versionedKeyFor — git node without committish', async t => { @@ -751,9 +970,11 @@ t.test('versionedKeyFor — registry resolved that versionFromTgz cannot parse r // breadcrumb path in versionedKeyFor, including each fallback branch // of the `node.path || node.name || ''` label expression. const resolved = 'https://private-mirror.example.com/blobs/abc123' - t.equal(versionedKeyFor({ + const nodeWithPath = { path: '/fake/mystery', name: 'mystery', resolved, isRegistryDependency: true, - }), null, 'falls back when node has a path') + } + t.equal(nameKeyFor(nodeWithPath), null, 'does not fall back to the manifest name') + t.equal(versionedKeyFor(nodeWithPath), null, 'falls back when node has a path') t.equal(versionedKeyFor({ name: 'mystery', resolved, isRegistryDependency: true, }), null, 'falls back when node has only a name') diff --git a/workspaces/arborist/lib/script-allowed.js b/workspaces/arborist/lib/script-allowed.js index 8c9b3fe118a8e..9592e5240a000 100644 --- a/workspaces/arborist/lib/script-allowed.js +++ b/workspaces/arborist/lib/script-allowed.js @@ -19,7 +19,8 @@ const versionFromTgz = require('./version-from-tgz.js') // come from the tarball's own package.json and are therefore // attacker-controlled. A package can publish a tarball claiming any // name; the only trusted name is the one baked into the registry URL. -// - tarball / file / link / remote: exact match on node.resolved +// - tarball / file / link / remote: exact resolver identity from +// package-lock.json // - git: match on hosted.ssh() plus a short-SHA prefix of the // resolved committish @@ -100,38 +101,78 @@ const matches = (node, key, failClosed) => { } const resolvedSourceSpecs = (node) => { - const specs = [] - const seen = new Set() - const add = (spec) => { - if (typeof spec !== 'string' || spec === '' || seen.has(spec)) { + const resolved = node?.resolved + return typeof resolved === 'string' && resolved !== '' ? [resolved] : [] +} + +const isFileSpec = (spec, where) => { + /* istanbul ignore if: filePolicyIdentities filters these values before calling */ + if (typeof spec !== 'string' || spec === '') { + return false + } + try { + const parsed = npa(spec, where) + return parsed.type === 'file' || parsed.type === 'directory' + } catch { + return false + } +} + +const lockfileEntry = (node) => { + const meta = node?.root?.meta + if (!node?.path || typeof meta?.get !== 'function') { + return undefined + } + return meta.get(node.path) +} + +// Return exact file identities from package-lock metadata. Arborist's +// Link.resolved is synthesized relative to node_modules, so Link targets must +// read each incoming Link's lockfile entry instead. +const filePolicyIdentities = (node) => { + const identities = new Set() + const where = node?.root?.path || process.cwd() + const add = (spec, link = false) => { + if (typeof spec !== 'string' || spec === '') { return } - seen.add(spec) - specs.push(spec) + const identity = link && !spec.startsWith('file:') ? `file:${spec}` : spec + if (isFileSpec(identity, where)) { + identities.add(identity) + } } - add(node?.resolved) + const ownEntry = lockfileEntry(node) + if (ownEntry?.resolved !== undefined) { + add(ownEntry.resolved, ownEntry.link) + } else if (!node?.isLink) { + // Plain fixtures and callers without Arborist lockfile metadata still + // carry the resolver identity directly on ordinary Nodes. + add(node?.resolved) + } - if (!node?.resolved && node?.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') { - let hasIncomingLink = false + if (!node?.resolved && node?.linksIn && + typeof node.linksIn[Symbol.iterator] === 'function') { for (const link of node.linksIn) { - hasIncomingLink = true - add(link.resolved) - } - - if (hasIncomingLink) { - // Link targets for local directory deps are separate inventory nodes - // whose own `resolved` is null. The incoming Link carries the saved spec - // (for example `file:../pkg`, relative to node_modules), while policy - // entries written by hand often use the dependency spec from package.json - // (for example `file:pkg`, resolved by npa to this target path). Include - // the real target paths so both forms can match the same local dep. - add(node.realpath) - add(node.path) + const entry = lockfileEntry(link) + add(entry?.resolved, entry?.link) } } - return specs + return [...identities] +} + +// `undefined` means no file identity exists. `null` means multiple exact +// resolver identities reach one target, so writers must not choose one. +const filePolicyIdentity = (node) => { + const identities = filePolicyIdentities(node) + if (identities.length === 0) { + return undefined + } + if (identities.length === 1) { + return identities[0] + } + return null } const matchRegistry = (node, parsed, failClosed) => { @@ -327,10 +368,8 @@ const matchGit = (node, parsed) => { return nodeCommittish.startsWith(keyCommittish) } -const matchFileOrDir = (node, parsed) => { - return resolvedSourceSpecs(node) - .some(resolved => resolved === parsed.saveSpec || resolved === parsed.fetchSpec) -} +const matchFileOrDir = (node, parsed) => + filePolicyIdentities(node).includes(parsed.raw) const matchRemote = (node, parsed) => { return resolvedSourceSpecs(node) @@ -379,6 +418,8 @@ module.exports = isScriptAllowed module.exports.isScriptAllowed = isScriptAllowed module.exports.matches = matches module.exports.isExactVersionDisjunction = isExactVersionDisjunction +module.exports.filePolicyIdentities = filePolicyIdentities +module.exports.filePolicyIdentity = filePolicyIdentity module.exports.getTrustedRegistryIdentity = getTrustedRegistryIdentity module.exports.resolvedSourceSpecs = resolvedSourceSpecs module.exports.trustedDisplay = trustedDisplay diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index 6c3062be89a00..e4be4cccd4a34 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -182,7 +182,7 @@ t.test('allowScripts gates local file: dep scripts (npm/cli#9498)', async t => { const path = fixture(t, 'link-dep-lifecycle-scripts') const arb = newArb({ path, - allowScripts: { 'file:../a': true }, + allowScripts: { 'file:a': true }, dangerouslyAllowAllScripts: false, }) await arb.rebuild() @@ -194,7 +194,19 @@ t.test('allowScripts gates local file: dep scripts (npm/cli#9498)', async t => { const path = fixture(t, 'link-dep-lifecycle-scripts') const arb = newArb({ path, - allowScripts: { 'file:../a': false }, + allowScripts: { 'file:a': false }, + dangerouslyAllowAllScripts: false, + }) + await arb.rebuild() + t.throws(() => fs.statSync(aPrepare(path)), 'prepare did not run') + t.throws(() => fs.statSync(aPostinstall(path)), 'postinstall did not run') + }) + + t.test('path-equivalent allow entry does not authorize the target', async t => { + const path = fixture(t, 'link-dep-lifecycle-scripts') + const arb = newArb({ + path, + allowScripts: { 'file:../a': true }, dangerouslyAllowAllScripts: false, }) await arb.rebuild() diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index 943e340b22c1f..6b66da83b8d9a 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -2148,7 +2148,7 @@ console.log('ok 1 - this is fine') t.test('running lifecycle scripts of unchanged link nodes on reify', async t => { const path = fixture(t, 'link-dep-lifecycle-scripts') createRegistry(t, false) - t.matchSnapshot(await printReified(path, { allowScripts: { 'file:../a': true } }), 'result') + t.matchSnapshot(await printReified(path, { allowScripts: { 'file:a': true } }), 'result') t.ok(fs.lstatSync(resolve(path, 'a/a-prepare')).isFile(), 'should run prepare lifecycle scripts for links directly linked to the tree') diff --git a/workspaces/arborist/test/script-allowed.js b/workspaces/arborist/test/script-allowed.js index 218ccf1e28888..8456704216dfa 100644 --- a/workspaces/arborist/test/script-allowed.js +++ b/workspaces/arborist/test/script-allowed.js @@ -1,6 +1,12 @@ const t = require('tap') +const path = require('node:path') +const npa = require('npm-package-arg') const isScriptAllowed = require('../lib/script-allowed.js') -const { trustedDisplay } = isScriptAllowed +const { + filePolicyIdentities, + filePolicyIdentity, + trustedDisplay, +} = isScriptAllowed // Test nodes default to a consistent registry-tarball shape: the resolved // URL's name+version match the supplied name+version. Tests that need to @@ -24,6 +30,13 @@ const node = (overrides = {}) => { } } +const lockfileRoot = (rootPath, entries) => ({ + path: rootPath, + meta: { + get: nodePath => entries[nodePath] || {}, + }, +}) + t.test('returns null when no policy is set', t => { t.equal(isScriptAllowed(node(), null), null) t.equal(isScriptAllowed(node(), undefined), null) @@ -118,8 +131,72 @@ t.test('file path — exact resolved match', t => { t.end() }) -t.test('file path — link target matches incoming link source', t => { - const targetPath = require('node:path').resolve('local-pkg') +t.test('file path — ordinary nodes prefer exact lockfile identities', t => { + const rootPath = path.resolve('project') + const nodePath = path.resolve(rootPath, 'node_modules/local-pkg') + const root = lockfileRoot(rootPath, { + [nodePath]: { resolved: 'file:../local-pkg' }, + }) + const fileNode = node({ + name: 'local-pkg', + packageName: 'local-pkg', + version: '1.0.0', + resolved: npa(path.resolve(rootPath, '../local-pkg')).saveSpec, + path: nodePath, + root, + }) + + t.strictSame(filePolicyIdentities(fileNode), ['file:../local-pkg']) + t.equal(isScriptAllowed(fileNode, { 'file:../local-pkg': true }), true) + t.equal(isScriptAllowed(fileNode, { [fileNode.resolved]: true }), null) + t.end() +}) + +t.test('file path — unparseable lockfile identities fail closed', t => { + const rootPath = path.resolve('project') + const nodePath = path.resolve(rootPath, 'node_modules/local-pkg') + const root = lockfileRoot(rootPath, { + [nodePath]: { resolved: 'not valid' }, + }) + const fileNode = node({ + name: 'local-pkg', + packageName: 'local-pkg', + version: '1.0.0', + path: nodePath, + root, + }) + + t.strictSame(filePolicyIdentities(fileNode), []) + t.equal(filePolicyIdentity(fileNode), undefined) + t.equal(isScriptAllowed(fileNode, { 'file:../local-pkg': true }), null) + t.end() +}) + +t.test('file path — link nodes do not use transformed runtime identities', t => { + const rootPath = path.resolve('project') + const linkNode = { + isLink: true, + path: path.resolve(rootPath, 'node_modules/local-pkg'), + resolved: npa(path.resolve(rootPath, '../local-pkg')).saveSpec, + root: lockfileRoot(rootPath, {}), + } + + t.strictSame(filePolicyIdentities(linkNode), []) + t.end() +}) + +t.test('file path — link target matches exact lockfile source', t => { + const rootPath = path.resolve('project') + const targetPath = path.resolve(rootPath, '../local-pkg') + const linkPath = path.resolve(rootPath, 'node_modules/local-pkg') + const root = lockfileRoot(rootPath, { + [linkPath]: { resolved: '../local-pkg', link: true }, + }) + const link = { + path: linkPath, + resolved: 'file:../../local-pkg', + root, + } const target = node({ name: 'local-pkg', packageName: 'local-pkg', @@ -128,15 +205,109 @@ t.test('file path — link target matches incoming link source', t => { target.resolved = null target.path = targetPath target.realpath = targetPath - target.linksIn = new Set([{ resolved: 'file:../local-pkg' }]) + target.root = root + target.linksIn = new Set([link]) + t.equal(filePolicyIdentity(target), 'file:../local-pkg') t.equal(isScriptAllowed(target, { 'file:../local-pkg': true }), true) - t.equal(isScriptAllowed(target, { 'file:local-pkg': true }), true) + t.equal(isScriptAllowed(target, { 'file:local-pkg': true }), null) + t.equal(isScriptAllowed(target, { [npa(targetPath).saveSpec]: true }), null) t.equal(isScriptAllowed(target, { 'file:../local-pkg': false }), false) t.equal(isScriptAllowed(target, { 'file:../other': true }), null) t.end() }) +t.test('file path — distinct lockfile sources are not conflated by target paths', t => { + const rootPath = path.resolve('project') + const linkedTarget = (name, resolved) => { + const targetPath = path.resolve(rootPath, '..', name) + const linkPath = path.resolve(rootPath, `node_modules/${name}`) + const root = lockfileRoot(rootPath, { + [linkPath]: { resolved, link: true }, + }) + const target = node({ + name: 'tool', + packageName: 'tool', + version: '1.0.0', + }) + target.resolved = null + target.path = targetPath + target.realpath = targetPath + target.root = root + target.linksIn = new Set([{ + path: linkPath, + resolved: `file:../../${name}`, + root, + }]) + return target + } + + const approved = linkedTarget('good-tool', '../good-tool') + const attacker = linkedTarget('attacker-tool', '../attacker-tool') + const policy = { 'file:../good-tool': true } + t.equal(isScriptAllowed(approved, policy), true) + t.equal(isScriptAllowed(attacker, policy), null) + t.end() +}) + +t.test('file path — unresolved non-file link targets fail closed', t => { + const remoteUrl = 'https://example.com/remote-pkg.tgz' + const targetPath = path.resolve('remote-pkg') + const target = node({ + name: 'remote-pkg', + packageName: 'remote-pkg', + version: '1.0.0', + resolved: null, + path: targetPath, + realpath: targetPath, + linksIn: new Set([{ + path: `${targetPath}-link`, + root: { + path: path.dirname(targetPath), + meta: { get: () => ({ resolved: remoteUrl }) }, + }, + }]), + }) + + t.equal(filePolicyIdentity(target), undefined) + t.equal(isScriptAllowed(target, { [npa(targetPath).saveSpec]: true }), null) + t.equal(isScriptAllowed(target, { [remoteUrl]: true }), null) + t.end() +}) + +t.test('file path — multiple exact link identities are matchable but not writable', t => { + const rootPath = path.resolve('project') + const targetPath = path.resolve(rootPath, '../shared') + const firstLinkPath = path.resolve(rootPath, 'first') + const secondLinkPath = path.resolve(rootPath, 'second') + const root = lockfileRoot(rootPath, { + [firstLinkPath]: { resolved: '../shared', link: true }, + [secondLinkPath]: { resolved: 'vendor/shared', link: true }, + }) + const target = node({ + name: 'shared', + packageName: 'shared', + version: '1.0.0', + resolved: null, + path: targetPath, + realpath: targetPath, + root, + linksIn: new Set([ + { path: firstLinkPath, root }, + { path: secondLinkPath, root }, + ]), + }) + + t.strictSame(filePolicyIdentities(target), [ + 'file:../shared', + 'file:vendor/shared', + ]) + t.equal(filePolicyIdentity(target), null) + t.equal(isScriptAllowed(target, { 'file:../shared': true }), true) + t.equal(isScriptAllowed(target, { 'file:vendor/shared': false }), false) + t.end() +}) + t.test('file path — registry nodes do not match by install path', t => { const reg = node({ name: 'sharp', @@ -163,11 +334,32 @@ t.test('file path — empty link sets do not add install paths', t => { target.realpath = targetPath target.linksIn = new Set() + t.equal(filePolicyIdentity(target), undefined) t.equal(isScriptAllowed(target, { 'file:local-pkg': true }), null) t.equal(isScriptAllowed(target, { [targetPath]: true }), null) t.end() }) +t.test('file path — incomplete link metadata does not create an identity', t => { + const rootPath = path.resolve('project') + const linkPath = path.resolve(rootPath, 'node_modules/local-pkg') + const root = lockfileRoot(rootPath, { + [linkPath]: { link: true }, + }) + const target = node({ + name: 'local-pkg', + packageName: 'local-pkg', + resolved: null, + path: path.resolve(rootPath, '../local-pkg'), + root, + linksIn: new Set([{ path: linkPath, root }]), + }) + + t.strictSame(filePolicyIdentities(target), []) + t.equal(filePolicyIdentity(target), undefined) + t.end() +}) + t.test('directory key — npa parses absolute paths as type=directory', t => { // npa treats absolute paths as { type: 'directory' }, which the // matcher shares with the 'file' case. path.resolve produces a diff --git a/workspaces/arborist/test/unreviewed-scripts.js b/workspaces/arborist/test/unreviewed-scripts.js index 34e4878b4625a..905cf2b0c8cd1 100644 --- a/workspaces/arborist/test/unreviewed-scripts.js +++ b/workspaces/arborist/test/unreviewed-scripts.js @@ -1,4 +1,5 @@ const t = require('tap') +const { resolve } = require('node:path') const { collectUnreviewedScripts, strictAllowScriptsError, @@ -129,22 +130,38 @@ t.test('collectUnreviewedScripts', async t => { }) t.test('skips reviewed local directory link targets', async t => { + const rootPath = resolve('project') + const linkPath = resolve(rootPath, 'node_modules/local') + const root = { + path: rootPath, + meta: { + get: nodePath => nodePath === linkPath + ? { resolved: '../local', link: true } + : {}, + }, + } const target = node({ name: 'local', scripts: { install: 'x' } }) target.resolved = null target.isRegistryDependency = false - target.path = require('node:path').resolve('local') + target.path = resolve(rootPath, '../local') target.realpath = target.path - target.linksIn = new Set([{ resolved: 'file:../local' }]) + target.root = root + target.linksIn = new Set([{ + path: linkPath, + resolved: 'file:../../local', + root, + }]) t.strictSame(await collectUnreviewedScripts({ tree: tree([target]), policy: { 'file:../local': false }, }), []) - t.strictSame(await collectUnreviewedScripts({ + const unreviewed = await collectUnreviewedScripts({ tree: tree([target]), policy: { 'file:local': true }, - }), []) + }) + t.equal(unreviewed.length, 1) }) t.test('detects synthetic node-gyp via binding.gyp runtime check', async t => { diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js index f932d8f48103c..e80489182fcc4 100644 --- a/workspaces/config/lib/definitions/definitions.js +++ b/workspaces/config/lib/definitions/definitions.js @@ -1915,8 +1915,11 @@ const definitions = { description: ` Write pinned (\`pkg@version\`) entries when approving install scripts. Set to \`false\` to write name-only entries that allow any version. - Has no effect on \`npm deny-scripts\`, which always writes name-only - entries regardless of this setting. + Has no effect on \`npm deny-scripts\`, which uses a source-appropriate + trusted identity regardless of this setting: a name-only key for + registry packages, the exact source for direct remote and file + dependencies, and the hosted repository shortcut without a committish + for hosted git dependencies. `, flatten, }),