Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 72 additions & 4 deletions src/filesystem/__tests__/lib.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {
normalizeLineEndings,
createUnifiedDiff,
// Security & validation functions
PATH_VALIDATION_REASON,
PathValidationError,
validatePath,
setAllowedDirectories,
// File operations
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down
49 changes: 46 additions & 3 deletions src/filesystem/__tests__/nested-parents.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -22,14 +22,57 @@ 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);
});

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,
});
});
});
47 changes: 43 additions & 4 deletions src/filesystem/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down Expand Up @@ -130,7 +149,10 @@ async function resolveUnicodeEquivalentPath(absolutePath: string): Promise<strin

currentPath = await fs.realpath(path.join(currentPath, equivalentMatches[0]));
if (!isPathWithinAllowedDirectories(normalizePath(currentPath), allowedDirectories)) {
throw new Error(`Access denied - symlink target outside allowed directories: ${currentPath} not in ${allowedDirectories.join(', ')}`);
throw new PathValidationError(
PATH_VALIDATION_REASON.PARENT_OUTSIDE_ALLOWED,
`Access denied - parent directory outside allowed directories: ${currentPath} not in ${allowedDirectories.join(', ')}`,
);
}
}

Expand All @@ -154,7 +176,10 @@ export async function validatePath(requestedPath: string): Promise<string> {
// 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
Expand All @@ -163,18 +188,32 @@ export async function validatePath(requestedPath: string): Promise<string> {
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;
}
Expand Down