fix(studio-server): stop mislabeling two more read-route failure shapes (#4363)

* fix(studio-server): stop mislabeling two more read-route failure shapes

Follow-up to the stale-tree investigation this week — two more cases
where the read route answers with the wrong shape for what happened.

A path a listing (`walkDir`) can show but this route cannot serve, now
correctly reported:

- **Replaced by a directory.** `existsSync` passes, `readFileSync` throws
  `EISDIR`, and Hono answers plain-text "Internal Server Error" — not
  JSON, and not a reason. `statSync(...).isFile()` after the exists check
  turns this into `404 { why: "not_a_file" }`, same shape as any other
  missing-file 404 the client already handles.

- **A dangling symlink.** `isSafePath`'s `realpathSync` throws on a link
  whose target does not exist, and (by design — see its comment) that
  fails closed as unsafe, same as a real traversal attempt. Correct for
  containment, wrong for the response: a broken link scans as an attack
  when it is broken plumbing. Added `isDanglingSymlink`, checked only
  after `resolveWithinProject` has already refused the path, so nothing
  about what is permitted changes — only which `why` a refusal carries.
  A symlink resolving to something real outside the project is still
  `outside_project` 403, unchanged; pinned by test so this can't
  regress into loosening containment.

Tests: dir-in-place → `not_a_file`; dangling symlink → `dangling_symlink`;
symlink-to-outside-project still → `outside_project` 403 (the guard the
new branch must not weaken). 102/102 in the touched suite, 247 green
across studio-server routes overall. oxlint, oxfmt, tsc clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(studio-server): stop dangling_symlink from leaking outside-project existence

Review on #4363: `isDanglingSymlink` ran on the *lexical* path even when
`resolveWithinProject` had already failed — so a path reached via `../` that
happened to be a dangling symlink out there, or a leaf reached through an
escaping directory symlink, came back `404 dangling_symlink` instead of `403
outside_project`. Worse: an in-project symlink pointing outside the project
got `404` if the outside target was missing and `403` if it existed — a
small oracle for "does this path outside the sandbox exist."

`isDanglingSymlinkInProject` now requires containment on both ends: the
link's own directory must resolve inside the project (`isSafePath(dir,
dirname(lexicalPath))`), and its target, read via `readlinkSync` and
resolved against the link's directory, must also resolve inside the project.
Only then does "target exists nowhere" get the new label — anything that
touches outside the project keeps the plain `outside_project` 403, with no
distinction based on what's actually out there.

4 new regression tests: a dangling symlink reached by `..` (403, not 404),
a dangling leaf behind an escaping directory symlink (403, not 404), and a
same-status check across a missing vs. existing outside target (both
`outside_project`, not one of each). files.pathSafety.test.ts 25/25,
files.test.ts 80/80, oxlint/oxfmt/tsc clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(studio-server): close a TOCTOU CodeQL flagged on the not_a_file check

CodeQL (js/file-system-race, high severity) on #4363: the new `not_a_file`
check stat'd `res.absPath` by path, then a separate `readFileSync` reopened
the same path — a directory-for-file swap between those two calls could
land a stale answer. Opens the file once instead and checks/reads through
that single descriptor (`fstatSync(fd)` then `readFileSync(fd)`), so the
check and the read are provably on the same inode; a swap after open can't
affect either. `openSync` on a directory still succeeds (POSIX allows
O_RDONLY there), so the `not_a_file` 404 still fires the same way; opening
a genuinely missing path still throws and hits the existing "optional" vs.
plain 404 branch unchanged.

105/105 in files.pathSafety.test.ts + files.test.ts, oxlint/oxfmt/tsc clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

packages/studio-server/src/routes/files.pathSafety.test.ts

@@ -235,6 +235,98 @@ describe("resolveProjectPath why", () => {
     expect(response.status).toBe(403);
     expect(await response.json()).toMatchObject({ why: "outside_project" });
   });
+
+  // A listing (`walkDir`) can show a path that has since been replaced by a
+  // directory — a rename, or an agent overwriting a file with a folder of the
+  // same name. `existsSync` passes; `readFileSync` would throw `EISDIR`, which
+  // Hono answers as plain-text "Internal Server Error" — not JSON, and not a
+  // reason. This must read the same as any other missing-file 404.
+  it("reports a path replaced by a directory as 404, not a bare server error", async () => {
+    const { app, project } = fixture();
+    rmSync(join(project, "inside.txt"));
+    mkdirSync(join(project, "inside.txt"));
+
+    const response = await app.request(fileUrl("inside.txt"));
+
+    expect(response.status).toBe(404);
+    expect(await response.json()).toMatchObject({ why: "not_a_file" });
+  });
+
+  // The dangling case: the link exists, its target does not, anywhere. This is
+  // broken plumbing (a stale symlink), not an attack — the read route should
+  // say so rather than reuse the path-traversal label.
+  it("reports a dangling symlink as 404, not 403", async (context) => {
+    const { app, project } = fixture();
+    linkOrSkip(context, join(project, "nope-target.html"), join(project, "dangling.html"), "file");
+
+    const response = await app.request(fileUrl("dangling.html"));
+
+    expect(response.status).toBe(404);
+    expect(await response.json()).toMatchObject({ why: "dangling_symlink" });
+  });
+
+  // The containment guard must not weaken: a symlink that resolves to
+  // something real outside the project is still the traversal case, whether
+  // or not it happens to be broken in some OTHER way. Only a target that
+  // exists nowhere gets the new label.
+  it("still reports a symlink resolving outside the project as 403 outside_project", async (context) => {
+    const { app, project, outside } = fixture();
+    linkOrSkip(context, join(outside, "secret.txt"), join(project, "escape.html"), "file");
+
+    const response = await app.request(fileUrl("escape.html"));
+
+    expect(response.status).toBe(403);
+    expect(await response.json()).toMatchObject({ why: "outside_project" });
+  });
+
+  // The "dangling_symlink" label must never apply to a path whose *own*
+  // location is outside the project (reached via `..`) — only to a symlink
+  // that lives inside the project. Otherwise the response leaks, to anyone
+  // who can hit the route, whether an out-of-project path happens to be a
+  // dangling symlink, which is exactly the containment guard's job to hide.
+  it("reports a dangling symlink reached by traversal as 403, not 404", async (context) => {
+    const { app, project, outside } = fixture();
+    linkOrSkip(context, join(outside, "nope-target.html"), join(outside, "dangling.html"), "file");
+
+    const response = await app.request(fileUrl(relative(project, join(outside, "dangling.html"))));
+
+    expect(response.status).toBe(403);
+    expect(await response.json()).toMatchObject({ why: "outside_project" });
+  });
+
+  // Same leak, one level removed: the leaf name is lexically inside the
+  // project, but it's reached through a directory symlink that itself
+  // escapes the project. The dangling-ness of the leaf must not surface.
+  it("reports a dangling leaf behind an escaping directory symlink as 403, not 404", async (context) => {
+    const { app, project, outside } = fixture();
+    linkOrSkip(context, outside, join(project, "ext"), "dir");
+
+    const response = await app.request(fileUrl("ext/nope-target.html"));
+
+    expect(response.status).toBe(403);
+    expect(await response.json()).toMatchObject({ why: "outside_project" });
+  });
+
+  // A symlink that lives inside the project but points *outside* it must
+  // read identically (403, same why) whether or not the outside target
+  // exists — the existence of an outside file is exactly what containment
+  // must never reveal, and `dangling_symlink` is a 404 an attacker could
+  // otherwise use to probe it.
+  it("does not distinguish an existing from a missing target across the project boundary", async (context) => {
+    const { app, project, outside } = fixture();
+    writeFileSync(join(outside, "b-target.txt"), "outside b");
+    linkOrSkip(context, join(outside, "a-missing.txt"), join(project, "a.html"), "file");
+    linkOrSkip(context, join(outside, "b-target.txt"), join(project, "b.html"), "file");
+
+    const [toMissing, toExisting] = await Promise.all([
+      app.request(fileUrl("a.html")),
+      app.request(fileUrl("b.html")),
+    ]);
+
+    expect(toMissing.status).toBe(toExisting.status);
+    expect(await toMissing.json()).toMatchObject({ why: "outside_project" });
+    expect(await toExisting.json()).toMatchObject({ why: "outside_project" });
+  });
 });
 
 describe("upload collision races", () => {

packages/studio-server/src/routes/files.ts

@@ -7,8 +7,10 @@ import { bodyLimit } from "hono/body-limit";
 import {
   closeSync,
   existsSync,
+  lstatSync,
   openSync,
   readFileSync,
+  readlinkSync,
   writeFileSync,
   writeSync,
   mkdirSync,
@@ -126,6 +128,46 @@ interface ResolvedGsapFile {
   absPath: string;
 }
 
+/**
+ * True only for a symlink that itself lives inside the project, whose target
+ * (once resolved against the link's own directory) also names a location
+ * inside the project, and does not exist anywhere — not for one that exists
+ * (that stays a real containment failure) and not for a plain missing path
+ * (the ordinary case `resolveWithinProject` already covers).
+ *
+ * Both containment checks matter, not just the second: a request can name a
+ * path lexically *outside* the project (reached via `..`) that happens to be
+ * a dangling symlink out there, or a real in-project symlink that points
+ * *outside* the project at a target that may or may not exist. Labeling
+ * either of those "not found" would leak, to anyone who can hit the route,
+ * whether an out-of-project path exists — the containment check exists
+ * precisely so that answer never depends on what's outside the project.
+ * `isSafePath` fails closed on a dangling in-project symlink by design (a
+ * write through it could later resolve outside the project once something
+ * creates the target) — this does not loosen that; it only tells the caller
+ * *why* the containment check refused, so the response can say "not found"
+ * instead of a path-traversal-shaped "forbidden" for a case that scans as
+ * broken plumbing, not an attack.
+ */
+function isDanglingSymlinkInProject(projectDir: string, lexicalPath: string): boolean {
+  if (!isSafePath(projectDir, dirname(lexicalPath))) return false;
+  let stats;
+  try {
+    stats = lstatSync(lexicalPath);
+  } catch {
+    return false;
+  }
+  if (!stats.isSymbolicLink()) return false;
+  const target = resolve(dirname(lexicalPath), readlinkSync(lexicalPath));
+  if (!isSafePath(projectDir, target)) return false;
+  try {
+    statSync(lexicalPath);
+    return false;
+  } catch {
+    return true;
+  }
+}
+
 /** Resolve project + safe absolute path for any project-scoped route. */
 async function resolveProjectPath(
   c: RouteContext,
@@ -160,6 +202,9 @@ async function resolveProjectPath(
 
   const absPath = resolveWithinProject(project.dir, filePath);
   if (!absPath) {
+    if (isDanglingSymlinkInProject(project.dir, resolve(project.dir, filePath))) {
+      return { error: c.json({ error: "not found", why: "dangling_symlink" }, 404) } as const;
+    }
     return { error: c.json({ error: "forbidden", why: "outside_project" }, 403) } as const;
   }
 
@@ -2276,7 +2321,13 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
     const res = await resolveProjectFile(c, adapter);
     if ("error" in res) return res.error;
 
-    if (!existsSync(res.absPath)) {
+    // Opened once and checked/read through the same descriptor, not the path,
+    // so a directory-for-file swap (or anything else) between the check below
+    // and the read can't land a stale answer — both act on the identical inode.
+    let fd: number;
+    try {
+      fd = openSync(res.absPath, "r");
+    } catch {
       if (c.req.query("optional") === "1") {
         // `missing: true` separates the absent-file shim from a genuinely
         // 0-byte file — both answer `content: ""`, and the caller could not
@@ -2289,20 +2340,34 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
       }
       return c.json({ error: "not found" }, 404);
     }
+    try {
+      // A listing built from `walkDir` can show a path that has since been
+      // replaced by a directory (a rename, or an agent overwriting a file
+      // with a folder of the same name) — opening it succeeds (POSIX allows
+      // O_RDONLY on a directory), and reading it would throw `EISDIR`, which
+      // Hono answers as a plain-text 500. The caller already handles a 404
+      // with `why`; this reports the same shape instead of an opaque server
+      // error for something that is not one.
+      if (!fstatSync(fd).isFile()) {
+        return c.json({ error: "not found", why: "not_a_file" }, 404);
+      }
 
-    const content = readFileSync(res.absPath);
-    const version = fileContentVersion(content);
-    c.header("ETag", version);
-    // `missing: false` on the read path too, so its PRESENCE is what tells a
-    // caller this server distinguishes the two empty answers at all. Without
-    // it here, a real 0-byte file from a new server looks exactly like either
-    // case from an old one, and the split above buys nothing.
-    return c.json({
-      filename: res.filePath,
-      content: content.toString("utf-8"),
-      version,
-      missing: false,
-    });
+      const content = readFileSync(fd);
+      const version = fileContentVersion(content);
+      c.header("ETag", version);
+      // `missing: false` on the read path too, so its PRESENCE is what tells a
+      // caller this server distinguishes the two empty answers at all. Without
+      // it here, a real 0-byte file from a new server looks exactly like either
+      // case from an old one, and the split above buys nothing.
+      return c.json({
+        filename: res.filePath,
+        content: content.toString("utf-8"),
+        version,
+        missing: false,
+      });
+    } finally {
+      closeSync(fd);
+    }
   });
 
   // ── Write (overwrite) ──