feat: Phase 1 of allowScripts opt-in install-script policy#9360
Conversation
3bb254c to
e4270f0
Compare
|
I think it would be bad to support an 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
Why is the Also, sidebar, the naming of |
|
@bakkot Yeah, all of that sounds good. I'll switch to your proposed phasing. Concretely:
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. |
|
SGTM.
I don't feel strongly about this point, but fwiw 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 |
…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
…t, restrict --allow-scripts to global/exec
… log.warn advisory, dedupe trustedDisplay, reject @latest)
71e336f to
d885072
Compare
* 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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/9360Error details |
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)
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 installlisting packages whose install scripts haven't been reviewed via the newallowScriptsfield inpackage.json. A future release will turn that advisory into an actual block.What landed
allowScriptsfield inpackage.json, read at install timeallow-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-scriptsandnpm deny-scriptscommands, with the RFC's asymmetric pin rule (approves can pin, denies are always name-only)npm install,ci,update, andrebuild.npm exec/npxconsult only the user/global.npmrclayer per the RFC, with the policy threaded through libnpmexec for Phase 2 enforcement.@npmcli/arboristcovering registry, git, file, and remote tarballs. Registry identity is derived from the lockfile's resolved URL (viaversionFromTgz), never fromnode.packageNameornode.version. Those getters read the installed tarball'spackage.jsonand can be forged.trusted@npm:naughty@1.0.0is approved by writingnaughty, nottrusted. Holds even underomitLockfileRegistryResolved, where the install location alone (node_modules/trusted) would be misleading. The underlying name is derived from the incoming edge's aliassubSpec.npm approve-scripts --alland positional matches. Per RFC they cannot be allowlisted in Phase 1.allowScriptsWhat's deliberately deferred
arb.rebuild()'s build set still runs everything. Phase 2 will gate#addToBuildSeton the matcher.parent@1.2.3 > bundled-nameform for a follow-up.Refs: npm/rfcs#868