From add6783775d567562180bdf5bb0cf77396b8f690 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 23 Sep 2026 20:53:10 -0700 Subject: [PATCH] fix(studio-server): stop mislabeling two more read-route failure shapes (#4363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 * 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 --------- Co-authored-by: Claude Sonnet 5 --- .../src/routes/files.pathSafety.test.ts | 92 ++++++++++++++++++ packages/studio-server/src/routes/files.ts | 93 ++++++++++++++++--- 2 files changed, 171 insertions(+), 14 deletions(-) diff --git a/packages/studio-server/src/routes/files.pathSafety.test.ts b/packages/studio-server/src/routes/files.pathSafety.test.ts index ba4c364cab..c66cdbe769 100644 --- a/packages/studio-server/src/routes/files.pathSafety.test.ts +++ b/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", () => { diff --git a/packages/studio-server/src/routes/files.ts b/packages/studio-server/src/routes/files.ts index 7a6f20ec65..6aaa9a2bfa 100644 --- a/packages/studio-server/src/routes/files.ts +++ b/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) ──