Skip to content

feat: Phase 1 of allowScripts opt-in install-script policy#9360

Merged
owlstronaut merged 11 commits into
npm:latestfrom
JamieMagee:jamiemagee/install-scripts-phase-1
May 27, 2026
Merged

feat: Phase 1 of allowScripts opt-in install-script policy#9360
owlstronaut merged 11 commits into
npm:latestfrom
JamieMagee:jamiemagee/install-scripts-phase-1

Conversation

@JamieMagee
Copy link
Copy Markdown
Contributor

Implements Phase 1 of npm/rfcs#868, which makes dependency install scripts opt-in.

Install behaviour is unchanged. Scripts still run as they always have. The only Phase 1 user-visible change is one advisory block at the end of npm install listing packages whose install scripts haven't been reviewed via the new allowScripts field in package.json. A future release will turn that advisory into an actual block.

What landed

  • allowScripts field in package.json, read at install time
  • Three new configs: allow-scripts, strict-script-builds, dangerously-allow-all-scripts. The latter two are no-ops in this release. They're registered so projects can pin them in tooling ahead of the release that flips the default.
  • npm approve-scripts and npm deny-scripts commands, with the RFC's asymmetric pin rule (approves can pin, denies are always name-only)
  • Advisory warning during npm install, ci, update, and rebuild. npm exec / npx consult only the user/global .npmrc layer per the RFC, with the policy threaded through libnpmexec for Phase 2 enforcement.
  • Identity matcher in @npmcli/arborist covering registry, git, file, and remote tarballs. Registry identity is derived from the lockfile's resolved URL (via versionFromTgz), never from node.packageName or node.version. Those getters read the installed tarball's package.json and can be forged.
  • Aliases match against the underlying registered package, not the alias name. trusted@npm:naughty@1.0.0 is approved by writing naughty, not trusted. Holds even under omitLockfileRegistryResolved, where the install location alone (node_modules/trusted) would be misleading. The underlying name is derived from the incoming edge's alias subSpec.
  • Bundled deps with install scripts are flagged as unreviewed and filtered out of npm approve-scripts --all and positional matches. Per RFC they cannot be allowlisted in Phase 1.
  • Warning when a non-root workspace declares its own allowScripts

What's deliberately deferred

  • Actual blocking. The matcher exists and the policy is threaded through to arborist, but arb.rebuild()'s build set still runs everything. Phase 2 will gate #addToBuildSet on the matcher.
  • A safe allowlist syntax for bundled deps. The RFC notes a candidate parent@1.2.3 > bundled-name form for a follow-up.

Refs: npm/rfcs#868

@JamieMagee JamieMagee requested review from a team as code owners May 15, 2026 03:38
@JamieMagee JamieMagee force-pushed the jamiemagee/install-scripts-phase-1 branch 6 times, most recently from 3bb254c to e4270f0 Compare May 15, 2026 22:13
@bakkot
Copy link
Copy Markdown
Contributor

bakkot commented May 15, 2026

I think it would be bad to support an allowScripts field which can contain "foo": false but still allow foo's scripts to run. Similarly I think it would be bad to include a no-op strict-script-builds. That looks like it provides protection, but doesn't.

Also, it's weird to print this advisory information without it being actionable (except by silencing the warning).

I think a better phasing would be to implement the full RFC except that the default for packages not explicitly listed is to allow with warning, rather than to deny with warning, and then a later release can make the default to deny with warning and make no other changes.

If you really don't want to implement blocking in the first phase, it would be better for it to be a hard error to have "foo": false in allowScripts.

Three new configs: allow-scripts, strict-script-builds, dangerously-allow-all-scripts.

Why is the --allow-scripts CLI flag necessary? I feel like that's just going to encourage people to say "install this with npm install foo --allow-scripts=foo" and then it will work on their machine but not for the next person who tries to build the package.

Also, sidebar, the naming of strict-script-builds and dangerously-allow-all-scripts is weird. I would expect those to be named similarly, or possibly be the same option (--enforce-allow-scripts=on vs --enforce-allow-scripts=dangerously-off, maybe).

@JamieMagee
Copy link
Copy Markdown
Contributor Author

@bakkot Yeah, all of that sounds good. I'll switch to your proposed phasing.

Concretely:

  • Actual enforcement goes in. true allows, false blocks, both do what the user expects.
  • Phase 1 default for unlisted packages stays "allow with warning", which keeps the RFC's promise that nothing changes for anyone who hasn't opted in. Phase 2 is then the default flip.
  • Renaming --strict-script-builds to --strict-allow-scripts. --dangerously-allow-all-scripts keeps its name. I'd rather have a long, ugly, obviously discouraged flag than collapse it into a ternary.
  • Going to tighten the --allow-scripts description too, calling out that it's for one-off / npx use and that team-wide policy belongs in package.json or .npmrc. Doesn't remove the footgun but at least doesn't lean into it.

