Skip to content

fix(arborist): apply allowScripts gate to scripts pacote runs at extract - #9777

Open
ubeddulla wants to merge 2 commits into
npm:latestfrom
ubeddulla:allow-scripts-extract-gate
Open

ubeddulla wants to merge 2 commits into
npm:latestfrom
ubeddulla:allow-scripts-extract-gate

Conversation

@ubeddulla

Copy link
Copy Markdown
Contributor

pacote runs a git or directory dep's prepare script while extracting it, which happens before the node ever reaches the rebuild queues where the allowScripts gate is applied. With allowScripts: {} a git dependency's prepare script still runs on install, and npm prints its "install scripts blocked because they are not covered by allowScripts" warning naming that same package while the script is running, so the policy reads as enforced when it is not. Both of pacote's fetchers already honor ignoreScripts, so deriving it from the same check rebuild uses closes this at the two pacote.extract call sites (reify, and the bundle-dep crack-open under --package-lock-only); approved packages and --dangerously-allow-all-scripts still run prepare exactly as before.

@ubeddulla

Copy link
Copy Markdown
Contributor Author

any update on this?

@martinrrm

Copy link
Copy Markdown
Contributor

@ubeddulla Thanks for working on this. The implementation is aligned with npm/rfcs#868: the RFC explicitly includes prepare for Git and local non-registry dependencies in the allowScripts policy.

The production change also appears correct: passing the resolved policy through ignoreScripts closes the gap where pacote could execute prepare before Arborist reached its normal rebuild gate. --ignore-scripts still takes precedence, and explicitly approved dependencies plus --dangerously-allow-all-scripts remain enabled.

Before merging, we need stronger regression coverage for the behavior this PR is fixing.

Current test does not reproduce the bug

The new test in workspaces/arborist/test/arborist/reify.js installs:

dependencies: { abbrev: '1.1.1' }

abbrev is a registry dependency. Registry extraction does not use pacote's Git/directory preparation path, so the test currently proves only that Arborist passes an ignoreScripts boolean to pacote.extract.

It does not prove that:

  • an unapproved Git or directory dependency's prepare script is actually blocked;
  • an explicitly approved dependency's prepare script still runs;
  • --dangerously-allow-all-scripts preserves existing behavior;
  • --ignore-scripts still wins;
  • the package-lock-only extraction path changed in build-ideal-tree.js is covered.

The test could pass even if pacote ignored the option or if the relevant source dependency path remained vulnerable.

Requested regression coverage

Please replace or supplement the abbrev option-spy test with a real non-registry dependency fixture.
A useful fixture would have a prepare script with an observable, cross-platform side effect, for example:

{
  "name": "prepare-fixture",
  "version": "1.0.0",
  "scripts": {
    "prepare": "node prepare.js"
  }
}
// prepare.js
require('node:fs').writeFileSync('prepare-ran', '')

Please use Node for the side effect rather than shell redirection so that the test works on Windows.
The test should exercise the actual pacote preparation path and verify the filesystem result, rather than only inspecting the option passed to pacote.extract.

Required cases

  1. Unreviewed dependency is blocked
    Install the Git or directory dependency with:

    {
      allowScripts: {},
      dangerouslyAllowAllScripts: false,
    }

    Assert that prepare-ran was not created.

  2. Explicitly approved dependency is allowed
    Install the same dependency with a matching trusted allowScripts key.
    Assert that prepare-ran was created.
    For Git dependencies, use the resolved Git identity expected by script-allowed.js, rather than the package's self-reported name. For a directory dependency, use the matching file: identity.

  3. The global escape hatch remains compatible
    Install with:

    {
      dangerouslyAllowAllScripts: true,
    }

    Assert that prepare-ran was created.

  4. ignoreScripts still takes precedence
    Install with:

    {
      ignoreScripts: true,
      dangerouslyAllowAllScripts: true,
    }

    Assert that prepare-ran was not created.

A local fixture or mocked Git transport is preferable; the test must not contact GitHub or another live network service.

Cover the second production call site

This PR also modifies the pacote.extract call in:

workspaces/arborist/lib/arborist/build-ideal-tree.js

That path is used when Arborist builds a complete ideal tree and cracks open a dependency containing bundled dependencies, including package-lock-only operations. The current test only exercises the extraction call in reify.js.

Please add targeted coverage in:

workspaces/arborist/test/arborist/build-ideal-tree.js

At minimum, verify that this extraction receives:

ignoreScripts: true

for an unreviewed dependency, and:

ignoreScripts: false

for an explicitly approved dependency or when dangerouslyAllowAllScripts is enabled.

If practical, use the same observable prepare fixture so the test covers behavior rather than only option plumbing.

Acceptance criteria

The updated tests should fail against the PR's parent commit for the intended reason: the unapproved source dependency executes prepare. They should then pass with this PR by demonstrating that:

  • unreviewed Git/directory preparation is blocked;
  • explicit approval still permits preparation;
  • dangerouslyAllowAllScripts still permits preparation;
  • ignoreScripts always wins;
  • both modified pacote.extract call sites are covered.

We investigated the broader approve-after-install recovery behavior separately and found that Git dependencies may require re-extraction rather than a normal rebuild. That is substantially larger follow-up work and is not being requested as part of this PR. This PR should remain focused on closing the extraction-time policy bypass and adding accurate regression tests for that fix.

@martinrrm martinrrm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tests are not correct and we should add more tests

@ubeddulla

Copy link
Copy Markdown
Contributor Author

Updated the tests.

In reify.js the option-spy is replaced with a real non-registry fixture: a directory dependency whose prepare writes a prepare-ran marker with node, installed via installLinks so it's actually extracted rather than symlinked. The assertions check that file, not the option:

  • unreviewed (allowScripts: {}): prepare doesn't run
  • explicitly approved (matched on the resolved file: identity): prepare runs
  • dangerouslyAllowAllScripts: prepare runs
  • ignoreScripts: prepare doesn't run

I used a directory dep rather than git so the fixture stays local and cross-platform, and it exercises DirFetcher#prepareDir, one of the two prepare paths pacote runs. The unreviewed case fails against the parent commit for the intended reason (prepare runs) and passes here.

For the second call site I added coverage in build-ideal-tree.js. It spies the crack-open pacote.extract under complete: true and asserts it receives ignoreScripts: true for an unreviewed dep and false under dangerouslyAllowAllScripts (and true when ignoreScripts is set). That path only fires for bundle deps, so it's the option check you described as the minimum there.

Both the reify and build-ideal-tree suites are green and lint is clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants