Skip to content

Commit add6783

Browse files
vanceingallsclaude
andauthored
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>
1 parent c59326d commit add6783

2 files changed

Lines changed: 171 additions & 14 deletions

File tree

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

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,98 @@ describe("resolveProjectPath why", () => {
235235
expect(response.status).toBe(403);
236236
expect(await response.json()).toMatchObject({ why: "outside_project" });
237237
});
238+
239+
// A listing (`walkDir`) can show a path that has since been replaced by a
240+
// directory — a rename, or an agent overwriting a file with a folder of the
241+
// same name. `existsSync` passes; `readFileSync` would throw `EISDIR`, which
242+
// Hono answers as plain-text "Internal Server Error" — not JSON, and not a
243+
// reason. This must read the same as any other missing-file 404.
244+
it("reports a path replaced by a directory as 404, not a bare server error", async () => {
245+
const { app, project } = fixture();
246+
rmSync(join(project, "inside.txt"));
247+
mkdirSync(join(project, "inside.txt"));
248+
249+
const response = await app.request(fileUrl("inside.txt"));
250+
251+
expect(response.status).toBe(404);
252+
expect(await response.json()).toMatchObject({ why: "not_a_file" });
253+
});
254+
255+
// The dangling case: the link exists, its target does not, anywhere. This is
256+
// broken plumbing (a stale symlink), not an attack — the read route should
257+
// say so rather than reuse the path-traversal label.
258+
it("reports a dangling symlink as 404, not 403", async (context) => {
259+
const { app, project } = fixture();
260+
linkOrSkip(context, join(project, "nope-target.html"), join(project, "dangling.html"), "file");
261+
262+
const response = await app.request(fileUrl("dangling.html"));
263+
264+
expect(response.status).toBe(404);
265+
expect(await response.json()).toMatchObject({ why: "dangling_symlink" });
266+
});
267+
268+
// The containment guard must not weaken: a symlink that resolves to
269+
// something real outside the project is still the traversal case, whether
270+
// or not it happens to be broken in some OTHER way. Only a target that
271+
// exists nowhere gets the new label.
272+
it("still reports a symlink resolving outside the project as 403 outside_project", async (context) => {
273+
const { app, project, outside } = fixture();
274+
linkOrSkip(context, join(outside, "secret.txt"), join(project, "escape.html"), "file");
275+
276+
const response = await app.request(fileUrl("escape.html"));
277+
278+
expect(response.status).toBe(403);
279+
expect(await response.json()).toMatchObject({ why: "outside_project" });
280+
});
281+
282+
// The "dangling_symlink" label must never apply to a path whose *own*
283+
// location is outside the project (reached via `..`) — only to a symlink
284+
// that lives inside the project. Otherwise the response leaks, to anyone
285+
// who can hit the route, whether an out-of-project path happens to be a
286+
// dangling symlink, which is exactly the containment guard's job to hide.
287+
it("reports a dangling symlink reached by traversal as 403, not 404", async (context) => {
288+
const { app, project, outside } = fixture();
289+
linkOrSkip(context, join(outside, "nope-target.html"), join(outside, "dangling.html"), "file");
290+
291+
const response = await app.request(fileUrl(relative(project, join(outside, "dangling.html"))));
292+
293+
expect(response.status).toBe(403);
294+
expect(await response.json()).toMatchObject({ why: "outside_project" });
295+
});
296+
297+
// Same leak, one level removed: the leaf name is lexically inside the
298+
// project, but it's reached through a directory symlink that itself
299+
// escapes the project. The dangling-ness of the leaf must not surface.
300+
it("reports a dangling leaf behind an escaping directory symlink as 403, not 404", async (context) => {
301+
const { app, project, outside } = fixture();
302+
linkOrSkip(context, outside, join(project, "ext"), "dir");
303+
304+
const response = await app.request(fileUrl("ext/nope-target.html"));
305+
306+
expect(response.status).toBe(403);
307+
expect(await response.json()).toMatchObject({ why: "outside_project" });
308+
});
309+
310+
// A symlink that lives inside the project but points *outside* it must
311+
// read identically (403, same why) whether or not the outside target
312+
// exists — the existence of an outside file is exactly what containment
313+
// must never reveal, and `dangling_symlink` is a 404 an attacker could
314+
// otherwise use to probe it.
315+
it("does not distinguish an existing from a missing target across the project boundary", async (context) => {
316+
const { app, project, outside } = fixture();
317+
writeFileSync(join(outside, "b-target.txt"), "outside b");
318+
linkOrSkip(context, join(outside, "a-missing.txt"), join(project, "a.html"), "file");
319+
linkOrSkip(context, join(outside, "b-target.txt"), join(project, "b.html"), "file");
320+
321+
const [toMissing, toExisting] = await Promise.all([
322+
app.request(fileUrl("a.html")),
323+
app.request(fileUrl("b.html")),
324+
]);
325+
326+
expect(toMissing.status).toBe(toExisting.status);
327+
expect(await toMissing.json()).toMatchObject({ why: "outside_project" });
328+
expect(await toExisting.json()).toMatchObject({ why: "outside_project" });
329+
});
238330
});
239331

240332
describe("upload collision races", () => {

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

Lines changed: 79 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@ import { bodyLimit } from "hono/body-limit";
77
import {
88
closeSync,
99
existsSync,
10+
lstatSync,
1011
openSync,
1112
readFileSync,
13+
readlinkSync,
1214
writeFileSync,
1315
writeSync,
1416
mkdirSync,
@@ -126,6 +128,46 @@ interface ResolvedGsapFile {
126128
absPath: string;
127129
}
128130

131+
/**
132+
* True only for a symlink that itself lives inside the project, whose target
133+
* (once resolved against the link's own directory) also names a location
134+
* inside the project, and does not exist anywhere — not for one that exists
135+
* (that stays a real containment failure) and not for a plain missing path
136+
* (the ordinary case `resolveWithinProject` already covers).
137+
*
138+
* Both containment checks matter, not just the second: a request can name a
139+
* path lexically *outside* the project (reached via `..`) that happens to be
140+
* a dangling symlink out there, or a real in-project symlink that points
141+
* *outside* the project at a target that may or may not exist. Labeling
142+
* either of those "not found" would leak, to anyone who can hit the route,
143+
* whether an out-of-project path exists — the containment check exists
144+
* precisely so that answer never depends on what's outside the project.
145+
* `isSafePath` fails closed on a dangling in-project symlink by design (a
146+
* write through it could later resolve outside the project once something
147+
* creates the target) — this does not loosen that; it only tells the caller
148+
* *why* the containment check refused, so the response can say "not found"
149+
* instead of a path-traversal-shaped "forbidden" for a case that scans as
150+
* broken plumbing, not an attack.
151+
*/
152+
function isDanglingSymlinkInProject(projectDir: string, lexicalPath: string): boolean {
153+
if (!isSafePath(projectDir, dirname(lexicalPath))) return false;
154+
let stats;
155+
try {
156+
stats = lstatSync(lexicalPath);
157+
} catch {
158+
return false;
159+
}
160+
if (!stats.isSymbolicLink()) return false;
161+
const target = resolve(dirname(lexicalPath), readlinkSync(lexicalPath));
162+
if (!isSafePath(projectDir, target)) return false;
163+
try {
164+
statSync(lexicalPath);
165+
return false;
166+
} catch {
167+
return true;
168+
}
169+
}
170+
129171
/** Resolve project + safe absolute path for any project-scoped route. */
130172
async function resolveProjectPath(
131173
c: RouteContext,
@@ -160,6 +202,9 @@ async function resolveProjectPath(
160202

161203
const absPath = resolveWithinProject(project.dir, filePath);
162204
if (!absPath) {
205+
if (isDanglingSymlinkInProject(project.dir, resolve(project.dir, filePath))) {
206+
return { error: c.json({ error: "not found", why: "dangling_symlink" }, 404) } as const;
207+
}
163208
return { error: c.json({ error: "forbidden", why: "outside_project" }, 403) } as const;
164209
}
165210

@@ -2276,7 +2321,13 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
22762321
const res = await resolveProjectFile(c, adapter);
22772322
if ("error" in res) return res.error;
22782323

2279-
if (!existsSync(res.absPath)) {
2324+
// Opened once and checked/read through the same descriptor, not the path,
2325+
// so a directory-for-file swap (or anything else) between the check below
2326+
// and the read can't land a stale answer — both act on the identical inode.
2327+
let fd: number;
2328+
try {
2329+
fd = openSync(res.absPath, "r");
2330+
} catch {
22802331
if (c.req.query("optional") === "1") {
22812332
// `missing: true` separates the absent-file shim from a genuinely
22822333
// 0-byte file — both answer `content: ""`, and the caller could not
@@ -2289,20 +2340,34 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
22892340
}
22902341
return c.json({ error: "not found" }, 404);
22912342
}
2343+
try {
2344+
// A listing built from `walkDir` can show a path that has since been
2345+
// replaced by a directory (a rename, or an agent overwriting a file
2346+
// with a folder of the same name) — opening it succeeds (POSIX allows
2347+
// O_RDONLY on a directory), and reading it would throw `EISDIR`, which
2348+
// Hono answers as a plain-text 500. The caller already handles a 404
2349+
// with `why`; this reports the same shape instead of an opaque server
2350+
// error for something that is not one.
2351+
if (!fstatSync(fd).isFile()) {
2352+
return c.json({ error: "not found", why: "not_a_file" }, 404);
2353+
}
22922354

2293-
const content = readFileSync(res.absPath);
2294-
const version = fileContentVersion(content);
2295-
c.header("ETag", version);
2296-
// `missing: false` on the read path too, so its PRESENCE is what tells a
2297-
// caller this server distinguishes the two empty answers at all. Without
2298-
// it here, a real 0-byte file from a new server looks exactly like either
2299-
// case from an old one, and the split above buys nothing.
2300-
return c.json({
2301-
filename: res.filePath,
2302-
content: content.toString("utf-8"),
2303-
version,
2304-
missing: false,
2305-
});
2355+
const content = readFileSync(fd);
2356+
const version = fileContentVersion(content);
2357+
c.header("ETag", version);
2358+
// `missing: false` on the read path too, so its PRESENCE is what tells a
2359+
// caller this server distinguishes the two empty answers at all. Without
2360+
// it here, a real 0-byte file from a new server looks exactly like either
2361+
// case from an old one, and the split above buys nothing.
2362+
return c.json({
2363+
filename: res.filePath,
2364+
content: content.toString("utf-8"),
2365+
version,
2366+
missing: false,
2367+
});
2368+
} finally {
2369+
closeSync(fd);
2370+
}
23062371
});
23072372

23082373
// ── Write (overwrite) ──

0 commit comments

Comments
 (0)