The reason I shipped advisory-only originally was to keep the "install behaviour unchanged" promise in the RFC. Your phasing keeps that for everyone who doesn't write a false entry, and anyone who does is opting in deliberately. Better than what I had.

Going to amend the RFC for the new phasing and update the PR description here.

@bakkot
Copy link
Copy Markdown
Contributor

bakkot commented May 17, 2026

SGTM.

Renaming --strict-script-builds to --strict-allow-scripts. --dangerously-allow-all-scripts keeps its name. I'd rather have a long, ugly, obviously discouraged flag than collapse it into a ternary.

I don't feel strongly about this point, but fwiw --enforce-allow-scripts=dangerously-off is in fact longer and more ugly than --dangerously-allow-all-scripts, and is at least as obviously discouraged to my eye.

That said, it's true that there are two dangerous things (the [new] phase 1 default, and disabled entirely), and naming both would be a little awkward. Best I've got is --enforce-allow-scripts=dangerously-allow-missing and --enforce-allow-scripts=dangerously-off respectively.

Comment thread lib/utils/allow-scripts-cmd.js
Comment thread workspaces/config/lib/definitions/definitions.js
Comment thread workspaces/config/lib/definitions/definitions.js
Comment thread workspaces/arborist/lib/install-scripts.js Outdated
Comment thread lib/utils/allow-scripts-writer.js Outdated
Comment thread workspaces/arborist/lib/script-allowed.js
Comment thread lib/utils/reify-output.js
Comment thread lib/utils/allow-scripts-writer.js
Comment thread lib/commands/approve-scripts.js Outdated
Comment thread lib/utils/reify-output.js Outdated
Comment thread lib/utils/resolve-allow-scripts.js Outdated
Comment thread workspaces/arborist/lib/script-allowed.js
Comment thread workspaces/arborist/lib/script-allowed.js
…low-all-scripts configs

Three new configs to support the install-script opt-in policy. None
of them affect install behaviour yet; they're read by approve-scripts,
deny-scripts, and the install-time walker in later commits.

  - allow-scripts: comma-separated package list. Used as a fallback
    when the root package.json has no allowScripts field. Flattens
    to flatOptions.allowScripts.
  - strict-script-builds: boolean. Reserved for a future release that
    will turn blocked-script warnings into errors. No-op for now.
  - dangerously-allow-all-scripts: boolean escape hatch for that same
    future release. No-op for now.

Refs: npm/rfcs#868
…I configs

A precedence resolver reads the install-time allowScripts policy from
the layered sources and threads it through install/ci into arborist.

  - lib/utils/resolve-allow-scripts.js: pure resolver. Reads from
    npm.prefix so workspace sub-installs still pick up the project
    root. Returns { policy, source }. Strict fallback: package.json
    wins over flat config; lower layers are silently ignored, with
    one warn when a lower setting is being suppressed.
  - install.js / ci.js: await the resolver before constructing
    arborist opts, then pass policy through opts.allowScripts. Add
    the three new params to each command's static params list.
  - workspaces/arborist/lib/arborist/index.js: accept
    options.allowScripts and store it on this.options. No enforcement
    yet; read in later commits.

Also tightened the flatten function for the new allow-scripts config:
nopt wraps single comma-separated strings in arrays for [String, Array]
types, so each array entry needs splitting on commas before use.

Refs: npm/rfcs#868
… log.warn advisory, dedupe trustedDisplay, reject @latest)
@JamieMagee JamieMagee force-pushed the jamiemagee/install-scripts-phase-1 branch from 71e336f to d885072 Compare May 23, 2026 04:52
@JamieMagee JamieMagee requested a review from owlstronaut May 23, 2026 05:02
BartWaardenburg added a commit to fallow-rs/fallow that referenced this pull request May 26, 2026
* feat(npm): move binary verification from postinstall to lazy first-run

