fix: skip symlinks pointing outside package root in git and file deps (#10493)

.changeset/fix-symlink-path-traversal.md

@@ -0,0 +1,10 @@
+---
+"@pnpm/store.cafs": patch
+"pnpm": patch
+---
+
+When pnpm installs a `file:` or `git:` dependency, it now validates that symlinks point within the package directory. Symlinks to paths outside the package root are skipped to prevent local data from being leaked into `node_modules`.
+
+This fixes a security issue where a malicious package could create symlinks to sensitive files (e.g., `/etc/passwd`, `~/.ssh/id_rsa`) and have their contents copied when the package is installed.
+
+Note: This only affects `file:` and `git:` dependencies. Registry packages (npm) have symlinks stripped during publish and are not affected.

AGENTS.md

@@ -136,6 +136,33 @@ To ensure your code adheres to the style guide, run:
 pnpm run lint
 ```
 
+## Common Gotchas
+
+### Error Type Checking in Jest
+
+When checking if a caught error is an `Error` object, **do not use `instanceof Error`**. Jest runs tests in a VM context where `instanceof` checks can fail across realms.
+
+Instead, use `util.types.isNativeError()`:
+
+```typescript
+import util from 'util'
+
+try {
+  // ... some operation
+} catch (err: unknown) {
+  // ❌ Wrong - may fail in Jest
+  if (err instanceof Error && 'code' in err && err.code === 'ENOENT') {
+    return null
+  }
+  
+  // ✅ Correct - works across realms
+  if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') {
+    return null
+  }
+  throw err
+}
+```
+
 ## Key Configuration Files
 
 -   `pnpm-workspace.yaml`: Defines the workspace structure.

store/cafs/package.json

@@ -36,6 +36,7 @@
     "@pnpm/store-controller-types": "workspace:*",
     "@zkochan/rimraf": "catalog:",
     "is-gzip": "catalog:",
+    "is-subdir": "catalog:",
     "p-limit": "catalog:",
     "rename-overwrite": "catalog:",
     "ssri": "catalog:",

store/cafs/src/addFilesFromDir.ts

@@ -8,6 +8,7 @@ import {
 } from '@pnpm/cafs-types'
 import gfs from '@pnpm/graceful-fs'
 import { type DependencyManifest } from '@pnpm/types'
+import isSubdir from 'is-subdir'
 import { parseJsonBufferSync } from './parseJson.js'
 
 export function addFilesFromDir (
@@ -21,17 +22,14 @@ export function addFilesFromDir (
   const filesIndex: FilesIndex = {}
   let manifest: DependencyManifest | undefined
   let files: File[]
+  // Resolve the package root to a canonical path for security validation
+  const resolvedRoot = fs.realpathSync(dirname)
   if (opts.files) {
     files = []
     for (const file of opts.files) {
       const absolutePath = path.join(dirname, file)
-      let stat: Stats
-      try {
-        stat = fs.statSync(absolutePath)
-      } catch (err: unknown) {
-        if (!(util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT')) {
-          throw err
-        }
+      const stat = getStatIfContained(absolutePath, resolvedRoot)
+      if (!stat) {
         continue
       }
       files.push({
@@ -41,7 +39,7 @@ export function addFilesFromDir (
       })
     }
   } else {
-    files = findFilesInDir(dirname)
+    files = findFilesInDir(dirname, resolvedRoot)
   }
   for (const { absolutePath, relativePath, stat } of files) {
     const buffer = gfs.readFileSync(absolutePath)
@@ -65,41 +63,123 @@ interface File {
   stat: Stats
 }
 
-function findFilesInDir (dir: string): File[] {
+/**
+ * Resolves a path and validates it stays within the allowed root directory.
+ * If the path is a symlink, resolves it and validates the target.
+ * Returns null if the path is a symlink pointing outside the root, or if target is inaccessible.
+ */
+function getStatIfContained (
+  absolutePath: string,
+  rootDir: string
+): Stats | null {
+  let lstat: Stats
+  try {
+    lstat = fs.lstatSync(absolutePath)
+  } catch (err: unknown) {
+    if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') {
+      return null
+    }
+    throw err
+  }
+  if (lstat.isSymbolicLink()) {
+    return getSymlinkStatIfContained(absolutePath, rootDir)?.stat ?? null
+  }
+  return lstat
+}
+
+/**
+ * Validates a known symlink points within the allowed root directory.
+ * Returns null if the symlink points outside the root or if target is inaccessible.
+ */
+function getSymlinkStatIfContained (
+  absolutePath: string,
+  rootDir: string
+): { stat: Stats, realPath: string } | null {
+  let realPath: string
+  try {
+    realPath = fs.realpathSync(absolutePath)
+  } catch (err: unknown) {
+    // Broken symlink or inaccessible target
+    if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') {
+      return null
+    }
+    throw err
+  }
+  // isSubdir returns true if realPath is within rootDir OR if they are equal
+  if (!isSubdir(rootDir, realPath)) {
+    return null // Symlink points outside package - skip
+  }
+  return { stat: fs.statSync(realPath), realPath }
+}
+
+function findFilesInDir (dir: string, rootDir: string): File[] {
   const files: File[] = []
-  findFiles(files, dir)
+  const ctx: FindFilesContext = {
+    filesList: files,
+    rootDir,
+    visited: new Set([rootDir]),
+  }
+  findFiles(ctx, dir, '', rootDir)
   return files
 }
 
+interface FindFilesContext {
+  filesList: File[]
+  rootDir: string
+  visited: Set<string>
+}
+
 function findFiles (
-  filesList: File[],
+  ctx: FindFilesContext,
   dir: string,
-  relativeDir = ''
+  relativeDir: string,
+  currentRealPath: string
 ): void {
   const files = fs.readdirSync(dir, { withFileTypes: true })
   for (const file of files) {
     const relativeSubdir = `${relativeDir}${relativeDir ? '/' : ''}${file.name}`
-    if (file.isDirectory()) {
+    const absolutePath = path.join(dir, file.name)
+    let nextRealDir: string | undefined
+
+    if (file.isSymbolicLink()) {
+      const res = getSymlinkStatIfContained(absolutePath, ctx.rootDir)
+      if (!res) {
+        continue
+      }
+      if (res.stat.isDirectory()) {
+        nextRealDir = res.realPath
+      } else {
+        ctx.filesList.push({
+          relativePath: relativeSubdir,
+          absolutePath,
+          stat: res.stat,
+        })
+        continue
+      }
+    } else if (file.isDirectory()) {
+      nextRealDir = path.join(currentRealPath, file.name)
+    }
+
+    if (nextRealDir) {
+      if (ctx.visited.has(nextRealDir)) continue
       if (relativeDir !== '' || file.name !== 'node_modules') {
-        findFiles(filesList, path.join(dir, file.name), relativeSubdir)
+        ctx.visited.add(nextRealDir)
+        findFiles(ctx, absolutePath, relativeSubdir, nextRealDir)
+        ctx.visited.delete(nextRealDir)
       }
       continue
     }
-    const absolutePath = path.join(dir, file.name)
+
     let stat: Stats
     try {
       stat = fs.statSync(absolutePath)
-    } catch (err: any) { // eslint-disable-line
-      if (err.code !== 'ENOENT') {
-        throw err
+    } catch (err: unknown) {
+      if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') {
+        continue
       }
-      continue
+      throw err
     }
-    if (stat.isDirectory()) {
-      findFiles(filesList, path.join(dir, file.name), relativeSubdir)
-      continue
-    }
-    filesList.push({
+    ctx.filesList.push({
       relativePath: relativeSubdir,
       absolutePath,
       stat,

store/cafs/test/index.ts

@@ -69,6 +69,83 @@ describe('cafs', () => {
     expect(filesIndex['lib/index.js']).toBeDefined()
     expect(filesIndex['lib/index.js']).toStrictEqual(filesIndex['lib-symlink/index.js'])
   })
+
+  // Security test: symlinks pointing outside the package root should be rejected
+  // This prevents file: and git: dependencies from leaking local data via malicious symlinks
+  it('rejects symlinks pointing outside the package directory', () => {
+    const storeDir = temporaryDirectory()
+    const srcDir = temporaryDirectory()
+
+    // Create a legitimate file inside the package
+    fs.writeFileSync(path.join(srcDir, 'legit.txt'), 'legitimate content')
+
+    // Create a file outside the package that a malicious symlink tries to leak
+    const outsideDir = temporaryDirectory()
+    const secretFile = path.join(outsideDir, 'secret.txt')
+    fs.writeFileSync(secretFile, 'secret content')
+
+    // Create a symlink pointing to the file outside the package
+    fs.symlinkSync(secretFile, path.join(srcDir, 'leak.txt'))
+
+    const { filesIndex } = createCafs(storeDir).addFilesFromDir(srcDir)
+
+    // The legitimate file should be included
+    expect(filesIndex.get('legit.txt')).toBeDefined()
+
+    // The symlink pointing outside should be skipped (security fix)
+    expect(filesIndex.get('leak.txt')).toBeUndefined()
+  })
+
+  // Security test: symlinked directories pointing outside the package should be rejected
+  it('rejects symlinked directories pointing outside the package', () => {
+    const storeDir = temporaryDirectory()
+    const srcDir = temporaryDirectory()
+
+    // Create a legitimate file inside the package
+    fs.writeFileSync(path.join(srcDir, 'legit.txt'), 'legitimate content')
+
+    // Create a directory with secret files outside the package
+    const outsideDir = temporaryDirectory()
+    fs.writeFileSync(path.join(outsideDir, 'secret.txt'), 'secret content')
+
+    // Create a symlink to the outside directory
+    fs.symlinkSync(outsideDir, path.join(srcDir, 'leak-dir'))
+
+    const { filesIndex } = createCafs(storeDir).addFilesFromDir(srcDir)
+
+    // The legitimate file should be included
+    expect(filesIndex.get('legit.txt')).toBeDefined()
+
+    // Files from the symlinked directory pointing outside should NOT be included
+    expect(filesIndex.get('leak-dir/secret.txt')).toBeUndefined()
+  })
+
+  // Symlinked node_modules at the root should be skipped just like regular node_modules
+  it('skips symlinked node_modules directory at root', () => {
+    const storeDir = temporaryDirectory()
+    const srcDir = temporaryDirectory()
+
+    // Create a legitimate file inside the package
+    fs.writeFileSync(path.join(srcDir, 'index.js'), '// code')
+
+    // Create a target directory for the symlink (inside the package to pass containment check)
+    const targetDir = path.join(srcDir, '.deps')
+    fs.mkdirSync(targetDir)
+    fs.writeFileSync(path.join(targetDir, 'dep.js'), '// dep')
+
+    // Create a symlinked node_modules directory at the root
+    fs.symlinkSync(targetDir, path.join(srcDir, 'node_modules'))
+
+    const { filesIndex } = createCafs(storeDir).addFilesFromDir(srcDir)
+
+    // The legitimate file should be included
+    expect(filesIndex.get('index.js')).toBeDefined()
+    // The target files under .deps should be included
+    expect(filesIndex.get('.deps/dep.js')).toBeDefined()
+
+    // Files from symlinked node_modules at root should NOT be included
+    expect(filesIndex.get('node_modules/dep.js')).toBeUndefined()
+  })
 })
 
 describe('checkPkgFilesIntegrity()', () => {

store/cafs/test/recursiveSymlink.test.ts

@@ -0,0 +1,19 @@
+import fs from 'fs'
+import path from 'path'
+import { temporaryDirectory } from 'tempy'
+import { createCafs } from '../src/index.js'
+
+test('addFilesFromDir does not loop infinitely on recursive symlinks', () => {
+  const storeDir = temporaryDirectory()
+  const srcDir = temporaryDirectory()
+
+  fs.writeFileSync(path.join(srcDir, 'file.txt'), 'content')
+  // Create a symlink pointing to the current directory
+  fs.symlinkSync('.', path.join(srcDir, 'self'))
+
+  const cafs = createCafs(storeDir)
+  const { filesIndex } = cafs.addFilesFromDir(srcDir)
+
+  expect(filesIndex.has('file.txt')).toBe(true)
+  expect(filesIndex.has('self/file.txt')).toBe(false)
+})