fix: corrupted side effects cache should be ignored (#5930)

close #4997

.changeset/chatty-pens-end.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/cafs": major
+---
+
+checkFilesIntegrity renamed to checkPkgFilesIntegrity and its API has changed.

.changeset/fluffy-pianos-complain.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/package-requester": patch
+"@pnpm/cafs": patch
+"pnpm": patch
+---
+
+The store integrity check should validate the side effects cache of the installed package. If the side effects cache is broken, the package needs to be rebuilt [#4997](https://github.com/pnpm/pnpm/issues/4997).

pkg-manager/core/test/install/sideEffects.ts

@@ -1,7 +1,7 @@
-import { promises as fs, readFileSync } from 'fs'
+import { promises as fs, existsSync, readFileSync } from 'fs'
 import path from 'path'
-import { addDependenciesToPackage } from '@pnpm/core'
-import { getFilePathInCafs, PackageFilesIndex } from '@pnpm/cafs'
+import { addDependenciesToPackage, install } from '@pnpm/core'
+import { getFilePathInCafs, getFilePathByModeInCafs, PackageFilesIndex } from '@pnpm/cafs'
 import { getIntegrity, REGISTRY_MOCK_PORT } from '@pnpm/registry-mock'
 import { prepareEmpty } from '@pnpm/prepare'
 import { ENGINE_NAME } from '@pnpm/constants'
@@ -186,3 +186,36 @@ test('a postinstall script does not modify the original sources added to the sto
 
   expect(readFileSync(getFilePathInCafs(cafsDir, originalFileIntegrity, 'nonexec'), 'utf8')).toEqual('')
 })
+
+test('a corrupted side-effects cache is ignored', async () => {
+  prepareEmpty()
+
+  const opts = await testDefaults({
+    fastUnpack: false,
+    sideEffectsCacheRead: true,
+    sideEffectsCacheWrite: true,
+  })
+  const manifest = await addDependenciesToPackage({}, ['@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'], opts)
+
+  const cafsDir = path.join(opts.storeDir, 'files')
+  const filesIndexFile = getFilePathInCafs(cafsDir, getIntegrity('@pnpm.e2e/pre-and-postinstall-scripts-example', '1.0.0'), 'index')
+  const filesIndex = await loadJsonFile<PackageFilesIndex>(filesIndexFile)
+  expect(filesIndex.sideEffects).toBeTruthy() // files index has side effects
+  const sideEffectsKey = `${ENGINE_NAME}-${JSON.stringify({ '/@pnpm.e2e/hello-world-js-bin/1.0.0': {} })}`
+  expect(filesIndex.sideEffects).toHaveProperty([sideEffectsKey, 'generated-by-preinstall.js'])
+  const sideEffectFileStat = filesIndex.sideEffects![sideEffectsKey]['generated-by-preinstall.js']
+  const sideEffectFile = getFilePathByModeInCafs(cafsDir, sideEffectFileStat.integrity, sideEffectFileStat.mode)
+  expect(existsSync(sideEffectFile)).toBeTruthy()
+  await rimraf(sideEffectFile) // we remove the side effect file to break the store
+
+  await rimraf('node_modules')
+  const opts2 = await testDefaults({
+    fastUnpack: false,
+    sideEffectsCacheRead: true,
+    sideEffectsCacheWrite: true,
+    storeDir: opts.storeDir,
+  })
+  await install(manifest, opts2)
+
+  expect(await exists(path.resolve('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js'))).toBeTruthy() // side effects cache correctly used
+})

pkg-manager/package-requester/src/packageRequester.ts

@@ -1,7 +1,7 @@
 import { createReadStream, promises as fs } from 'fs'
 import path from 'path'
 import {
-  checkFilesIntegrity as _checkFilesIntegrity,
+  checkPkgFilesIntegrity as _checkFilesIntegrity,
   readManifestFromStore as _readManifestFromStore,
   FileType,
   getFilePathByModeInCafs as _getFilePathByModeInCafs,
@@ -323,7 +323,7 @@ function getFilesIndexFilePath (
 function fetchToStore (
   ctx: {
     checkFilesIntegrity: (
-      pkgIndex: Record<string, PackageFileInfo>,
+      pkgIndex: PackageFilesIndex,
       manifest?: DeferredManifestPromise
     ) => Promise<boolean>
     fetch: (
@@ -496,7 +496,7 @@ This means that the lockfile is broken. Expected package: ${opts.expectedPkg.nam
 Actual package in the store by the given integrity: ${pkgFilesIndex.name}@${pkgFilesIndex.version}.`)
             /* eslint-enable @typescript-eslint/restrict-template-expressions */
           }
-          const verified = await ctx.checkFilesIntegrity(pkgFilesIndex.files, manifest)
+          const verified = await ctx.checkFilesIntegrity(pkgFilesIndex, manifest)
           if (verified) {
             files.resolve({
               filesIndex: pkgFilesIndex.files,

store/cafs/src/checkFilesIntegrity.ts → store/cafs/src/checkPkgFilesIntegrity.ts

@@ -28,32 +28,59 @@ export interface PackageFilesIndex {
   sideEffects?: Record<string, Record<string, PackageFileInfo>>
 }
 
-export async function checkFilesIntegrity (
+export async function checkPkgFilesIntegrity (
   cafsDir: string,
-  pkgIndex: Record<string, PackageFileInfo>,
+  pkgIndex: PackageFilesIndex,
+  manifest?: DeferredManifestPromise
+) {
+  // It might make sense to use this cache for all files in the store
+  // but there's a smaller chance that the same file will be checked twice
+  // so it's probably not worth the memory (this assumption should be verified)
+  const verifiedFilesCache = new Set<string>()
+  const _checkFilesIntegrity = checkFilesIntegrity.bind(null, verifiedFilesCache, cafsDir)
+  const verified = await _checkFilesIntegrity(pkgIndex.files, manifest)
+  if (!verified) return false
+  if (pkgIndex.sideEffects) {
+    // We verify all side effects cache. We could optimize it to verify only the side effects cache
+    // that satisfies the current os/arch/platform.
+    // However, it likely won't make a big difference.
+    await Promise.all(
+      Object.entries(pkgIndex.sideEffects).map(async ([sideEffectName, files]) => {
+        if (!await _checkFilesIntegrity(files)) {
+          delete pkgIndex.sideEffects![sideEffectName]
+        }
+      })
+    )
+  }
+  return true
+}
+
+async function checkFilesIntegrity (
+  verifiedFilesCache: Set<string>,
+  cafsDir: string,
+  files: Record<string, PackageFileInfo>,
   manifest?: DeferredManifestPromise
 ): Promise<boolean> {
-  let verified = true
+  let allVerified = true
   await Promise.all(
-    Object.entries(pkgIndex)
+    Object.entries(files)
       .map(async ([f, fstat]) =>
         limit(async () => {
           if (!fstat.integrity) {
             throw new Error(`Integrity checksum is missing for ${f}`)
           }
-          if (
-            !await verifyFile(
-              getFilePathByModeInCafs(cafsDir, fstat.integrity, fstat.mode),
-              fstat,
-              f === 'package.json' ? manifest : undefined
-            )
-          ) {
-            verified = false
+          const filename = getFilePathByModeInCafs(cafsDir, fstat.integrity, fstat.mode)
+          const deferredManifest = manifest && f === 'package.json' ? manifest : undefined
+          if (!deferredManifest && verifiedFilesCache.has(filename)) return
+          if (await verifyFile(filename, fstat, deferredManifest)) {
+            verifiedFilesCache.add(filename)
+          } else {
+            allVerified = false
           }
         })
       )
   )
-  return verified
+  return allVerified
 }
 
 type FileInfo = Pick<PackageFileInfo, 'size' | 'checkedAt'> & {

store/cafs/src/index.ts

@@ -8,10 +8,10 @@ import ssri from 'ssri'
 import { addFilesFromDir } from './addFilesFromDir'
 import { addFilesFromTarball } from './addFilesFromTarball'
 import {
-  checkFilesIntegrity,
+  checkPkgFilesIntegrity,
   PackageFilesIndex,
   verifyFileIntegrity,
-} from './checkFilesIntegrity'
+} from './checkPkgFilesIntegrity'
 import { readManifestFromStore } from './readManifestFromStore'
 import {
   getFilePathInCafs,
@@ -25,7 +25,7 @@ import { writeFile } from './writeFile'
 export { IntegrityLike } from 'ssri'
 
 export {
-  checkFilesIntegrity,
+  checkPkgFilesIntegrity,
   readManifestFromStore,
   FileType,
   getFilePathByModeInCafs,

store/cafs/src/readManifestFromStore.ts

@@ -1,14 +1,15 @@
-import type { DeferredManifestPromise, PackageFileInfo } from '@pnpm/cafs-types'
+import type { DeferredManifestPromise } from '@pnpm/cafs-types'
 import gfs from '@pnpm/graceful-fs'
+import { PackageFilesIndex } from './checkPkgFilesIntegrity'
 import { getFilePathByModeInCafs } from './getFilePathInCafs'
 import { parseJsonBuffer } from './parseJson'
 
 export async function readManifestFromStore (
   cafsDir: string,
-  pkgIndex: Record<string, PackageFileInfo>,
+  pkgIndex: PackageFilesIndex,
   deferredManifest?: DeferredManifestPromise
 ) {
-  const pkg = pkgIndex['package.json']
+  const pkg = pkgIndex.files['package.json']
 
   if (deferredManifest) {
     if (pkg) {

store/cafs/test/index.ts

@@ -5,7 +5,7 @@ import path from 'path'
 import tempy from 'tempy'
 import {
   createCafs,
-  checkFilesIntegrity,
+  checkPkgFilesIntegrity,
   getFilePathInCafs,
 } from '../src'
 
@@ -45,14 +45,16 @@ describe('cafs', () => {
   })
 })
 
-describe('checkFilesIntegrity()', () => {
+describe('checkPkgFilesIntegrity()', () => {
   it("doesn't fail if file was removed from the store", async () => {
     const storeDir = tempy.directory()
-    expect(await checkFilesIntegrity(storeDir, {
-      foo: {
-        integrity: 'sha512-8xCvrlC7W3TlwXxetv5CZTi53szYhmT7tmpXF/ttNthtTR9TC7Y7WJFPmJToHaSQ4uObuZyOARdOJYNYuTSbXA==',
-        mode: 420,
-        size: 10,
+    expect(await checkPkgFilesIntegrity(storeDir, {
+      files: {
+        foo: {
+          integrity: 'sha512-8xCvrlC7W3TlwXxetv5CZTi53szYhmT7tmpXF/ttNthtTR9TC7Y7WJFPmJToHaSQ4uObuZyOARdOJYNYuTSbXA==',
+          mode: 420,
+          size: 10,
+        },
       },
     })).toBeFalsy()
   })