fix: gate npm pack --ignore-scripts on install_allow_scripts alone - #1819
Conversation
There was a problem hiding this comment.
Code Review
This pull request ensures that npm pack respects the allowInstallScripts configuration during git-reference deploys by conditionally appending the --ignore-scripts flag. This prevents unauthorized scripts from running during the pack step. A comprehensive regression test suite has been added to validate this behavior. The review feedback recommends using Node's native node:assert/strict instead of node:assert in the new test file to align with strict assertion standards.
|
Reviewed; no blockers found. Latest push (a671efd) addresses both prior findings: |
…ture #1797 (git credential sessions + semver-range committish resolution) merged to main after this branch was created, and both PRs independently rewrote the same region of Application.ts (parseGitReference, packGitReferenceWithoutScripts, and the extractApplication gating logic). A plain `git rebase main` applied cleanly with no conflict markers but silently discarded #1797's entire credential-threading and semver-resolution work — the patch's context lines matched at the edges of the rewritten block while the middle content differed, so the merge machinery replaced the whole span. Caught by diffing the rebase result against main and finding gitCredentialEnv, resolveCommittish, etc. entirely missing; reconciled by hand instead of trusting the automatic result. The merged implementation keeps: - From #1797: git-clone credential threading (gitCredentialEnv passed to the clone spawn), and semver-range committish resolution via resolveCommittish (including its SAFE_TAG_NAME shell-injection guard). - From #1819: looksLikeGitReference + BARE_GIT_HOST_URL_PREFIX (a bare https/http URL to a known git host is a git reference too), stricter owner/repo validation on hosted shorthand, the runNpmPack dedup helper, and — per #1819's own PR description, which already named this reconciliation — broadening script-suppression to gate on install_allow_scripts alone rather than requiring a credential to be present. parseGitReference now rejects only `#path:` (npm's still-unimplemented git-url subdirectory extension) rather than also rejecting `#semver:`, since #1797's resolveCommittish makes that form handleable regardless of URL shape; #1819's own "refuses a bare-URL git reference it can't handle" test is updated to use `#path:` instead, since `#semver:` no longer qualifies as unhandleable. Broadening the gate to `!allowInstallScripts` (dropping the credential requirement) means an uncredentialed git-reference deploy with the default (scripts disallowed) now also takes the reclone-and-strip-scripts path — this is #1819's whole point, closing the same gap for uncredentialed deploys that #1797 only closed for credentialed ones. Two pre-existing #1799 tests in gitCredentialClone.test.js asserted the old plain-`npm pack` error wording ("Failed to download package") for an uncredentialed clone failure; updated to the reclone path's wording ("Failed to clone package") since that's the path now taken. All of #1797's semver tests (gitSemverCommittish.test.js), #1799's credential tests (gitCredentials.test.js, gitCredentialClone.test.js), and #1819's own tests (gitPackIgnoreScripts.test.js) pass against the merged implementation, along with Application.test.js and the broader components suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
a6540da to
6798265
Compare
…est lint Codex CLI review of the reconciliation commit found: - parseGitReference didn't decode a percent-encoded committish for the `owner:repo` shorthand or a bare host URL, though npm's own resolution for those forms (via hosted-git-info) does. Verified against hosted-git-info's source (`committish = decodeURIComponent(parsed.hash.slice(1))`) and added matching decoding — but only for those two forms: an explicit `git+`/`git:` URL goes through npm-package-arg's own URL.hash extraction instead, which does NOT decode the committish, so this doesn't either (confirmed by reading npm-package-arg's fromURL/setGitAttrs directly, rather than guessing at parity). - gitPackIgnoreScripts.test.js (inherited from #1819) imported the repo-restricted `node:assert/strict` via `require()`, evading the lint rule that only checks ESM imports. Switched to `node:assert` with explicit `assert.strictEqual`. Two further findings are noted but not fixed here, as pre-existing gaps in #1819's own design (not introduced by this reconciliation) that would need non-trivial, independently-risky changes to close properly — flagging for the human reviewer rather than guessing at a fix under this rebase's scope: - looksLikeGitReference/parseGitReference don't recognize npm's SSH shorthand forms (`git@host:owner/repo.git`, `ssh://git@host/...`), so those still fall through to the less-reliable plain `npm pack --ignore-scripts` path (still attempted, just not the more robust reclone path — see the pacote/npm<11 caveat already documented above packGitReferenceWithoutScripts). - BARE_GIT_HOST_URL_PREFIX matches any path under a known host, including a hosted tarball archive URL (e.g. .../archive/refs/tags/v1.0.0.tar.gz), which would misclassify a previously-working tarball deploy as a git clone. Properly distinguishing the two needs per-host path-shape rules (matching hosted-git-info's own archive-URL detection), which is a separate, focused piece of work. Added test coverage for the decode behavior (and its intentional asymmetry across URL forms) in Application.test.js's parseGitReference suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ture #1797 (git credential sessions + semver-range committish resolution) merged to main after this branch was created, and both PRs independently rewrote the same region of Application.ts (parseGitReference, packGitReferenceWithoutScripts, and the extractApplication gating logic). A plain `git rebase main` applied cleanly with no conflict markers but silently discarded #1797's entire credential-threading and semver-resolution work — the patch's context lines matched at the edges of the rewritten block while the middle content differed, so the merge machinery replaced the whole span. Caught by diffing the rebase result against main and finding gitCredentialEnv, resolveCommittish, etc. entirely missing; reconciled by hand instead of trusting the automatic result. The merged implementation keeps: - From #1797: git-clone credential threading (gitCredentialEnv passed to the clone spawn), and semver-range committish resolution via resolveCommittish (including its SAFE_TAG_NAME shell-injection guard). - From #1819: looksLikeGitReference + BARE_GIT_HOST_URL_PREFIX (a bare https/http URL to a known git host is a git reference too), stricter owner/repo validation on hosted shorthand, the runNpmPack dedup helper, and — per #1819's own PR description, which already named this reconciliation — broadening script-suppression to gate on install_allow_scripts alone rather than requiring a credential to be present. parseGitReference now rejects only `#path:` (npm's still-unimplemented git-url subdirectory extension) rather than also rejecting `#semver:`, since #1797's resolveCommittish makes that form handleable regardless of URL shape; #1819's own "refuses a bare-URL git reference it can't handle" test is updated to use `#path:` instead, since `#semver:` no longer qualifies as unhandleable. Broadening the gate to `!allowInstallScripts` (dropping the credential requirement) means an uncredentialed git-reference deploy with the default (scripts disallowed) now also takes the reclone-and-strip-scripts path — this is #1819's whole point, closing the same gap for uncredentialed deploys that #1797 only closed for credentialed ones. Two pre-existing #1799 tests in gitCredentialClone.test.js asserted the old plain-`npm pack` error wording ("Failed to download package") for an uncredentialed clone failure; updated to the reclone path's wording ("Failed to clone package") since that's the path now taken. All of #1797's semver tests (gitSemverCommittish.test.js), #1799's credential tests (gitCredentials.test.js, gitCredentialClone.test.js), and #1819's own tests (gitPackIgnoreScripts.test.js) pass against the merged implementation, along with Application.test.js and the broader components suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
npm packon a git-referencepackageIdentifierisn't just a download — npm clones the repo and, if its manifest has aprepare/build/installscript, runsnpm installinside the clone and then that script. That means the repo's own code (and its dependencies' install scripts) can execute on this node during the pack step alone, before the laternpm installeven runs.install_allow_scriptsis the operator-facing switch for whether a deployed component may run scripts on this node at all, but it never reached this pack step — so a git-reference deploy withinstall_allow_scripts: falsestill ran pack-time scripts unconditionally.Fixes this by passing
--ignore-scriptstonpm packwheneverinstall_allow_scriptsis not explicitly enabled, for any non-file:packageIdentifier.Refs #1818
Note on scope vs. the issue text
The issue (and #1799, still open/unmerged) describes this as a
gitCredentialEnv &&-gated condition.gitCredentialEnvdoesn't exist onmainyet — it's introduced by #1799, which isn't merged. What does exist onmaintoday is the more fundamental gap:npm packnever passed--ignore-scriptsat all, regardless ofinstall_allow_scripts, for any git-reference deploy (credentialed or not). This PR fixes that root gap directly, unconditionally on!application.install?.allowInstallScripts— nogitCredentialEnvinvolved, since it isn't in scope onmain.When #1799 lands, its
gitCredentialEnv-gated--ignore-scriptscondition will need to be reconciled with this change — per the issue's own suggested fix, that just means dropping theapplication.gitCredentialEnv &&prefix, since the gate this PR adds already covers it.The issue also asks to re-examine the adjacent warn-log branch (only fires today when
gitCredentialEnvis set and scripts are allowed). That branch doesn't exist onmaineither — it's credential-specific wording tied to #1799's still-unmerged feature, so there's nothing to change here; #1799 is the right place to decide whether that log should also fire for the uncredentialed allow-scripts case.Test coverage
Added
unitTests/components/gitPackIgnoreScripts.test.js— an end-to-end test against a real local git repo and realnpm pack(no mocking of the spawn args), following the pattern already used by #1799'sgitCredentialClone.test.js:install_allow_scriptsis unset (default), with no git credential involvedinstall_allow_scripts: trueis explicitly setExisting
Application.test.js,extractApplicationSwap.test.js,applicationSpawn.test.js, andpackageComponent.test.jspass unchanged, as does the fulltest:unit:mainsuite (3423 passing; the 10globalIsolation.test.jsfailures are a pre-existing worktree-environment issue — anode_modulespath-resolution mismatch unrelated to this change, confirmed by reproducing them againstmainwith this diff stashed out).Update: rebased onto main now that #1797/#1799 are merged
#1797 (git credential sessions) and #1799 (semver-range committish resolution) both merged to
mainafter this branch was created, and both independently rewrote the same region ofApplication.ts(parseGitReference,packGitReferenceWithoutScripts, and theextractApplicationgating logic this PR touches). A plaingit rebase mainapplied with no conflict markers but silently discarded #1797/#1799's entire credential-threading and semver-resolution work — confirmed by diffing the rebase result againstmainand findinggitCredentialEnv,resolveCommittish, etc. missing entirely. Reconciled by hand instead of trusting that result; see the "Rebase onto main and reconcile..." commit message for the detailed reasoning.The merged implementation keeps #1797's credential-threading and semver resolution, plus this PR's
looksLikeGitReference/bare-URL recognition,runNpmPackdedup, and — per this PR's own note above — broadens the script-suppression gate to!allowInstallScriptsalone (dropping the credential requirement).parseGitReferencenow rejects only#path:(still unimplemented) rather than also#semver:, since #1797 makes that form handleable.A follow-up push addressed two Codex CLI findings from cross-model review of the rebase:
parseGitReferencenow decodes a percent-encoded committish for theowner:reposhorthand and bare host URLs (matching hosted-git-info's actual behavior, verified by reading its source), while deliberately leaving explicitgit+/git:URL forms undecoded (matching npm-package-arg's own, different behavior for those forms) — plus a test-lint fix.Two further findings from that review are open, not fixed in this PR — pre-existing gaps in this PR's own
looksLikeGitReference/BARE_GIT_HOST_URL_PREFIXdesign (not introduced by the rebase), each judged to need independently-risky, non-trivial changes to close properly rather than a quick guess under this rebase's scope:looksLikeGitReference/parseGitReferencedon't recognize npm's SSH shorthand forms (git@host:owner/repo.git,ssh://git@host/...) or SourceHut. Those still fall through to the plainnpm pack --ignore-scriptspath — still attempted (this PR's core fix does apply), just not the more robust reclone-and-strip-scripts path, so they retain the pacote/npm<11--ignore-scriptscaveat already documented abovepackGitReferenceWithoutScripts.BARE_GIT_HOST_URL_PREFIXmatches any path under a known git host, including a hosted tarball archive URL (e.g..../archive/refs/tags/v1.0.0.tar.gz), which would misclassify a previously-working tarball deploy as a git clone and attemptgit cloneagainst it. Properly distinguishing the two needs per-host path-shape rules matching hosted-git-info's own archive-URL detection — worth a focused follow-up.Rebased test suite (
gitSemverCommittish.test.js,Application.test.js,gitCredentials.test.js,gitCredentialClone.test.js,gitPackIgnoreScripts.test.js,extractApplicationSwap.test.js,applicationSpawn.test.js,packageComponent.test.js,credentials.test.js) all pass — 89 tests.Generated by Claude Sonnet 5.
🤖 Generated with Claude Code