From e6338e5c7199c7641c37f3b17ff856048c03627b Mon Sep 17 00:00:00 2001 From: nerdosaurus <58043629+nerdosaurus@users.noreply.github.com> Date: Mon, 8 Jun 2026 13:06:20 -0400 Subject: [PATCH] fix: harden atomic file writes (#1453) Co-authored-by: Bohdan Triapitsyn --- packages/web/server/lib/fs/routes.js | 26 ++++-- packages/web/server/lib/fs/routes.test.js | 105 +++++++++++++++++++++- scripts/dev-web-hmr.mjs | 7 -- 3 files changed, 123 insertions(+), 15 deletions(-) diff --git a/packages/web/server/lib/fs/routes.js b/packages/web/server/lib/fs/routes.js index 62c82e49..e83db46f 100644 --- a/packages/web/server/lib/fs/routes.js +++ b/packages/web/server/lib/fs/routes.js @@ -679,6 +679,9 @@ export const registerFsRoutes = (app, dependencies) => { content = await fsPromises.readFile(canonicalPath, 'utf8'); if (content.length > 0) break; } + if (content.length === 0) { + console.warn(`Read retry exhausted for ${canonicalPath}: stat reported ${stats.size} bytes but content is empty`); + } } return res.type('text/plain').send(content); } catch (error) { @@ -790,23 +793,32 @@ export const registerFsRoutes = (app, dependencies) => { return res.status(400).json({ error: resolved.error }); } - const existing = await fsPromises.readFile(resolved.resolved, 'utf8').catch(() => null); + const writePath = await fsPromises.realpath(resolved.resolved).catch((error) => { + if (error && typeof error === 'object' && error.code === 'ENOENT') { + return resolved.resolved; + } + throw error; + }); + const canonicalBase = await fsPromises.realpath(resolved.base).catch(() => path.resolve(resolved.base)); + if (!isPathWithinRoot(writePath, canonicalBase, path, os)) { + return res.status(403).json({ error: 'Access denied' }); + } + + const existing = await fsPromises.readFile(writePath, 'utf8').catch(() => null); if (existing === content) { return res.json({ success: true, path: resolved.resolved }); } - await fsPromises.mkdir(path.dirname(resolved.resolved), { recursive: true }); + await fsPromises.mkdir(path.dirname(writePath), { recursive: true }); // Atomic write: write to temp then rename to avoid concurrent readers // seeing an empty file during the O_TRUNC window of direct writeFile. - const tmp = `${resolved.resolved}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; - let tmpExists = false; + const tmp = `${writePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; try { await fsPromises.writeFile(tmp, content, 'utf8'); - tmpExists = true; - await fsPromises.rename(tmp, resolved.resolved); + await fsPromises.rename(tmp, writePath); } catch (error) { - if (tmpExists) await fsPromises.unlink(tmp).catch(() => {}); + await fsPromises.unlink(tmp).catch(() => {}); throw error; } return res.json({ success: true, path: resolved.resolved }); diff --git a/packages/web/server/lib/fs/routes.test.js b/packages/web/server/lib/fs/routes.test.js index ac3c1eb8..a7550574 100644 --- a/packages/web/server/lib/fs/routes.test.js +++ b/packages/web/server/lib/fs/routes.test.js @@ -33,6 +33,13 @@ const createMockResponse = () => { body = payload; return this; }, + type() { + return this; + }, + send(payload) { + body = payload; + return this; + }, get statusCode() { return statusCode; }, @@ -125,6 +132,26 @@ const registerWrite = (fsPromises) => { return getRoute('POST', '/api/fs/write'); }; +const registerRead = (fsPromises) => { + const { app, getRoute } = createRouteRegistry(); + registerFsRoutes(app, { + os: { homedir: () => '/home/user' }, + path: path.posix, + fsPromises: { + realpath: async (targetPath) => targetPath, + ...fsPromises, + }, + spawn: vi.fn(), + crypto: { randomUUID: () => 'job-0' }, + normalizeDirectoryPath: (p) => p, + resolveProjectDirectory: async () => ({ directory: '/repo' }), + buildAugmentedPath: () => '/usr/bin', + resolveGitBinaryForSpawn: () => 'git', + openchamberUserConfigRoot: '/home/user/.config', + }); + return getRoute('GET', '/api/fs/read'); +}; + const callExec = async (handler, body) => { const res = createMockResponse(); await handler({ body }, res); @@ -137,6 +164,12 @@ const callWrite = async (handler, body) => { return res; }; +const callRead = async (handler, query) => { + const res = createMockResponse(); + await handler({ query }, res); + return res; +}; + describe('fs write', () => { it('does not rewrite a file when content is unchanged', async () => { const fsPromises = { @@ -157,6 +190,8 @@ describe('fs write', () => { readFile: vi.fn(async () => 'old'), mkdir: vi.fn(async () => undefined), writeFile: vi.fn(async () => undefined), + rename: vi.fn(async () => undefined), + unlink: vi.fn(async () => undefined), }; const handler = registerWrite(fsPromises); @@ -164,7 +199,75 @@ describe('fs write', () => { expect(res.body).toEqual({ success: true, path: '/repo/file.txt' }); expect(fsPromises.mkdir).toHaveBeenCalledWith('/repo', { recursive: true }); - expect(fsPromises.writeFile).toHaveBeenCalledWith('/repo/file.txt', 'new', 'utf8'); + const tmp = fsPromises.writeFile.mock.calls[0][0]; + expect(tmp).toMatch(/^\/repo\/file\.txt\.tmp-/); + expect(fsPromises.writeFile).toHaveBeenCalledWith(tmp, 'new', 'utf8'); + expect(fsPromises.rename).toHaveBeenCalledWith(tmp, '/repo/file.txt'); + expect(fsPromises.unlink).not.toHaveBeenCalled(); + }); + + it('writes through existing symlinks without replacing the link', async () => { + const fsPromises = { + realpath: vi.fn(async (targetPath) => { + if (targetPath === '/repo/link.txt') return '/repo/target.txt'; + return targetPath; + }), + readFile: vi.fn(async () => 'old'), + mkdir: vi.fn(async () => undefined), + writeFile: vi.fn(async () => undefined), + rename: vi.fn(async () => undefined), + unlink: vi.fn(async () => undefined), + }; + const handler = registerWrite(fsPromises); + + const res = await callWrite(handler, { path: '/repo/link.txt', content: 'new' }); + + expect(res.body).toEqual({ success: true, path: '/repo/link.txt' }); + expect(fsPromises.readFile).toHaveBeenCalledWith('/repo/target.txt', 'utf8'); + const tmp = fsPromises.writeFile.mock.calls[0][0]; + expect(tmp).toMatch(/^\/repo\/target\.txt\.tmp-/); + expect(fsPromises.rename).toHaveBeenCalledWith(tmp, '/repo/target.txt'); + expect(fsPromises.rename).not.toHaveBeenCalledWith(expect.any(String), '/repo/link.txt'); + }); + + it('rejects existing symlinks that resolve outside the workspace', async () => { + const fsPromises = { + realpath: vi.fn(async (targetPath) => { + if (targetPath === '/repo/link.txt') return '/outside/target.txt'; + return targetPath; + }), + readFile: vi.fn(async () => 'old'), + mkdir: vi.fn(async () => undefined), + writeFile: vi.fn(async () => undefined), + rename: vi.fn(async () => undefined), + unlink: vi.fn(async () => undefined), + }; + const handler = registerWrite(fsPromises); + + const res = await callWrite(handler, { path: '/repo/link.txt', content: 'new' }); + + expect(res.statusCode).toBe(403); + expect(res.body).toEqual({ error: 'Access denied' }); + expect(fsPromises.writeFile).not.toHaveBeenCalled(); + expect(fsPromises.rename).not.toHaveBeenCalled(); + }); +}); + +describe('fs read', () => { + it('logs when empty-read retries are exhausted after non-empty stat', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const fsPromises = { + stat: vi.fn(async () => ({ isFile: () => true, size: 3 })), + readFile: vi.fn(async () => ''), + }; + const handler = registerRead(fsPromises); + + const res = await callRead(handler, { path: '/repo/file.txt' }); + + expect(res.body).toBe(''); + expect(fsPromises.readFile).toHaveBeenCalledTimes(4); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Read retry exhausted for /repo/file.txt')); + warn.mockRestore(); }); }); diff --git a/scripts/dev-web-hmr.mjs b/scripts/dev-web-hmr.mjs index fc7e69e5..47cb8855 100644 --- a/scripts/dev-web-hmr.mjs +++ b/scripts/dev-web-hmr.mjs @@ -146,13 +146,6 @@ async function shutdown(exitCode = 0) { if (shuttingDown) return; shuttingDown = true; await Promise.all([stopChildTree(api), stopChildTree(vite)]); - // Clean up orphaned OpenCode processes that weren't killed by - // the Express server's shutdown (e.g. when nodemon is killed first). - try { - spawnSync('pkill', ['-f', 'opencode serve'], { stdio: 'ignore' }); - } catch { - // best-effort - } process.exit(exitCode); }