From 726875be95e0ad2d3c4960bb9bde3511177a14b8 Mon Sep 17 00:00:00 2001 From: Abhinav Prakash Date: Tue, 18 Aug 2026 01:08:19 +0530 Subject: [PATCH] fix(filesystem): support recursive create_directory and prevent move_file overwrites --- src/filesystem/__tests__/lib.test.ts | 47 +++++++++++--- .../__tests__/structured-content.test.ts | 61 +++++++++++++++++++ src/filesystem/index.ts | 12 ++++ src/filesystem/lib.ts | 33 +++++++--- 4 files changed, 135 insertions(+), 18 deletions(-) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index e0ae61224f..cfaab9cc30 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -188,18 +188,49 @@ describe('Lib Functions', () => { expect(result).toBe(path.resolve(newFilePath)); }); - it('rejects when parent directory does not exist', async () => { + it('handles non-existent paths with multiple non-existent parent directories by walking up to existing ancestor', async () => { + const nestedPath = process.platform === 'win32' ? 'C:\\Users\\test\\a\\b\\c' : '/home/user/a/b/c'; + const ancestorPath = process.platform === 'win32' ? 'C:\\Users\\test' : '/home/user'; + + const enoentError = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError.code = 'ENOENT'; + + // Mock ENOENT for the full path and intermediate directories, then resolve the existing ancestor + mockFs.realpath + .mockRejectedValueOnce(enoentError) // C:\Users\test\a\b\c + .mockRejectedValueOnce(enoentError) // C:\Users\test\a\b + .mockRejectedValueOnce(enoentError) // C:\Users\test\a + .mockResolvedValueOnce(ancestorPath); // C:\Users\test + + const result = await validatePath(nestedPath); + expect(result).toBe(path.resolve(nestedPath)); + }); + + it('rejects when ancestor directory resolves outside allowed directories', async () => { + const nestedPath = process.platform === 'win32' ? 'C:\\Users\\test\\symlink_out\\a\\b' : '/home/user/symlink_out/a/b'; + const outsidePath = process.platform === 'win32' ? 'C:\\Windows\\System32' : '/etc'; + + const enoentError = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError.code = 'ENOENT'; + + mockFs.realpath + .mockRejectedValueOnce(enoentError) // symlink_out/a/b + .mockRejectedValueOnce(enoentError) // symlink_out/a + .mockResolvedValueOnce(outsidePath); // symlink_out resolves outside + + await expect(validatePath(nestedPath)) + .rejects.toThrow('Access denied - parent directory outside allowed directories'); + }); + + it('rejects when no ancestor directory exists', async () => { const newFilePath = process.platform === 'win32' ? 'C:\\Users\\test\\nonexistent\\newfile.txt' : '/home/user/nonexistent/newfile.txt'; // Create errors with the ENOENT code - const enoentError1 = new Error('ENOENT') as NodeJS.ErrnoException; - enoentError1.code = 'ENOENT'; - const enoentError2 = new Error('ENOENT') as NodeJS.ErrnoException; - enoentError2.code = 'ENOENT'; + const enoentError = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError.code = 'ENOENT'; - mockFs.realpath - .mockRejectedValueOnce(enoentError1) - .mockRejectedValueOnce(enoentError2); + // All ancestors fail to resolve + mockFs.realpath.mockRejectedValue(enoentError); await expect(validatePath(newFilePath)) .rejects.toThrow('Parent directory does not exist'); diff --git a/src/filesystem/__tests__/structured-content.test.ts b/src/filesystem/__tests__/structured-content.test.ts index 4605b72a8f..d75dca2d23 100644 --- a/src/filesystem/__tests__/structured-content.test.ts +++ b/src/filesystem/__tests__/structured-content.test.ts @@ -96,6 +96,38 @@ describe('structuredContent schema compliance', () => { }); }); + describe('create_directory (issue #4629)', () => { + it('should create multiple nested directories in one operation', async () => { + const nestedDir = path.join(testDir, 'nested', 'level1', 'level2', 'level3'); + + const result = await client.callTool({ + name: 'create_directory', + arguments: { path: nestedDir } + }); + + expect(result.isError).toBeFalsy(); + const stats = await fs.stat(nestedDir); + expect(stats.isDirectory()).toBe(true); + + const structuredContent = result.structuredContent as { content: unknown }; + expect(typeof structuredContent.content).toBe('string'); + expect(structuredContent.content).toContain('Successfully created directory'); + }); + + it('should succeed silently if directory already exists', async () => { + const existingDir = path.join(testDir, 'subdir'); + + const result = await client.callTool({ + name: 'create_directory', + arguments: { path: existingDir } + }); + + expect(result.isError).toBeFalsy(); + const stats = await fs.stat(existingDir); + expect(stats.isDirectory()).toBe(true); + }); + }); + describe('move_file', () => { it('should return structuredContent.content as a string, not an array', async () => { const sourcePath = path.join(testDir, 'test.txt'); @@ -122,6 +154,35 @@ describe('structuredContent schema compliance', () => { // The content should contain success message expect(structuredContent.content).toContain('Successfully moved'); }); + + it('should fail when destination already exists and not overwrite content (issue #4628)', async () => { + const sourcePath = path.join(testDir, 'source.txt'); + const destPath = path.join(testDir, 'dest.txt'); + + await fs.writeFile(sourcePath, 'source content'); + await fs.writeFile(destPath, 'original destination content'); + + const result = await client.callTool({ + name: 'move_file', + arguments: { + source: sourcePath, + destination: destPath + } + }); + + // Operation must fail + expect(result.isError).toBe(true); + const textContent = (result.content as Array<{ type: string; text?: string }>)[0]?.text; + expect(textContent).toContain(`Destination already exists: ${destPath}`); + + // Destination content must NOT have been overwritten + const destContent = await fs.readFile(destPath, 'utf-8'); + expect(destContent).toBe('original destination content'); + + // Source file should still exist + const sourceContent = await fs.readFile(sourcePath, 'utf-8'); + expect(sourceContent).toBe('source content'); + }); }); describe('list_directory (control - already working)', () => { diff --git a/src/filesystem/index.ts b/src/filesystem/index.ts index 234605bb13..2f202c40f0 100644 --- a/src/filesystem/index.ts +++ b/src/filesystem/index.ts @@ -631,6 +631,18 @@ server.registerTool( async (args: z.infer) => { const validSourcePath = await validatePath(args.source); const validDestPath = await validatePath(args.destination); + let destExists = false; + try { + await fs.lstat(validDestPath); + destExists = true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error; + } + } + if (destExists) { + throw new Error(`Destination already exists: ${args.destination}`); + } await fs.rename(validSourcePath, validDestPath); const text = `Successfully moved ${args.source} to ${args.destination}`; const contentBlock = { type: "text" as const, text }; diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index ce4af9f38a..bddce70f07 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -120,19 +120,32 @@ export async function validatePath(requestedPath: string): Promise { } return realPath; } catch (error) { - // Security: For new files that don't exist yet, verify parent directory - // This ensures we can't create files in unauthorized locations + // Security: For new files/directories that don't exist yet, verify ancestor directory + // This ensures we can't create files in unauthorized locations while allowing nested creations if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - const parentDir = path.dirname(absolute); - try { - 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(', ')}`); + let currentDir = path.dirname(absolute); + while (true) { + let realAncestorPath: string; + try { + realAncestorPath = await fs.realpath(currentDir); + } catch (ancestorError) { + if ((ancestorError as NodeJS.ErrnoException).code === 'ENOENT') { + const parent = path.dirname(currentDir); + if (parent === currentDir) { + // Reached root without finding an existing directory + throw new Error(`Parent directory does not exist: ${path.dirname(absolute)}`); + } + currentDir = parent; + continue; + } + throw ancestorError; + } + + const normalizedAncestor = normalizePath(realAncestorPath); + if (!isPathWithinAllowedDirectories(normalizedAncestor, allowedDirectories)) { + throw new Error(`Access denied - parent directory outside allowed directories: ${realAncestorPath} not in ${allowedDirectories.join(', ')}`); } return absolute; - } catch { - throw new Error(`Parent directory does not exist: ${parentDir}`); } } throw error;