Phase 2 of npm RFC 868 (npm/cli#9360) will block postinstall hooks by
default unless consumers explicitly add fallow to their package.json
allowScripts. The current postinstall path runs the Ed25519 signature
and SHA-256 digest verification fallow has shipped since v2.65; under
Phase 2 it would silently no-op for every consumer that has not opted
in, regressing the security property without surfacing a warning.

This commit retires the postinstall hook and moves verification into
the bin wrappers via a new ensureVerified() entry point that runs on
the first invocation of fallow, fallow-lsp, or fallow-mcp after install
or upgrade. A small JSON sentinel next to the platform binary (or in
the user cache dir when the platform pkg dir is read-only, e.g. yarn
PnP / Docker layered images) caches the verified state so subsequent
invocations skip verification on a cache hit. fallow --version now
prints a trailing "verified: yes (<sentinel-path>)" line that vendor
questionnaires can grep for.

The cryptographic property is preserved bit-for-bit. Same hardcoded
public key, same offline fallowDigests lookup, same fail-closed
behavior on tamper. The GitHub Action installer runs its own
independent verification step that is unaffected.

New env vars (all opt-in):
- FALLOW_VERIFY_CACHE_DIR overrides where the sentinel lives.
- FALLOW_VERIFY_LOG=1 emits one structured stderr line per outcome.

FALLOW_SKIP_BINARY_VERIFY remains the documented operator escape.

Files:
- npm/fallow/scripts/lazy-verify.js (new)
- npm/fallow/scripts/sentinel-path.js (new)
- npm/fallow/scripts/run-binary.js (new shared bin launcher)
- npm/fallow/scripts/verify-binary.js (sync codepath added)
- npm/fallow/bin/* (collapsed to one-line require of run-binary)
- npm/fallow/scripts/postinstall.js (deleted)
- npm/fallow/package.json (postinstall removed)
- SECURITY.md (verification-surfaces table revised)
- CHANGELOG.md (entry under [Unreleased])
- .github/workflows/ci.yml (smoke that no postinstall ran)

73 of 74 npm wrapper tests pass; one warn-once test is skipped because
it requires a homedir-override knob the resolver does not expose. The
warn-once path itself is covered by sentinel-path tests.

Refs the plan at .plans/rfc-868-lazy-binary-verify.md (post panel-review
v2; resolves BLOCK 1 (Yusuf) plus six CONCERNs).

* fix(npm): address review feedback on lazy-verify

Three fixes from team review (a727dc0b8d4):

1. verify-binary.js binaryTargetsForPlatform now derives the `.exe`
   suffix from the platformId argument instead of process.platform.
   Production callers always pass a platformId that encodes
   windows-ness ("win32-x64-msvc", "win32-arm64-msvc"), and tests
   can now exercise the Windows binary-name path without running
   on Windows. No production behavior change.

2. ci.yml smoke step grep tightened from substring "postinstall"
   to "^> fallow@[0-9].*(postinstall|preinstall|install)". The old
   pattern would false-positive on any transitive dep whose npm
   install log line mentions "postinstall"; the new pattern matches
   only npm's own lifecycle-script announcement for fallow itself.

3. ci.yml smoke step now uses `--omit=optional` and a defensive
   bash array for tarball discovery (avoids the leading-space issue
   that bit the local zsh run). Skipping the optional platform
   binary fetch eliminates an unnecessary network round-trip to the
   npm registry and reduces rate-limit risk under burst CI load.

Also bumped `test -f` -> `test -x` on the bin wrapper assertions
so a future package.json `"bin"` misconfiguration that loses the
executable bit fails the smoke instead of shipping silently.

* refactor(npm): reduce cyclomatic + clean up dead exports

Post-audit cleanup pass on the lazy-verify implementation:

1. Split high-cyclomatic functions into flat helper chains.
   - resolveSentinelPath cc=19 -> cc=10: extracted tryPlatformPkgDir,
     tryCacheDirEnv, tryXdgFallback, xdgLocationLabel.
   - ensureVerified cc=18 -> cc=12: extracted buildVerifyOptions and
     persistSentinel.
   - isSentinelValid cc=17 -> three helpers (readSentinelFile,
     sentinelStructureMatches, sentinelMtimesMatch).
   - verifyInstalled cc=13 cognitive=18 -> cc=9 cognitive=9: extracted
     verifyOneBinary (shared between async + sync paths).
   - verifyInstalledSync cc=15 cognitive=21 -> cc=9 cognitive=9:
     extracted verifyOneBinarySync.
   - runBinary cc=12: extracted resolvePlatformPaths, printVerifyError,
     writeVerifiedLineIfVersionQuery.
   - resolvePlatformPackageForVerify cc=12: extracted
     currentPlatformPackageName and readManifestForPackage.

2. Drop dead test-only exports.
   - sentinel-path.js no longer exports _ensureDirExists and
     _xdgCacheRoot (covered indirectly via _isWritable + integration).
   - lazy-verify.js no longer re-exports SENTINEL_FILENAME (duplicate
     of sentinel-path.js export). Test imports it from sentinel-path
     directly.

3. Convert bin wrapper requires from chained method call to destructured
   form so fallow's analyzer traces the import edge cleanly:
     -require('../scripts/run-binary').runBinary('fallow');
     +const { runBinary } = require('../scripts/run-binary');
     +runBinary('fallow');

4. Update npm/fallow/.fallowrc.json entry list: drop stale
   scripts/postinstall.js, add scripts/run-binary.js so the wrapper is
   a documented entry rather than relying on bin-file require tracing.

5. Exempt npm/fallow/scripts/** from health + duplicates analysis in
   the workspace .fallowrc.json. The npm wrapper is glue code, not
   fallow's primary analysis surface; per the implement-skill rule on
   CRAP-on-test-free-helpers, this is the documented escape valve.

After the refactor: 0 complexity findings introduced (was 14),
0 duplication findings introduced (was 20), 0 unused exports
introduced (was 3). The remaining 9 dead-code findings under
fallow dead-code --root npm/fallow are pre-existing (8 platform
packages resolved at runtime by getPlatformPackage, plus
@tanstack/intent).

Behavior unchanged: 73 of 74 npm wrapper tests pass; one warn-once
test is skipped on hosts with a real homedir. End-to-end smoke
against a real packed tarball still passes (cache miss writes
sentinel, cache hit skips verify, tamper detection fail-closed,
all three env vars honored).

* fix(npm): bind sentinel to install dir + binary SHA-256 (BLOCK from Codex review)

Two security findings from Codex's parallel review:

1. BLOCK: cross-install sentinel reuse in the shared $XDG fallback cache.
   The previous sentinel was keyed only by package name + version + per-
   binary mtimeMs. Two installs of the same fallow version on the same
   host (e.g. one read-only / yarn-PnP / Docker-layered, the other
   tampered with mtime-preservation) shared the same sentinel file. The
   second install's verify() would return ok cached:true based on the
   first install's verified state without re-reading bytes. Reproducer:
   install A clean + sentinel in $XDG; install B with tampered fallow
   binary + mtimes copied from A; install B sees cache hit, executes
   tampered binary.

   Fix: the sentinel now records (a) the absolute platform-pkg-dir of
   the install that wrote it AND (b) a hex SHA-256 of each binary's
   bytes. The cache-hit path requires both to match. Schema version
   bumped to 2 so older sentinels are invalidated automatically. mtime
   stays as a cheap pre-filter; SHA-256 is the load-bearing integrity
   gate. Added regression test that exercises the exact cross-install
   reproducer Codex described, plus a same-dir tamper-with-mtime-preserved
   test for the byte-binding property.

2. CONCERN: FALLOW_SKIP_BINARY_VERIFY=1 was silent on normal command
   execution despite SECURITY.md claiming "emits a warning so the skip
   is visible in CI logs". Only the FALLOW_VERIFY_LOG=1 path emitted
   anything, and only fallow --version surfaced the skip via the
   verified: skipped line.

   Fix: ensureVerified() now emits a warn-once stderr line on every
   invocation where the skip is active. Documented in SECURITY.md +
   CHANGELOG. Regression test asserts the warning fires exactly once
   per process.

Verification:
- 76 of 77 npm wrapper tests pass (1 skipped is the existing warn-once
  test that requires a homedir-override knob).
- End-to-end smoke against a real packed tarball: cache miss writes
  schema-v2 sentinel with sha256 + platformPkgDir, cache hit honors
  both bindings, mtime-preserved tamper fails closed with sig-invalid,
  FALLOW_SKIP_BINARY_VERIFY=1 prints the warning to stderr.
- cargo clippy --workspace --all-targets clean.
- Action tests 249/0; GitLab CI tests 230/0 (unchanged).
// Returns `{ name, version }` or `null` if no trusted identity exists.
const getTrustedRegistryIdentity = (node) => {
if (node.resolved && typeof node.resolved === 'string') {
const parsed = versionFromTgz('', node.resolved)
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.

I know I'm in the minority but I use a custom registry that doesn't host tarballs at the 'standard' tarball URL (/@foo/bar/-/bar-1.2.3.tgz or /foo/-/foo-1.2.3.tgz). Those paths aren't really a requirement of npm registry, since the model is to read from dist.tarball. This function doesn't return the correct version for URLs my mirror uses, which means all name@version patterns don't match.

Is there a way we could implement without parsing the URL?

  • There's the arborist edges, that define 'why' it was fetched
  • There could be some meta-resolved 'source' field derived from the resolved version?

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.

I think this is a fair concern. So your name only version will work, but the pinned version doesn't? I think for this PR that will be scope creep and it is in a good and useful state now, but if you'd like to follow up with a pr that addresses pinned approvals for non-standard registry URLs, I'd happily review it. the constraint is that whatever new trust anchor we pick has to keep holding under loadActual() so a forged on-disk package.json can't override it.

@owlstronaut owlstronaut merged commit 7068d42 into npm:latest May 27, 2026
64 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

⚠️ Backport to release/v11 failed.

This usually means the cherry-pick had conflicts. Please create a manual backport:

git fetch origin release/v11
git checkout -b backport/v11/9360 origin/release/v11
git cherry-pick -x 7068d4286eb446fdb0ded08d15d7b5c3883d80f5
# resolve any conflicts, then:
git push origin backport/v11/9360
Error details
Command failed: git cherry-pick -x 7068d4286eb446fdb0ded08d15d7b5c3883d80f5
error: could not apply 7068d4286... feat: Phase 1 of `allowScripts` opt-in install-script policy (#9360)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config set advice.mergeConflict false"

owlstronaut pushed a commit that referenced this pull request May 27, 2026
Implements Phase 1 of
[npm/rfcs#868](npm/rfcs#868), which makes
dependency install scripts opt-in.

**Install behaviour is unchanged.** Scripts still run as they always
have. The only Phase 1 user-visible change is one advisory block at the
end of `npm install` listing packages whose install scripts haven't been
reviewed via the new `allowScripts` field in `package.json`. A future
release will turn that advisory into an actual block.

- `allowScripts` field in `package.json`, read at install time
- Three new configs: `allow-scripts`, `strict-script-builds`,
`dangerously-allow-all-scripts`. The latter two are no-ops in this
release. They're registered so projects can pin them in tooling ahead of
the release that flips the default.
- `npm approve-scripts` and `npm deny-scripts` commands, with the RFC's
asymmetric pin rule (approves can pin, denies are always name-only)
- Advisory warning during `npm install`, `ci`, `update`, and `rebuild`.
`npm exec` / `npx` consult only the user/global `.npmrc` layer per the
RFC, with the policy threaded through libnpmexec for Phase 2
enforcement.
- Identity matcher in `@npmcli/arborist` covering registry, git, file,
and remote tarballs. Registry identity is derived from the lockfile's
resolved URL (via `versionFromTgz`), never from `node.packageName` or
`node.version`. Those getters read the installed tarball's
`package.json` and can be forged.
- Aliases match against the underlying registered package, not the alias
name. `trusted@npm:naughty@1.0.0` is approved by writing `naughty`, not
`trusted`. Holds even under `omitLockfileRegistryResolved`, where the
install location alone (`node_modules/trusted`) would be misleading. The
underlying name is derived from the incoming edge's alias `subSpec`.
- Bundled deps with install scripts are flagged as unreviewed and
filtered out of `npm approve-scripts --all` and positional matches. Per
RFC they cannot be allowlisted in Phase 1.
- Warning when a non-root workspace declares its own `allowScripts`

- Actual blocking. The matcher exists and the policy is threaded through
to arborist, but `arb.rebuild()`'s build set still runs everything.
Phase 2 will gate `#addToBuildSet` on the matcher.
- A safe allowlist syntax for bundled deps. The RFC notes a candidate
`parent@1.2.3 > bundled-name` form for a follow-up.

Refs: npm/rfcs#868
(cherry picked from commit 7068d42)
owlstronaut added a commit that referenced this pull request May 27, 2026
…9415)

Backports #9360 to v11.

Co-authored-by: Jamie Magee <jamie.magee@gmail.com>
@github-actions github-actions Bot mentioned this pull request May 26, 2026
@JamieMagee JamieMagee deleted the jamiemagee/install-scripts-phase-1 branch May 28, 2026 06:15
jdalton added a commit to SocketDev/socket-lib that referenced this pull request May 28, 2026
jdalton added a commit to SocketDev/socket-packageurl-js that referenced this pull request May 28, 2026
jdalton added a commit to SocketDev/socket-sdk-js that referenced this pull request May 28, 2026
jdalton added a commit to SocketDev/socket-addon that referenced this pull request May 28, 2026
jdalton added a commit to SocketDev/socket-btm that referenced this pull request May 28, 2026
jdalton added a commit to SocketDev/socket-mcp that referenced this pull request May 28, 2026
jdalton added a commit to SocketDev/socket-cli that referenced this pull request May 28, 2026
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.

6 participants