diff --git a/.changeset/fix-symlink-path-traversal.md b/.changeset/fix-symlink-path-traversal.md new file mode 100644 index 0000000000..267926d06e --- /dev/null +++ b/.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. diff --git a/AGENTS.md b/AGENTS.md index 49606b3831..6b0696b19e 100644 --- a/AGENTS.md +++ b/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. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5ca1e2ab1b..75e2de4503 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -428,7 +428,7 @@ catalogs: version: 2.0.0 ini: specifier: 6.0.0 - version: 5.0.0 + version: 6.0.0 is-gzip: specifier: 2.0.0 version: 2.0.0 @@ -659,7 +659,7 @@ catalogs: version: 4.2.0 ssri: specifier: 13.0.0 - version: 12.0.0 + version: 13.0.0 stacktracey: specifier: ^2.1.8 version: 2.1.8 @@ -3366,7 +3366,7 @@ importers: version: 13.3.4 ssri: specifier: 'catalog:' - version: 12.0.0 + version: 13.0.0 tempy: specifier: 'catalog:' version: 3.0.0 @@ -5235,7 +5235,7 @@ importers: version: 7.0.0 write-json-file: specifier: 'catalog:' - version: 6.0.0 + version: 7.0.0 write-yaml-file: specifier: 'catalog:' version: 5.0.0 @@ -5487,7 +5487,7 @@ importers: version: 3.0.0 write-json-file: specifier: 'catalog:' - version: 6.0.0 + version: 7.0.0 pkg-manager/hoist: dependencies: @@ -6088,7 +6088,7 @@ importers: version: 3.0.0 write-json-file: specifier: 'catalog:' - version: 6.0.0 + version: 7.0.0 write-package: specifier: 'catalog:' version: 7.2.0 @@ -6767,7 +6767,7 @@ importers: version: 7.1.0 ini: specifier: 'catalog:' - version: 5.0.0 + version: 6.0.0 is-windows: specifier: 'catalog:' version: 1.0.2 @@ -6818,7 +6818,7 @@ importers: version: 1.2.2 write-json-file: specifier: 'catalog:' - version: 6.0.0 + version: 7.0.0 write-package: specifier: 'catalog:' version: 7.2.0 @@ -8118,6 +8118,9 @@ importers: is-gzip: specifier: 'catalog:' version: 2.0.0 + is-subdir: + specifier: 'catalog:' + version: 1.2.0 p-limit: specifier: 'catalog:' version: 7.1.0 @@ -8397,7 +8400,7 @@ importers: version: safe-execa@0.2.0 ssri: specifier: 'catalog:' - version: 12.0.0 + version: 13.0.0 tempy: specifier: 'catalog:' version: 3.0.0 @@ -13355,10 +13358,6 @@ packages: resolution: {integrity: sha512-QQnnxNyfvmHFIsj7gkPcYymR8Jdw/o7mp5ZFihxn6h8Ci6fh3Dx4E1gPjpQEpIuPo9XVNY/ZUwh4BPMjGyL01g==} engines: {node: ^14.17.0 || ^16.13.0 || >=18.0.0} - ini@5.0.0: - resolution: {integrity: sha512-+N0ngpO3e7cRUWOJAS7qw0IZIVc6XPrW4MlFBdD066F2L4k1L6ker3hLqSq7iXxU5tgS4WGkIUElWn5vogAEnw==} - engines: {node: ^18.17.0 || >=20.5.0} - ini@6.0.0: resolution: {integrity: sha512-IBTdIkzZNOpqm7q3dRqJvMaldXjDHWkEDfrwGEQTs5eaQMWV+djAhR+wahyNNMAa+qpbDUhBMVt4ZKNwpPm7xQ==} engines: {node: ^20.17.0 || >=22.9.0} @@ -22135,8 +22134,6 @@ snapshots: ini@4.1.1: {} - ini@5.0.0: {} - ini@6.0.0: {} inquirer@9.3.7: diff --git a/store/cafs/package.json b/store/cafs/package.json index 5bf26ce107..ceeeabca7d 100644 --- a/store/cafs/package.json +++ b/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:", diff --git a/store/cafs/src/addFilesFromDir.ts b/store/cafs/src/addFilesFromDir.ts index 7ce6995afd..58436aaaf4 100644 --- a/store/cafs/src/addFilesFromDir.ts +++ b/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 = new Map() as 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 +} + 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, diff --git a/store/cafs/test/index.ts b/store/cafs/test/index.ts index 3b47dc9717..9f22d72e22 100644 --- a/store/cafs/test/index.ts +++ b/store/cafs/test/index.ts @@ -69,6 +69,83 @@ describe('cafs', () => { expect(filesIndex.get('lib/index.js')).toBeDefined() expect(filesIndex.get('lib/index.js')).toStrictEqual(filesIndex.get('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()', () => { diff --git a/store/cafs/test/recursiveSymlink.test.ts b/store/cafs/test/recursiveSymlink.test.ts new file mode 100644 index 0000000000..ce1c699132 --- /dev/null +++ b/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) +})