From e55f4b5efd0311b3971ffe04f9ca185d40865946 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Tue, 26 May 2026 23:15:03 +0200 Subject: [PATCH] fix: require integrity for tarball-shaped lockfile resolutions (#11966) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(lockfile.utils): require integrity for tarball-shaped lockfile resolutions A tampered lockfile that strips the `integrity` field from a tarball resolution let the worker download the URL contents and mint a fresh integrity from the unverified bytes, so an attacker who could also serve content at the referenced URL would install a tampered package without any error — including under `--frozen-lockfile`. pnpm now rejects such entries at lockfile-read time with `ERR_PNPM_MISSING_TARBALL_INTEGRITY`, matching pacquet's existing `pacquet_package_manager::missing_tarball_integrity` guard. * test(lockfile.utils): drop redundant integrity-less snapshot that fails strict typecheck * test(pacquet/package-manager): cover MissingTarballIntegrity rejection in snapshot_cache_key Match the upstream guard landed alongside pnpm/pnpm#11966 (`lockfile/utils/src/pkgSnapshotToResolution.ts`) with a test on the pacquet side: a `LockfileResolution::Tarball` with `integrity: None` — what a tampered lockfile looks like — must short-circuit the warm-batch cache-key derivation by surfacing `InstallPackageBySnapshotError::MissingTarballIntegrity`. The structural guard already existed but had no negative test. * fix(lockfile.utils): exempt git-hosted and file: tarballs from the integrity guard The strict guard added in the parent commit broke pnpm's own `with-git-protocol-dep` and `with-non-package-dep` fixtures: the install pipeline writes git-hosted tarball entries (codeload.github.com URLs) to the lockfile without an `integrity:` line, because the commit SHA in the URL is the integrity anchor — git's content-addressed model binds the bytes to the commit, so a separate hash adds nothing. Exempt git-hosted tarballs (detected either via the `gitHosted: true` flag or a URL on the known git hosts, matching the URL fallback in `toLockfileResolution`) and `file:` tarballs (local paths the user already controls). The strict check still fires for any other remote tarball — which is where the AutoFyn-reported vector actually manifests. Also export `isGitHostedTarballUrl` from `toLockfileResolution.ts` so the URL fallback can be shared rather than duplicated. * test(pacquet/package-manager): trim doc comment to the contract-level intent Per the repo convention that tests are documentation, the test name and body already cover what's being asserted; the prior comment duplicated that. Keep only the non-obvious why: why this guard exists at the cache-key site at all (warm-batch short-circuit) when the install-side check also rejects the same input. --- .changeset/require-tarball-integrity.md | 6 ++ lockfile/utils/src/pkgSnapshotToResolution.ts | 33 ++++++++++- lockfile/utils/src/toLockfileResolution.ts | 2 +- .../utils/test/pkgSnapshotToResolution.ts | 59 +++++++++++++++++++ .../src/create_virtual_store/tests.rs | 46 ++++++++++++++- 5 files changed, 141 insertions(+), 5 deletions(-) create mode 100644 .changeset/require-tarball-integrity.md diff --git a/.changeset/require-tarball-integrity.md b/.changeset/require-tarball-integrity.md new file mode 100644 index 0000000000..f8c34b6467 --- /dev/null +++ b/.changeset/require-tarball-integrity.md @@ -0,0 +1,6 @@ +--- +"@pnpm/lockfile.utils": patch +"pnpm": patch +--- + +Reject `pnpm-lock.yaml` entries whose remote tarball `resolution:` block is missing the `integrity` field. Previously the worker that extracts a downloaded tarball skipped hash verification when no integrity was supplied and minted a fresh one from the unverified bytes, so an attacker who could both alter the lockfile (e.g. via a pull request that strips `integrity:`) and serve modified content at the referenced tarball URL could install a tampered package without any error — including under `--frozen-lockfile`. pnpm now fails closed at lockfile-read time with `ERR_PNPM_MISSING_TARBALL_INTEGRITY`. Git-hosted tarballs (`gitHosted: true` or a URL on codeload.github.com / bitbucket.org / gitlab.com) and `file:` tarballs are exempt — the commit SHA in a git-host URL and the user-controlled local path already anchor the bytes. diff --git a/lockfile/utils/src/pkgSnapshotToResolution.ts b/lockfile/utils/src/pkgSnapshotToResolution.ts index 021b1369bb..5b210f43f6 100644 --- a/lockfile/utils/src/pkgSnapshotToResolution.ts +++ b/lockfile/utils/src/pkgSnapshotToResolution.ts @@ -1,22 +1,49 @@ import url from 'node:url' import * as dp from '@pnpm/deps.path' +import { PnpmError } from '@pnpm/error' import type { PackageSnapshot, TarballResolution } from '@pnpm/lockfile.types' import type { Resolution } from '@pnpm/resolving.resolver-base' import type { Registries } from '@pnpm/types' import getNpmTarballUrl from 'get-npm-tarball-url' import { nameVerFromPkgSnapshot } from './nameVerFromPkgSnapshot.js' +import { isGitHostedTarballUrl } from './toLockfileResolution.js' export function pkgSnapshotToResolution ( depPath: string, pkgSnapshot: PackageSnapshot, registries: Registries ): Resolution { + const resolution = pkgSnapshot.resolution as TarballResolution + // Tarball-shaped resolutions (no `type` field) must carry `integrity`, + // except where the URL itself anchors the bytes: + // - `file:` tarballs (local file on the user's machine; integrity + // adds nothing the user doesn't already control). + // - Git-hosted tarballs (URL contains the commit SHA; git's content- + // addressed model binds the bytes to the commit). The `gitHosted` + // flag may be absent on legacy lockfiles, so fall back to a URL + // match — same logic as `toLockfileResolution`. + // For any other tarball entry a missing integrity is what a tampered + // lockfile looks like: the worker would mint a fresh integrity from + // whatever bytes the URL returned, so we fail closed here. Pacquet + // enforces the same invariant via + // `pacquet_package_manager::missing_tarball_integrity`. if ( - Boolean((pkgSnapshot.resolution as TarballResolution).type) || - (pkgSnapshot.resolution as TarballResolution).tarball?.startsWith('file:') || - (pkgSnapshot.resolution as TarballResolution).gitHosted === true + resolution.type == null && + resolution.integrity == null && + !resolution.tarball?.startsWith('file:') && + !(resolution.gitHosted === true || (resolution.tarball != null && isGitHostedTarballUrl(resolution.tarball))) + ) { + throw new PnpmError('MISSING_TARBALL_INTEGRITY', + `Cannot install package "${depPath}": its lockfile entry has no "integrity" field, so pnpm cannot verify the downloaded tarball.`, + { hint: 'The lockfile may be corrupted or have been tampered with. Restore it from a trusted source, or delete it and re-run installation without --frozen-lockfile to regenerate.' } + ) + } + if ( + Boolean(resolution.type) || + resolution.tarball?.startsWith('file:') || + resolution.gitHosted === true ) { return pkgSnapshot.resolution as Resolution } diff --git a/lockfile/utils/src/toLockfileResolution.ts b/lockfile/utils/src/toLockfileResolution.ts index 324ae2ee9e..0a1348861d 100644 --- a/lockfile/utils/src/toLockfileResolution.ts +++ b/lockfile/utils/src/toLockfileResolution.ts @@ -74,7 +74,7 @@ function preservingGitHosted // Inlined to avoid pulling @pnpm/fetching.pick-fetcher into the lockfile-utils // dep graph. Used as a fallback when callers haven't pre-set the // `gitHosted` field on TarballResolution. -function isGitHostedTarballUrl (url: string): boolean { +export function isGitHostedTarballUrl (url: string): boolean { return ( url.startsWith('https://codeload.github.com/') || url.startsWith('https://bitbucket.org/') || diff --git a/lockfile/utils/test/pkgSnapshotToResolution.ts b/lockfile/utils/test/pkgSnapshotToResolution.ts index ca816e657f..2e0c9ebc8d 100644 --- a/lockfile/utils/test/pkgSnapshotToResolution.ts +++ b/lockfile/utils/test/pkgSnapshotToResolution.ts @@ -33,9 +33,11 @@ test('pkgSnapshotToResolution()', () => { expect(pkgSnapshotToResolution('@cdn.sheetjs.com/xlsx-0.18.9/xlsx-0.18.9.tgz', { resolution: { + integrity: 'sha512-CCCC', tarball: 'https://cdn.sheetjs.com/xlsx-0.18.9/xlsx-0.18.9.tgz', }, }, { default: 'https://registry.npmjs.org/' })).toEqual({ + integrity: 'sha512-CCCC', tarball: 'https://cdn.sheetjs.com/xlsx-0.18.9/xlsx-0.18.9.tgz', }) @@ -51,3 +53,60 @@ test('pkgSnapshotToResolution()', () => { tarball: 'file:test-package-1.0.0.tgz', }) }) + +test('pkgSnapshotToResolution() rejects a remote tarball resolution that has no integrity', () => { + // A tampered or malformed lockfile that strips the `integrity` field + // would otherwise let pnpm download the URL contents unchecked. The + // helper must fail closed so neither install path nor any read-only + // consumer (sbom, list, etc.) silently trusts the lockfile entry. + expect(() => pkgSnapshotToResolution('foo@1.0.0', { + resolution: { + tarball: 'https://registry.npmjs.org/foo/-/foo-1.0.0.tgz', + }, + }, { default: 'https://registry.npmjs.org/' })).toThrow(expect.objectContaining({ code: 'ERR_PNPM_MISSING_TARBALL_INTEGRITY' })) + + // A tarball URL on an arbitrary CDN (no `gitHosted` flag, no known git + // host pattern) is still a regular remote tarball — integrity required. + expect(() => pkgSnapshotToResolution('xlsx@https+++cdn.sheetjs.com+xlsx-0.18.9+xlsx-0.18.9.tgz', { + resolution: { + tarball: 'https://cdn.sheetjs.com/xlsx-0.18.9/xlsx-0.18.9.tgz', + }, + }, { default: 'https://registry.npmjs.org/' })).toThrow(expect.objectContaining({ code: 'ERR_PNPM_MISSING_TARBALL_INTEGRITY' })) +}) + +test('pkgSnapshotToResolution() allows git-hosted and file: tarballs to lack integrity', () => { + // Git-hosted tarballs are anchored by the commit SHA in their URL — + // pnpm's own install pipeline writes them without `integrity:` (see + // the `with-git-protocol-dep` fixture). Both the explicit + // `gitHosted: true` flag and a URL on a known git host must bypass + // the integrity check, matching the URL-fallback logic in + // `toLockfileResolution`. + expect(pkgSnapshotToResolution('foo@https+++github.com+foo+bar', { + resolution: { + tarball: 'https://codeload.github.com/foo/bar/tar.gz/abc1234', + gitHosted: true, + }, + }, { default: 'https://registry.npmjs.org/' })).toEqual({ + tarball: 'https://codeload.github.com/foo/bar/tar.gz/abc1234', + gitHosted: true, + }) + + expect(pkgSnapshotToResolution('is-negative@https+++codeload.github.com+kevva+is-negative+tar.gz+abc', { + resolution: { + tarball: 'https://codeload.github.com/kevva/is-negative/tar.gz/abc1234', + }, + }, { default: 'https://registry.npmjs.org/' })).toEqual({ + tarball: 'https://codeload.github.com/kevva/is-negative/tar.gz/abc1234', + }) + + // `file:` tarballs are local files; the user already controls the + // bytes, and the install pipeline may write them without integrity. + expect(pkgSnapshotToResolution('local-pkg@file:local-pkg-1.0.0.tgz', { + resolution: { + tarball: 'file:local-pkg-1.0.0.tgz', + }, + version: '1.0.0', + }, { default: 'https://registry.npmjs.org/' })).toEqual({ + tarball: 'file:local-pkg-1.0.0.tgz', + }) +}) diff --git a/pacquet/crates/package-manager/src/create_virtual_store/tests.rs b/pacquet/crates/package-manager/src/create_virtual_store/tests.rs index 91257e0bae..de722fbb81 100644 --- a/pacquet/crates/package-manager/src/create_virtual_store/tests.rs +++ b/pacquet/crates/package-manager/src/create_virtual_store/tests.rs @@ -1,5 +1,6 @@ use super::{ - emit_warm_snapshot_progress, integrity_equal, snapshot_cache_key, snapshot_deps_equal, + CreateVirtualStoreError, InstallPackageBySnapshotError, emit_warm_snapshot_progress, + integrity_equal, snapshot_cache_key, snapshot_deps_equal, }; use pacquet_lockfile::{ GitResolution, LockfileResolution, PackageKey, PackageMetadata, PkgName, PkgVerPeer, @@ -253,3 +254,46 @@ fn snapshot_cache_key_for_git_hosted_tarball_uses_git_hosted_key() { "git-hosted tarball resolutions must route through gitHostedStoreIndexKey", ); } + +/// Failing closed at the cache-key site (rather than only at the +/// install-side guard) is the whole point of the check duplication — +/// otherwise a malformed lockfile burns the warm rayon batch before +/// the install path fires the same error. +#[test] +fn snapshot_cache_key_rejects_tarball_without_integrity() { + let pkg = key("foo", "1.0.0"); + let packages = HashMap::from([(pkg.clone(), tarball_metadata_without_integrity())]); + + let err = + snapshot_cache_key(&pkg, &packages).expect_err("missing integrity must reject upfront"); + assert!( + matches!( + &err, + CreateVirtualStoreError::InstallPackageBySnapshot( + InstallPackageBySnapshotError::MissingTarballIntegrity { package_key }, + ) if package_key == &pkg.to_string(), + ), + "expected MissingTarballIntegrity for `{pkg}`, got {err:?}", + ); +} + +fn tarball_metadata_without_integrity() -> PackageMetadata { + PackageMetadata { + resolution: LockfileResolution::Tarball(TarballResolution { + tarball: "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz".to_string(), + integrity: None, + git_hosted: None, + path: None, + }), + engines: None, + cpu: None, + os: None, + libc: None, + deprecated: None, + has_bin: None, + prepare: None, + bundled_dependencies: None, + peer_dependencies: None, + peer_dependencies_meta: None, + } +}