From 0169c47e503d6bd2a918e112485126c0b50e43db Mon Sep 17 00:00:00 2001 From: 0ywfe Date: Sat, 11 Jul 2026 16:59:46 +0100 Subject: [PATCH] fix(filesystem): write in place to preserve birthtime and file identity (#4512) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit writeFileContent and applyFileEdits replaced existing files via a temp-file + fs.rename pattern. While this provided crash-atomicity and symlink safety, every write created a new inode, destroying the file's creation timestamp (birthtime) and severing hard links. The server's own get_file_info reports birthtime as a first-class field, making this internally inconsistent. Replace the temp+rename pattern for existing files with an in-place write using O_RDWR | O_NOFOLLOW: 1. fs.open(filePath, O_RDWR | O_NOFOLLOW) — rejects symlinks with ELOOP, matching the security guarantee of the rename pattern. 2. fh.truncate(0) + fh.write(content) — writes through the existing file handle, preserving the inode, birthtime, and hard links. New files still use the wx flag (exclusive create), which is unchanged. Trade-off: in-place writes lose crash-atomicity (a crash mid-write can leave a partially written file). The rename pattern avoided this. If crash-atomicity is considered essential for a specific deployment, a follow-up could add an opt-in flag to select the rename strategy. Tests updated to mock the new fs.open + FileHandle path. Added a dedicated test for the EEXIST -> writeInPlace flow in writeFileContent. --- src/filesystem/__tests__/lib.test.ts | 155 +++++++++++++-------------- src/filesystem/lib.ts | 49 ++++----- 2 files changed, 94 insertions(+), 110 deletions(-) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index e0ae61224f..731e7061c6 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -301,13 +301,31 @@ describe('Lib Functions', () => { }); describe('writeFileContent', () => { - it('writes file content', async () => { + it('creates new file with wx flag', async () => { mockFs.writeFile.mockResolvedValueOnce(undefined); - + await writeFileContent('/test/file.txt', 'new content'); - + expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' }); }); + + it('writes in place when file exists (preserves inode / birthtime)', async () => { + const eexist = Object.assign(new Error('EEXIST'), { code: 'EEXIST' }); + mockFs.writeFile.mockRejectedValueOnce(eexist); + const mockFh = { + truncate: vi.fn().mockResolvedValue(undefined), + write: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; + mockFs.open.mockResolvedValueOnce(mockFh); + + await writeFileContent('/test/file.txt', 'updated'); + + expect(mockFs.open).toHaveBeenCalledWith('/test/file.txt', expect.any(Number)); + expect(mockFh.truncate).toHaveBeenCalledWith(0); + expect(mockFh.write).toHaveBeenCalledWith('updated', 0, 'utf-8'); + expect(mockFh.close).toHaveBeenCalled(); + }); }); }); @@ -413,31 +431,34 @@ describe('Lib Functions', () => { describe('File Editing Functions', () => { describe('applyFileEdits', () => { + let mockFileHandle: any; + beforeEach(() => { mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); - mockFs.writeFile.mockResolvedValue(undefined); + mockFileHandle = { + truncate: vi.fn().mockResolvedValue(undefined), + write: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; + mockFs.open.mockResolvedValue(mockFileHandle); }); it('applies simple text replacement', async () => { const edits = [ { oldText: 'line2', newText: 'modified line2' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + const result = await applyFileEdits('/test/file.txt', edits, false); - + expect(result).toContain('modified line2'); - // Should write to temporary file then rename - expect(mockFs.writeFile).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + expect(mockFs.open).toHaveBeenCalledWith('/test/file.txt', expect.any(Number)); + expect(mockFileHandle.truncate).toHaveBeenCalledWith(0); + expect(mockFileHandle.write).toHaveBeenCalledWith( 'line1\nmodified line2\nline3\n', + 0, 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), - '/test/file.txt' - ); + expect(mockFileHandle.close).toHaveBeenCalled(); }); it('treats dollar signs in replacement text literally', async () => { @@ -445,13 +466,11 @@ describe('Lib Functions', () => { { oldText: 'line2', newText: "price=$$; match=$&; before=$`; after=$'" } ]; - mockFs.rename.mockResolvedValueOnce(undefined); - await applyFileEdits('/test/file.txt', edits, false); - expect(mockFs.writeFile).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + expect(mockFileHandle.write).toHaveBeenCalledWith( "line1\nprice=$$; match=$&; before=$`; after=$'\nline3\n", + 0, 'utf-8' ); }); @@ -460,11 +479,11 @@ describe('Lib Functions', () => { const edits = [ { oldText: 'line2', newText: 'modified line2' } ]; - + const result = await applyFileEdits('/test/file.txt', edits, true); - + expect(result).toContain('modified line2'); - expect(mockFs.writeFile).not.toHaveBeenCalled(); + expect(mockFs.open).not.toHaveBeenCalled(); }); it('applies multiple edits sequentially', async () => { @@ -472,123 +491,93 @@ describe('Lib Functions', () => { { oldText: 'line1', newText: 'first line' }, { oldText: 'line3', newText: 'third line' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + await applyFileEdits('/test/file.txt', edits, false); - - expect(mockFs.writeFile).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + + expect(mockFileHandle.write).toHaveBeenCalledWith( 'first line\nline2\nthird line\n', + 0, 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), - '/test/file.txt' - ); }); it('handles whitespace-flexible matching', async () => { mockFs.readFile.mockResolvedValue(' line1\n line2\n line3\n'); - + const edits = [ { oldText: 'line2', newText: 'modified line2' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + await applyFileEdits('/test/file.txt', edits, false); - - expect(mockFs.writeFile).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + + expect(mockFileHandle.write).toHaveBeenCalledWith( ' line1\n modified line2\n line3\n', + 0, 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), - '/test/file.txt' - ); }); it('throws error for non-matching edits', async () => { const edits = [ { oldText: 'nonexistent line', newText: 'replacement' } ]; - + await expect(applyFileEdits('/test/file.txt', edits, false)) .rejects.toThrow('Could not find exact match for edit'); }); it('handles complex multi-line edits with indentation', async () => { mockFs.readFile.mockResolvedValue('function test() {\n console.log("hello");\n return true;\n}'); - + const edits = [ - { - oldText: ' console.log("hello");\n return true;', - newText: ' console.log("world");\n console.log("test");\n return false;' + { + oldText: ' console.log("hello");\n return true;', + newText: ' console.log("world");\n console.log("test");\n return false;' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + await applyFileEdits('/test/file.js', edits, false); - - expect(mockFs.writeFile).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/), + + expect(mockFileHandle.write).toHaveBeenCalledWith( 'function test() {\n console.log("world");\n console.log("test");\n return false;\n}', + 0, 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/), - '/test/file.js' - ); }); it('handles edits with different indentation patterns', async () => { mockFs.readFile.mockResolvedValue(' if (condition) {\n doSomething();\n }'); - + const edits = [ - { - oldText: 'doSomething();', - newText: 'doSomethingElse();\n doAnotherThing();' + { + oldText: 'doSomething();', + newText: 'doSomethingElse();\n doAnotherThing();' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + await applyFileEdits('/test/file.js', edits, false); - - expect(mockFs.writeFile).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/), + + expect(mockFileHandle.write).toHaveBeenCalledWith( ' if (condition) {\n doSomethingElse();\n doAnotherThing();\n }', + 0, 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/), - '/test/file.js' - ); }); it('handles CRLF line endings in file content', async () => { mockFs.readFile.mockResolvedValue('line1\r\nline2\r\nline3\r\n'); - + const edits = [ { oldText: 'line2', newText: 'modified line2' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + await applyFileEdits('/test/file.txt', edits, false); - - expect(mockFs.writeFile).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + + expect(mockFileHandle.write).toHaveBeenCalledWith( 'line1\nmodified line2\nline3\n', + 0, 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), - '/test/file.txt' - ); }); }); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index ce4af9f38a..a9fb823f16 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -1,7 +1,7 @@ import fs from "fs/promises"; +import { constants } from 'fs'; import path from "path"; import os from 'os'; -import { randomBytes } from 'crypto'; import { diffLines, createTwoFilesPatch } from 'diff'; import { minimatch } from 'minimatch'; import { normalizePath, expandHome } from './path-utils.js'; @@ -165,25 +165,29 @@ 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. - 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; - } + // Security: O_NOFOLLOW rejects symlinks with ELOOP, giving the same + // TOCTOU protection the old temp+rename pattern provided. Writing + // in place preserves the file's inode, birthtime, and hard links, + // which the rename strategy destroyed on every write (#4512). + await writeInPlace(filePath, content); } else { throw error; } } } +async function writeInPlace(filePath: string, content: string): Promise { + // O_NOFOLLOW rejects symlinks (ELOOP), matching the security guarantee of + // the old temp+rename pattern without replacing the inode. + const fh = await fs.open(filePath, constants.O_RDWR | constants.O_NOFOLLOW); + try { + await fh.truncate(0); + await fh.write(content, 0, 'utf-8'); + } finally { + await fh.close(); + } +} + // File Editing Functions interface FileEdit { @@ -263,19 +267,10 @@ 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. - 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; - } + // Write in place to preserve the file's inode, birthtime, and hard + // links (#4512). O_NOFOLLOW rejects symlinks with ELOOP, maintaining + // the same security guarantee the old temp+rename pattern provided. + await writeInPlace(filePath, modifiedContent); } return formattedDiff;