fix: don't write data from the lockfile to the global store (#4395)

pnpm · Feb 27, 2022 · 8233842 · 8233842
1 parent f207186
commit 8233842
Show file tree

Hide file tree

Showing 9 changed files with 135 additions and 43 deletions.
diff --git a/.changeset/nine-eggs-prove.md b/.changeset/nine-eggs-prove.md
@@ -0,0 +1,7 @@
+---
+"@pnpm/core": patch
+"@pnpm/resolve-dependencies": patch
+"pnpm": patch
+---
+
+In order to guarantee that only correct data is written to the store, data from the lockfile should not be written to the store. Only data directly from the package tarball or package metadata.
diff --git a/.changeset/thirty-days-carry.md b/.changeset/thirty-days-carry.md
@@ -0,0 +1,6 @@
+---
+"@pnpm/package-requester": major
+"@pnpm/store-controller-types": major
+---
+
+Changes to RequestPackageOptions: currentPkg.name and currentPkg.version removed.
diff --git a/packages/core/test/lockfile.ts b/packages/core/test/lockfile.ts
@@ -1337,3 +1337,40 @@ test('build metadata is always ignored in versions and the lockfile is not flick
   const updatedLockfile = await project.readLockfile()
   expect(initialPkgEntry).toStrictEqual(updatedLockfile.packages[depPath])
 })
