From bf1b731ee6c0ea98709e671ff0f46bf654480ab8 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 10 Jun 2026 12:05:28 +0200 Subject: [PATCH] fix: harden allowBuilds artifact approvals (#12294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Package-name `allowBuilds` entries no longer approve lifecycle scripts for artifacts whose identity a name cannot pin: git, git-hosted tarball, direct tarball, and local directory dependencies. To approve such an artifact explicitly, use its peer-suffix-free lockfile depPath as the `allowBuilds` key — error hints, `pnpm ignored-builds`, and `pnpm approve-builds` print exactly that key. - `AllowBuild` policy functions identify packages by `DepPath` instead of caller-supplied name/version. The policy parses name and version out of the depPath itself, so name-keyed rules can never be fed an identity that disagrees with the gated artifact. `AllowBuildContext` carries only an explicit `trustPackageIdentity` override, used to evaluate a previously recorded policy under its original semantics when detecting revoked approvals. - Identity trust is derived from the depPath shape: a registry-style depPath (`name@semver`) is a trusted identity. This is sound because lockfile verification runs a new unconditional, offline structural pass that rejects lockfiles where such a key is backed by a git, directory, or git-hosted tarball resolution (`ERR_PNPM_RESOLUTION_SHAPE_MISMATCH`), while the npm resolution verifier already binds explicit tarball URLs of semver-keyed entries to the registry's own `dist.tarball` unconditionally. The pass runs inside the existing candidate walk and participates in the verification cache key (`resolutionShapeCheck`) on both the gate's and the fresh-resolve record paths, so the stat-only cache fast path stays sound and records written before the rule existed are re-verified. - Installs detect approvals that were revoked (or stopped applying) for git/tarball artifacts and surface those packages as ignored builds; approvals granted for previously ignored builds trigger a rebuild of exactly those packages. - `preparePackage` always treats the fetched manifest as an untrusted identity: it requires a `pkgResolutionId` and gates on the synthesized `name@` depPath. scp-style git URLs are normalized to `ssh://` form in resolution ids, and the git fetcher reuses `createGitHostedPkgId` from the resolver instead of re-deriving ids. - Under the global virtual store, `pnpm rebuild` locates a projection created before the approval was granted by following the project's node_modules link, since the projection hash includes the allowBuilds verdict (relocating the projection instead is tracked in https://github.com/pnpm/pnpm/issues/12302). - New shared helpers: `removePeersSuffix()` in `@pnpm/deps.path` (string-level peer-suffix stripping for user-written keys) and `allowBuildKeyFromIgnoredBuild()` in `@pnpm/building.policy` (the key under which an ignored build is approved). - pacquet mirrors the whole policy: `AllowBuildPolicy::check(dep_path)` derives trust from the dep path, the git-fetcher allow-build closures take only the dep path, `pacquet-lockfile-verification` gains the same structural pass, error code, and cache identity, and dep-path keys normalize via `remove_suffix`. - `shell-quote` is overridden to 1.8.4 (GHSA-w7jw-789q-3m8p / CVE-2026-9277). - Test-harness fix: a module-level drain keeps the global log stream flowing in the deps-installer lifecycle tests, so reporter assertions no longer receive the buffered backlog of earlier installs that ran without a reporter. --- .changeset/tough-allow-builds-identities.md | 17 ++ building/after-install/src/index.ts | 64 ++++++- building/commands/package.json | 1 + building/commands/src/policy/approveBuilds.ts | 4 +- .../policy/getAutomaticallyIgnoredBuilds.ts | 4 +- building/commands/test/build/index.ts | 53 +++++- building/commands/tsconfig.json | 3 + building/during-install/src/index.ts | 2 +- building/policy/src/index.ts | 99 ++++++++-- building/policy/test/index.ts | 98 ++++++++-- core/types/src/config.ts | 8 +- .../with-git-protocol-peer-deps/package.json | 2 +- .../commands/test/licenses/index.ts | 4 +- deps/graph-builder/src/lockfileToDepGraph.ts | 2 +- deps/graph-hasher/src/index.ts | 8 +- .../test/allowBuildIdentity.test.ts | 41 ++++ deps/path/src/index.ts | 15 +- exec/commands/test/dlx.e2e.ts | 4 +- exec/prepare-package/src/index.ts | 13 +- exec/prepare-package/test/index.ts | 18 +- fetching/git-fetcher/package.json | 1 + fetching/git-fetcher/src/index.ts | 2 + fetching/git-fetcher/test/index.ts | 6 +- fetching/git-fetcher/tsconfig.json | 3 + .../src/gitHostedTarballFetcher.ts | 9 + fetching/tarball-fetcher/test/fetch.ts | 2 +- installing/commands/package.json | 1 + .../commands/src/handleIgnoredBuilds.ts | 4 +- installing/commands/tsconfig.json | 3 + .../deps-installer/src/install/index.ts | 22 ++- installing/deps-installer/src/install/link.ts | 2 +- .../src/install/recordLockfileVerified.ts | 3 +- .../src/install/verifyLockfileResolutions.ts | 100 +++++++++- .../test/install/lifecycleScripts.ts | 121 +++++++++++- .../test/install/verifyLockfileResolutions.ts | 148 ++++++++++++++- installing/deps-restorer/src/index.ts | 2 +- .../deps-restorer/src/linkHoistedModules.ts | 2 +- lockfile/fs/src/lockfileFormatConverters.ts | 14 +- lockfile/utils/src/index.ts | 2 +- lockfile/utils/src/toLockfileResolution.ts | 13 +- pacquet/crates/git-fetcher/src/fetcher.rs | 7 +- .../crates/git-fetcher/src/fetcher/tests.rs | 106 ++++++++++- .../crates/git-fetcher/src/prepare_package.rs | 18 +- .../git-fetcher/src/prepare_package/tests.rs | 81 +++++++- .../crates/git-fetcher/src/tarball_fetcher.rs | 7 +- .../git-fetcher/src/tarball_fetcher/tests.rs | 9 +- .../lockfile-verification/src/errors.rs | 14 ++ .../crates/lockfile-verification/src/lib.rs | 4 +- .../src/record_lockfile_verified.rs | 7 +- .../src/verify_lockfile_resolutions.rs | 144 ++++++++++++-- .../src/verify_lockfile_resolutions/tests.rs | 178 ++++++++++++++++++ pacquet/crates/lockfile/src/resolution.rs | 19 +- .../package-manager/src/build_modules.rs | 108 +++++++++-- .../src/build_modules/tests.rs | 101 ++++++---- .../src/install_package_by_snapshot.rs | 2 +- .../src/virtual_store_layout.rs | 7 +- .../src/virtual_store_layout/tests.rs | 37 +++- pnpm-lock.yaml | 10 + pnpm-workspace.yaml | 2 + pnpm/test/dlx.ts | 8 +- .../git-resolver/src/createGitHostedPkgId.ts | 13 +- .../test/createGitHostedPkgId.test.ts | 4 + 62 files changed, 1574 insertions(+), 232 deletions(-) create mode 100644 .changeset/tough-allow-builds-identities.md create mode 100644 deps/graph-hasher/test/allowBuildIdentity.test.ts diff --git a/.changeset/tough-allow-builds-identities.md b/.changeset/tough-allow-builds-identities.md new file mode 100644 index 0000000000..d4fddeced4 --- /dev/null +++ b/.changeset/tough-allow-builds-identities.md @@ -0,0 +1,17 @@ +--- +"@pnpm/building.after-install": patch +"@pnpm/building.during-install": patch +"@pnpm/building.policy": patch +"@pnpm/deps.graph-builder": patch +"@pnpm/deps.graph-hasher": patch +"@pnpm/exec.prepare-package": patch +"@pnpm/fetching.git-fetcher": patch +"@pnpm/fetching.tarball-fetcher": patch +"@pnpm/installing.deps-installer": patch +"@pnpm/installing.deps-resolver": patch +"@pnpm/installing.deps-restorer": patch +"@pnpm/types": patch +"pnpm": patch +--- + +Require trusted package identity before package-name `allowBuilds` entries can approve lifecycle scripts for git, git-hosted tarball, direct tarball, and local directory artifacts. To approve one of those artifacts explicitly, use its peer-suffix-free lockfile depPath as the `allowBuilds` key. Lockfile verification now rejects lockfiles where a registry-style dependency path (`name@semver`) is backed by a git, directory, or git-hosted tarball resolution (`ERR_PNPM_RESOLUTION_SHAPE_MISMATCH`), so the dependency path is a reliable artifact identity by the time scripts can run. diff --git a/building/after-install/src/index.ts b/building/after-install/src/index.ts index 4d54423aec..a13790b6b2 100644 --- a/building/after-install/src/index.ts +++ b/building/after-install/src/index.ts @@ -1,4 +1,5 @@ import assert from 'node:assert' +import fs from 'node:fs' import path from 'node:path' import util from 'node:util' @@ -37,6 +38,7 @@ import { pickStoreIndexKey, StoreIndex } from '@pnpm/store.index' import type { DepPath, IgnoredBuilds, + PkgIdWithPatchHash, ProjectId, ProjectManifest, ProjectRootDir, @@ -78,19 +80,23 @@ function findPackages ( }) return false } - return matches(searched, pkgInfo) + return matches(searched, pkgInfo, dp.getPkgIdWithPatchHash(relativeDepPath)) }) } // TODO: move this logic to separate package as this is also used in tree-builder function matches ( searched: PackageSelector[], - manifest: { name: string, version?: string } + manifest: { name: string, version?: string }, + pkgIdWithPatchHash: PkgIdWithPatchHash ): boolean { return searched.some((searchedPkg) => { if (typeof searchedPkg === 'string') { return manifest.name === searchedPkg } + if ('pkgIdWithPatchHash' in searchedPkg) { + return searchedPkg.pkgIdWithPatchHash === pkgIdWithPatchHash + } return searchedPkg.name === manifest.name && !!manifest.version && semver.satisfies(manifest.version, searchedPkg.range) }) @@ -99,6 +105,9 @@ function matches ( type PackageSelector = string | { name: string range: string +} | { + /** A user-written depPath spec, normalized with the peer suffix stripped. */ + pkgIdWithPatchHash: string } export async function buildSelectedPkgs ( @@ -117,6 +126,9 @@ export async function buildSelectedPkgs ( const packages = ctx.currentLockfile.packages const searched: PackageSelector[] = pkgSpecs.map((arg) => { + if (matchesDepPath(packages, arg)) { + return { pkgIdWithPatchHash: dp.removePeersSuffix(arg) } + } const { fetchSpec, name, raw, type } = npa(arg) if (raw === name) { return name @@ -168,6 +180,11 @@ export async function buildSelectedPkgs ( } } +function matchesDepPath (packages: PackageSnapshots, pkgSpec: string): boolean { + const normalizedPkgSpec = dp.removePeersSuffix(pkgSpec) + return Object.keys(packages).some((depPath) => dp.removePeersSuffix(depPath) === normalizedPkgSpec) +} + export async function buildProjects ( projects: Array<{ buildIndex: number, manifest: ProjectManifest, rootDir: ProjectRootDir }>, maybeOpts: BuildOptions @@ -330,8 +347,8 @@ async function _rebuild ( const ignoredPkgs = new Set() const _allowBuild = createAllowBuildFunction(opts) ?? (() => undefined) - const allowBuild = (pkgName: string, version: string, depPath: DepPath) => { - switch (_allowBuild(pkgName, version)) { + const allowBuild = (depPath: DepPath) => { + switch (_allowBuild(depPath)) { case true: return true case undefined: { ignoredPkgs.add(depPath) @@ -356,7 +373,10 @@ async function _rebuild ( opts.supportedArchitectures, nodeVersion )) { - gvsDirByDepPath.set(pkgMeta.depPath, path.join(globalVirtualStoreDir, hash)) + const preferredGvsDir = path.join(globalVirtualStoreDir, hash) + gvsDirByDepPath.set(pkgMeta.depPath, fs.existsSync(preferredGvsDir) + ? preferredGvsDir + : findLinkedGvsDir(pkgMeta.name, Object.values(ctx.projects), globalVirtualStoreDir) ?? preferredGvsDir) } } const pkgModulesDir = (depPath: DepPath): string => @@ -438,7 +458,7 @@ async function _rebuild ( requiresBuild = pkgRequiresBuild(pgkManifest, new Map()) } - const hasSideEffects = requiresBuild && allowBuild(pkgInfo.name, pkgInfo.version, depPath) && await runPostinstallHooks({ + const hasSideEffects = requiresBuild && allowBuild(depPath) && await runPostinstallHooks({ depPath, extraBinPaths, extraEnv: opts.extraEnv, @@ -530,6 +550,38 @@ async function _rebuild ( return { pkgsThatWereRebuilt, ignoredPkgs } } +// TODO: delete once rebuild relocates GVS projections to the newly computed +// hash instead of building in place (https://github.com/pnpm/pnpm/issues/12302). +function findLinkedGvsDir ( + pkgName: string, + projects: Array<{ rootDir: ProjectRootDir }>, + globalVirtualStoreDir: string +): string | undefined { + const normalizedGvsRoot = `${path.resolve(globalVirtualStoreDir)}${path.sep}` + for (const { rootDir } of projects) { + const pkgLink = path.join(rootDir, 'node_modules', pkgName) + try { + const target = fs.readlinkSync(pkgLink) + const pkgRoot = path.resolve(path.dirname(pkgLink), target) + if (!pkgRoot.startsWith(normalizedGvsRoot)) continue + return nthAncestorDir(pkgRoot, pkgName.split('/').length + 1) + } catch (err: unknown) { + // EINVAL: pkgLink exists but is not a symlink. + if (util.types.isNativeError(err) && 'code' in err && (err.code === 'EINVAL' || err.code === 'ENOENT')) continue + throw err + } + } + return undefined +} + +function nthAncestorDir (dir: string, levels: number): string { + let result = dir + for (let i = 0; i < levels; i++) { + result = path.dirname(result) + } + return result +} + function binDirsInAllParentDirs (pkgRoot: string, lockfileDir: string): string[] { const binDirs: string[] = [] let dir = pkgRoot diff --git a/building/commands/package.json b/building/commands/package.json index ac7f0bc33b..1219ebed14 100644 --- a/building/commands/package.json +++ b/building/commands/package.json @@ -34,6 +34,7 @@ "dependencies": { "@inquirer/prompts": "catalog:", "@pnpm/building.after-install": "workspace:*", + "@pnpm/building.policy": "workspace:*", "@pnpm/cli.command": "workspace:*", "@pnpm/cli.common-cli-options-help": "workspace:*", "@pnpm/cli.utils": "workspace:*", diff --git a/building/commands/src/policy/approveBuilds.ts b/building/commands/src/policy/approveBuilds.ts index 7fb60eb6fd..8cfd096a6d 100644 --- a/building/commands/src/policy/approveBuilds.ts +++ b/building/commands/src/policy/approveBuilds.ts @@ -1,8 +1,8 @@ import { checkbox, confirm } from '@inquirer/prompts' +import { allowBuildKeyFromIgnoredBuild } from '@pnpm/building.policy' import type { CommandHandlerMap } from '@pnpm/cli.command' import type { Config, ConfigContext } from '@pnpm/config.reader' import { writeSettings } from '@pnpm/config.writer' -import { parse } from '@pnpm/deps.path' import { PnpmError } from '@pnpm/error' import { install } from '@pnpm/installing.commands' import { type StrictModules, writeModulesManifest } from '@pnpm/installing.modules-yaml' @@ -195,7 +195,7 @@ export async function handler (opts: ApproveBuildsCommandOpts & RebuildCommandOp if (params.length) { const decided = new Set([...approved, ...denied]) for (const depPath of Array.from(modulesManifest.ignoredBuilds)) { - const name = parse(depPath).name ?? depPath + const name = allowBuildKeyFromIgnoredBuild(depPath) if (decided.has(name)) { modulesManifest.ignoredBuilds.delete(depPath) } diff --git a/building/commands/src/policy/getAutomaticallyIgnoredBuilds.ts b/building/commands/src/policy/getAutomaticallyIgnoredBuilds.ts index 81fba9eb9c..18ecd2ec81 100644 --- a/building/commands/src/policy/getAutomaticallyIgnoredBuilds.ts +++ b/building/commands/src/policy/getAutomaticallyIgnoredBuilds.ts @@ -1,6 +1,6 @@ import path from 'node:path' -import { parse } from '@pnpm/deps.path' +import { allowBuildKeyFromIgnoredBuild } from '@pnpm/building.policy' import { type Modules, readModulesManifest } from '@pnpm/installing.modules-yaml' import type { IgnoredBuildsCommandOpts } from './ignoredBuilds.js' @@ -18,7 +18,7 @@ export async function getAutomaticallyIgnoredBuilds (opts: IgnoredBuildsCommandO if (modulesManifest?.ignoredBuilds) { const ignoredPkgNames = new Set() for (const depPath of modulesManifest.ignoredBuilds) { - ignoredPkgNames.add(parse(depPath).name ?? depPath) + ignoredPkgNames.add(allowBuildKeyFromIgnoredBuild(depPath)) } automaticallyIgnoredBuilds = Array.from(ignoredPkgNames) } else { diff --git a/building/commands/test/build/index.ts b/building/commands/test/build/index.ts index 762f1b8139..ed788d2b62 100644 --- a/building/commands/test/build/index.ts +++ b/building/commands/test/build/index.ts @@ -47,6 +47,7 @@ test('rebuilds dependencies', async () => { '@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0', 'test-git-fetch@https://codeload.github.com/pnpm/test-git-fetch/tar.gz/8b333f12d5357f4f25a654c305c826294cb073bf', ]) + const gitDepPath = modules!.pendingBuilds[1] const modulesManifest = project.readModulesManifest() await rebuild.handler({ @@ -56,7 +57,7 @@ test('rebuilds dependencies', async () => { pending: false, registries: modulesManifest!.registries!, storeDir, - allowBuilds: { '@pnpm.e2e/pre-and-postinstall-scripts-example': true, 'test-git-fetch': true }, + allowBuilds: { '@pnpm.e2e/pre-and-postinstall-scripts-example': true, [gitDepPath]: true }, }, []) modules = project.readModulesManifest() @@ -329,6 +330,7 @@ test('rebuilds specific dependencies', async () => { ]) const modulesManifest = project.readModulesManifest() + const gitDepPath = modulesManifest!.pendingBuilds.find((depPath) => depPath.startsWith('install-scripts-example-for-pnpm@'))! await rebuild.handler({ ...DEFAULT_OPTS, cacheDir, @@ -336,7 +338,7 @@ test('rebuilds specific dependencies', async () => { pending: false, registries: modulesManifest!.registries!, storeDir, - allowBuilds: { 'install-scripts-example-for-pnpm': true }, + allowBuilds: { [gitDepPath]: true }, }, ['install-scripts-example-for-pnpm']) project.hasNot('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall') @@ -382,6 +384,7 @@ test('rebuild with pending option', async () => { // not to // install-scripts-example-for-pnpm@https://codeload.github.com/pnpm-e2e/install-scripts-example/tar.gz/b6cfdb8af6f8d5ebc5e7de6831af9d38084d765b expect(modules!.pendingBuilds[1]).toMatch(/^install-scripts-example-for-pnpm@.*b6cfdb8af6f8d5ebc5e7de6831af9d38084d765b.*/) + const gitDepPath = modules!.pendingBuilds[1] project.hasNot('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall') project.hasNot('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall') @@ -396,7 +399,7 @@ test('rebuild with pending option', async () => { pending: true, registries: modules!.registries!, storeDir, - allowBuilds: { '@pnpm.e2e/pre-and-postinstall-scripts-example': true, 'install-scripts-example-for-pnpm': true }, + allowBuilds: { '@pnpm.e2e/pre-and-postinstall-scripts-example': true, [gitDepPath]: true }, }, []) modules = project.readModulesManifest() @@ -538,3 +541,47 @@ test(`rebuild should not fail on incomplete ${WANTED_LOCKFILE}`, async () => { }, []) }) +test('rebuilds in the global virtual store when the approval was granted after the install', async () => { + const project = prepare() + const cacheDir = path.resolve('cache') + const storeDir = path.resolve('store') + + // No allowBuilds at install time: the GVS projection is created under the + // not-built hash. The approval arrives only at rebuild time, so the rebuild + // recomputes a different (built) hash and must locate the existing + // projection through the project's node_modules link. + fs.writeFileSync('pnpm-workspace.yaml', [ + 'enableGlobalVirtualStore: true', + '', + ].join('\n')) + + await execa('node', [ + pnpmBin, + 'add', + '--save-dev', + '@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0', + `--registry=${REGISTRY}`, + `--store-dir=${storeDir}`, + '--ignore-scripts', + `--cache-dir=${cacheDir}`, + ]) + + const pkgVersionDir = path.join(storeDir, STORE_VERSION, 'links/@pnpm.e2e/pre-and-postinstall-scripts-example/1.0.0') + const hash = fs.readdirSync(pkgVersionDir)[0] + const pkgInGvs = path.join(pkgVersionDir, hash, 'node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example') + expect(fs.existsSync(path.join(pkgInGvs, 'generated-by-postinstall.js'))).toBeFalsy() + + const modulesManifest = project.readModulesManifest() + await rebuild.handler({ + ...DEFAULT_OPTS, + cacheDir, + dir: process.cwd(), + enableGlobalVirtualStore: true, + pending: false, + registries: modulesManifest!.registries!, + storeDir, + allowBuilds: { '@pnpm.e2e/pre-and-postinstall-scripts-example': true }, + }, []) + + expect(fs.existsSync(path.join(pkgInGvs, 'generated-by-postinstall.js'))).toBeTruthy() +}) diff --git a/building/commands/tsconfig.json b/building/commands/tsconfig.json index 4d75f9b864..d9f2b9b990 100644 --- a/building/commands/tsconfig.json +++ b/building/commands/tsconfig.json @@ -86,6 +86,9 @@ }, { "path": "../after-install" + }, + { + "path": "../policy" } ] } diff --git a/building/during-install/src/index.ts b/building/during-install/src/index.ts index e9f5b2f7ac..37463b5d37 100644 --- a/building/during-install/src/index.ts +++ b/building/during-install/src/index.ts @@ -91,7 +91,7 @@ export async function buildModules ( if (!ignoreScripts) { const node = depGraph[depPath] if (node.requiresBuild) { - const allowed = allowBuild(node.name, node.version) + const allowed = allowBuild(node.depPath) switch (allowed) { case false: // Explicitly disallowed - don't report as ignored diff --git a/building/policy/src/index.ts b/building/policy/src/index.ts index d63afa9058..cd212006c8 100644 --- a/building/policy/src/index.ts +++ b/building/policy/src/index.ts @@ -1,12 +1,9 @@ import { expandPackageVersionSpecs } from '@pnpm/config.version-policy' import * as dp from '@pnpm/deps.path' -import type { AllowBuild, DepPath } from '@pnpm/types' +import type { AllowBuild, AllowBuildContext, DepPath } from '@pnpm/types' export function isBuildExplicitlyDisallowed (depPath: DepPath, allowBuild?: AllowBuild): boolean { - if (!allowBuild) return false - const { name, version } = dp.parse(depPath) - if (!name || !version) return false - return allowBuild(name, version) === false + return allowBuild?.(depPath) === false } export function createAllowBuildFunction ( @@ -17,26 +14,59 @@ export function createAllowBuildFunction ( ): undefined | AllowBuild { if (opts.dangerouslyAllowAllBuilds) return () => true if (opts.allowBuilds != null) { - const allowedBuilds = new Set() - const disallowedBuilds = new Set() + const allowedPackageBuilds = new Set() + const disallowedPackageBuilds = new Set() + const allowedDepPathBuilds = new Set() + const disallowedDepPathBuilds = new Set() for (const [pkg, value] of Object.entries(opts.allowBuilds)) { switch (value) { case true: - allowedBuilds.add(pkg) + addAllowBuildRule(pkg, { + depPaths: allowedDepPathBuilds, + packageSpecs: allowedPackageBuilds, + }) break case false: - disallowedBuilds.add(pkg) + addAllowBuildRule(pkg, { + depPaths: disallowedDepPathBuilds, + packageSpecs: disallowedPackageBuilds, + }) break } } - const expandedAllowed = expandPackageVersionSpecs(Array.from(allowedBuilds)) - const expandedDisallowed = expandPackageVersionSpecs(Array.from(disallowedBuilds)) - return (pkgName, version) => { - const pkgWithVersion = `${pkgName}@${version}` - if (expandedDisallowed.has(pkgName) || expandedDisallowed.has(pkgWithVersion)) { + const expandedAllowed = expandPackageVersionSpecs(Array.from(allowedPackageBuilds)) + const expandedDisallowed = expandPackageVersionSpecs(Array.from(disallowedPackageBuilds)) + return (depPath, context?: AllowBuildContext) => { + const pkgIdWithPatchHash = dp.getPkgIdWithPatchHash(depPath) + if (disallowedDepPathBuilds.has(pkgIdWithPatchHash)) { return false } - if (expandedAllowed.has(pkgName) || expandedAllowed.has(pkgWithVersion)) { + const { name, version, nonSemverVersion } = dp.parse(depPath) + const nameAtVersion = name != null && version != null ? `${name}@${version}` : undefined + if ( + (name != null && expandedDisallowed.has(name)) || + (nameAtVersion != null && expandedDisallowed.has(nameAtVersion)) + ) { + return false + } + if (allowedDepPathBuilds.has(pkgIdWithPatchHash)) { + return true + } + // Package-name rules require a trusted package identity. A + // registry-style depPath (name@semver) is the trust signal: the + // lockfile verification gate rejects lockfiles where such a key is + // backed by a non-registry resolution, so by the time scripts can + // run, the shape proves the artifact came from a registry. The + // override exists for callers that must evaluate name rules under + // legacy semantics (e.g. comparing against a policy recorded before + // identity trust existed). + const trustPackageIdentity = context?.trustPackageIdentity ?? + (name != null && version != null && nonSemverVersion == null) + if (!trustPackageIdentity) return undefined + if ( + (name != null && expandedAllowed.has(name)) || + (nameAtVersion != null && expandedAllowed.has(nameAtVersion)) + ) { return true } return undefined @@ -44,3 +74,42 @@ export function createAllowBuildFunction ( } return undefined } + +/** + * The `allowBuilds` key under which an ignored build should be approved: + * the package name for registry packages, the peer-suffix-free depPath for + * git/tarball artifacts, whose name alone must not approve builds. + */ +export function allowBuildKeyFromIgnoredBuild (depPath: DepPath): string { + const pkgIdWithPatchHash = dp.getPkgIdWithPatchHash(depPath) + const parsed = dp.parse(pkgIdWithPatchHash) + if (parsed.nonSemverVersion != null || parsed.name == null) return pkgIdWithPatchHash + return parsed.name +} + +function addAllowBuildRule ( + pkg: string, + target: { + depPaths: Set + packageSpecs: Set + } +): void { + if (isDepPathAllowBuildKey(pkg)) { + target.depPaths.add(dp.removePeersSuffix(pkg)) + } else { + target.packageSpecs.add(pkg) + } +} + +function isDepPathAllowBuildKey (pkg: string): boolean { + if (dp.removePeersSuffix(pkg) !== pkg) return true + if (pkg.includes('||')) return false + const parsed = dp.parse(pkg) + if (parsed.nonSemverVersion != null) return isSourceLikeDepPathVersion(parsed.nonSemverVersion) + if (parsed.name != null || pkg.startsWith('@')) return false + return pkg.includes('/') || pkg.includes(':') +} + +function isSourceLikeDepPathVersion (version: string): boolean { + return version.includes(':') || version.includes('/') || version.includes('#') +} diff --git a/building/policy/test/index.ts b/building/policy/test/index.ts index 48ed94f283..20037927ec 100644 --- a/building/policy/test/index.ts +++ b/building/policy/test/index.ts @@ -2,16 +2,20 @@ import { expect, it } from '@jest/globals' import { createAllowBuildFunction, isBuildExplicitlyDisallowed } from '@pnpm/building.policy' import type { DepPath } from '@pnpm/types' +function depPath (value: string): DepPath { + return value as DepPath +} + it('should allowBuilds with true value', () => { const allowBuild = createAllowBuildFunction({ allowBuilds: { foo: true, 'qar@1.0.0 || 2.0.0': true }, }) expect(typeof allowBuild).toBe('function') - expect(allowBuild!('foo', '1.0.0')).toBe(true) - expect(allowBuild!('bar', '1.0.0')).toBeUndefined() - expect(allowBuild!('qar', '1.1.0')).toBeUndefined() - expect(allowBuild!('qar', '1.0.0')).toBe(true) - expect(allowBuild!('qar', '2.0.0')).toBe(true) + expect(allowBuild!(depPath('foo@1.0.0'))).toBe(true) + expect(allowBuild!(depPath('bar@1.0.0'))).toBeUndefined() + expect(allowBuild!(depPath('qar@1.1.0'))).toBeUndefined() + expect(allowBuild!(depPath('qar@1.0.0'))).toBe(true) + expect(allowBuild!(depPath('qar@2.0.0'))).toBe(true) }) it('should allowBuilds with false value', () => { @@ -19,9 +23,9 @@ it('should allowBuilds with false value', () => { allowBuilds: { foo: false, bar: true }, }) expect(typeof allowBuild).toBe('function') - expect(allowBuild!('foo', '1.0.0')).toBe(false) - expect(allowBuild!('bar', '1.0.0')).toBe(true) - expect(allowBuild!('baz', '1.0.0')).toBeUndefined() + expect(allowBuild!(depPath('foo@1.0.0'))).toBe(false) + expect(allowBuild!(depPath('bar@1.0.0'))).toBe(true) + expect(allowBuild!(depPath('baz@1.0.0'))).toBeUndefined() }) it('should not allow patterns in allowBuilds', () => { @@ -29,7 +33,13 @@ it('should not allow patterns in allowBuilds', () => { allowBuilds: { 'is-*': true }, }) expect(typeof allowBuild).toBe('function') - expect(allowBuild!('is-odd', '1.0.0')).toBeUndefined() + expect(allowBuild!(depPath('is-odd@1.0.0'))).toBeUndefined() +}) + +it('should reject invalid package versions in allowBuilds', () => { + expect(() => createAllowBuildFunction({ + allowBuilds: { 'foo@not-a-version': true }, + })).toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_VERSION_UNION' })) }) it('should return undefined if no policy is set', () => { @@ -41,23 +51,81 @@ it('should allow everything when dangerouslyAllowAllBuilds is true', () => { dangerouslyAllowAllBuilds: true, }) expect(typeof allowBuild).toBe('function') - expect(allowBuild!('foo', '1.0.0')).toBeTruthy() + expect(allowBuild!(depPath('foo@1.0.0'))).toBeTruthy() + expect(allowBuild!(depPath('foo@git+https://github.com/org/foo.git#abc123'))).toBeTruthy() +}) + +it('should not apply package-name rules to artifact depPaths', () => { + const allowBuild = createAllowBuildFunction({ + allowBuilds: { foo: true, bar: true }, + }) + expect(allowBuild!(depPath('foo@git+https://github.com/org/foo.git#abc123'))).toBeUndefined() + expect(allowBuild!(depPath('bar@https://example.com/bar.tgz'))).toBeUndefined() + expect(allowBuild!(depPath('foo@1.0.0'))).toBe(true) +}) + +it('should apply package-name rules to artifact depPaths when identity trust is overridden', () => { + const allowBuild = createAllowBuildFunction({ + allowBuilds: { foo: true }, + }) + expect(allowBuild!(depPath('foo@git+https://github.com/org/foo.git#abc123'), { + trustPackageIdentity: true, + })).toBe(true) + expect(allowBuild!(depPath('foo@1.0.0'), { trustPackageIdentity: false })).toBeUndefined() +}) + +it('should deny by package name regardless of identity trust', () => { + const allowBuild = createAllowBuildFunction({ + allowBuilds: { foo: false }, + }) + expect(allowBuild!(depPath('foo@git+https://github.com/org/foo.git#abc123'))).toBe(false) + expect(allowBuild!(depPath('foo@1.0.0'))).toBe(false) +}) + +it('should allow artifact depPaths by depPath key', () => { + const allowBuild = createAllowBuildFunction({ + allowBuilds: { + 'foo@git+https://github.com/org/foo.git#abc123': true, + 'bar@https://codeload.github.com/org/bar/tar.gz/abc123': false, + foo: true, + }, + }) + expect(allowBuild!(depPath('foo@git+https://github.com/org/foo.git#abc123(react@19.0.0)'))).toBe(true) + expect(allowBuild!(depPath('foo@git+https://github.com/attacker/foo.git#abc123'))).toBeUndefined() + expect(allowBuild!(depPath('bar@https://codeload.github.com/org/bar/tar.gz/abc123'))).toBe(false) +}) + +it('should preserve patch hash in depPath allowBuild keys', () => { + const allowBuild = createAllowBuildFunction({ + allowBuilds: { + 'foo@https://example.com/foo.tgz(patch_hash=aaaa)': true, + }, + }) + expect(allowBuild!(depPath('foo@https://example.com/foo.tgz(patch_hash=aaaa)(react@19.0.0)'))).toBe(true) + expect(allowBuild!(depPath('foo@https://example.com/foo.tgz(patch_hash=bbbb)(react@19.0.0)'))).toBeUndefined() +}) + +it('should allow untrusted package identity by source-only depPath', () => { + const allowBuild = createAllowBuildFunction({ + allowBuilds: { 'github.com/org/foo/abc123': true }, + }) + expect(allowBuild!(depPath('github.com/org/foo/abc123(react@19.0.0)'))).toBe(true) }) it('isBuildExplicitlyDisallowed() flags only builds the policy explicitly forbids', () => { const allowBuild = createAllowBuildFunction({ allowBuilds: { foo: false, bar: true }, }) - expect(isBuildExplicitlyDisallowed('foo@1.0.0' as DepPath, allowBuild)).toBe(true) - expect(isBuildExplicitlyDisallowed('bar@1.0.0' as DepPath, allowBuild)).toBe(false) - expect(isBuildExplicitlyDisallowed('baz@1.0.0' as DepPath, allowBuild)).toBe(false) + expect(isBuildExplicitlyDisallowed(depPath('foo@1.0.0'), allowBuild)).toBe(true) + expect(isBuildExplicitlyDisallowed(depPath('bar@1.0.0'), allowBuild)).toBe(false) + expect(isBuildExplicitlyDisallowed(depPath('baz@1.0.0'), allowBuild)).toBe(false) }) it('isBuildExplicitlyDisallowed() returns false when no policy is set', () => { - expect(isBuildExplicitlyDisallowed('foo@1.0.0' as DepPath, undefined)).toBe(false) + expect(isBuildExplicitlyDisallowed(depPath('foo@1.0.0'), undefined)).toBe(false) }) it('isBuildExplicitlyDisallowed() returns false for unparsable depPaths', () => { const allowBuild = createAllowBuildFunction({ allowBuilds: { foo: false } }) - expect(isBuildExplicitlyDisallowed('not-a-valid-dep-path' as DepPath, allowBuild)).toBe(false) + expect(isBuildExplicitlyDisallowed(depPath('not-a-valid-dep-path'), allowBuild)).toBe(false) }) diff --git a/core/types/src/config.ts b/core/types/src/config.ts index e70c4e6332..6fe6a24ac6 100644 --- a/core/types/src/config.ts +++ b/core/types/src/config.ts @@ -1,5 +1,11 @@ +import type { DepPath } from './misc.js' + export type PackageVersionPolicy = (pkgName: string) => boolean | string[] -export type AllowBuild = (pkgName: string, pkgVersion: string) => boolean | undefined +export interface AllowBuildContext { + trustPackageIdentity?: boolean +} + +export type AllowBuild = (depPath: DepPath, context?: AllowBuildContext) => boolean | undefined export type TrustPolicy = 'no-downgrade' | 'off' diff --git a/deps/compliance/commands/test/licenses/fixtures/with-git-protocol-peer-deps/package.json b/deps/compliance/commands/test/licenses/fixtures/with-git-protocol-peer-deps/package.json index 6bdb81b2fc..24d78025ef 100644 --- a/deps/compliance/commands/test/licenses/fixtures/with-git-protocol-peer-deps/package.json +++ b/deps/compliance/commands/test/licenses/fixtures/with-git-protocol-peer-deps/package.json @@ -2,6 +2,6 @@ "name": "with-git-protocol-peer-deps", "version": "1.0.0", "dependencies": { - "ajv-keywords": "github:ajv-validator/ajv-keywords" + "ajv-keywords": "github:ajv-validator/ajv-keywords#a11389b4d1934d360fb2a24dd920ec597295c8fc" } } diff --git a/deps/compliance/commands/test/licenses/index.ts b/deps/compliance/commands/test/licenses/index.ts index 6a00a404b9..2e50938a17 100644 --- a/deps/compliance/commands/test/licenses/index.ts +++ b/deps/compliance/commands/test/licenses/index.ts @@ -313,7 +313,9 @@ test('pnpm licenses should work with git protocol dep that have peerDependencies await install.handler({ ...DEFAULT_OPTS, dir: workspaceDir, - allowBuilds: { 'ajv-keywords': true }, + allowBuilds: { + 'ajv-keywords@https://codeload.github.com/ajv-validator/ajv-keywords/tar.gz/a11389b4d1934d360fb2a24dd920ec597295c8fc': true, + }, pnpmHomeDir: '', storeDir, }) diff --git a/deps/graph-builder/src/lockfileToDepGraph.ts b/deps/graph-builder/src/lockfileToDepGraph.ts index 0d2ddf4e2a..4209f0831a 100644 --- a/deps/graph-builder/src/lockfileToDepGraph.ts +++ b/deps/graph-builder/src/lockfileToDepGraph.ts @@ -241,7 +241,7 @@ async function buildGraphFromPackages ( // marker indicating a previous build failed or was interrupted. When the // marker is present, skip the fast path to force a re-fetch/re-import/re-build. const mightNeedBuild = opts.enableGlobalVirtualStore && - opts.allowBuild?.(pkgName, pkgVersion) === true + opts.allowBuild?.(depPath) === true let dirExists: boolean | undefined if ( diff --git a/deps/graph-hasher/src/index.ts b/deps/graph-hasher/src/index.ts index 42b726ada1..202bda0320 100644 --- a/deps/graph-hasher/src/index.ts +++ b/deps/graph-hasher/src/index.ts @@ -331,13 +331,13 @@ export function lockfileToDepGraph ( } function computeBuiltDepPaths ( - entries: Iterable<{ depPath: DepPath; name: string; version: string }>, + entries: Iterable, allowBuild: AllowBuild ): Set { const builtDepPaths = new Set() - for (const { depPath, name, version } of entries) { - if (allowBuild(name, version) === true) { - builtDepPaths.add(depPath) + for (const entry of entries) { + if (allowBuild(entry.depPath) === true) { + builtDepPaths.add(entry.depPath) } } return builtDepPaths diff --git a/deps/graph-hasher/test/allowBuildIdentity.test.ts b/deps/graph-hasher/test/allowBuildIdentity.test.ts new file mode 100644 index 0000000000..8ff6621878 --- /dev/null +++ b/deps/graph-hasher/test/allowBuildIdentity.test.ts @@ -0,0 +1,41 @@ +import { expect, it } from '@jest/globals' +import { iterateHashedGraphNodes } from '@pnpm/deps.graph-hasher' +import type { AllowBuild, DepPath } from '@pnpm/types' + +it('gates built dep paths through the allowBuild policy by depPath', () => { + const registryDepPath = 'foo@1.0.0' as DepPath + const directTarballDepPath = 'foo@https://example.com/foo.tgz' as DepPath + const checkedDepPaths: DepPath[] = [] + const allowBuild: AllowBuild = (depPath) => { + checkedDepPaths.push(depPath) + return depPath === registryDepPath + } + + Array.from(iterateHashedGraphNodes( + { + [registryDepPath]: { + children: {}, + fullPkgId: 'foo@1.0.0:sha512-abc', + }, + [directTarballDepPath]: { + children: {}, + fullPkgId: 'foo@1.0.0:sha512-def', + }, + }, + [ + { + depPath: registryDepPath, + name: 'foo', + version: '1.0.0', + }, + { + depPath: directTarballDepPath, + name: 'foo', + version: '1.0.0', + }, + ].values(), + allowBuild + )) + + expect(checkedDepPaths).toStrictEqual([registryDepPath, directTarballDepPath]) +}) diff --git a/deps/path/src/index.ts b/deps/path/src/index.ts index 0deebc4e18..62ea607853 100644 --- a/deps/path/src/index.ts +++ b/deps/path/src/index.ts @@ -60,13 +60,16 @@ export function removeSuffix (relDepPath: string): string { return relDepPath } -export function getPkgIdWithPatchHash (depPath: DepPath): PkgIdWithPatchHash { - let pkgId: string = depPath - const { peersIndex: sepIndex } = indexOfDepPathSuffix(pkgId) - if (sepIndex !== -1) { - pkgId = pkgId.substring(0, sepIndex) +export function removePeersSuffix (relDepPath: string): string { + const { peersIndex } = indexOfDepPathSuffix(relDepPath) + if (peersIndex !== -1) { + return relDepPath.substring(0, peersIndex) } - return pkgId as PkgIdWithPatchHash + return relDepPath +} + +export function getPkgIdWithPatchHash (depPath: DepPath): PkgIdWithPatchHash { + return removePeersSuffix(depPath) as PkgIdWithPatchHash } export function tryGetPackageId (relDepPath: DepPath): PkgId { diff --git a/exec/commands/test/dlx.e2e.ts b/exec/commands/test/dlx.e2e.ts index 6dd8a044b1..6936d6848a 100644 --- a/exec/commands/test/dlx.e2e.ts +++ b/exec/commands/test/dlx.e2e.ts @@ -106,7 +106,9 @@ test('dlx install from git', async () => { dir: process.cwd(), storeDir: path.resolve('store'), cacheDir: path.resolve('cache'), - allowBuild: ['shx'], + // A git-hosted artifact has an untrusted package identity, so it has to + // be approved by its depPath, not by package name. + allowBuild: ['shx@https://codeload.github.com/shelljs/shx/tar.gz/0dcbb9d1022037268959f8b706e0f06a6fd43fde'], }, ['shelljs/shx#0dcbb9d1022037268959f8b706e0f06a6fd43fde', 'touch', 'foo']) expect(fs.existsSync('foo')).toBeTruthy() diff --git a/exec/prepare-package/src/index.ts b/exec/prepare-package/src/index.ts index e98b1ab82d..4efa6085f8 100644 --- a/exec/prepare-package/src/index.ts +++ b/exec/prepare-package/src/index.ts @@ -6,7 +6,7 @@ import util from 'node:util' import { PnpmError } from '@pnpm/error' import { runLifecycleHook, type RunLifecycleHookOptions } from '@pnpm/exec.lifecycle' import { safeReadPackageJsonFromDir } from '@pnpm/pkg-manifest.reader' -import type { AllowBuild, PackageManifest } from '@pnpm/types' +import type { AllowBuild, DepPath, PackageManifest } from '@pnpm/types' import { rimraf } from '@zkochan/rimraf' import { preferredPM } from 'preferred-pm' @@ -22,6 +22,7 @@ const PREPUBLISH_SCRIPTS = [ export interface PreparePackageOptions { allowBuild?: AllowBuild ignoreScripts?: boolean + pkgResolutionId: string unsafePerm?: boolean userAgent?: string } @@ -32,15 +33,19 @@ export async function preparePackage (opts: PreparePackageOptions, gitRootDir: s if (manifest?.scripts == null || !packageShouldBeBuilt(manifest, pkgDir)) return { shouldBeBuilt: false, pkgDir } if (opts.ignoreScripts) return { shouldBeBuilt: true, pkgDir } // Check if the package is allowed to run build scripts - // If allowBuild is undefined or returns false, block the build - if (!opts.allowBuild?.(manifest.name, manifest.version)) { + // If allowBuild is undefined or returns false, block the build. + // The depPath is synthesized from the resolution id rather than read from + // a lockfile; resolution ids of git and tarball artifacts are never + // semver-shaped, so the policy derives an untrusted package identity. + const depPath = `${manifest.name}@${opts.pkgResolutionId}` as DepPath + if (!opts.allowBuild?.(depPath)) { throw new PnpmError( 'GIT_DEP_PREPARE_NOT_ALLOWED', `The git-hosted package "${manifest.name}@${manifest.version}" needs to execute build scripts but is not in the "allowBuilds" allowlist.`, { hint: `Add the package to "allowBuilds" in your project's pnpm-workspace.yaml to allow it to run scripts. For example: allowBuilds: - ${manifest.name}: true`, + ${depPath}: true`, } ) } diff --git a/exec/prepare-package/test/index.ts b/exec/prepare-package/test/index.ts index 641d3552a2..bccc1cf200 100644 --- a/exec/prepare-package/test/index.ts +++ b/exec/prepare-package/test/index.ts @@ -7,23 +7,35 @@ import { fixtures } from '@pnpm/test-fixtures' import { createTestIpcServer } from '@pnpm/test-ipc-server' const f = fixtures(import.meta.dirname) +const pkgResolutionId = 'https://codeload.example.com/org/repo/tar.gz/0000000000000000000000000000000000000000' const allowBuild = () => true +const allowRegistryArtifactsOnly = (depPath: string) => !depPath.includes('://') test('prepare package runs the prepublish script', async () => { const tmp = tempDir() await using server = await createTestIpcServer(path.join(tmp, 'test.sock')) f.copy('has-prepublish-script', tmp) - await preparePackage({ allowBuild }, tmp, '') + await preparePackage({ allowBuild, pkgResolutionId }, tmp, '') expect(server.getLines()).toStrictEqual([ 'prepublish', ]) }) +test('prepare package gates the build on the artifact depPath', async () => { + const tmp = tempDir() + f.copy('has-prepublish-script', tmp) + + await expect(preparePackage({ + allowBuild: allowRegistryArtifactsOnly, + pkgResolutionId, + }, tmp, '')).rejects.toThrow('needs to execute build scripts but is not in the "allowBuilds" allowlist') +}) + test('prepare package does not run the prepublish script if the main file is present', async () => { const tmp = tempDir() await using server = await createTestIpcServer(path.join(tmp, 'test.sock')) f.copy('has-prepublish-script-and-main-file', tmp) - await preparePackage({ allowBuild }, tmp, '') + await preparePackage({ allowBuild, pkgResolutionId }, tmp, '') expect(server.getLines()).toStrictEqual([ 'prepublish', ]) @@ -33,7 +45,7 @@ test('prepare package runs the prepublish script in the sub folder if pkgDir is const tmp = tempDir() await using server = await createTestIpcServer(path.join(tmp, 'test.sock')) f.copy('has-prepublish-script-in-workspace', tmp) - await preparePackage({ allowBuild }, tmp, 'packages/foo') + await preparePackage({ allowBuild, pkgResolutionId }, tmp, 'packages/foo') expect(server.getLines()).toStrictEqual([ 'prepublish', ]) diff --git a/fetching/git-fetcher/package.json b/fetching/git-fetcher/package.json index 8bf4825fe1..31ba48036d 100644 --- a/fetching/git-fetcher/package.json +++ b/fetching/git-fetcher/package.json @@ -36,6 +36,7 @@ "@pnpm/exec.prepare-package": "workspace:*", "@pnpm/fetching.fetcher-base": "workspace:*", "@pnpm/fs.packlist": "workspace:*", + "@pnpm/resolving.git-resolver": "workspace:*", "@pnpm/store.index": "workspace:*", "@zkochan/rimraf": "catalog:", "execa": "catalog:" diff --git a/fetching/git-fetcher/src/index.ts b/fetching/git-fetcher/src/index.ts index 1d35259886..887ecdc61b 100644 --- a/fetching/git-fetcher/src/index.ts +++ b/fetching/git-fetcher/src/index.ts @@ -8,6 +8,7 @@ import { preparePackage } from '@pnpm/exec.prepare-package' import type { GitFetcher } from '@pnpm/fetching.fetcher-base' import { packlist } from '@pnpm/fs.packlist' import { globalWarn } from '@pnpm/logger' +import { createGitHostedPkgId } from '@pnpm/resolving.git-resolver' import type { StoreIndex } from '@pnpm/store.index' import { addFilesFromDir } from '@pnpm/worker' import { rimraf } from '@zkochan/rimraf' @@ -47,6 +48,7 @@ export function createGitFetcher (createOpts: CreateGitFetcherOptions): { git: G const prepareResult = await preparePackage({ allowBuild: opts.allowBuild, ignoreScripts: createOpts.ignoreScripts, + pkgResolutionId: createGitHostedPkgId(resolution), unsafePerm: createOpts.unsafePerm, userAgent: createOpts.userAgent, }, tempLocation, resolution.path ?? '') diff --git a/fetching/git-fetcher/test/index.ts b/fetching/git-fetcher/test/index.ts index fdc11afe28..f69e33a320 100644 --- a/fetching/git-fetcher/test/index.ts +++ b/fetching/git-fetcher/test/index.ts @@ -139,7 +139,7 @@ test('fetch a package from Git that has a prepare script', async () => { type: 'git', }, { - allowBuild: (pkgName) => pkgName === 'test-git-fetch', + allowBuild: (depPath) => depPath.startsWith('test-git-fetch@'), filesIndexFile: path.join(storeDir, 'index.json'), } ) @@ -221,7 +221,7 @@ test('fail when preparing a git-hosted package', async () => { repo: 'https://github.com/pnpm-e2e/prepare-script-fails.git', type: 'git', }, { - allowBuild: (pkgName) => pkgName === '@pnpm.e2e/prepare-script-fails', + allowBuild: (depPath) => depPath.startsWith('@pnpm.e2e/prepare-script-fails@'), filesIndexFile: path.join(storeDir, 'index.json'), }) ).rejects.toThrow('Failed to prepare git-hosted package fetched from "https://github.com/pnpm-e2e/prepare-script-fails.git": @pnpm.e2e/prepare-script-fails@1.0.0 npm-install: `npm install`') @@ -306,7 +306,7 @@ test('allow git package with prepare script', async () => { repo: 'https://github.com/pnpm-e2e/prepare-script-works.git', type: 'git', }, { - allowBuild: (pkgName) => pkgName === '@pnpm.e2e/prepare-script-works', + allowBuild: (depPath) => depPath.startsWith('@pnpm.e2e/prepare-script-works@'), filesIndexFile: path.join(storeDir, 'index.json'), }) expect(filesMap.has('package.json')).toBeTruthy() diff --git a/fetching/git-fetcher/tsconfig.json b/fetching/git-fetcher/tsconfig.json index 6b7c1a567f..49299dbc1b 100644 --- a/fetching/git-fetcher/tsconfig.json +++ b/fetching/git-fetcher/tsconfig.json @@ -24,6 +24,9 @@ { "path": "../../fs/packlist" }, + { + "path": "../../resolving/git-resolver" + }, { "path": "../../store/cafs" }, diff --git a/fetching/tarball-fetcher/src/gitHostedTarballFetcher.ts b/fetching/tarball-fetcher/src/gitHostedTarballFetcher.ts index c570b8391a..ba4cc50f22 100644 --- a/fetching/tarball-fetcher/src/gitHostedTarballFetcher.ts +++ b/fetching/tarball-fetcher/src/gitHostedTarballFetcher.ts @@ -83,6 +83,7 @@ async function prepareGitHostedPkg ( const { shouldBeBuilt, pkgDir } = await preparePackage({ ...opts, allowBuild: fetcherOpts.allowBuild, + pkgResolutionId: createGitHostedTarballPkgResolutionId(resolution), }, tempLocation, resolution.path ?? '') const files = await packlist(pkgDir) const { storeIndex } = opts @@ -123,3 +124,11 @@ async function prepareGitHostedPkg ( ignoredBuild: Boolean(opts.ignoreScripts), } } + +function createGitHostedTarballPkgResolutionId (resolution: Resolution): string { + let pkgResolutionId = resolution.tarball + if (resolution.path) { + pkgResolutionId += `#path:${resolution.path}` + } + return pkgResolutionId +} diff --git a/fetching/tarball-fetcher/test/fetch.ts b/fetching/tarball-fetcher/test/fetch.ts index 065db6dc56..63ac6a306c 100644 --- a/fetching/tarball-fetcher/test/fetch.ts +++ b/fetching/tarball-fetcher/test/fetch.ts @@ -573,7 +573,7 @@ test('fail when preparing a git-hosted package', async () => { await expect( fetch.gitHostedTarball(cafs, resolution, { - allowBuild: (pkgName) => pkgName === '@pnpm.e2e/prepare-script-fails', + allowBuild: (depPath) => depPath.startsWith('@pnpm.e2e/prepare-script-fails@'), filesIndexFile, lockfileDir: process.cwd(), pkg, diff --git a/installing/commands/package.json b/installing/commands/package.json index e8b23a42b9..9448482078 100644 --- a/installing/commands/package.json +++ b/installing/commands/package.json @@ -34,6 +34,7 @@ "dependencies": { "@inquirer/prompts": "catalog:", "@pnpm/building.after-install": "workspace:*", + "@pnpm/building.policy": "workspace:*", "@pnpm/catalogs.types": "workspace:*", "@pnpm/cli.command": "workspace:*", "@pnpm/cli.common-cli-options-help": "workspace:*", diff --git a/installing/commands/src/handleIgnoredBuilds.ts b/installing/commands/src/handleIgnoredBuilds.ts index 9e1feee09a..3ca11963fc 100644 --- a/installing/commands/src/handleIgnoredBuilds.ts +++ b/installing/commands/src/handleIgnoredBuilds.ts @@ -1,5 +1,5 @@ +import { allowBuildKeyFromIgnoredBuild } from '@pnpm/building.policy' import { writeSettings } from '@pnpm/config.writer' -import { parse } from '@pnpm/deps.path' import { IgnoredBuildsError, } from '@pnpm/installing.deps-installer' @@ -47,5 +47,5 @@ async function writeIgnoredBuildsToAllowBuilds ( } function packageNamesFromIgnoredBuilds (ignoredBuilds: IgnoredBuilds): string[] { - return Array.from(new Set(Array.from(ignoredBuilds).map((dp) => parse(dp).name ?? dp))).sort(lexCompare) + return Array.from(new Set(Array.from(ignoredBuilds).map(allowBuildKeyFromIgnoredBuild))).sort(lexCompare) } diff --git a/installing/commands/tsconfig.json b/installing/commands/tsconfig.json index 1867f41169..8d30b0d2d1 100644 --- a/installing/commands/tsconfig.json +++ b/installing/commands/tsconfig.json @@ -24,6 +24,9 @@ { "path": "../../building/after-install" }, + { + "path": "../../building/policy" + }, { "path": "../../catalogs/types" }, diff --git a/installing/deps-installer/src/install/index.ts b/installing/deps-installer/src/install/index.ts index 9cbd21e76e..2385860a84 100644 --- a/installing/deps-installer/src/install/index.ts +++ b/installing/deps-installer/src/install/index.ts @@ -465,7 +465,7 @@ export async function mutateModules ( let ignoredBuilds = result.ignoredBuilds if (!opts.ignoreScripts && ignoredBuilds?.size) { - ignoredBuilds = await runUnignoredDependencyBuilds(opts, ignoredBuilds, allowBuild) + ignoredBuilds = await runUnignoredDependencyBuilds(opts, ignoredBuilds, ctx.wantedLockfile, allowBuild) } // Detect packages whose build approval was revoked between the previous // and current install. A package is considered revoked when it was @@ -481,9 +481,12 @@ export async function mutateModules ( if (oldAllowBuild) { for (const depPath of Object.keys(ctx.wantedLockfile.packages) as DepPath[]) { if (ignoredBuilds?.has(depPath)) continue - const { name, version } = dp.parse(depPath) - if (!name || !version) continue - if (oldAllowBuild(name, version) === true && allowBuild?.(name, version) === undefined) { + // The old policy is evaluated with identity trust overridden so that + // package-name approvals count as they did when they were granted, + // even for git/tarball artifacts that the current policy no longer + // approves by name. + if (oldAllowBuild(depPath, { trustPackageIdentity: true }) !== true) continue + if (allowBuild?.(depPath) === undefined) { ignoredBuilds ??= new Set() ignoredBuilds.add(depPath) } @@ -1091,6 +1094,7 @@ Note that in CI environments, this setting is enabled by default.`, async function runUnignoredDependencyBuilds ( opts: StrictInstallOptions, previousIgnoredBuilds: IgnoredBuilds, + currentLockfile: LockfileObject, allowBuild?: AllowBuild ): Promise> { if (!allowBuild) { @@ -1098,12 +1102,10 @@ async function runUnignoredDependencyBuilds ( } const pkgsToBuild: string[] = [] for (const ignoredPkg of previousIgnoredBuilds) { - const parsed = dp.parse(ignoredPkg) - if (!parsed.name || !parsed.version) continue - const allowed = allowBuild(parsed.name, parsed.version) - if (allowed === true) { + if (currentLockfile.packages?.[ignoredPkg] == null) continue + if (allowBuild(ignoredPkg) === true) { // Package is explicitly allowed - rebuild it - pkgsToBuild.push(`${parsed.name}@${parsed.version}`) + pkgsToBuild.push(dp.getPkgIdWithPatchHash(ignoredPkg)) } } if (pkgsToBuild.length) { @@ -2049,7 +2051,7 @@ export class IgnoredBuildsError extends PnpmError { } function dedupePackageNamesFromIgnoredBuilds (ignoredBuilds: IgnoredBuilds): string[] { - return Array.from(new Set(Array.from(ignoredBuilds ?? []).map(dp.removeSuffix))).sort(lexCompare) + return Array.from(new Set(Array.from(ignoredBuilds ?? []).map(depPath => dp.getPkgIdWithPatchHash(depPath)))).sort(lexCompare) } /** diff --git a/installing/deps-installer/src/install/link.ts b/installing/deps-installer/src/install/link.ts index e331733b84..9f86709fe8 100644 --- a/installing/deps-installer/src/install/link.ts +++ b/installing/deps-installer/src/install/link.ts @@ -486,7 +486,7 @@ async function linkAllPkgs ( depNode.requiresBuild = files.requiresBuild let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && files.sideEffectsMaps && !isEmpty(files.sideEffectsMaps)) { - if (opts.allowBuild?.(depNode.name, depNode.version) === true) { + if (opts.allowBuild?.(depNode.depPath) === true) { sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.depPath, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built patchFileHash: depNode.patch?.hash, diff --git a/installing/deps-installer/src/install/recordLockfileVerified.ts b/installing/deps-installer/src/install/recordLockfileVerified.ts index f837537b01..f76d5840e5 100644 --- a/installing/deps-installer/src/install/recordLockfileVerified.ts +++ b/installing/deps-installer/src/install/recordLockfileVerified.ts @@ -2,6 +2,7 @@ import { hashObject } from '@pnpm/crypto.object-hasher' import type { LockfileObject } from '@pnpm/lockfile.fs' import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base' +import { withResolutionShapeCacheIdentity } from './verifyLockfileResolutions.js' import { recordVerification } from './verifyLockfileResolutionsCache.js' export interface RecordLockfileVerifiedOptions { @@ -30,7 +31,7 @@ export function recordLockfileVerified (opts: RecordLockfileVerifiedOptions): vo if (!opts.lockfile.packages) return recordVerification(opts.cacheDir, { lockfilePath: opts.lockfilePath, - verifiers: opts.resolutionVerifiers, + verifiers: withResolutionShapeCacheIdentity(opts.resolutionVerifiers), hashLockfile: () => hashObject(opts.lockfile), }) } diff --git a/installing/deps-installer/src/install/verifyLockfileResolutions.ts b/installing/deps-installer/src/install/verifyLockfileResolutions.ts index 74b3b67717..a3f18cd92b 100644 --- a/installing/deps-installer/src/install/verifyLockfileResolutions.ts +++ b/installing/deps-installer/src/install/verifyLockfileResolutions.ts @@ -2,7 +2,7 @@ import { lockfileVerificationLogger } from '@pnpm/core-loggers' import { hashObject } from '@pnpm/crypto.object-hasher' import { PnpmError } from '@pnpm/error' import type { LockfileObject } from '@pnpm/lockfile.fs' -import { nameVerFromPkgSnapshot } from '@pnpm/lockfile.utils' +import { isGitHostedTarballUrl, nameVerFromPkgSnapshot } from '@pnpm/lockfile.utils' import type { Resolution, ResolutionPolicyViolation, @@ -14,6 +14,7 @@ import pLimit from 'p-limit' import { recordVerification, tryLockfileVerificationCache, + type VerifierCacheIdentity, } from './verifyLockfileResolutionsCache.js' // Re-exported for back-compat with the existing import surface. @@ -31,6 +32,25 @@ const MAX_VIOLATIONS_TO_PRINT = 20 // verification pass doesn't push past what the rest of the install respects. const DEFAULT_CONCURRENCY = 16 +export const RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE = 'RESOLUTION_SHAPE_MISMATCH' + +const RESOLUTION_SHAPE_CACHE_IDENTITY: VerifierCacheIdentity = { + policy: { resolutionShapeCheck: true }, + canTrustPastCheck: (cached) => cached.resolutionShapeCheck === true, +} + +/** + * Every verifier list that flows into the verification cache must carry + * the resolution-shape identity, so records written before the shape rule + * existed cannot stat-fast-path around it. Used by the gate itself and by + * {@link recordLockfileVerified}, whose freshly-resolved lockfile satisfies + * the shape invariant by construction (the writer derives every key from + * the resolution it just produced). + */ +export function withResolutionShapeCacheIdentity (verifiers: readonly VerifierCacheIdentity[]): VerifierCacheIdentity[] { + return [...verifiers, RESOLUTION_SHAPE_CACHE_IDENTITY] +} + export interface VerifyLockfileResolutionsOptions { concurrency?: number /** @@ -78,7 +98,6 @@ export async function verifyLockfileResolutions ( verifiers: ResolutionVerifier[], options?: VerifyLockfileResolutionsOptions ): Promise { - if (verifiers.length === 0) return if (!lockfile.packages) return // Caching kicks in only when the caller surfaced both a writable @@ -89,6 +108,8 @@ export async function verifyLockfileResolutions ( ? { cacheDir: options.cacheDir, lockfilePath: options.lockfilePath } : undefined + const cacheVerifiers = withResolutionShapeCacheIdentity(verifiers) + // Cache lookup runs before any registry I/O — the fast path is a // single stat() of the lockfile when the previous install already // verified it under a policy that's at least as strict as today's. @@ -106,7 +127,7 @@ export async function verifyLockfileResolutions ( if (cache) { const result = tryLockfileVerificationCache(cache.cacheDir, { lockfilePath: cache.lockfilePath, - verifiers, + verifiers: cacheVerifiers, hashLockfile, }) if (result.hit) return @@ -120,12 +141,16 @@ export async function verifyLockfileResolutions ( // A degenerate lockfile where every snapshot fails the // name/version extraction (so candidates is empty) skips emission // entirely — no work, no noise. - const candidates = collectCandidates(lockfile) + const { candidates, shapeViolations } = collectCandidates(lockfile) + if (shapeViolations.length > 0) { + throw buildVerificationError(shapeViolations) + } + if (verifiers.length === 0) return if (candidates.size === 0) { if (cache) { recordVerification(cache.cacheDir, { lockfilePath: cache.lockfilePath, - verifiers, + verifiers: cacheVerifiers, hashLockfile, }, cachePrecomputed) } @@ -152,7 +177,7 @@ export async function verifyLockfileResolutions ( if (cache) { recordVerification(cache.cacheDir, { lockfilePath: cache.lockfilePath, - verifiers, + verifiers: cacheVerifiers, hashLockfile, }, cachePrecomputed) } @@ -225,7 +250,49 @@ export async function collectResolutionPolicyViolations ( ): Promise { if (verifiers.length === 0) return [] if (!lockfile.packages) return [] - return iterateLockfileViolations(collectCandidates(lockfile), verifiers, options?.concurrency) + // Shape violations are deliberately not collected here: they are hard + // tampering failures, not policy picks a caller may auto-exclude. + return iterateLockfileViolations(collectCandidates(lockfile).candidates, verifiers, options?.concurrency) +} + +function isRegistryShapedResolution (resolution: unknown): boolean { + if (resolution == null) return true + if (typeof resolution !== 'object') return false + const { type, gitHosted, tarball, variants } = resolution as { + type?: unknown + gitHosted?: unknown + tarball?: unknown + variants?: unknown + } + if (type === 'variations') { + return Array.isArray(variants) && variants.every( + (variant) => isRegistryShapedResolution((variant as { resolution?: unknown })?.resolution) + ) + } + // Custom resolver protocols (`type: 'custom:*'`) are a legitimate + // non-registry source the user opted into. They can only be materialized by + // a project-configured custom fetcher — an unrecognized custom type throws at + // fetch time (see @pnpm/fetching.pick-fetcher) — so a forged custom type + // cannot launder an artifact past this gate into a build. + if (typeof type === 'string' && type.startsWith('custom:')) return true + if (type != null) return false + // Plain tarball / registry resolution. The lockfile is parsed from YAML + // without schema validation, so the `gitHosted` flag is not trustworthy on + // its own: a tampered entry could set a non-boolean (dodging a strict + // `=== true`) or an explicit `false` on a git-host URL (the loader only + // backfills the flag when absent). Treat any non-boolean flag as git-hosted + // and gate on the URL so the verdict never depends on the flag alone. + if (gitHosted != null && (typeof gitHosted !== 'boolean' || gitHosted)) return false + // A registry resolution reconstructs its tarball URL from name+version, so + // an absent/empty `tarball` is registry-shaped. When a URL is present it + // must be an http(s) registry artifact: the npm verifier's tarball-URL + // binding skips non-http(s) schemes (file:, etc.), so a `file:` tarball + // under a name@semver key would otherwise be trusted with no safety net. + if (typeof tarball === 'string' && tarball !== '') { + if (!/^https?:\/\//i.test(tarball)) return false + if (isGitHostedTarballUrl(tarball)) return false + } + return true } interface Candidate { @@ -248,11 +315,26 @@ interface Candidate { // skips the tarball/policy checks, so if the wrong shape wins the dedup the // surviving entry is verified under the wrong rules and the real one is // never checked. -function collectCandidates (lockfile: LockfileObject): Map { +function collectCandidates (lockfile: LockfileObject): { candidates: Map, shapeViolations: ResolutionPolicyViolation[] } { const candidates = new Map() + const shapeViolations: ResolutionPolicyViolation[] = [] for (const [depPath, snapshot] of Object.entries(lockfile.packages ?? {})) { const { name, version, nonSemverVersion } = nameVerFromPkgSnapshot(depPath as DepPath, snapshot) if (!name || !version) continue + // A registry-style depPath (name@semver) must be backed by a + // registry-shaped resolution: the allowBuilds policy derives a + // trusted package identity from that key shape, which is only sound + // while this invariant holds. The check is offline, so it applies + // even when no policy verifiers are active. + if (nonSemverVersion == null && !isRegistryShapedResolution(snapshot.resolution)) { + shapeViolations.push({ + name, + version, + resolution: snapshot.resolution as Resolution, + code: RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE, + reason: 'a registry-style dependency path is backed by a non-registry resolution', + }) + } const key = `${name}@${version}@${nonSemverVersion ?? ''}@${JSON.stringify(snapshot.resolution)}` candidates.set(key, { name, @@ -261,7 +343,7 @@ function collectCandidates (lockfile: LockfileObject): Map { resolution: snapshot.resolution as Resolution, }) } - return candidates + return { candidates, shapeViolations } } async function iterateLockfileViolations ( diff --git a/installing/deps-installer/test/install/lifecycleScripts.ts b/installing/deps-installer/test/install/lifecycleScripts.ts index b555bfbd3c..50e016ab66 100644 --- a/installing/deps-installer/test/install/lifecycleScripts.ts +++ b/installing/deps-installer/test/install/lifecycleScripts.ts @@ -1,5 +1,6 @@ import fs from 'node:fs' import * as path from 'node:path' +import { pathToFileURL } from 'node:url' import { expect, jest, test } from '@jest/globals' import { assertProject } from '@pnpm/assert-project' @@ -10,17 +11,26 @@ import { mutateModules, mutateModulesInSingleProject, } from '@pnpm/installing.deps-installer' +import { streamParser } from '@pnpm/logger' import { prepareEmpty, preparePackages } from '@pnpm/prepare' import { createTestIpcServer } from '@pnpm/test-ipc-server' +import { REGISTRY_MOCK_PORT } from '@pnpm/testing.registry-mock' import type { ProjectRootDir } from '@pnpm/types' import { restartWorkerPool } from '@pnpm/worker' import { rimrafSync } from '@zkochan/rimraf' +import { safeExeca as execa } from 'execa' import isWindows from 'is-windows' import { loadJsonFileSync } from 'load-json-file' import PATH from 'path-name' import { testDefaults } from '../utils/index.js' +// Until a first `data` listener appears, the paused log stream buffers +// every event, and the first test to attach a reporter receives the +// whole backlog from earlier installs that ran without a reporter. Keep +// the stream flowing so each reporter only sees its own installs' events. +streamParser.on('data', () => {}) + const testOnNonWindows = isWindows() ? test.skip : test test('run pre/postinstall scripts', async () => { @@ -310,10 +320,11 @@ test('run lifecycle scripts of dependent packages after running scripts of their test('run prepare script for git-hosted dependencies', async () => { const project = prepareEmpty() + const gitDependency = await createGitPreparePackage() - await addDependenciesToPackage({}, ['pnpm/test-git-fetch#8b333f12d5357f4f25a654c305c826294cb073bf'], testDefaults({ + await addDependenciesToPackage({}, [gitDependency], testDefaults({ fastUnpack: false, - allowBuilds: { 'test-git-fetch': true }, + allowBuilds: { [`test-git-fetch@${gitDependency}`]: true }, })) const scripts = project.requireModule('test-git-fetch/output.json') @@ -328,6 +339,112 @@ test('run prepare script for git-hosted dependencies', async () => { ]) }) +async function createGitPreparePackage (): Promise { + const repoDir = path.resolve('test-git-fetch-src') + fs.mkdirSync(repoDir) + fs.writeFileSync(path.join(repoDir, 'append.js'), [ + 'const fs = require(\'fs\')', + 'const file = \'output.json\'', + 'let scripts = []', + 'try { scripts = JSON.parse(fs.readFileSync(file, \'utf8\')) } catch {}', + 'scripts.push(process.argv[2])', + 'fs.writeFileSync(file, JSON.stringify(scripts))', + '', + ].join('\n')) + fs.writeFileSync(path.join(repoDir, 'index.js'), "module.exports = 'ok'\n") + fs.writeFileSync(path.join(repoDir, 'package.json'), JSON.stringify({ + name: 'test-git-fetch', + version: '1.0.0', + main: 'index.js', + scripts: { + prepare: 'node append prepare', + preinstall: 'node append preinstall', + install: 'node append install', + postinstall: 'node append postinstall', + }, + })) + fs.writeFileSync(path.join(repoDir, 'package-lock.json'), JSON.stringify({ + name: 'test-git-fetch', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'test-git-fetch', + version: '1.0.0', + }, + }, + })) + await execa('git', ['init', '-q', '-b', 'main'], { cwd: repoDir }) + await execa('git', ['config', 'user.email', 'test@example.invalid'], { cwd: repoDir }) + await execa('git', ['config', 'user.name', 'Test'], { cwd: repoDir }) + await execa('git', ['add', '-A'], { cwd: repoDir }) + await execa('git', ['-c', 'commit.gpgsign=false', 'commit', '-q', '-m', 'init'], { cwd: repoDir }) + const commit = String((await execa('git', ['rev-parse', 'HEAD'], { cwd: repoDir })).stdout).trim() + return `git+${pathToFileURL(repoDir).href}#${commit}` +} + +test('allowBuilds does not run lifecycle scripts for direct tarball identities', async () => { + prepareEmpty() + const registries = { + default: `http://localhost:${REGISTRY_MOCK_PORT}/`, + '@direct': `http://127.0.0.1:${REGISTRY_MOCK_PORT}/`, + } + const tarball = `http://127.0.0.1:${REGISTRY_MOCK_PORT}/@pnpm.e2e/pre-and-postinstall-scripts-example/-/pre-and-postinstall-scripts-example-1.0.0.tgz` + const depPath = `@pnpm.e2e/pre-and-postinstall-scripts-example@${tarball}` + + const { updatedManifest: manifest } = await addDependenciesToPackage({}, [tarball], testDefaults({ + fastUnpack: false, + allowBuilds: { '@pnpm.e2e/pre-and-postinstall-scripts-example': true }, + registries, + }, { registries })) + + expect(fs.existsSync('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js')).toBe(false) + expect(fs.existsSync('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js')).toBe(false) + + rimrafSync('node_modules') + + await install(manifest, testDefaults({ + fastUnpack: false, + allowBuilds: { [depPath]: true }, + registries, + }, { registries })) + + expect(fs.existsSync('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js')).toBe(true) + expect(fs.existsSync('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js')).toBe(true) +}) + +test('surfaces a tarball artifact as an ignored build when its depPath approval is revoked', async () => { + prepareEmpty() + const registries = { + default: `http://localhost:${REGISTRY_MOCK_PORT}/`, + '@direct': `http://127.0.0.1:${REGISTRY_MOCK_PORT}/`, + } + const tarball = `http://127.0.0.1:${REGISTRY_MOCK_PORT}/@pnpm.e2e/pre-and-postinstall-scripts-example/-/pre-and-postinstall-scripts-example-1.0.0.tgz` + const depPath = `@pnpm.e2e/pre-and-postinstall-scripts-example@${tarball}` + + // First install approves the artifact by its depPath, so the build runs and + // the approval is recorded in .modules.yaml. + const { updatedManifest: manifest } = await addDependenciesToPackage({}, [tarball], testDefaults({ + fastUnpack: false, + allowBuilds: { [depPath]: true }, + registries, + }, { registries })) + expect(fs.existsSync('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js')).toBe(true) + + // Reinstalling with the approval removed must surface the artifact as an + // ignored build. This is the revocation path that iterates the lockfile by + // snapshot name/version, so it reaches non-semver artifact depPaths. + const { ignoredBuilds } = await install(manifest, testDefaults({ + fastUnpack: false, + frozenLockfile: true, + allowBuilds: {}, + registries, + }, { registries })) + + expect(Array.from(ignoredBuilds ?? [])).toContain(depPath) +}) + test('lifecycle scripts run before linking bins', async () => { const project = prepareEmpty() diff --git a/installing/deps-installer/test/install/verifyLockfileResolutions.ts b/installing/deps-installer/test/install/verifyLockfileResolutions.ts index 7b1bf0adbb..5be290ea96 100644 --- a/installing/deps-installer/test/install/verifyLockfileResolutions.ts +++ b/installing/deps-installer/test/install/verifyLockfileResolutions.ts @@ -142,15 +142,16 @@ test('dedupes peer/patch-suffix variants and invokes the verifier once per (name test('does not collapse same (name, version) with different resolutions', async () => { // Two entries sharing a name@version but pinned via different protocols - // (npm registry vs. git). If the dedup key were just `name@version` one - // would silently overwrite the other and a protocol-scoped verifier - // would short-circuit on the survivor — letting the real entry skip - // the gate. + // (npm registry vs. a URL-keyed tarball whose snapshot copied the same + // semver `version` from its manifest). If the dedup key were just + // `name@version` one would silently overwrite the other and a + // protocol-scoped verifier would short-circuit on the survivor — + // letting the real entry skip the gate. const npmResolution = tarballResolution('sha512-a') - const gitResolution = { type: 'git', repo: 'x', commit: 'abc' } + const directTarballResolution = { integrity: 'sha512-b', tarball: 'https://example.com/foo.tgz' } const lockfile = makeLockfile({ 'foo@1.0.0': { resolution: npmResolution }, - 'foo@1.0.0(peer-x)': { resolution: gitResolution }, + 'foo@https://example.com/foo.tgz': { resolution: directTarballResolution, version: '1.0.0' }, }) const seenResolutions: unknown[] = [] const verifier = wrap(async (resolution) => { @@ -159,7 +160,7 @@ test('does not collapse same (name, version) with different resolutions', async }) await verifyLockfileResolutions(lockfile, [verifier]) - expect(seenResolutions).toEqual(expect.arrayContaining([npmResolution, gitResolution])) + expect(seenResolutions).toEqual(expect.arrayContaining([npmResolution, directTarballResolution])) expect(seenResolutions).toHaveLength(2) }) @@ -168,7 +169,7 @@ test('the verifier sees the resolution shape verbatim', async () => { const gitResolution = { type: 'git', repo: 'x', commit: 'abc' } const lockfile = makeLockfile({ 'npm-pkg@1.0.0': { resolution: npmResolution }, - 'git-pkg@1.0.0': { resolution: gitResolution }, + 'git-pkg@git+https://example.com/git-pkg.git#abc': { resolution: gitResolution, version: '1.0.0' }, }) const received: unknown[] = [] const verifier = wrap(async (resolution) => { @@ -290,3 +291,134 @@ test('does not write a cache record when verification rejects', async () => { await fs.promises.rm(tmpDir, { recursive: true, force: true }) } }) + +test('rejects a registry-style depPath backed by a git resolution, even with no verifiers', async () => { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { type: 'git', repo: 'https://example.com/foo.git', commit: 'abc123' } }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', + message: expect.stringMatching(/foo@1\.0\.0/), + }) +}) + +test('rejects a registry-style depPath backed by a git-hosted tarball resolution', async () => { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { integrity: 'sha512-deadbeef', tarball: 'https://codeload.github.com/org/foo/tar.gz/abc123', gitHosted: true } }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', + }) +}) + +test('rejects a registry-style depPath backed by a directory resolution', async () => { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { type: 'directory', directory: '../foo' } }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', + }) +}) + +test('accepts registry-style depPaths with registry and all-registry variations resolutions', async () => { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: tarballResolution() }, + 'bar@1.0.0': { + resolution: { + type: 'variations', + variants: [ + { targets: [{ os: 'darwin' }], resolution: tarballResolution('sha512-a') }, + { targets: [{ os: 'linux' }], resolution: tarballResolution('sha512-b') }, + ], + }, + }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).resolves.toBeUndefined() +}) + +test('rejects a registry-style depPath whose variations resolution hides a git variant', async () => { + const lockfile = makeLockfile({ + 'bar@1.0.0': { + resolution: { + type: 'variations', + variants: [ + { targets: [{ os: 'darwin' }], resolution: tarballResolution('sha512-a') }, + { targets: [{ os: 'linux' }], resolution: { type: 'git', repo: 'https://example.com/bar.git', commit: 'abc123' } }, + ], + }, + }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', + }) +}) + +test('does not flag artifact depPaths with non-registry resolutions', async () => { + const lockfile = makeLockfile({ + 'foo@git+https://example.com/foo.git#abc123': { resolution: { type: 'git', repo: 'https://example.com/foo.git', commit: 'abc123' }, version: '1.0.0' }, + 'bar@https://example.com/bar.tgz': { resolution: { integrity: 'sha512-deadbeef', tarball: 'https://example.com/bar.tgz' }, version: '1.0.0' }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).resolves.toBeUndefined() +}) + +test('rejects a registry-style depPath whose git-host tarball clears the gitHosted flag', async () => { + // A tampered lockfile sets a non-truthy gitHosted on a codeload URL to + // dodge a flag-only check. The URL itself must still flag it. + for (const gitHosted of [false, 'true', 'false', 0, 1]) { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { integrity: 'sha512-deadbeef', tarball: 'https://codeload.github.com/org/foo/tar.gz/abc123', gitHosted } as never }, + }) + // eslint-disable-next-line no-await-in-loop + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', + }) + } +}) + +test('rejects a registry-style depPath with a non-boolean gitHosted flag', async () => { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { integrity: 'sha512-deadbeef', tarball: 'https://registry.npmjs.org/foo/-/foo-1.0.0.tgz', gitHosted: 'true' } as never }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', + }) +}) + +test('accepts a registry-style depPath backed by a custom resolver resolution', async () => { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { type: 'custom:cdn', source: 'foo' } as never }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).resolves.toBeUndefined() +}) + +test('rejects a registry-style depPath backed by a non-http(s) tarball URL', async () => { + // The npm verifier skips non-http(s) tarballs, so a file: artifact under a + // semver key would be trusted with no tarball-URL binding to catch it. + for (const tarball of ['file:///tmp/evil.tgz', 'ftp://example.com/evil.tgz']) { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { integrity: 'sha512-deadbeef', tarball } as never }, + }) + // eslint-disable-next-line no-await-in-loop + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', + }) + } +}) + +test('accepts a registry-style depPath whose tarball is an http(s) registry URL', async () => { + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { integrity: 'sha512-deadbeef', tarball: 'https://registry.npmjs.org/foo/-/foo-1.0.0.tgz' } as never }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).resolves.toBeUndefined() +}) + +test('rejects a registry-style depPath whose git-host tarball varies the host casing', async () => { + // Hostnames are case-insensitive; an upper-case codeload host paired with + // gitHosted: false must not pass as registry-shaped. + const lockfile = makeLockfile({ + 'foo@1.0.0': { resolution: { integrity: 'sha512-deadbeef', tarball: 'https://CODELOAD.GITHUB.COM/org/foo/tar.gz/abc123', gitHosted: false } as never }, + }) + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', + }) +}) diff --git a/installing/deps-restorer/src/index.ts b/installing/deps-restorer/src/index.ts index cb06121004..bb7853cf21 100644 --- a/installing/deps-restorer/src/index.ts +++ b/installing/deps-restorer/src/index.ts @@ -932,7 +932,7 @@ async function linkAllPkgs ( depNode.requiresBuild = filesResponse.requiresBuild let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && filesResponse.sideEffectsMaps && !isEmpty(filesResponse.sideEffectsMaps)) { - if (opts.allowBuild?.(depNode.name, depNode.version) === true) { + if (opts.allowBuild?.(depNode.depPath) === true) { sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.dir, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built patchFileHash: depNode.patch?.hash, diff --git a/installing/deps-restorer/src/linkHoistedModules.ts b/installing/deps-restorer/src/linkHoistedModules.ts index 169ddb7ed1..9c6c84c96d 100644 --- a/installing/deps-restorer/src/linkHoistedModules.ts +++ b/installing/deps-restorer/src/linkHoistedModules.ts @@ -135,7 +135,7 @@ async function linkAllPkgsInOrder ( depNode.requiresBuild = filesResponse.requiresBuild let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && filesResponse.sideEffectsMaps && !isEmpty(filesResponse.sideEffectsMaps)) { - if (opts.allowBuild?.(depNode.name, depNode.version) === true) { + if (opts.allowBuild?.(depNode.depPath) === true) { sideEffectsCacheKey = calcDepState(graph, opts.depsStateCache, dir, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built patchFileHash: depNode.patch?.hash, diff --git a/lockfile/fs/src/lockfileFormatConverters.ts b/lockfile/fs/src/lockfileFormatConverters.ts index 6f1e81aa64..c2374a54da 100644 --- a/lockfile/fs/src/lockfileFormatConverters.ts +++ b/lockfile/fs/src/lockfileFormatConverters.ts @@ -12,22 +12,10 @@ import type { ResolvedDependencies, TarballResolution, } from '@pnpm/lockfile.types' +import { isGitHostedTarballUrl } from '@pnpm/lockfile.utils' import { DEPENDENCIES_FIELDS, type DepPath } from '@pnpm/types' import { isEmpty, map as _mapValues, omit, pick, pickBy } from 'ramda' -// Minimal duplicate of `isGitHostedPkgUrl` from `@pnpm/fetching.pick-fetcher`, -// inlined to avoid pulling the fetcher dep into the lockfile I/O layer. Used -// to enrich entries written by older pnpm versions (which didn't record the -// `gitHosted` field on TarballResolution) so every downstream reader can rely -// on the field directly. -function isGitHostedTarballUrl (url: string): boolean { - return ( - url.startsWith('https://codeload.github.com/') || - url.startsWith('https://bitbucket.org/') || - url.startsWith('https://gitlab.com/') - ) && url.includes('tar.gz') -} - export function convertToLockfileFile (lockfile: LockfileObject): LockfileFile { const packages: Record = {} const snapshots: Record = {} diff --git a/lockfile/utils/src/index.ts b/lockfile/utils/src/index.ts index 0130d787ed..3159de46ee 100644 --- a/lockfile/utils/src/index.ts +++ b/lockfile/utils/src/index.ts @@ -5,7 +5,7 @@ export { packageIdFromSnapshot } from './packageIdFromSnapshot.js' export { packageIsIndependent } from './packageIsIndependent.js' export { pkgSnapshotToResolution } from './pkgSnapshotToResolution.js' export { refIsLocalDirectory, refIsLocalTarball } from './refIsLocalTarball.js' -export { toLockfileResolution } from './toLockfileResolution.js' +export { isGitHostedTarballUrl, toLockfileResolution } from './toLockfileResolution.js' export * from '@pnpm/lockfile.types' // for backward compatibility diff --git a/lockfile/utils/src/toLockfileResolution.ts b/lockfile/utils/src/toLockfileResolution.ts index 0a1348861d..5363e8aea5 100644 --- a/lockfile/utils/src/toLockfileResolution.ts +++ b/lockfile/utils/src/toLockfileResolution.ts @@ -75,11 +75,16 @@ function preservingGitHosted // dep graph. Used as a fallback when callers haven't pre-set the // `gitHosted` field on TarballResolution. export function isGitHostedTarballUrl (url: string): boolean { + // Schemes and hostnames are case-insensitive, so match against a lowercased + // copy: a tampered `https://CODELOAD.GITHUB.COM/...` must not slip past as a + // non-git-hosted (and therefore registry-trusted) tarball. Only the + // lowercased copy is inspected; the original URL is never rewritten. + const lowerUrl = url.toLowerCase() return ( - url.startsWith('https://codeload.github.com/') || - url.startsWith('https://bitbucket.org/') || - url.startsWith('https://gitlab.com/') - ) && url.includes('tar.gz') + lowerUrl.startsWith('https://codeload.github.com/') || + lowerUrl.startsWith('https://bitbucket.org/') || + lowerUrl.startsWith('https://gitlab.com/') + ) && lowerUrl.includes('tar.gz') } function removeProtocol (url: string): string { diff --git a/pacquet/crates/git-fetcher/src/fetcher.rs b/pacquet/crates/git-fetcher/src/fetcher.rs index 6c1d3e806c..a85e036959 100644 --- a/pacquet/crates/git-fetcher/src/fetcher.rs +++ b/pacquet/crates/git-fetcher/src/fetcher.rs @@ -16,7 +16,7 @@ use crate::{ cas_io::{ImportedFiles, import_into_cas}, error::{GitFetcherError, PreparePackageError}, packlist::packlist, - prepare_package::{PreparePackageOptions, PreparedPackage, prepare_package}, + prepare_package::{AllowBuildRef, PreparePackageOptions, PreparedPackage, prepare_package}, }; use pacquet_executor::ScriptsPrependNodePath; use pacquet_package_manifest::safe_read_package_json_from_dir; @@ -44,7 +44,7 @@ pub struct GitFetcher<'a> { /// `allow_build`. The caller (typically the install dispatcher) is /// responsible for plumbing whatever policy structure it has into /// this closure shape. - pub allow_build: &'a (dyn Fn(&str, &str) -> bool + Send + Sync), + pub allow_build: AllowBuildRef<'a>, pub ignore_scripts: bool, pub unsafe_perm: bool, pub user_agent: Option<&'a str>, @@ -146,7 +146,8 @@ impl<'a> GitFetcher<'a> { // brittle to future expression-reshape edits in this block. let empty_env: HashMap = HashMap::new(); let prepare_opts = PreparePackageOptions { - allow_build: Box::new(|name, version| (self.allow_build)(name, version)), + allow_build: Box::new(|dep_path| (self.allow_build)(dep_path)), + dep_path: self.package_id, ignore_scripts: self.ignore_scripts, unsafe_perm: self.unsafe_perm, user_agent: self.user_agent, diff --git a/pacquet/crates/git-fetcher/src/fetcher/tests.rs b/pacquet/crates/git-fetcher/src/fetcher/tests.rs index b00472af3b..2a5b64b885 100644 --- a/pacquet/crates/git-fetcher/src/fetcher/tests.rs +++ b/pacquet/crates/git-fetcher/src/fetcher/tests.rs @@ -1,5 +1,5 @@ use super::{GitFetcher, exec_git_with, extract_host, is_valid_commit_hash, should_use_shallow}; -use crate::error::GitFetcherError; +use crate::{error::GitFetcherError, prepare_package::AllowBuildRef}; use pacquet_executor::ScriptsPrependNodePath; use pacquet_reporter::SilentReporter; use pacquet_store_dir::StoreDir; @@ -47,8 +47,8 @@ fn make_bare_repo_with_prepare_script(tmp: &Path, prepare_script: &str) -> (Path (bare, commit) } -fn allow_all_builds<'a>() -> &'a (dyn Fn(&str, &str) -> bool + Send + Sync) { - &|_, _| true +fn allow_all_builds<'a>() -> AllowBuildRef<'a> { + &|_| true } /// Create a tiny bare git repo whose single commit ships a @@ -77,8 +77,8 @@ fn make_bare_repo(tmp: &Path) -> (PathBuf, String) { (bare, commit) } -fn deny_all_builds<'a>() -> &'a (dyn Fn(&str, &str) -> bool + Send + Sync) { - &|_, _| false +fn deny_all_builds<'a>() -> AllowBuildRef<'a> { + &|_| false } #[test] @@ -659,10 +659,9 @@ async fn fetcher_runs_prepare_when_allow_build_returns_true() { let repo_url = format!("file://{}", bare.display()); // Targeted allow_build that returns true for *this* package only — - // catches a regression where the gate ignores the (name, version) - // pair and falls through to default-allow or default-deny. - let allow_x_only: &(dyn Fn(&str, &str) -> bool + Send + Sync) = - &|name, version| name == "x" && version == "1.0.0"; + // catches a regression where the gate ignores the dep path and + // falls through to default-allow or default-deny. + let allow_x_only: AllowBuildRef<'_> = &|dep_path| dep_path == "x@1.0.0"; let received = GitFetcher { repo: &repo_url, @@ -699,6 +698,95 @@ async fn fetcher_runs_prepare_when_allow_build_returns_true() { ); } +#[tokio::test(flavor = "multi_thread")] +async fn fetcher_rejects_untrusted_manifest_identity() { + let tmp = tempdir().unwrap(); + let (bare, commit) = make_bare_repo_with_prepare_script( + tmp.path(), + r#"node -e "require('fs').writeFileSync('BUILD_RAN.marker', 'ok')""#, + ); + let store_root = tempdir().unwrap(); + let store_dir = StoreDir::from(store_root.path().to_path_buf()); + let repo_url = format!("file://{}", bare.display()); + let allow_registry_artifacts_only: AllowBuildRef<'_> = &|dep_path| !dep_path.contains("://"); + + let err = GitFetcher { + repo: &repo_url, + commit: &commit, + path: None, + git_shallow_hosts: &[], + allow_build: allow_registry_artifacts_only, + ignore_scripts: false, + unsafe_perm: true, + user_agent: None, + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + script_shell: None, + node_execpath: None, + npm_execpath: None, + store_dir: &store_dir, + package_id: "x@git+file:///tmp/repo.git#abc123", + requester: "/test", + store_index_writer: None, + files_index_file: "x@git+file:///tmp/repo.git#abc123\tbuilt", + git_bin: None, + } + .run::() + .await + .unwrap_err(); + + match err { + GitFetcherError::Prepare(crate::error::PreparePackageError::NotAllowed { + name, + version, + }) => { + assert_eq!(name, "x"); + assert_eq!(version, "1.0.0"); + } + other => panic!("expected NotAllowed, got {other:?}"), + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn fetcher_allows_untrusted_manifest_identity_by_dep_path() { + let tmp = tempdir().unwrap(); + let (bare, commit) = make_bare_repo_with_prepare_script( + tmp.path(), + r#"node -e "require('fs').writeFileSync('BUILD_RAN.marker', 'ok')""#, + ); + let store_root = tempdir().unwrap(); + let store_dir = StoreDir::from(store_root.path().to_path_buf()); + let repo_url = format!("file://{}", bare.display()); + let package_id = "x@git+file:///tmp/repo.git#abc123"; + let allow_dep_path: AllowBuildRef<'_> = &|dep_path| dep_path == package_id; + + let received = GitFetcher { + repo: &repo_url, + commit: &commit, + path: None, + git_shallow_hosts: &[], + allow_build: allow_dep_path, + ignore_scripts: false, + unsafe_perm: true, + user_agent: None, + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + script_shell: None, + node_execpath: None, + npm_execpath: None, + store_dir: &store_dir, + package_id, + requester: "/test", + store_index_writer: None, + files_index_file: "x@git+file:///tmp/repo.git#abc123\tbuilt", + git_bin: None, + } + .run::() + .await + .unwrap(); + + assert!(received.built); + assert!(received.cas_paths.contains_key("BUILD_RAN.marker")); +} + /// Write a `git` shim shell script to `dir/git` that: /// /// - Appends every invocation to `$PACQUET_GIT_SHIM_LOG` as one diff --git a/pacquet/crates/git-fetcher/src/prepare_package.rs b/pacquet/crates/git-fetcher/src/prepare_package.rs index d5159d5ace..278f0af2d7 100644 --- a/pacquet/crates/git-fetcher/src/prepare_package.rs +++ b/pacquet/crates/git-fetcher/src/prepare_package.rs @@ -32,20 +32,20 @@ use std::{ /// nor Yarn run it for git-hosted deps. const PREPUBLISH_SCRIPTS: &[&str] = &["prepublish", "prepack", "publish"]; -/// Closure shape used to ask the install policy whether a package -/// (`name`, `version`) is allowed to run lifecycle scripts. Mirrors -/// upstream's `opts.allowBuild?.(name, version)` at -/// [`index.ts:36`](https://github.com/pnpm/pnpm/blob/94240bc046/exec/prepare-package/src/index.ts#L36). +/// Closure shape used to ask the install policy whether the package at +/// a dep path is allowed to run lifecycle scripts. /// /// We pass a closure rather than `&AllowBuildPolicy` so the /// `pacquet-git-fetcher` crate stays free of a back-edge into /// `pacquet-package-manager`. The caller adapts whatever policy /// structure it has into this shape. -pub type AllowBuildFn<'a> = Box bool + Send + Sync + 'a>; +pub type AllowBuildFn<'a> = Box bool + Send + Sync + 'a>; +pub type AllowBuildRef<'a> = &'a (dyn Fn(&str) -> bool + Send + Sync); /// Caller-supplied context for [`prepare_package`]. pub struct PreparePackageOptions<'a> { pub allow_build: AllowBuildFn<'a>, + pub dep_path: &'a str, pub ignore_scripts: bool, pub unsafe_perm: bool, pub user_agent: Option<&'a str>, @@ -96,11 +96,13 @@ pub fn prepare_package( } // `allowBuild` check before any spawn. Upstream throws when - // `opts.allowBuild?.(name, version)` is missing or false, with - // GIT_DEP_PREPARE_NOT_ALLOWED. + // `opts.allowBuild?.(depPath)` is missing or false, with + // GIT_DEP_PREPARE_NOT_ALLOWED. The manifest comes from the fetched + // artifact itself, so its name and version only feed the error + // message; the dep path is the gated identity. let name = manifest.get("name").and_then(Value::as_str).unwrap_or(""); let version = manifest.get("version").and_then(Value::as_str).unwrap_or(""); - if !(opts.allow_build)(name, version) { + if !(opts.allow_build)(opts.dep_path) { return Err(PreparePackageError::NotAllowed { name: name.to_string(), version: version.to_string(), diff --git a/pacquet/crates/git-fetcher/src/prepare_package/tests.rs b/pacquet/crates/git-fetcher/src/prepare_package/tests.rs index e396ce9794..e51c782852 100644 --- a/pacquet/crates/git-fetcher/src/prepare_package/tests.rs +++ b/pacquet/crates/git-fetcher/src/prepare_package/tests.rs @@ -31,7 +31,8 @@ fn write_manifest(dir: &Path, manifest: &serde_json::Value) { fn opts<'a>(allow: bool, ignore_scripts: bool) -> PreparePackageOptions<'a> { static EMPTY_BIN_PATHS: &[std::path::PathBuf] = &[]; PreparePackageOptions { - allow_build: Box::new(move |_name, _version| allow), + allow_build: Box::new(move |_dep_path| allow), + dep_path: "x@https://example.com/x.tgz", ignore_scripts, unsafe_perm: true, user_agent: None, @@ -44,6 +45,40 @@ fn opts<'a>(allow: bool, ignore_scripts: bool) -> PreparePackageOptions<'a> { } } +fn opts_allow_registry_artifacts_only<'a>() -> PreparePackageOptions<'a> { + static EMPTY_BIN_PATHS: &[std::path::PathBuf] = &[]; + PreparePackageOptions { + allow_build: Box::new(move |dep_path| !dep_path.contains("://")), + dep_path: "x@https://example.com/x.tgz", + ignore_scripts: false, + unsafe_perm: true, + user_agent: None, + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + script_shell: None, + node_execpath: None, + npm_execpath: None, + extra_bin_paths: EMPTY_BIN_PATHS, + extra_env: empty_env(), + } +} + +fn opts_allow_dep_path<'a>(dep_path: &'a str) -> PreparePackageOptions<'a> { + static EMPTY_BIN_PATHS: &[std::path::PathBuf] = &[]; + PreparePackageOptions { + allow_build: Box::new(move |actual_dep_path| actual_dep_path == dep_path), + dep_path, + ignore_scripts: false, + unsafe_perm: true, + user_agent: None, + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + script_shell: None, + node_execpath: None, + npm_execpath: None, + extra_bin_paths: EMPTY_BIN_PATHS, + extra_env: empty_env(), + } +} + #[test] fn package_should_be_built_false_for_empty_scripts() { let dir = tempdir().unwrap(); @@ -143,6 +178,50 @@ fn prepare_rejects_when_allow_build_returns_false() { } } +#[test] +fn prepare_rejects_untrusted_manifest_identity() { + let dir = tempdir().unwrap(); + write_manifest( + dir.path(), + &json!({ + "name": "naughty", "version": "1.0.0", + "scripts": { "prepare": "tsc" }, + }), + ); + + let err = + prepare_package::(&opts_allow_registry_artifacts_only(), dir.path(), None) + .unwrap_err(); + match err { + PreparePackageError::NotAllowed { name, version } => { + assert_eq!(name, "naughty"); + assert_eq!(version, "1.0.0"); + } + other => panic!("expected NotAllowed, got {other:?}"), + } +} + +#[test] +fn prepare_allows_untrusted_manifest_identity_by_dep_path() { + let dir = tempdir().unwrap(); + write_manifest( + dir.path(), + &json!({ + "name": "trusted-name", + "version": "1.0.0", + "scripts": { "prepack": r#"node -e "require('fs').writeFileSync('built.txt', 'ok')""# }, + }), + ); + + let dep_path = "trusted-name@git+https://example.com/org/repo.git#abc123"; + let result = + prepare_package::(&opts_allow_dep_path(dep_path), dir.path(), None) + .expect("depPath-specific allow should permit prepare"); + + assert!(result.should_be_built); + assert!(dir.path().join("built.txt").exists()); +} + #[test] fn safe_join_path_rejects_escapes() { let dir = tempdir().unwrap(); diff --git a/pacquet/crates/git-fetcher/src/tarball_fetcher.rs b/pacquet/crates/git-fetcher/src/tarball_fetcher.rs index 57593f93c0..5625b021af 100644 --- a/pacquet/crates/git-fetcher/src/tarball_fetcher.rs +++ b/pacquet/crates/git-fetcher/src/tarball_fetcher.rs @@ -33,7 +33,7 @@ use crate::{ error::GitFetcherError, fetcher::GitFetchOutput, packlist::packlist, - prepare_package::{PreparePackageOptions, PreparedPackage, prepare_package}, + prepare_package::{AllowBuildRef, PreparePackageOptions, PreparedPackage, prepare_package}, }; use pacquet_executor::ScriptsPrependNodePath; use pacquet_package_manifest::safe_read_package_json_from_dir; @@ -63,7 +63,7 @@ pub struct GitHostedTarballFetcher<'a> { /// `None` packs the tarball root. pub path: Option<&'a str>, /// Routed through to [`crate::prepare_package()`]'s `allow_build`. - pub allow_build: &'a (dyn Fn(&str, &str) -> bool + Send + Sync), + pub allow_build: AllowBuildRef<'a>, pub ignore_scripts: bool, pub unsafe_perm: bool, pub user_agent: Option<&'a str>, @@ -108,7 +108,8 @@ impl<'a> GitHostedTarballFetcher<'a> { // `should_be_built` flag. let empty_env: HashMap = HashMap::new(); let prepare_opts = PreparePackageOptions { - allow_build: Box::new(|name, version| (self.allow_build)(name, version)), + allow_build: Box::new(|dep_path| (self.allow_build)(dep_path)), + dep_path: self.package_id, ignore_scripts: self.ignore_scripts, unsafe_perm: self.unsafe_perm, user_agent: self.user_agent, diff --git a/pacquet/crates/git-fetcher/src/tarball_fetcher/tests.rs b/pacquet/crates/git-fetcher/src/tarball_fetcher/tests.rs index 9501ff1ed2..08ac27fb7f 100644 --- a/pacquet/crates/git-fetcher/src/tarball_fetcher/tests.rs +++ b/pacquet/crates/git-fetcher/src/tarball_fetcher/tests.rs @@ -1,13 +1,16 @@ use super::GitHostedTarballFetcher; -use crate::error::{GitFetcherError, PreparePackageError}; +use crate::{ + error::{GitFetcherError, PreparePackageError}, + prepare_package::AllowBuildRef, +}; use pacquet_executor::ScriptsPrependNodePath; use pacquet_reporter::SilentReporter; use pacquet_store_dir::{StoreDir, StoreIndex, StoreIndexWriter}; use std::{collections::HashMap, fs, path::PathBuf, sync::Arc}; use tempfile::tempdir; -fn deny_all_builds<'a>() -> &'a (dyn Fn(&str, &str) -> bool + Send + Sync) { - &|_, _| false +fn deny_all_builds<'a>() -> AllowBuildRef<'a> { + &|_| false } /// Build the `cas_paths` map the dispatcher would hand the fetcher diff --git a/pacquet/crates/lockfile-verification/src/errors.rs b/pacquet/crates/lockfile-verification/src/errors.rs index 5d1f1086c8..915e0194f0 100644 --- a/pacquet/crates/lockfile-verification/src/errors.rs +++ b/pacquet/crates/lockfile-verification/src/errors.rs @@ -68,6 +68,17 @@ pub enum VerifyError { breakdown: String, }, + /// Every violation tripped `RESOLUTION_SHAPE_MISMATCH` — a + /// registry-style dependency path backed by a non-registry + /// resolution. + #[display("{count} lockfile entries failed verification:\n{breakdown}")] + #[diagnostic(code(ERR_PNPM_RESOLUTION_SHAPE_MISMATCH), help("{HINT}"))] + ResolutionShapeMismatch { + #[error(not(source))] + count: usize, + breakdown: String, + }, + /// Mixed batch — at least two distinct violation codes — so the /// throw code escalates to the generic /// `LOCKFILE_RESOLUTION_VERIFICATION` and each entry's code goes @@ -134,6 +145,9 @@ impl VerifyError { pacquet_resolving_npm_resolver_violation_codes::TRUST_DOWNGRADE => { VerifyError::TrustDowngrade { count, breakdown } } + crate::RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE => { + VerifyError::ResolutionShapeMismatch { count, breakdown } + } // Unknown verifier code (future-proofing): fall back // to the generic envelope rather than fabricating a // variant we don't have. diff --git a/pacquet/crates/lockfile-verification/src/lib.rs b/pacquet/crates/lockfile-verification/src/lib.rs index 733fbd757e..e71a0b06a8 100644 --- a/pacquet/crates/lockfile-verification/src/lib.rs +++ b/pacquet/crates/lockfile-verification/src/lib.rs @@ -38,6 +38,6 @@ pub use errors::{RenderedViolation, VerifyError}; pub use hash_lockfile::hash_lockfile; pub use record_lockfile_verified::record_lockfile_verified; pub use verify_lockfile_resolutions::{ - VerifyLockfileResolutionsOptions, collect_resolution_policy_violations, - verify_lockfile_resolutions, + RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE, VerifyLockfileResolutionsOptions, + collect_resolution_policy_violations, verify_lockfile_resolutions, }; diff --git a/pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs b/pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs index 23dd91c743..2f49b5f506 100644 --- a/pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs +++ b/pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs @@ -20,7 +20,10 @@ use std::{path::Path, sync::Arc}; use pacquet_lockfile::Lockfile; use pacquet_resolving_resolver_base::ResolutionVerifier; -use crate::{cache::record_verification, hash_lockfile}; +use crate::{ + cache::record_verification, hash_lockfile, + verify_lockfile_resolutions::with_resolution_shape_cache_identity, +}; /// Persist the post-resolution lockfile as already-verified. /// Inputs match upstream's `RecordLockfileVerifiedOptions`: @@ -46,7 +49,7 @@ pub fn record_lockfile_verified( record_verification( cache_dir, lockfile_path, - verifiers, + &with_resolution_shape_cache_identity(verifiers), || hash_lockfile(lockfile), Default::default(), ); diff --git a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs index 8abf3ccbc1..9791b1e9c2 100644 --- a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs +++ b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs @@ -19,12 +19,12 @@ use std::{collections::BTreeMap, path::Path, sync::Arc, time::Instant}; use futures_util::{StreamExt, stream::FuturesUnordered}; -use pacquet_lockfile::{Lockfile, LockfileResolution, PkgName}; +use pacquet_lockfile::{Lockfile, LockfileResolution, PkgName, is_git_hosted_tarball_url}; use pacquet_reporter::{ LockfileVerificationLog, LockfileVerificationMessage, LogEvent, LogLevel, Reporter, }; use pacquet_resolving_resolver_base::{ - ResolutionPolicyViolation, ResolutionVerification, ResolutionVerifier, VerifyCtx, + ResolutionPolicyViolation, ResolutionVerification, ResolutionVerifier, VerifyCtx, VerifyFuture, }; use tokio::sync::Semaphore; @@ -82,7 +82,7 @@ pub async fn verify_lockfile_resolutions( verifiers: &[Arc], opts: &VerifyLockfileResolutionsOptions<'_>, ) -> Result<(), VerifyError> { - if verifiers.is_empty() || lockfile.packages.is_none() { + if lockfile.packages.is_none() { return Ok(()); } @@ -93,6 +93,8 @@ pub async fn verify_lockfile_resolutions( // logic via the same code path). let cache_inputs = opts.cache_dir.zip(opts.lockfile_path); + let cache_verifiers = with_resolution_shape_cache_identity(verifiers); + // Memoised content hash. Used by both the lookup (when the // stat-shortcut doesn't apply) and the recorder (after the // gate passes). The closure is `FnMut` so multiple lazy calls @@ -109,15 +111,25 @@ pub async fn verify_lockfile_resolutions( let mut cache_precomputed: CachePrecomputed = CachePrecomputed::default(); if let Some((cache_dir, lockfile_path)) = cache_inputs { - let result = - try_lockfile_verification_cache(cache_dir, lockfile_path, verifiers, &mut hash_once); + let result = try_lockfile_verification_cache( + cache_dir, + lockfile_path, + &cache_verifiers, + &mut hash_once, + ); if result.hit { return Ok(()); } cache_precomputed = result.precomputed; } - let candidates = collect_candidates(lockfile); + let (candidates, shape_violations) = collect_candidates(lockfile); + if !shape_violations.is_empty() { + return Err(build_verification_error(shape_violations)); + } + if verifiers.is_empty() { + return Ok(()); + } let lockfile_path_str = opts.lockfile_path.map(|path| path.to_string_lossy().into_owned()); if candidates.is_empty() { // Persist the success so the next install can stat-only the @@ -128,7 +140,7 @@ pub async fn verify_lockfile_resolutions( record_verification( cache_dir, lockfile_path, - verifiers, + &cache_verifiers, &mut hash_once, cache_precomputed, ); @@ -161,7 +173,7 @@ pub async fn verify_lockfile_resolutions( record_verification( cache_dir, lockfile_path, - verifiers, + &cache_verifiers, &mut hash_once, cache_precomputed, ); @@ -184,10 +196,98 @@ pub async fn collect_resolution_policy_violations( if verifiers.is_empty() || lockfile.packages.is_none() { return Vec::new(); } - let candidates = collect_candidates(lockfile); + // Shape violations are deliberately not collected here: they are + // hard tampering failures, not policy picks a caller may + // auto-exclude. + let (candidates, _shape_violations) = collect_candidates(lockfile); run_fan_out(candidates, verifiers, concurrency).await } +pub const RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE: &str = "RESOLUTION_SHAPE_MISMATCH"; + +/// Cache-key participant for the offline resolution-shape pass: a +/// record written before the rule existed lacks the flag, so +/// `can_trust_past_check` rejects it and forces a re-verification. +/// `verify` is never invoked — the identity is appended only to the +/// verifier lists handed to the cache lookup and recorder. +struct ResolutionShapeCacheIdentity { + policy: serde_json::Map, +} + +fn resolution_shape_cache_identity() -> Arc { + let mut policy = serde_json::Map::new(); + policy.insert("resolutionShapeCheck".to_string(), serde_json::Value::Bool(true)); + Arc::new(ResolutionShapeCacheIdentity { policy }) +} + +/// Every verifier list that flows into the verification cache must +/// carry the resolution-shape identity, so records written before the +/// shape rule existed cannot stat-fast-path around it. Used by the +/// gate itself and by [`crate::record_lockfile_verified()`], whose +/// freshly-resolved lockfile satisfies the shape invariant by +/// construction (the writer derives every key from the resolution it +/// just produced). +pub(crate) fn with_resolution_shape_cache_identity( + verifiers: &[Arc], +) -> Vec> { + verifiers.iter().cloned().chain(std::iter::once(resolution_shape_cache_identity())).collect() +} + +impl ResolutionVerifier for ResolutionShapeCacheIdentity { + fn verify<'a>( + &'a self, + _resolution: &'a LockfileResolution, + _ctx: VerifyCtx<'a>, + ) -> VerifyFuture<'a> { + Box::pin(async { ResolutionVerification::Ok }) + } + + fn policy(&self) -> &serde_json::Map { + &self.policy + } + + fn can_trust_past_check(&self, cached: &serde_json::Map) -> bool { + cached.get("resolutionShapeCheck") == Some(&serde_json::Value::Bool(true)) + } +} + +/// Mirrors upstream's `isRegistryShapedResolution`: a plain tarball +/// resolution is registry-shaped because the npm verifier unconditionally +/// binds explicit tarball URLs of semver-keyed entries to the registry's +/// own `dist.tarball`. A git-hosted tarball is not — and trust is gated on +/// the tarball URL rather than the `gitHosted` flag alone, so a tampered +/// entry that clears the flag (`gitHosted: false`) on a git-host URL is +/// still rejected. +fn is_registry_shaped_resolution(resolution: &LockfileResolution) -> bool { + match resolution { + LockfileResolution::Registry(_) => true, + LockfileResolution::Tarball(tarball) => { + // The tarball URL must be an http(s) registry artifact and not + // git-hosted. The npm verifier's tarball-URL binding skips + // non-http(s) schemes (file:, etc.), so a `file:` tarball under a + // name@semver key would otherwise be trusted with no safety net. + is_http_tarball_url(&tarball.tarball) + && tarball.git_hosted != Some(true) + && !is_git_hosted_tarball_url(&tarball.tarball) + } + LockfileResolution::Variations(variations) => variations + .variants + .iter() + .all(|variant| is_registry_shaped_resolution(&variant.resolution)), + LockfileResolution::Directory(_) + | LockfileResolution::Git(_) + | LockfileResolution::Binary(_) => false, + } +} + +/// Whether a tarball URL uses an http(s) scheme — the only schemes a +/// registry artifact is served over. Case-insensitive to reject a +/// tampered uppercase scheme. +fn is_http_tarball_url(url: &str) -> bool { + let lower = url.to_ascii_lowercase(); + lower.starts_with("https://") || lower.starts_with("http://") +} + /// One `(name, version, resolution)` tuple deduplicated from /// `lockfile.packages`. Mirrors upstream's inline `Candidate` /// interface. @@ -207,14 +307,34 @@ struct Candidate { /// `BTreeMap` over a serialized key gives deterministic iteration /// order for tests; the fan-out runs across the value iter so order /// doesn't affect correctness, only the reproducibility of failures. -fn collect_candidates(lockfile: &Lockfile) -> Vec { +fn collect_candidates(lockfile: &Lockfile) -> (Vec, Vec) { let Some(packages) = lockfile.packages.as_ref() else { - return Vec::new(); + return (Vec::new(), Vec::new()); }; let mut deduped: BTreeMap = BTreeMap::new(); + let mut shape_violations = Vec::new(); for (key, metadata) in packages { let name = key.name.clone(); let version = key.suffix.version().to_string(); + // A registry-style dep path (`name@semver`, no `runtime:`-style + // prefix) must be backed by a registry-shaped resolution: the + // allowBuilds policy derives a trusted package identity from + // that key shape, which is only sound while this invariant + // holds. The check is offline, so it applies even when no + // policy verifiers are active. + if key.suffix.prefix() == pacquet_lockfile::Prefix::None + && matches!(key.suffix.version(), pacquet_lockfile::VersionPart::Semver(_)) + && !is_registry_shaped_resolution(&metadata.resolution) + { + shape_violations.push(ResolutionPolicyViolation { + name: name.clone(), + version: version.clone(), + resolution: metadata.resolution.clone(), + code: RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE, + reason: "a registry-style dependency path is backed by a non-registry resolution" + .to_string(), + }); + } // Every `LockfileResolution` variant derives `Serialize`, and // the wire shape never contains non-string keys or non-finite // numbers — the only way this `expect` could fire is a future @@ -230,7 +350,7 @@ fn collect_candidates(lockfile: &Lockfile) -> Vec { resolution: metadata.resolution.clone(), }); } - deduped.into_values().collect() + (deduped.into_values().collect(), shape_violations) } /// Run every active verifier against every candidate with a diff --git a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs index 370889d24a..331ab7b47b 100644 --- a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs +++ b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs @@ -527,3 +527,181 @@ async fn ctx_borrows_have_expected_lifetimes() { .await; assert!(result.is_ok()); } + +#[tokio::test] +async fn rejects_registry_style_key_backed_by_git_resolution_even_with_no_verifiers() { + let lockfile = parse( + "lockfileVersion: '9.0' + +importers: + + .: + dependencies: + acme: + specifier: ^1.0.0 + version: 1.0.0 + +packages: + + acme@1.0.0: + resolution: {type: git, repo: https://example.com/acme.git, commit: 0123456789abcdef0123456789abcdef01234567} + +snapshots: + + acme@1.0.0: {} +", + ); + let err = verify_lockfile_resolutions::( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect_err("registry-style key with a git resolution must be rejected"); + let VerifyError::ResolutionShapeMismatch { count, breakdown } = err else { + panic!("expected ResolutionShapeMismatch, got {err:?}"); + }; + assert_eq!(count, 1); + assert!(breakdown.contains("acme@1.0.0"), "breakdown: {breakdown}"); +} + +#[tokio::test] +async fn accepts_artifact_keys_with_non_registry_resolutions() { + let lockfile = parse( + "lockfileVersion: '9.0' + +importers: + + .: + dependencies: + acme: + specifier: github:org/acme + version: git+https://example.com/acme.git#0123456789abcdef0123456789abcdef01234567 + +packages: + + acme@git+https://example.com/acme.git#0123456789abcdef0123456789abcdef01234567: + resolution: {type: git, repo: https://example.com/acme.git, commit: 0123456789abcdef0123456789abcdef01234567} + version: 1.0.0 + +snapshots: + + acme@git+https://example.com/acme.git#0123456789abcdef0123456789abcdef01234567: {} +", + ); + verify_lockfile_resolutions::( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect("artifact-keyed git entry passes the structural gate"); +} + +#[tokio::test] +async fn rejects_git_host_tarball_when_git_hosted_flag_is_cleared() { + // A tampered lockfile sets gitHosted: false on a codeload URL under a + // semver key to dodge the flag-only check; the URL must still flag it. + let lockfile = parse( + "lockfileVersion: '9.0' + +importers: + + .: + dependencies: + acme: + specifier: ^1.0.0 + version: 1.0.0 + +packages: + + acme@1.0.0: + resolution: {integrity: sha512-deadbeef, tarball: 'https://codeload.github.com/org/acme/tar.gz/abc123', gitHosted: false} + +snapshots: + + acme@1.0.0: {} +", + ); + let err = verify_lockfile_resolutions::( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect_err("git-host tarball under a semver key must be rejected regardless of the flag"); + let VerifyError::ResolutionShapeMismatch { count, .. } = err else { + panic!("expected ResolutionShapeMismatch, got {err:?}"); + }; + assert_eq!(count, 1); +} + +#[tokio::test] +async fn rejects_semver_key_backed_by_non_http_tarball() { + // A file: tarball under a semver key is not registry-backed and the npm + // verifier skips non-http(s) tarballs, so the shape pass must reject it. + let lockfile = parse( + "lockfileVersion: '9.0' + +importers: + + .: + dependencies: + acme: + specifier: ^1.0.0 + version: 1.0.0 + +packages: + + acme@1.0.0: + resolution: {integrity: sha512-deadbeef, tarball: 'file:///tmp/evil.tgz'} + +snapshots: + + acme@1.0.0: {} +", + ); + let err = verify_lockfile_resolutions::( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect_err("file: tarball under a semver key must be rejected"); + assert!(matches!(err, VerifyError::ResolutionShapeMismatch { .. }), "got {err:?}"); +} + +#[tokio::test] +async fn rejects_git_host_tarball_with_uppercased_host() { + // Hostnames are case-insensitive; an uppercased codeload host with + // gitHosted: false must still be rejected under a semver key. + let lockfile = parse( + "lockfileVersion: '9.0' + +importers: + + .: + dependencies: + acme: + specifier: ^1.0.0 + version: 1.0.0 + +packages: + + acme@1.0.0: + resolution: {integrity: sha512-deadbeef, tarball: 'https://CODELOAD.GITHUB.COM/org/acme/tar.gz/abc123', gitHosted: false} + +snapshots: + + acme@1.0.0: {} +", + ); + let err = verify_lockfile_resolutions::( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect_err("uppercased git-host tarball must be rejected"); + assert!(matches!(err, VerifyError::ResolutionShapeMismatch { .. }), "got {err:?}"); +} diff --git a/pacquet/crates/lockfile/src/resolution.rs b/pacquet/crates/lockfile/src/resolution.rs index 6efbe57556..05070c998b 100644 --- a/pacquet/crates/lockfile/src/resolution.rs +++ b/pacquet/crates/lockfile/src/resolution.rs @@ -380,14 +380,19 @@ impl From for LockfileResolution { } /// Best-effort URL-prefix check used to back-fill `gitHosted` on tarball -/// resolutions written by older pnpm versions. Mirrors upstream's -/// `isGitHostedTarballUrl` at +/// resolutions written by older pnpm versions, and to gate trust on the +/// tarball URL rather than the (tamper-prone) `gitHosted` flag. Mirrors +/// upstream's `isGitHostedTarballUrl` at /// . -fn is_git_hosted_tarball_url(url: &str) -> bool { - (url.starts_with("https://codeload.github.com/") - || url.starts_with("https://bitbucket.org/") - || url.starts_with("https://gitlab.com/")) - && url.contains("tar.gz") +pub fn is_git_hosted_tarball_url(url: &str) -> bool { + // Schemes and hostnames are case-insensitive, so match against a lowercased + // copy: a tampered `https://CODELOAD.GITHUB.COM/...` must not slip past as a + // non-git-hosted (and therefore registry-trusted) tarball. + let lower = url.to_ascii_lowercase(); + (lower.starts_with("https://codeload.github.com/") + || lower.starts_with("https://bitbucket.org/") + || lower.starts_with("https://gitlab.com/")) + && lower.contains("tar.gz") } impl From for ResolutionSerde { diff --git a/pacquet/crates/package-manager/src/build_modules.rs b/pacquet/crates/package-manager/src/build_modules.rs index b720294512..37f303a341 100644 --- a/pacquet/crates/package-manager/src/build_modules.rs +++ b/pacquet/crates/package-manager/src/build_modules.rs @@ -6,6 +6,7 @@ use crate::{ use derive_more::{Display, Error}; use miette::Diagnostic; use pacquet_config::Config; +use pacquet_deps_path::remove_suffix; use pacquet_executor::{ LifecycleScriptError, RunPostinstallHooks, ScriptsPrependNodePath, run_postinstall_hooks, }; @@ -84,6 +85,8 @@ pub enum BuildModulesError { pub struct AllowBuildPolicy { expanded_allowed: HashSet, expanded_disallowed: HashSet, + allowed_dep_paths: HashSet, + disallowed_dep_paths: HashSet, dangerously_allow_all: bool, } @@ -99,7 +102,29 @@ impl AllowBuildPolicy { expanded_disallowed: HashSet, dangerously_allow_all: bool, ) -> Self { - Self { expanded_allowed, expanded_disallowed, dangerously_allow_all } + Self { + expanded_allowed, + expanded_disallowed, + allowed_dep_paths: HashSet::new(), + disallowed_dep_paths: HashSet::new(), + dangerously_allow_all, + } + } + + pub fn new_with_dep_paths( + expanded_allowed: HashSet, + expanded_disallowed: HashSet, + allowed_dep_paths: HashSet, + disallowed_dep_paths: HashSet, + dangerously_allow_all: bool, + ) -> Self { + Self { + expanded_allowed, + expanded_disallowed, + allowed_dep_paths, + disallowed_dep_paths, + dangerously_allow_all, + } } /// Build the policy from a resolved [`Config`]. Reads @@ -120,16 +145,32 @@ impl AllowBuildPolicy { pub fn from_config(config: &Config) -> Result { let mut allowed_specs: Vec<&str> = Vec::new(); let mut disallowed_specs: Vec<&str> = Vec::new(); + let mut allowed_dep_paths = HashSet::new(); + let mut disallowed_dep_paths = HashSet::new(); for (spec, &value) in &config.allow_builds { - if value { - allowed_specs.push(spec); + if is_dep_path_allow_build_key(spec) { + if value { + allowed_dep_paths.insert(normalize_build_dep_path(spec)); + } else { + disallowed_dep_paths.insert(normalize_build_dep_path(spec)); + } } else { - disallowed_specs.push(spec); + if value { + allowed_specs.push(spec); + } else { + disallowed_specs.push(spec); + } } } let expanded_allowed = expand_package_version_specs(allowed_specs)?; let expanded_disallowed = expand_package_version_specs(disallowed_specs)?; - Ok(Self::new(expanded_allowed, expanded_disallowed, config.dangerously_allow_all_builds)) + Ok(Self::new_with_dep_paths( + expanded_allowed, + expanded_disallowed, + allowed_dep_paths, + disallowed_dep_paths, + config.dangerously_allow_all_builds, + )) } /// Check whether a package is allowed to run build scripts. @@ -146,18 +187,34 @@ impl AllowBuildPolicy { /// lookup means `*` wildcards in specs do NOT match real /// package names — see [`expand_package_version_specs`] for /// the rationale. - pub fn check(&self, name: &str, version: &str) -> Option { + pub fn check(&self, dep_path: &str) -> Option { if self.dangerously_allow_all { return Some(true); } + let normalized_dep_path = normalize_build_dep_path(dep_path); + if self.disallowed_dep_paths.contains(&normalized_dep_path) { + return Some(false); + } + let (name, version) = parse_name_version_from_key(&normalized_dep_path); let name_at_version = format!("{name}@{version}"); - if self.expanded_disallowed.contains(name) + if self.expanded_disallowed.contains(&name) || self.expanded_disallowed.contains(&name_at_version) { return Some(false); } - if self.expanded_allowed.contains(name) || self.expanded_allowed.contains(&name_at_version) + if self.allowed_dep_paths.contains(&normalized_dep_path) { + return Some(true); + } + // Package-name rules require a trusted package identity. A + // registry-style dep path (`name@semver`) is the trust signal: the + // lockfile verification gate rejects lockfiles where such a key is + // backed by a non-registry resolution, so by the time scripts can + // run, the shape proves the artifact came from a registry. + if node_semver::Version::parse(&version).is_err() { + return None; + } + if self.expanded_allowed.contains(&name) || self.expanded_allowed.contains(&name_at_version) { return Some(true); } @@ -166,6 +223,33 @@ impl AllowBuildPolicy { } } +/// Strips the peer suffix (and, matching [`PkgVerPeer::without_peer`]'s +/// lumped suffix handling, the patch hash) so config keys compare equal +/// to the `metadata_key.to_string()` form used at the runtime call sites. +/// +/// [`PkgVerPeer::without_peer`]: pacquet_lockfile::PkgVerPeer::without_peer +pub(crate) fn normalize_build_dep_path(dep_path: &str) -> String { + remove_suffix(dep_path).to_string() +} + +fn is_dep_path_allow_build_key(spec: &str) -> bool { + if normalize_build_dep_path(spec) != spec { + return true; + } + if spec.contains("||") { + return false; + } + let (_, version) = parse_name_version_from_key(spec); + if version.is_empty() { + return !spec.starts_with('@') && (spec.contains('/') || spec.contains(':')); + } + node_semver::Version::parse(&version).is_err() && is_source_like_dep_path_version(&version) +} + +fn is_source_like_dep_path_version(version: &str) -> bool { + version.contains(':') || version.contains('/') || version.contains('#') +} + /// Run lifecycle scripts for all packages that require a build. /// /// Ports the core of `buildModules` from @@ -547,7 +631,8 @@ fn build_one_snapshot( // `ignoreScripts = true; break` pattern. let mut should_run_scripts = requires_build; if requires_build { - match allow_build_policy.check(&name, &version) { + let dep_path = metadata_key.to_string(); + match allow_build_policy.check(&dep_path) { Some(false) => { should_run_scripts = false; } @@ -560,10 +645,7 @@ fn build_one_snapshot( // the end of `BuildModules::run` for the safety // argument (BTreeSet insertion is atomic from the // data-structure's POV). - ignored_builds - .lock() - .unwrap_or_else(|e| e.into_inner()) - .insert(metadata_key.to_string()); + ignored_builds.lock().unwrap_or_else(|e| e.into_inner()).insert(dep_path); should_run_scripts = false; } Some(true) => {} diff --git a/pacquet/crates/package-manager/src/build_modules/tests.rs b/pacquet/crates/package-manager/src/build_modules/tests.rs index 5f8f947270..dcbb7bb594 100644 --- a/pacquet/crates/package-manager/src/build_modules/tests.rs +++ b/pacquet/crates/package-manager/src/build_modules/tests.rs @@ -21,26 +21,19 @@ use tempfile::tempdir; /// Build an [`AllowBuildPolicy`] from a list of `(spec, allowed)` /// pairs, mirroring how `pnpm-workspace.yaml`'s `allowBuilds` map /// would arrive at the policy. Each spec is parsed through -/// [`crate::expand_package_version_specs`] so version unions -/// (`foo@1.0.0 || 2.0.0`) work the same way they do at runtime. +/// [`AllowBuildPolicy::from_config`] so version unions and depPath +/// keys work the same way they do at runtime. /// Panics on any parse failure — test inputs must be valid. fn policy_from_specs( entries: [(&str, bool); LEN], dangerously_allow_all: bool, ) -> AllowBuildPolicy { - use crate::expand_package_version_specs; - let mut allowed_specs: Vec<&str> = Vec::new(); - let mut disallowed_specs: Vec<&str> = Vec::new(); + let mut config = Config::new(); + config.dangerously_allow_all_builds = dangerously_allow_all; for (spec, value) in entries { - if value { - allowed_specs.push(spec); - } else { - disallowed_specs.push(spec); - } + config.allow_builds.insert(spec.to_string(), value); } - let expanded_allowed = expand_package_version_specs(allowed_specs).expect("valid specs"); - let expanded_disallowed = expand_package_version_specs(disallowed_specs).expect("valid specs"); - AllowBuildPolicy::new(expanded_allowed, expanded_disallowed, dangerously_allow_all) + AllowBuildPolicy::from_config(&config).expect("valid specs") } #[test] @@ -74,25 +67,55 @@ fn parse_key_without_leading_slash() { #[test] fn default_policy_denies_all() { let policy = AllowBuildPolicy::default(); - assert_eq!(policy.check("any-package", "1.0.0"), None); + assert_eq!(policy.check("any-package@1.0.0"), None); } #[test] fn explicit_allow() { let policy = policy_from_specs([("@pnpm.e2e/install-script-example", true)], false); - assert_eq!(policy.check("@pnpm.e2e/install-script-example", "1.0.0"), Some(true)); + assert_eq!(policy.check("@pnpm.e2e/install-script-example@1.0.0"), Some(true)); +} + +#[test] +fn explicit_allow_requires_registry_style_dep_path() { + let policy = policy_from_specs([("@pnpm.e2e/install-script-example", true)], false); + assert_eq!( + policy.check("@pnpm.e2e/install-script-example@git+https://example.com/x.git#abc123"), + None, + ); + assert_eq!(policy.check("@pnpm.e2e/install-script-example@1.0.0"), Some(true)); +} + +#[test] +fn explicit_allow_by_dep_path_allows_untrusted_package_identity() { + let policy = policy_from_specs( + [("foo@git+https://github.com/org/foo.git#abc123", true), ("foo", true)], + false, + ); + assert_eq!( + policy.check("foo@git+https://github.com/org/foo.git#abc123(react@19.0.0)"), + Some(true), + ); + assert_eq!(policy.check("foo@git+https://github.com/attacker/foo.git#abc123"), None); +} + +#[test] +fn explicit_allow_by_tarball_dep_path_allows_untrusted_package_identity() { + let policy = policy_from_specs([("foo@https://example.com/foo.tgz", true)], false); + + assert_eq!(policy.check("foo@https://example.com/foo.tgz"), Some(true)); } #[test] fn explicit_deny() { let policy = policy_from_specs([("@pnpm.e2e/bad-package", false)], false); - assert_eq!(policy.check("@pnpm.e2e/bad-package", "1.0.0"), Some(false)); + assert_eq!(policy.check("@pnpm.e2e/bad-package@1.0.0"), Some(false)); } #[test] fn unlisted_returns_none() { let policy = policy_from_specs([("@pnpm.e2e/allowed", true)], false); - assert_eq!(policy.check("@pnpm.e2e/not-listed", "1.0.0"), None); + assert_eq!(policy.check("@pnpm.e2e/not-listed@1.0.0"), None); } /// Upstream checks `expandedDisallowed` before `expandedAllowed` @@ -106,8 +129,8 @@ fn unlisted_returns_none() { fn disallow_bare_name_wins_over_allow_exact_version() { let policy = policy_from_specs([("@pnpm.e2e/pkg@1.0.0", true), ("@pnpm.e2e/pkg", false)], false); - assert_eq!(policy.check("@pnpm.e2e/pkg", "1.0.0"), Some(false)); - assert_eq!(policy.check("@pnpm.e2e/pkg", "2.0.0"), Some(false)); + assert_eq!(policy.check("@pnpm.e2e/pkg@1.0.0"), Some(false)); + assert_eq!(policy.check("@pnpm.e2e/pkg@2.0.0"), Some(false)); } /// The converse: a bare-name allow combined with an exact-version @@ -117,27 +140,33 @@ fn disallow_bare_name_wins_over_allow_exact_version() { fn disallow_exact_version_with_allow_bare_name() { let policy = policy_from_specs([("@pnpm.e2e/pkg", true), ("@pnpm.e2e/pkg@1.0.0", false)], false); - assert_eq!(policy.check("@pnpm.e2e/pkg", "1.0.0"), Some(false)); - assert_eq!(policy.check("@pnpm.e2e/pkg", "2.0.0"), Some(true)); + assert_eq!(policy.check("@pnpm.e2e/pkg@1.0.0"), Some(false)); + assert_eq!(policy.check("@pnpm.e2e/pkg@2.0.0"), Some(true)); } #[test] fn empty_rules_denies_all() { let policy = policy_from_specs([], false); - assert_eq!(policy.check("any-package", "1.0.0"), None); + assert_eq!(policy.check("any-package@1.0.0"), None); } #[test] fn dangerously_allow_all_builds() { let policy = policy_from_specs([], true); - assert_eq!(policy.check("any-package", "1.0.0"), Some(true)); - assert_eq!(policy.check("other-package", "2.0.0"), Some(true)); + assert_eq!(policy.check("any-package@1.0.0"), Some(true)); + assert_eq!(policy.check("other-package@2.0.0"), Some(true)); +} + +#[test] +fn dangerously_allow_all_allows_artifact_dep_paths() { + let policy = policy_from_specs([], true); + assert_eq!(policy.check("anything@git+https://example.com/x.git#abc123"), Some(true)); } #[test] fn dangerously_allow_all_overrides_deny() { let policy = policy_from_specs([("@pnpm.e2e/pkg", false)], true); - assert_eq!(policy.check("@pnpm.e2e/pkg", "1.0.0"), Some(true)); + assert_eq!(policy.check("@pnpm.e2e/pkg@1.0.0"), Some(true)); } /// Mirrors upstream's @@ -148,11 +177,11 @@ fn dangerously_allow_all_overrides_deny() { #[test] fn allow_via_version_union() { let policy = policy_from_specs([("foo", true), ("qar@1.0.0 || 2.0.0", true)], false); - assert_eq!(policy.check("foo", "1.0.0"), Some(true)); - assert_eq!(policy.check("bar", "1.0.0"), None); - assert_eq!(policy.check("qar", "1.0.0"), Some(true)); - assert_eq!(policy.check("qar", "2.0.0"), Some(true)); - assert_eq!(policy.check("qar", "1.1.0"), None); + assert_eq!(policy.check("foo@1.0.0"), Some(true)); + assert_eq!(policy.check("bar@1.0.0"), None); + assert_eq!(policy.check("qar@1.0.0"), Some(true)); + assert_eq!(policy.check("qar@2.0.0"), Some(true)); + assert_eq!(policy.check("qar@1.1.0"), None); } /// Mirrors upstream's @@ -165,8 +194,8 @@ fn allow_via_version_union() { #[test] fn wildcard_name_in_allow_builds_does_not_match_real_package() { let policy = policy_from_specs([("is-*", true)], false); - assert_eq!(policy.check("is-odd", "1.0.0"), None); - assert_eq!(policy.check("is-positive", "1.0.0"), None); + assert_eq!(policy.check("is-odd@1.0.0"), None); + assert_eq!(policy.check("is-positive@1.0.0"), None); } /// `from_config` propagates `expand_package_version_specs` errors — @@ -201,7 +230,7 @@ fn from_config_propagates_name_pattern_in_version_union() { #[test] fn empty_config_denies_all() { let policy = AllowBuildPolicy::from_config(&Config::new()).expect("empty config never errors"); - assert_eq!(policy.check("anything", "1.0.0"), None); + assert_eq!(policy.check("anything@1.0.0"), None); } #[test] @@ -212,9 +241,9 @@ fn from_config_consumes_allow_builds_and_dangerously_allow_all_builds() { config.allow_builds.insert("@pnpm.e2e/bad-package".to_string(), false); let policy = AllowBuildPolicy::from_config(&config).expect("valid specs"); - assert_eq!(policy.check("@pnpm.e2e/install-script-example", "1.0.0"), Some(true)); - assert_eq!(policy.check("@pnpm.e2e/bad-package", "1.0.0"), Some(false)); - assert_eq!(policy.check("@pnpm.e2e/unrelated", "1.0.0"), None); + assert_eq!(policy.check("@pnpm.e2e/install-script-example@1.0.0"), Some(true)); + assert_eq!(policy.check("@pnpm.e2e/bad-package@1.0.0"), Some(false)); + assert_eq!(policy.check("@pnpm.e2e/unrelated@1.0.0"), None); } fn name(text: &str) -> PkgName { diff --git a/pacquet/crates/package-manager/src/install_package_by_snapshot.rs b/pacquet/crates/package-manager/src/install_package_by_snapshot.rs index 0e6da2cb72..054dc5fbcf 100644 --- a/pacquet/crates/package-manager/src/install_package_by_snapshot.rs +++ b/pacquet/crates/package-manager/src/install_package_by_snapshot.rs @@ -280,7 +280,7 @@ impl<'a> InstallPackageBySnapshot<'a> { // (`None → false`) matches pnpm v11's policy: build scripts // have to be explicitly opted in to run. let allow_build_closure = - |name: &str, version: &str| allow_build_policy.check(name, version).unwrap_or(false); + |dep_path: &str| allow_build_policy.check(dep_path).unwrap_or(false); let scripts_prepend_node_path = match config.scripts_prepend_node_path { pacquet_config::ScriptsPrependNodePath::Always => ExecScriptsPrependNodePath::Always, pacquet_config::ScriptsPrependNodePath::Never => ExecScriptsPrependNodePath::Never, diff --git a/pacquet/crates/package-manager/src/virtual_store_layout.rs b/pacquet/crates/package-manager/src/virtual_store_layout.rs index 4b64d224e7..f3ef5bcfce 100644 --- a/pacquet/crates/package-manager/src/virtual_store_layout.rs +++ b/pacquet/crates/package-manager/src/virtual_store_layout.rs @@ -198,12 +198,7 @@ impl VirtualStoreLayout { let built_dep_paths: Option> = allow_build_policy.map(|policy| { snapshots .keys() - .filter(|key| { - let metadata_key = key.without_peer(); - let name = metadata_key.name.to_string(); - let version = metadata_key.suffix.version().to_string(); - policy.check(&name, &version) == Some(true) - }) + .filter(|key| policy.check(&key.without_peer().to_string()) == Some(true)) .cloned() .collect() }); diff --git a/pacquet/crates/package-manager/src/virtual_store_layout/tests.rs b/pacquet/crates/package-manager/src/virtual_store_layout/tests.rs index a8d63ed994..9156d3e9f5 100644 --- a/pacquet/crates/package-manager/src/virtual_store_layout/tests.rs +++ b/pacquet/crates/package-manager/src/virtual_store_layout/tests.rs @@ -5,7 +5,10 @@ use pacquet_lockfile::{ SnapshotEntry, }; use pretty_assertions::{assert_eq, assert_ne}; -use std::{collections::HashMap, path::PathBuf}; +use std::{ + collections::{HashMap, HashSet}, + path::PathBuf, +}; /// Build a `Config` test-double with the GVS-relevant fields /// wired explicitly. `gvs_dir` populates `global_virtual_store_dir` @@ -259,6 +262,38 @@ fn slot_dir_engine_specific_when_snapshot_is_built() { assert_ne!(darwin, linux, "builder snapshot must partition GVS slot by engine string"); } +#[test] +fn missing_metadata_keeps_source_dep_path_untrusted_for_gvs() { + let config = make_config( + true, + PathBuf::from("/tmp/proj/node_modules/.pnpm"), + PathBuf::from("/tmp/store/links"), + ); + let key: PackageKey = "spoofed@git-hosted#abc123".parse().unwrap(); + let mut snapshots = HashMap::new(); + snapshots.insert(key.clone(), SnapshotEntry::default()); + let packages = HashMap::new(); + let allowed: HashSet = ["spoofed".to_string()].into_iter().collect(); + let policy = crate::AllowBuildPolicy::new(allowed, HashSet::new(), false); + let darwin = VirtualStoreLayout::new( + &config, + Some("darwin-arm64-node20"), + Some(&snapshots), + Some(&packages), + Some(&policy), + ) + .slot_dir(&key); + let linux = VirtualStoreLayout::new( + &config, + Some("linux-x64-node22"), + Some(&snapshots), + Some(&packages), + Some(&policy), + ) + .slot_dir(&key); + assert_eq!(darwin, linux, "source depPath with missing metadata must not be name-allowed"); +} + /// Per-snapshot `engines.runtime` resolution: two builder /// siblings that pin *different* Node majors must land on /// different GVS slots even when given the same install-wide diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b624553383..c9d91b0da7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1051,6 +1051,7 @@ overrides: send@<0.19.0: ^0.19.0 shell-quote@<1.8.4: '>=1.8.4' serve-static@<1.16.0: ^1.16.0 + shell-quote: 1.8.4 socks@2: ^2.8.1 tar@<=7.5.9: '>=7.5.10' tmp@<=0.2.3: '>=0.2.4' @@ -1818,6 +1819,9 @@ importers: '@pnpm/building.after-install': specifier: workspace:* version: link:../after-install + '@pnpm/building.policy': + specifier: workspace:* + version: link:../policy '@pnpm/cli.command': specifier: workspace:* version: link:../../cli/command @@ -4601,6 +4605,9 @@ importers: '@pnpm/fs.packlist': specifier: workspace:* version: link:../../fs/packlist + '@pnpm/resolving.git-resolver': + specifier: workspace:* + version: link:../../resolving/git-resolver '@pnpm/store.index': specifier: workspace:* version: link:../../store/index @@ -5214,6 +5221,9 @@ importers: '@pnpm/building.after-install': specifier: workspace:* version: link:../../building/after-install + '@pnpm/building.policy': + specifier: workspace:* + version: link:../../building/policy '@pnpm/catalogs.types': specifier: workspace:* version: link:../../catalogs/types diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 8f45edc559..bf92ae4d05 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -440,6 +440,8 @@ overrides: send@<0.19.0: ^0.19.0 shell-quote@<1.8.4: '>=1.8.4' serve-static@<1.16.0: ^1.16.0 + # GHSA-w7jw-789q-3m8p / CVE-2026-9277: 1.8.4 patches critical shell-quote vulnerabilities in <=1.8.3. + shell-quote: 1.8.4 socks@2: ^2.8.1 tar@<=7.5.9: '>=7.5.10' tmp@<=0.2.3: '>=0.2.4' diff --git a/pnpm/test/dlx.ts b/pnpm/test/dlx.ts index 50d838aff3..3bb593a67d 100644 --- a/pnpm/test/dlx.ts +++ b/pnpm/test/dlx.ts @@ -331,7 +331,13 @@ test('dlx creates cache and store prune cleans cache', async () => { '--config.dlx-cache-max-age=50', // big number to avoid false negative should test unexpectedly takes too long to run ] - await Promise.all(Object.entries(commands).map(([cmd, args]) => execPnpm([...settings, '--allow-build=shx', 'dlx', cmd, ...args]))) + // The git-hosted artifact has an untrusted package identity, so it has to + // be approved by its depPath; the registry shx is approved by name. + const allowBuilds = [ + '--allow-build=shx', + '--allow-build=shx@https://codeload.github.com/shelljs/shx/tar.gz/61aca968cd7afc712ca61a4fc4ec3201e3770dc7', + ] + await Promise.all(Object.entries(commands).map(([cmd, args]) => execPnpm([...settings, ...allowBuilds, 'dlx', cmd, ...args]))) // ensure that the dlx cache has certain structure const dlxBaseDir = path.resolve('cache', 'dlx') diff --git a/resolving/git-resolver/src/createGitHostedPkgId.ts b/resolving/git-resolver/src/createGitHostedPkgId.ts index 51496f2c1a..e2c6a1208b 100644 --- a/resolving/git-resolver/src/createGitHostedPkgId.ts +++ b/resolving/git-resolver/src/createGitHostedPkgId.ts @@ -1,10 +1,21 @@ import type { PkgResolutionId } from '@pnpm/resolving.resolver-base' export function createGitHostedPkgId ({ repo, commit, path }: { repo: string, commit: string, path?: string }): PkgResolutionId { - let id = `${repo.includes('://') ? '' : 'https://'}${repo}#${commit}` + const normalizedRepo = normalizeGitRepoForPkgResolutionId(repo) + let id = `${normalizedRepo.includes('://') ? '' : 'https://'}${normalizedRepo}#${commit}` if (!id.startsWith('git+')) id = `git+${id}` if (path) { id += `&path:${path}` } return id as PkgResolutionId } + +function normalizeGitRepoForPkgResolutionId (repo: string): string { + // Only scp-style shorthand (`user@host:path`) needs rewriting. A repo that + // already carries a URL scheme (e.g. `ssh://user@host:2222/path`) is left + // alone — its `@host:port` would otherwise match the scp pattern and get + // mangled into `ssh://ssh://…`. + if (repo.includes('://')) return repo + const scp = /^([^@\s]+@[^:\s]+):(.+)$/.exec(repo) + return scp == null ? repo : `ssh://${scp[1]}/${scp[2]}` +} diff --git a/resolving/git-resolver/test/createGitHostedPkgId.test.ts b/resolving/git-resolver/test/createGitHostedPkgId.test.ts index 9b1ca9a8c5..931f8f515e 100644 --- a/resolving/git-resolver/test/createGitHostedPkgId.test.ts +++ b/resolving/git-resolver/test/createGitHostedPkgId.test.ts @@ -3,6 +3,10 @@ import { createGitHostedPkgId } from '@pnpm/resolving.git-resolver' test.each([ [{ repo: 'ssh://git@example.com/org/repo.git', commit: 'cba04669e621b85fbdb33371604de1a2898e68e9' }, 'git+ssh://git@example.com/org/repo.git#cba04669e621b85fbdb33371604de1a2898e68e9'], + // A fully-qualified ssh URL with a port must not be rewritten by the scp + // shorthand normalization (its `@host:2222` looks scp-like). + [{ repo: 'ssh://git@example.com:2222/org/repo.git', commit: 'cba04669e621b85fbdb33371604de1a2898e68e9' }, 'git+ssh://git@example.com:2222/org/repo.git#cba04669e621b85fbdb33371604de1a2898e68e9'], + [{ repo: 'git@example.com:org/repo.git', commit: 'cba04669e621b85fbdb33371604de1a2898e68e9', path: 'packages/pkg' }, 'git+ssh://git@example.com/org/repo.git#cba04669e621b85fbdb33371604de1a2898e68e9&path:packages/pkg'], [{ repo: 'https://0000000000000000000000000000000000000000:x-oauth-basic@github.com/foo/bar.git', commit: '0000000000000000000000000000000000000000' }, 'git+https://0000000000000000000000000000000000000000:x-oauth-basic@github.com/foo/bar.git#0000000000000000000000000000000000000000'], [{ repo: 'file:///Users/zoltan/src/pnpm/pnpm/resolving/git-resolver', commit: '988c61e11dc8d9ca0b5580cb15291951812549dc' }, 'git+file:///Users/zoltan/src/pnpm/pnpm/resolving/git-resolver#988c61e11dc8d9ca0b5580cb15291951812549dc'], ])('createGitHostedPkgId', (resolution, id) => {