diff --git a/src/filesystem/README.md b/src/filesystem/README.md index e9ddc2b1e2..ef0c2a01d9 100644 --- a/src/filesystem/README.md +++ b/src/filesystem/README.md @@ -241,7 +241,7 @@ Note: all directories must be mounted to `/projects` by default. "command": "npx", "args": [ "-y", - "@modelcontextprotocol/server-filesystem", + "@modelcontextprotocol/server-filesystem@0.6.3", "/Users/username/Desktop", "/path/to/other/allowed/dir" ] @@ -300,7 +300,7 @@ Note: all directories must be mounted to `/projects` by default. "command": "npx", "args": [ "-y", - "@modelcontextprotocol/server-filesystem", + "@modelcontextprotocol/server-filesystem@0.6.3", "${workspaceFolder}" ] } diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index f7e585af22..38ae5476d4 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -205,6 +205,66 @@ describe('Lib Functions', () => { .rejects.toThrow('Parent directory does not exist'); }); + it('does not leak filesystem paths in access denied errors', async () => { + const testPath = process.platform === 'win32' ? 'C:\\Windows\\System32\\file.txt' : '/etc/passwd'; + try { + await validatePath(testPath); + // Should not reach here + expect(true).toBe(false); + } catch (error: any) { + // Error message should NOT contain the requested path or allowed directories + expect(error.message).not.toContain(testPath); + for (const dir of (process.platform === 'win32' ? ['C:\\Users\\test', 'C:\\temp'] : ['/home/user', '/tmp'])) { + expect(error.message).not.toContain(dir); + } + // Should still convey the denial reason + expect(error.message).toBe('Access denied - path outside allowed directories'); + } + }); + + it('does not leak filesystem paths in parent directory errors', async () => { + const newFilePath = process.platform === 'win32' ? 'C:\\Users\\test\\nonexistent\\newfile.txt' : '/home/user/nonexistent/newfile.txt'; + + const enoentError1 = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError1.code = 'ENOENT'; + const enoentError2 = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError2.code = 'ENOENT'; + + mockFs.realpath + .mockRejectedValueOnce(enoentError1) + .mockRejectedValueOnce(enoentError2); + + try { + await validatePath(newFilePath); + expect(true).toBe(false); + } catch (error: any) { + // Error message should NOT contain the parent directory path + const parentDir = process.platform === 'win32' ? 'C:\\Users\\test\\nonexistent' : '/home/user/nonexistent'; + expect(error.message).not.toContain(parentDir); + expect(error.message).toBe('Parent directory does not exist'); + } + }); + + it('does not leak filesystem paths in symlink denied errors', async () => { + const testPath = process.platform === 'win32' ? 'C:\\Users\\test\\file.txt' : '/home/user/file.txt'; + const outsidePath = process.platform === 'win32' ? 'C:\\Windows\\System32\\secret' : '/etc/shadow'; + + // Simulate symlink resolving to outside allowed directories + mockFs.realpath.mockResolvedValueOnce(outsidePath); + + try { + await validatePath(testPath); + expect(true).toBe(false); + } catch (error: any) { + // Error message should NOT contain the real symlink target path + expect(error.message).not.toContain(outsidePath); + for (const dir of (process.platform === 'win32' ? ['C:\\Users\\test', 'C:\\temp'] : ['/home/user', '/tmp'])) { + expect(error.message).not.toContain(dir); + } + expect(error.message).toBe('Access denied - symlink target outside allowed directories'); + } + }); + it('resolves relative paths against allowed directories instead of process.cwd()', async () => { const relativePath = 'test-file.txt'; const originalCwd = process.cwd; diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 17e4654cd5..672f80340d 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -107,7 +107,7 @@ export async function validatePath(requestedPath: 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 Error(`Access denied - path outside allowed directories`); } // Security: Handle symlinks by checking their real path to prevent symlink attacks @@ -116,7 +116,7 @@ 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 Error(`Access denied - symlink target outside allowed directories`); } return realPath; } catch (error) { @@ -128,11 +128,11 @@ 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 new Error(`Access denied - parent directory outside allowed directories`); } return absolute; } catch { - throw new Error(`Parent directory does not exist: ${parentDir}`); + throw new Error(`Parent directory does not exist`); } } throw error;