From f29dacd5714c1515f536e93e3adb01b097c27818 Mon Sep 17 00:00:00 2001 From: Tiago Vilas Boas Date: Thu, 10 Sep 2026 22:38:47 +0000 Subject: [PATCH 1/2] fix(filesystem): emit stable path validation reason codes Throw PathValidationError with machine-readable reasons for fail-closed denials, and rethrow coded errors in inner catch blocks so PARENT_OUTSIDE_ALLOWED is not remapped to PARENT_DIRECTORY_NOT_FOUND. Co-authored-by: Tiago Vilas Boas --- src/filesystem/lib.ts | 47 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 6cd2165b5a..e9693e3663 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -41,6 +41,25 @@ export interface SearchResult { isDirectory: boolean; } +export const PATH_VALIDATION_REASON = { + PATH_OUTSIDE_ALLOWED: "path_outside_allowed", + SYMLINK_TARGET_OUTSIDE_ALLOWED: "symlink_target_outside_allowed", + PARENT_OUTSIDE_ALLOWED: "parent_outside_allowed", + PARENT_DIRECTORY_NOT_FOUND: "parent_directory_not_found", +} as const; + +export type PathValidationReason = (typeof PATH_VALIDATION_REASON)[keyof typeof PATH_VALIDATION_REASON]; + +export class PathValidationError extends Error { + constructor( + public readonly reason: PathValidationReason, + message: string, + ) { + super(message); + this.name = "PathValidationError"; + } +} + // Pure Utility Functions export function formatSize(bytes: number): string { const units = ['B', 'KB', 'MB', 'GB', 'TB']; @@ -130,7 +149,10 @@ async function resolveUnicodeEquivalentPath(absolutePath: string): Promise { // Security: Check if path is within allowed directories before any file operations const isAllowed = isPathWithinAllowedDirectories(normalizedRequested, allowedDirectories); if (!isAllowed) { - throw new Error(`Access denied - path outside allowed directories: ${absolute} not in ${allowedDirectories.join(', ')}`); + throw new PathValidationError( + PATH_VALIDATION_REASON.PATH_OUTSIDE_ALLOWED, + `Access denied - path outside allowed directories: ${absolute} not in ${allowedDirectories.join(', ')}`, + ); } // Security: Handle symlinks by checking their real path to prevent symlink attacks @@ -163,18 +188,32 @@ export async function validatePath(requestedPath: string): Promise { const realPath = await fs.realpath(absolute); const normalizedReal = normalizePath(realPath); if (!isPathWithinAllowedDirectories(normalizedReal, allowedDirectories)) { - throw new Error(`Access denied - symlink target outside allowed directories: ${realPath} not in ${allowedDirectories.join(', ')}`); + throw new PathValidationError( + PATH_VALIDATION_REASON.SYMLINK_TARGET_OUTSIDE_ALLOWED, + `Access denied - symlink target outside allowed directories: ${realPath} not in ${allowedDirectories.join(', ')}`, + ); } return realPath; } catch (error) { + // Keep coded denials intact. A bare catch would remap PARENT_OUTSIDE_ALLOWED + // and SYMLINK_TARGET_OUTSIDE_ALLOWED to PARENT_DIRECTORY_NOT_FOUND. + if (error instanceof PathValidationError) { + throw error; + } // Security: For new files that don't exist yet, verify parent directory // This ensures we can't create files in unauthorized locations if ((error as NodeJS.ErrnoException).code === 'ENOENT') { try { return await resolveUnicodeEquivalentPath(absolute); } catch (resolutionError) { + if (resolutionError instanceof PathValidationError) { + throw resolutionError; + } if ((resolutionError as NodeJS.ErrnoException).code === 'ENOENT') { - throw new Error(`Parent directory does not exist: ${path.dirname(absolute)}`); + throw new PathValidationError( + PATH_VALIDATION_REASON.PARENT_DIRECTORY_NOT_FOUND, + `Parent directory does not exist: ${path.dirname(absolute)}`, + ); } throw resolutionError; } From faf1629689ffdf977c53a12cc89ecb74eb67d9e6 Mon Sep 17 00:00:00 2001 From: Tiago Vilas Boas Date: Thu, 10 Sep 2026 22:38:58 +0000 Subject: [PATCH 2/2] test(filesystem): assert path validation reason codes Cover PATH_OUTSIDE_ALLOWED, SYMLINK_TARGET_OUTSIDE_ALLOWED, PARENT_OUTSIDE_ALLOWED, and PARENT_DIRECTORY_NOT_FOUND with structured assertions, including a real-filesystem parent-escape regression. Co-authored-by: Tiago Vilas Boas --- src/filesystem/__tests__/lib.test.ts | 76 ++++++++++++++++++- .../__tests__/nested-parents.test.ts | 49 +++++++++++- 2 files changed, 118 insertions(+), 7 deletions(-) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index 24eb18a6de..5f20c67275 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -8,6 +8,8 @@ import { normalizeLineEndings, createUnifiedDiff, // Security & validation functions + PATH_VALIDATION_REASON, + PathValidationError, validatePath, setAllowedDirectories, // File operations @@ -195,8 +197,64 @@ describe('Lib Functions', () => { it('rejects disallowed paths', async () => { const testPath = process.platform === 'win32' ? 'C:\\Windows\\System32\\file.txt' : '/etc/passwd'; - await expect(validatePath(testPath)) - .rejects.toThrow('Access denied - path outside allowed directories'); + let caughtError: unknown; + try { + await validatePath(testPath); + } catch (error) { + caughtError = error; + } + + expect(caughtError).toBeInstanceOf(PathValidationError); + expect(caughtError).toMatchObject({ + reason: PATH_VALIDATION_REASON.PATH_OUTSIDE_ALLOWED, + }); + expect((caughtError as Error).message).toContain('Access denied - path outside allowed directories'); + }); + + it('rejects symlink targets outside allowed directories with stable reason code', async () => { + const linkPath = process.platform === 'win32' ? 'C:\\Users\\test\\link.txt' : '/home/user/link.txt'; + const escapedTarget = process.platform === 'win32' ? 'C:\\Windows\\secret.txt' : '/etc/secret.txt'; + mockFs.realpath.mockResolvedValueOnce(escapedTarget); + + let caughtError: unknown; + try { + await validatePath(linkPath); + } catch (error) { + caughtError = error; + } + + expect(caughtError).toBeInstanceOf(PathValidationError); + expect(caughtError).toMatchObject({ + reason: PATH_VALIDATION_REASON.SYMLINK_TARGET_OUTSIDE_ALLOWED, + }); + }); + + it('rejects parent directories outside allowed directories with stable reason code', async () => { + const newFilePath = process.platform === 'win32' ? 'C:\\Users\\test\\link\\newfile.txt' : '/home/user/link/newfile.txt'; + const allowedParent = process.platform === 'win32' ? 'C:\\Users\\test' : '/home/user'; + const escapedParent = process.platform === 'win32' ? 'C:\\Windows' : '/etc'; + + const enoentError = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError.code = 'ENOENT'; + + mockFs.realpath + .mockRejectedValueOnce(enoentError) + .mockResolvedValueOnce(allowedParent) + .mockResolvedValueOnce(escapedParent); + mockFs.readdir.mockResolvedValueOnce(['link']); + + let caughtError: unknown; + try { + await validatePath(newFilePath); + } catch (error) { + caughtError = error; + } + + expect(caughtError).toBeInstanceOf(PathValidationError); + expect(caughtError).toMatchObject({ + reason: PATH_VALIDATION_REASON.PARENT_OUTSIDE_ALLOWED, + }); + expect((caughtError as Error).message).toContain('parent directory outside allowed directories'); }); it('handles non-existent files by checking parent directory', async () => { @@ -243,8 +301,18 @@ describe('Lib Functions', () => { // Every ancestor, all the way up to the filesystem root, is missing. mockFs.realpath.mockRejectedValue(enoentError); - await expect(validatePath(newFilePath)) - .rejects.toThrow('Parent directory does not exist'); + let caughtError: unknown; + try { + await validatePath(newFilePath); + } catch (error) { + caughtError = error; + } + + expect(caughtError).toBeInstanceOf(PathValidationError); + expect(caughtError).toMatchObject({ + reason: PATH_VALIDATION_REASON.PARENT_DIRECTORY_NOT_FOUND, + }); + expect((caughtError as Error).message).toContain('Parent directory does not exist'); }); it('resolves relative paths against allowed directories instead of process.cwd()', async () => { diff --git a/src/filesystem/__tests__/nested-parents.test.ts b/src/filesystem/__tests__/nested-parents.test.ts index 854dc4405f..d4ed6f3eb4 100644 --- a/src/filesystem/__tests__/nested-parents.test.ts +++ b/src/filesystem/__tests__/nested-parents.test.ts @@ -2,7 +2,7 @@ import fs from 'fs/promises'; import os from 'os'; import path from 'path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import { setAllowedDirectories, validatePath } from '../lib.js'; +import { PATH_VALIDATION_REASON, PathValidationError, setAllowedDirectories, validatePath } from '../lib.js'; // Regression coverage for #4629: validatePath must accept a path whose // ancestors are missing several levels deep, so create_directory can mkdir -p. @@ -22,6 +22,20 @@ describe('validatePath with multiple missing ancestors', () => { await fs.rm(outsideDir, { recursive: true, force: true }); }); + it('rejects paths outside allowed directories with a stable reason code', async () => { + let caughtError: unknown; + try { + await validatePath(path.join(outsideDir, 'file.txt')); + } catch (error) { + caughtError = error; + } + + expect(caughtError).toBeInstanceOf(PathValidationError); + expect(caughtError).toMatchObject({ + reason: PATH_VALIDATION_REASON.PATH_OUTSIDE_ALLOWED, + }); + }); + it('returns the full path when several ancestors do not exist', async () => { const requested = path.join(allowedDir, 'a', 'b', 'c'); await expect(validatePath(requested)).resolves.toBe(requested); @@ -29,7 +43,36 @@ describe('validatePath with multiple missing ancestors', () => { it('rejects when the nearest existing ancestor is a symlink out of the allowed tree', async () => { await fs.symlink(outsideDir, path.join(allowedDir, 'link'), 'junction'); - await expect(validatePath(path.join(allowedDir, 'link', 'a', 'b'))) - .rejects.toThrow('Access denied'); + + let caughtError: unknown; + try { + await validatePath(path.join(allowedDir, 'link', 'a', 'b')); + } catch (error) { + caughtError = error; + } + + expect(caughtError).toBeInstanceOf(PathValidationError); + expect(caughtError).toMatchObject({ + reason: PATH_VALIDATION_REASON.PARENT_OUTSIDE_ALLOWED, + }); + expect((caughtError as Error).message).toContain('Access denied'); + }); + + it('rejects an existing symlink whose target is outside allowed directories', async () => { + const escapedFile = path.join(outsideDir, 'secret.txt'); + await fs.writeFile(escapedFile, 'secret'); + await fs.symlink(escapedFile, path.join(allowedDir, 'link.txt'), 'file'); + + let caughtError: unknown; + try { + await validatePath(path.join(allowedDir, 'link.txt')); + } catch (error) { + caughtError = error; + } + + expect(caughtError).toBeInstanceOf(PathValidationError); + expect(caughtError).toMatchObject({ + reason: PATH_VALIDATION_REASON.SYMLINK_TARGET_OUTSIDE_ALLOWED, + }); }); });