mirror of
https://github.com/pnpm/pnpm.git
synced 2026-06-01 12:41:16 -04:00
fix: require integrity for tarball-shaped lockfile resolutions (#11966)
* 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.
This commit is contained in:
6
.changeset/require-tarball-integrity.md
Normal file
6
.changeset/require-tarball-integrity.md
Normal file
@@ -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.
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -74,7 +74,7 @@ function preservingGitHosted<T extends { tarball?: string, integrity: string }>
|
||||
// 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/') ||
|
||||
|
||||
@@ -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',
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user