Skip to content

fix(filesystem): write in place to preserve birthtime and file identity (#4512) #4516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
0ywfe wants to merge 1 commit into
base: main
Choose a base branch
from
+94 −110
Open

fix(filesystem): write in place to preserve birthtime and file identity (#4512) #4516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 72 additions & 83 deletions src/filesystem/__tests__/lib.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});

});
Expand Down Expand Up @@ -413,45 +431,46 @@ 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 () => {
const edits = [
{ 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'
);
});
Expand All @@ -460,135 +479,105 @@ 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 () => {
const edits = [
{ 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'
);
});
});

Expand Down
49 changes: 22 additions & 27 deletions src/filesystem/lib.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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<void> {
// 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 {
Expand Down Expand Up @@ -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;
Expand Down
Loading