+
+test('a broken lockfile should not break the store', async () => {
+  prepareEmpty()
+  const opts = await testDefaults()
+
+  const manifest = await addDependenciesToPackage({}, ['is-positive@1.0.0'], { ...opts, lockfileOnly: true })
+
+  const lockfile: Lockfile = await readYamlFile(WANTED_LOCKFILE)
+  lockfile.packages!['/is-positive/1.0.0'].name = 'bad-name'
+  lockfile.packages!['/is-positive/1.0.0'].version = '1.0.0'
+
+  await writeYamlFile(WANTED_LOCKFILE, lockfile)
+
+  await mutateModules([
+    {
+      buildIndex: 0,
+      manifest,
+      mutation: 'install',
+      rootDir: process.cwd(),
+    },
+  ], await testDefaults({ lockfileOnly: true, storeDir: path.resolve('store2') }))
+
+  delete lockfile.packages!['/is-positive/1.0.0'].name
+  delete lockfile.packages!['/is-positive/1.0.0'].version
+
+  await writeYamlFile(WANTED_LOCKFILE, lockfile)
+  await rimraf(path.resolve('node_modules'))
+
+  await mutateModules([
+    {
+      buildIndex: 0,
+      manifest,
+      mutation: 'install',
+      rootDir: process.cwd(),
+    },
+  ], await testDefaults({ lockfileOnly: true, storeDir: path.resolve('store2') }))
+})
diff --git a/packages/headless/src/lockfileToDepGraph.ts b/packages/headless/src/lockfileToDepGraph.ts
@@ -146,11 +146,13 @@ export default async function lockfileToDepGraph (
             force: false,
             lockfileDir: opts.lockfileDir,
             pkg: {
-              name: pkgName,
-              version: pkgVersion,
               id: packageId,
               resolution,
             },
+            expectedPkg: {
+              name: pkgName,
+              version: pkgVersion,
+            },
           })
           if (fetchResponse instanceof Promise) fetchResponse = await fetchResponse
         } catch (err: any) { // eslint-disable-line

diff --git a/packages/headless/src/lockfileToHoistedDepGraph.ts b/packages/headless/src/lockfileToHoistedDepGraph.ts
@@ -178,11 +178,13 @@ async function fetchDeps (
         force: false,
         lockfileDir: opts.lockfileDir,
         pkg: {
-          name: pkgName,
-          version: pkgVersion,
           id: packageId,
           resolution,
         },
+        expectedPkg: {
+          name: pkgName,
+          version: pkgVersion,
+        },
       })
       if (fetchResponse instanceof Promise) fetchResponse = await fetchResponse
     } catch (err: any) { // eslint-disable-line

diff --git a/packages/package-requester/src/packageRequester.ts b/packages/package-requester/src/packageRequester.ts
@@ -31,7 +31,9 @@ import {
 import {
   BundledManifest,
   FetchPackageToStoreFunction,
+  FetchPackageToStoreOptions,
   PackageResponse,
+  PkgNameVersion,
   RequestPackageFunction,
   RequestPackageOptions,
   WantedDependency,
@@ -242,15 +244,17 @@ async function resolveAndFetch (
     }
   }
 
+  const pkg = pick(['name', 'version'], manifest ?? {})
   const fetchResult = ctx.fetchPackageToStore({
     fetchRawManifest: true,
     force: forceFetch,
     lockfileDir: options.lockfileDir,
     pkg: {
-      ...pick(['name', 'version'], manifest ?? options.currentPkg ?? {}),
+      ...pkg,
       id,
       resolution,
     },
+    expectedPkg: options.expectedPkg?.name != null ? options.expectedPkg : pkg,
   })
 
   return {
@@ -297,23 +301,16 @@ function fetchToStore (
     storeDir: string
     verifyStoreIntegrity: boolean
   },
-  opts: {
-    pkg: {
-      name?: string
-      version?: string
-      id: string
-      resolution: Resolution
-    }
-    fetchRawManifest?: boolean
-    force: boolean
-    lockfileDir: string
-  }
+  opts: FetchPackageToStoreOptions
 ): {
     bundledManifest?: () => Promise<BundledManifest>
     filesIndexFile: string
     files: () => Promise<PackageFilesResponse>
     finishing: () => Promise<void>
   } {
+  if (!opts.pkg.name) {
+    opts.fetchRawManifest = true
+  }
   const targetRelative = depPathToFilename(opts.pkg.id, opts.lockfileDir)
   const target = path.join(ctx.storeDir, targetRelative)
 
@@ -445,23 +442,23 @@ function fetchToStore (
           if (
             (
               pkgFilesIndex.name != null &&
-              opts.pkg.name != null &&
-              pkgFilesIndex.name.toLowerCase() !== opts.pkg.name.toLowerCase()
+              opts.expectedPkg?.name != null &&
+              pkgFilesIndex.name.toLowerCase() !== opts.expectedPkg.name.toLowerCase()
             ) ||
             (
               pkgFilesIndex.version != null &&
-              opts.pkg.version != null &&
+              opts.expectedPkg?.version != null &&
               // We used to not normalize the package versions before writing them to the lockfile and store.
               // So it may happen that the version will be in different formats.
               // For instance, v1.0.0 and 1.0.0
               // Hence, we need to use semver.eq() to compare them.
-              !equalOrSemverEqual(pkgFilesIndex.version, opts.pkg.version)
+              !equalOrSemverEqual(pkgFilesIndex.version, opts.expectedPkg.version)
             )
           ) {
             /* eslint-disable @typescript-eslint/restrict-template-expressions */
             throw new PnpmError('UNEXPECTED_PKG_CONTENT_IN_STORE', `\
 Package name mismatch found while reading ${JSON.stringify(opts.pkg.resolution)} from the store. \
-This means that the lockfile is broken. Expected package: ${opts.pkg.name}@${opts.pkg.version}. \
+This means that the lockfile is broken. Expected package: ${opts.expectedPkg.name}@${opts.expectedPkg.version}. \
 Actual package in the store by the given integrity: ${pkgFilesIndex.name}@${pkgFilesIndex.version}.`)
             /* eslint-enable @typescript-eslint/restrict-template-expressions */
           }
@@ -550,11 +547,24 @@ Actual package in the store by the given integrity: ${pkgFilesIndex.name}@${pkgF
               }
             })
         )
-        await writeJsonFile(filesIndexFile, {
-          name: opts.pkg.name,
-          version: opts.pkg.version,
-          files: integrity,
-        })
+        if (opts.pkg.name && opts.pkg.version) {
+          await writeFilesIndexFile(filesIndexFile, {
+            pkg: opts.pkg,
+            files: integrity,
+          })
+        } else {
+          // Even though we could take the package name from the lockfile,
+          // it is not safe because the lockfile may be broken or manually edited.
+          // To be safe, we read the package name from the downloaded package's package.json instead.
+          /* eslint-disable @typescript-eslint/no-floating-promises */
+          bundledManifest.promise
+            .then((manifest) => writeFilesIndexFile(filesIndexFile, {
+              pkg: manifest,
+              files: integrity,
+            }))
+            .catch()
+          /* eslint-enable @typescript-eslint/no-floating-promises */
+        }
         filesResult = {
           fromStore: false,
           filesIndex: integrity,
@@ -584,6 +594,20 @@ Actual package in the store by the given integrity: ${pkgFilesIndex.name}@${pkgF
   }
 }
 
+async function writeFilesIndexFile (
+  filesIndexFile: string,
+  { pkg, files }: {
+    pkg: PkgNameVersion
+    files: Record<string, PackageFileInfo>
+  }
+) {
+  await writeJsonFile(filesIndexFile, {
+    name: pkg.name,
+    version: pkg.version,
+    files,
+  })
+}
+
 async function writeJsonFile (filePath: string, data: Object) {
   const targetDir = path.dirname(filePath)
   // TODO: use the API of @pnpm/cafs to write this file

diff --git a/packages/package-requester/test/index.ts b/packages/package-requester/test/index.ts
@@ -128,8 +128,6 @@ test('request package but skip fetching, when resolution is already available',
   const projectDir = tempy.directory()
   const pkgResponse = await requestPackage({ alias: 'is-positive', pref: '1.0.0' }, {
     currentPkg: {
-      name: 'is-positive',
-      version: '1.0.0',
       id: `localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`,
       resolution: {
         integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
@@ -203,8 +201,6 @@ test('refetch local tarball if its integrity has changed', async () => {
     const response = await requestPackage(wantedPackage, {
       ...requestPackageOpts,
       currentPkg: {
-        name: '@pnpm/package-requester',
-        version: '0.8.1',
         id: pkgId,
         resolution: {
           integrity: 'sha512-lqODmYcc/FKOGROEUByd5Sbugqhzgkv+Hij9PXH0sZVQsU2npTQ0x3L81GCtHilFKme8lhBtD31Vxg/AKYrAvg==',
@@ -238,8 +234,6 @@ test('refetch local tarball if its integrity has changed', async () => {
     const response = await requestPackage(wantedPackage, {
       ...requestPackageOpts,
       currentPkg: {
-        name: '@pnpm/package-requester',
-        version: '0.8.1',
         id: pkgId,
         resolution: {
           integrity: 'sha512-lqODmYcc/FKOGROEUByd5Sbugqhzgkv+Hij9PXH0sZVQsU2npTQ0x3L81GCtHilFKme8lhBtD31Vxg/AKYrAvg==',
@@ -267,8 +261,6 @@ test('refetch local tarball if its integrity has changed', async () => {
     const response = await requestPackage(wantedPackage, {
       ...requestPackageOpts,
       currentPkg: {
-        name: '@pnpm/package-requester',
-        version: '0.8.1',
         id: pkgId,
         resolution: {
           integrity: 'sha512-v3uhYkN+Eh3Nus4EZmegjQhrfpdPIH+2FjrkeBc6ueqZJWWRaLnSYIkD0An6m16D3v+6HCE18ox6t95eGxj5Pw==',
@@ -616,8 +608,6 @@ test('always return a package manifest in the response', async () => {
   {
     const pkgResponse = await requestPackage({ alias: 'is-positive', pref: '1.0.0' }, {
       currentPkg: {
-        name: 'is-positive',
-        version: '1.0.0',
         id: `localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`,
         resolution: {
           integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
@@ -894,6 +884,10 @@ test('throw exception if the package data in the store differs from the expected
         id: pkgResponse.body.id,
         resolution: pkgResponse.body.resolution,
       },
+      expectedPkg: {
+        name: 'is-negative',
+        version: '1.0.0',
+      },
     })
     await expect(files()).rejects.toThrow(/Package name mismatch found while reading/)
   }
@@ -917,6 +911,10 @@ test('throw exception if the package data in the store differs from the expected
         id: pkgResponse.body.id,
         resolution: pkgResponse.body.resolution,
       },
+      expectedPkg: {
+        name: 'is-negative',
+        version: '2.0.0',
+      },
     })
     await expect(files()).rejects.toThrow(/Package name mismatch found while reading/)
   }
@@ -940,6 +938,10 @@ test('throw exception if the package data in the store differs from the expected
         id: pkgResponse.body.id,
         resolution: pkgResponse.body.resolution,
       },
+      expectedPkg: {
+        name: 'is-positive',
+        version: 'v1.0.0',
+      },
     })
     await expect(files()).resolves.toStrictEqual(expect.anything())
   }
@@ -962,6 +964,10 @@ test('throw exception if the package data in the store differs from the expected
         id: pkgResponse.body.id,
         resolution: pkgResponse.body.resolution,
       },
+      expectedPkg: {
+        name: 'IS-positive',
+        version: 'v1.0.0',
+      },
     })
     await expect(files()).resolves.toStrictEqual(expect.anything())
   }

diff --git a/packages/resolve-dependencies/src/resolveDependencies.ts b/packages/resolve-dependencies/src/resolveDependencies.ts
@@ -609,12 +609,11 @@ async function resolveDependency (
       alwaysTryWorkspacePackages: ctx.linkWorkspacePackagesDepth >= options.currentDepth,
       currentPkg: currentPkg
         ? {
-          name: currentPkg.name,
-          version: currentPkg.version,
           id: currentPkg.pkgId,
           resolution: currentPkg.resolution,
         }
         : undefined,
+      expectedPkg: currentPkg,
       defaultTag: ctx.defaultTag,
       downloadPriority: -options.currentDepth,
       lockfileDir: ctx.lockfileDir,

diff --git a/packages/store-controller-types/src/index.ts b/packages/store-controller-types/src/index.ts
@@ -53,16 +53,23 @@ export type FetchPackageToStoreFunction = (
   finishing: () => Promise<void>
 }
 
+export interface PkgNameVersion {
+  name?: string
+  version?: string
+}
+
 export interface FetchPackageToStoreOptions {
   fetchRawManifest?: boolean
   force: boolean
   lockfileDir: string
-  pkg: {
+  pkg: PkgNameVersion & {
     id: string
-    name?: string
-    version?: string
     resolution: Resolution
   }
+  /**
+   * Expected package is the package name and version that are found in the lockfile.
+   */
+  expectedPkg?: PkgNameVersion
 }
 
 export type RequestPackageFunction = (
@@ -74,10 +81,12 @@ export interface RequestPackageOptions {
   alwaysTryWorkspacePackages?: boolean
   currentPkg?: {
     id?: string
-    name?: string
-    version?: string
     resolution?: Resolution
   }
+  /**
+   * Expected package is the package name and version that are found in the lockfile.
+   */
+  expectedPkg?: PkgNameVersion
   defaultTag?: string
   downloadPriority: number
   projectDir: string