fix: harden atomic file writes (#1453)

Co-authored-by: Bohdan Triapitsyn <artmore@protonmail.com>

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 });

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();
   });
 });
 

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);
 }