From 31a71c7f8e5c8e13d2ceba7942b3b375290437bd Mon Sep 17 00:00:00 2001 From: David Ahmann Date: Mon, 16 Mar 2026 10:23:25 -0400 Subject: [PATCH] filesystem: classify root and path validation failures (#3606) --- src/filesystem/__tests__/lib.test.ts | 6 ++-- .../__tests__/startup-validation.test.ts | 4 ++- src/filesystem/index.ts | 6 ++-- src/filesystem/lib.ts | 35 +++++++++++++++---- 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index f7e585af22..79cf6df93c 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -169,7 +169,7 @@ 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'); + .rejects.toThrow('[path_outside_allowed_roots] Access denied - path outside allowed directories'); }); it('handles non-existent files by checking parent directory', async () => { @@ -200,9 +200,9 @@ describe('Lib Functions', () => { mockFs.realpath .mockRejectedValueOnce(enoentError1) .mockRejectedValueOnce(enoentError2); - + await expect(validatePath(newFilePath)) - .rejects.toThrow('Parent directory does not exist'); + .rejects.toThrow('[path_parent_missing] Parent directory does not exist'); }); it('resolves relative paths against allowed directories instead of process.cwd()', async () => { diff --git a/src/filesystem/__tests__/startup-validation.test.ts b/src/filesystem/__tests__/startup-validation.test.ts index 3be283df74..14eeb323d0 100644 --- a/src/filesystem/__tests__/startup-validation.test.ts +++ b/src/filesystem/__tests__/startup-validation.test.ts @@ -81,7 +81,9 @@ describe('Startup Directory Validation', () => { // Should exit with error expect(result.exitCode).toBe(1); - expect(result.stderr).toContain('Error: None of the specified directories are accessible'); + expect(result.stderr).toMatch( + /argv_no_accessible_directories|missing_roots|Error: None of the specified directories are accessible/ + ); }); it('should warn when path is not a directory', async () => { diff --git a/src/filesystem/index.ts b/src/filesystem/index.ts index 7b67e63e58..ba632ae91a 100644 --- a/src/filesystem/index.ts +++ b/src/filesystem/index.ts @@ -83,7 +83,7 @@ for (const dir of allowedDirectories) { // Exit only if ALL paths are inaccessible (and some were specified) if (accessibleDirectories.length === 0 && allowedDirectories.length > 0) { - console.error("Error: None of the specified directories are accessible"); + console.error("Error [missing_roots]: None of the specified directories are accessible"); process.exit(1); } @@ -745,8 +745,8 @@ server.server.oninitialized = async () => { } else { if (allowedDirectories.length > 0) { console.error("Client does not support MCP Roots, using allowed directories set from server args:", allowedDirectories); - }else{ - throw new Error(`Server cannot operate: No allowed directories available. Server was started without command-line directories and client either does not support MCP roots protocol or provided empty roots. Please either: 1) Start server with directory arguments, or 2) Use a client that supports MCP roots protocol and provides valid root directories.`); + } else { + throw new Error(`[missing_roots] Server cannot operate: No allowed directories available. Server was started without command-line directories and client either does not support MCP roots protocol or provided empty roots. Please either: 1) Start server with directory arguments, or 2) Use a client that supports MCP roots protocol and provides valid root directories.`); } } }; diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 17e4654cd5..2d46ecc219 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -10,6 +10,10 @@ import { isPathWithinAllowedDirectories } from './path-validation.js'; // Global allowed directories - set by the main module let allowedDirectories: string[] = []; +function formatFilesystemValidationError(code: string, detail: string): Error { + return new Error(`[${code}] ${detail}`); +} + // Function to set allowed directories from the main module export function setAllowedDirectories(directories: string[]): void { allowedDirectories = [...directories]; @@ -98,16 +102,24 @@ function resolveRelativePathAgainstAllowedDirectories(relativePath: string): str // Security & Validation Functions export async function validatePath(requestedPath: string): Promise { const expandedPath = expandHome(requestedPath); - const absolute = path.isAbsolute(expandedPath) - ? path.resolve(expandedPath) - : resolveRelativePathAgainstAllowedDirectories(expandedPath); + let absolute: string; + try { + absolute = path.isAbsolute(expandedPath) + ? path.resolve(expandedPath) + : resolveRelativePathAgainstAllowedDirectories(expandedPath); + } catch { + throw formatFilesystemValidationError('path_invalid', `Invalid path: ${requestedPath}`); + } const normalizedRequested = normalizePath(absolute); // 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 formatFilesystemValidationError( + 'path_outside_allowed_roots', + `Access denied - path outside allowed directories: ${absolute} not in ${allowedDirectories.join(', ')}` + ); } // Security: Handle symlinks by checking their real path to prevent symlink attacks @@ -116,7 +128,10 @@ 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 formatFilesystemValidationError( + 'path_outside_allowed_roots', + `Access denied - symlink target outside allowed directories: ${realPath} not in ${allowedDirectories.join(', ')}` + ); } return realPath; } catch (error) { @@ -128,13 +143,19 @@ export async function validatePath(requestedPath: string): Promise { const realParentPath = await fs.realpath(parentDir); const normalizedParent = normalizePath(realParentPath); if (!isPathWithinAllowedDirectories(normalizedParent, allowedDirectories)) { - throw new Error(`Access denied - parent directory outside allowed directories: ${realParentPath} not in ${allowedDirectories.join(', ')}`); + throw formatFilesystemValidationError( + 'path_outside_allowed_roots', + `Access denied - parent directory outside allowed directories: ${realParentPath} not in ${allowedDirectories.join(', ')}` + ); } return absolute; } catch { - throw new Error(`Parent directory does not exist: ${parentDir}`); + throw formatFilesystemValidationError('path_parent_missing', `Parent directory does not exist: ${parentDir}`); } } + if ((error as NodeJS.ErrnoException).code === 'EACCES' || (error as NodeJS.ErrnoException).code === 'EPERM') { + throw formatFilesystemValidationError('path_permission_denied', `Permission denied: ${absolute}`); + } throw error; } }