diff --git a/src/fetch/uv.lock b/src/fetch/uv.lock index c2159b229f..0690b49f76 100644 --- a/src/fetch/uv.lock +++ b/src/fetch/uv.lock @@ -547,7 +547,7 @@ dev = [ [package.metadata] requires-dist = [ - { name = "httpx", specifier = "<0.28" }, + { name = "httpx", specifier = ">=0.27" }, { name = "markdownify", specifier = ">=0.13.1" }, { name = "mcp", specifier = ">=1.1.3" }, { name = "protego", specifier = ">=0.3.1" }, diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index f7e585af22..3b9cfcd203 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -308,6 +308,126 @@ describe('Lib Functions', () => { expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' }); }); + + it('uses direct overwrite before the symlink-unsafe fs.cp fallback', async () => { + const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; + eexistError.code = 'EEXIST'; + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + + mockFs.writeFile + .mockRejectedValueOnce(eexistError) // First write fails (file exists) + .mockResolvedValueOnce(undefined) // Temp file write succeeds + .mockResolvedValueOnce(undefined); // Direct overwrite succeeds + mockFs.rename.mockRejectedValueOnce(epermError); // Rename fails (locked) + mockFs.readFile.mockResolvedValueOnce(Buffer.from('new content')); + mockFs.unlink.mockResolvedValueOnce(undefined); // Temp cleanup succeeds + + await writeFileContent('/test/file.txt', 'new content'); + + expect(mockFs.rename).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt' + ); + expect(mockFs.readFile).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( + '/test/file.txt', + Buffer.from('new content') + ); + expect(mockFs.cp).not.toHaveBeenCalled(); + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + ); + }); + + it('falls back to fs.cp when direct overwrite fails', async () => { + const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; + eexistError.code = 'EEXIST'; + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + const ebusyWriteError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyWriteError.code = 'EBUSY'; + const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyError.code = 'EBUSY'; + + mockFs.writeFile + .mockRejectedValueOnce(eexistError) + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(ebusyWriteError); + mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.readFile.mockResolvedValueOnce(Buffer.from('new content')); + mockFs.cp.mockResolvedValueOnce(undefined); + mockFs.unlink.mockRejectedValueOnce(ebusyError); // Temp cleanup fails (e.g. antivirus) + + // Should NOT throw — the target file was written successfully + await expect(writeFileContent('/test/file.txt', 'new content')) + .resolves.toBeUndefined(); + + expect(mockFs.readFile).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( + '/test/file.txt', + Buffer.from('new content') + ); + expect(mockFs.cp).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt', + { force: true } + ); + }); + + it('cleans up temp file and re-throws when fs.cp fails after EPERM', async () => { + const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; + eexistError.code = 'EEXIST'; + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyError.code = 'EBUSY'; + const enospcError = new Error('ENOSPC') as NodeJS.ErrnoException; + enospcError.code = 'ENOSPC'; + + mockFs.writeFile + .mockRejectedValueOnce(eexistError) + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(ebusyError); + mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.readFile.mockResolvedValueOnce(Buffer.from('new content')); + mockFs.cp.mockRejectedValueOnce(enospcError); + mockFs.unlink.mockResolvedValue(undefined); + + await expect(writeFileContent('/test/file.txt', 'new content')) + .rejects.toThrow('ENOSPC'); + + expect(mockFs.writeFile).toHaveBeenLastCalledWith( + '/test/file.txt', + Buffer.from('new content') + ); + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + }); + + it('cleans up temp file and re-throws when temp write fails', async () => { + const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; + eexistError.code = 'EEXIST'; + const enospcError = new Error('ENOSPC') as NodeJS.ErrnoException; + enospcError.code = 'ENOSPC'; + + mockFs.writeFile + .mockRejectedValueOnce(eexistError) + .mockRejectedValueOnce(enospcError); + mockFs.unlink.mockResolvedValue(undefined); + + await expect(writeFileContent('/test/file.txt', 'new content')) + .rejects.toThrow('ENOSPC'); + + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + }); }); }); @@ -553,6 +673,98 @@ describe('Lib Functions', () => { ); }); + it('uses direct overwrite before the symlink-unsafe fs.cp fallback during file edit', async () => { + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + + mockFs.readFile + .mockResolvedValueOnce('line1\nline2\nline3\n') + .mockResolvedValueOnce(Buffer.from('line1\nmodified line2\nline3\n')); + mockFs.writeFile + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(undefined); + mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.unlink.mockResolvedValueOnce(undefined); + + const edits = [{ oldText: 'line2', newText: 'modified line2' }]; + const result = await applyFileEdits('/test/file.txt', edits, false); + + // Should try rename, then overwrite in place without invoking fs.cp. + expect(mockFs.rename).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt' + ); + expect(mockFs.readFile).toHaveBeenLastCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( + '/test/file.txt', + Buffer.from('line1\nmodified line2\nline3\n') + ); + expect(mockFs.cp).not.toHaveBeenCalled(); + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + ); + // Edit should still produce a valid diff + expect(result).toContain('modified line2'); + }); + + it('falls back to fs.cp when direct overwrite fails during file edit', async () => { + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + const ebusyWriteError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyWriteError.code = 'EBUSY'; + const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyError.code = 'EBUSY'; + + mockFs.readFile + .mockResolvedValueOnce('line1\nline2\nline3\n') + .mockResolvedValueOnce(Buffer.from('line1\nmodified line2\nline3\n')); + mockFs.writeFile + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(ebusyWriteError); + mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.cp.mockResolvedValueOnce(undefined); + mockFs.unlink.mockRejectedValueOnce(ebusyError); // Temp cleanup fails + + const edits = [{ oldText: 'line2', newText: 'modified line2' }]; + + // Should NOT throw — the target file was written successfully + const result = await applyFileEdits('/test/file.txt', edits, false); + + expect(result).toContain('modified line2'); + expect(mockFs.readFile).toHaveBeenLastCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( + '/test/file.txt', + Buffer.from('line1\nmodified line2\nline3\n') + ); + expect(mockFs.cp).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt', + { force: true } + ); + }); + + it('cleans up temp file and re-throws when temp write fails during file edit', async () => { + const enospcError = new Error('ENOSPC') as NodeJS.ErrnoException; + enospcError.code = 'ENOSPC'; + + mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); + mockFs.writeFile.mockRejectedValueOnce(enospcError); + mockFs.unlink.mockResolvedValue(undefined); + + const edits = [{ oldText: 'line2', newText: 'modified line2' }]; + + await expect(applyFileEdits('/test/file.txt', edits, false)) + .rejects.toThrow('ENOSPC'); + + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + }); + it('handles CRLF line endings in file content', async () => { mockFs.readFile.mockResolvedValue('line1\r\nline2\r\nline3\r\n'); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 17e4654cd5..97aadac734 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -140,6 +140,57 @@ export async function validatePath(requestedPath: string): Promise { } +/** + * Replace a target file with the contents of a temporary file. + * + * Uses `fs.rename` for an atomic swap when possible. On Windows, rename can + * fail with `EPERM` when the target is held open by another process (e.g. + * VS Code). In that case the function first overwrites the target in place, + * avoiding the destination unlink performed by `fs.cp({ force: true })`. + * If direct overwrite is also blocked, it falls back to `fs.cp`. + * + * **Limitations:** + * - Direct overwrite requires the locking process to share write access. + * - The `fs.cp` fallback requires delete sharing and is *not* atomic: there + * is a brief symlink race window between its internal `unlink(dest)` and + * `copyFile(src, dest)`. + * - Editors that grant neither write nor delete sharing still produce an + * error. + * + * @param tempPath Path to the temporary file that contains the new content. + * @param targetPath Path to the destination file to be replaced. + */ +async function cleanupTempFile(tempPath: string): Promise { + try { + await fs.unlink(tempPath); + } catch {} +} + +async function replaceFileFromTemp(tempPath: string, targetPath: string): Promise { + try { + await fs.rename(tempPath, targetPath); + } catch (renameError) { + if ((renameError as NodeJS.ErrnoException).code === 'EPERM') { + try { + const content = await fs.readFile(tempPath); + await fs.writeFile(targetPath, content); + } catch { + try { + await fs.cp(tempPath, targetPath, { force: true }); + } catch (copyError) { + await cleanupTempFile(tempPath); + throw copyError; + } + } + await cleanupTempFile(tempPath); + } else { + // For non-EPERM errors, clean up the temp file and re-throw + await cleanupTempFile(tempPath); + throw renameError; + } + } +} + // File Operations export async function getFileStats(filePath: string): Promise { const stats = await fs.stat(filePath); @@ -165,18 +216,15 @@ export async function writeFileContent(filePath: string, content: string): Promi await fs.writeFile(filePath, content, { encoding: "utf-8", flag: 'wx' }); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'EEXIST') { - // Security: Use atomic rename to prevent race conditions where symlinks - // could be created between validation and write. Rename operations - // replace the target file atomically and don't follow symlinks. + // Prefer atomic rename; replaceFileFromTemp documents the Windows + // locked-file fallbacks and their security tradeoffs. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, content, 'utf-8'); - await fs.rename(tempPath, filePath); - } catch (renameError) { - try { - await fs.unlink(tempPath); - } catch {} - throw renameError; + await replaceFileFromTemp(tempPath, filePath); + } catch (tempWriteError) { + await cleanupTempFile(tempPath); + throw tempWriteError; } } else { throw error; @@ -263,18 +311,15 @@ export async function applyFileEdits( const formattedDiff = `${'`'.repeat(numBackticks)}diff\n${diff}${'`'.repeat(numBackticks)}\n\n`; if (!dryRun) { - // Security: Use atomic rename to prevent race conditions where symlinks - // could be created between validation and write. Rename operations - // replace the target file atomically and don't follow symlinks. + // Prefer atomic rename; replaceFileFromTemp documents the Windows + // locked-file fallbacks and their security tradeoffs. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, modifiedContent, 'utf-8'); - await fs.rename(tempPath, filePath); - } catch (error) { - try { - await fs.unlink(tempPath); - } catch {} - throw error; + await replaceFileFromTemp(tempPath, filePath); + } catch (writeError) { + await cleanupTempFile(tempPath); + throw writeError; } }