From 51a8d26be20b82b3b5dc78f74bdda111b67939c2 Mon Sep 17 00:00:00 2001
From: Garry Tan <garrytan@gmail.com>
Date: Mon, 25 May 2026 14:32:06 -0700
Subject: [PATCH] fix(design): reload guard rejects directory paths
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

design/src/serve.ts:200-212 used to accept a path that resolved to the
allowedDir itself (the OR branch `|| resolvedReload === allowedDir`),
which then crashed readFileSync with EISDIR. Now:

  1. startsWith(allowedDir + path.sep) must pass — rejects the dir itself
     and anything outside (403).
  2. statSync(resolvedReload).isFile() must pass — rejects subdirectories
     inside allowedDir with a clear "Path must be a file" 400.

The test stub in serve.test.ts mirrors prod; both updated, plus two new
test cases for the previously-broken paths. Codex caught this in the
plan-review pass; it's a latent bug in shipping code, not a regression
from the daemon work.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 design/src/serve.ts       | 20 +++++++++++++-------
 design/test/serve.test.ts | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/design/src/serve.ts b/design/src/serve.ts
index 6ee1abf17..7fc3ed559 100644
--- a/design/src/serve.ts
+++ b/design/src/serve.ts
@@ -197,19 +197,25 @@ export async function serve(options: ServeOptions): Promise<void> {
       );
     }
 
-    // Security: resolve symlinks and validate the reload path is within the
-    // allowed directory (anchored to the initial HTML file's parent).
-    // Prevents path traversal via /api/reload reading arbitrary files.
+    // Security: resolve symlinks and validate the reload path is a FILE
+    // inside the allowed directory (anchored to the initial HTML file's
+    // parent). Prevents path traversal via /api/reload reading arbitrary
+    // files. A path resolving to the allowedDir itself (a directory) used
+    // to pass the guard and then crash readFileSync with EISDIR — reject
+    // it explicitly with a clear 400 instead.
     const resolvedReload = fs.realpathSync(path.resolve(newHtmlPath));
-    if (
-      !resolvedReload.startsWith(allowedDir + path.sep) &&
-      resolvedReload !== allowedDir
-    ) {
+    if (!resolvedReload.startsWith(allowedDir + path.sep)) {
       return Response.json(
         { error: `Path must be within: ${allowedDir}` },
         { status: 403 },
       );
     }
+    if (!fs.statSync(resolvedReload).isFile()) {
+      return Response.json(
+        { error: `Path must be a file, not a directory: ${newHtmlPath}` },
+        { status: 400 },
+      );
+    }
 
     // Swap the HTML content
     htmlContent = fs.readFileSync(resolvedReload, "utf-8");
diff --git a/design/test/serve.test.ts b/design/test/serve.test.ts
index d3a30bcdc..602c31431 100644
--- a/design/test/serve.test.ts
+++ b/design/test/serve.test.ts
@@ -311,9 +311,12 @@ describe('Serve /api/reload — path traversal protection', () => {
             }
             // Production path validation — same as design/src/serve.ts
             const resolvedReload = fs.realpathSync(path.resolve(body.html));
-            if (!resolvedReload.startsWith(allowedDir + path.sep) && resolvedReload !== allowedDir) {
+            if (!resolvedReload.startsWith(allowedDir + path.sep)) {
               return Response.json({ error: `Path must be within: ${allowedDir}` }, { status: 403 });
             }
+            if (!fs.statSync(resolvedReload).isFile()) {
+              return Response.json({ error: `Path must be a file, not a directory: ${body.html}` }, { status: 400 });
+            }
             htmlContent = fs.readFileSync(resolvedReload, 'utf-8');
             return Response.json({ reloaded: true });
           })();
@@ -372,6 +375,39 @@ describe('Serve /api/reload — path traversal protection', () => {
     const page = await fetch(baseUrl);
     expect(await page.text()).toContain('Safe reload');
   });
+
+  // Regression for the directory-instead-of-file guard (Codex finding).
+  // Before: resolvedReload === allowedDir passed the guard and then
+  // readFileSync threw EISDIR with no helpful message.
+  test('blocks reload when path resolves to the allowed directory itself', async () => {
+    const res = await fetch(`${baseUrl}/api/reload`, {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify({ html: tmpDir }),
+    });
+    // tmpDir does not satisfy startsWith(allowedDir + sep), so the within-dir
+    // check rejects with 403 — but importantly, no EISDIR crash.
+    expect(res.status).toBe(403);
+  });
+
+  test('blocks reload when path is a subdirectory (not a file)', async () => {
+    const subdir = path.join(tmpDir, 'subdir-not-a-file');
+    fs.mkdirSync(subdir, { recursive: true });
+    try {
+      const res = await fetch(`${baseUrl}/api/reload`, {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify({ html: subdir }),
+      });
+      // Inside allowedDir but a directory — must fail before readFileSync,
+      // with a clear "must be a file" error instead of EISDIR.
+      expect(res.status).toBe(400);
+      const data = await res.json();
+      expect(data.error).toContain('must be a file');
+    } finally {
+      try { fs.rmSync(subdir, { recursive: true, force: true }); } catch {}
+    }
+  });
 });
 
 // ─── Full lifecycle: regeneration round-trip ──────────────────────