From 91a34ffc8a6ecea34f67c0bc7ba0437ac82fd432 Mon Sep 17 00:00:00 2001 From: James Russo Date: Tue, 8 Sep 2026 18:51:58 -0400 Subject: [PATCH] fix(studio): claim upload filenames exclusively (#3786) * fix(studio): claim upload filenames exclusively * fix(studio): write uploads through exclusive descriptors --- .../src/routes/files.pathSafety.test.ts | 61 ++++++++++++++++++- packages/studio-server/src/routes/files.ts | 41 ++++++++++--- 2 files changed, 94 insertions(+), 8 deletions(-) diff --git a/packages/studio-server/src/routes/files.pathSafety.test.ts b/packages/studio-server/src/routes/files.pathSafety.test.ts index 989a5ab6fd..95561b0235 100644 --- a/packages/studio-server/src/routes/files.pathSafety.test.ts +++ b/packages/studio-server/src/routes/files.pathSafety.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, type TestContext } from "vitest"; +import { afterEach, describe, expect, it, vi, type TestContext } from "vitest"; import { Hono } from "hono"; import { existsSync, @@ -17,6 +17,7 @@ import type { StudioApiAdapter } from "../types"; const tempDirs: string[] = []; afterEach(() => { + vi.restoreAllMocks(); for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); @@ -201,3 +202,61 @@ describe("file route containment", () => { ); }); }); + +describe("upload collision races", () => { + function raceDuringRead(filename: string, collide: () => void) { + const file = new File(["new upload"], filename); + const read = file.arrayBuffer.bind(file); + vi.spyOn(file, "arrayBuffer").mockImplementation(async () => { + collide(); + return read(); + }); + const form = new FormData(); + form.append("files", file); + vi.spyOn(Request.prototype, "formData").mockResolvedValue(form); + } + + it("preserves an upload that appears while the body is read", async () => { + const { app, project } = fixture(); + raceDuringRead("upload.txt", () => writeFileSync(join(project, "upload.txt"), "other upload")); + const response = await upload(app); + expect(response.status).toBe(201); + expect(await response.json()).toMatchObject({ files: ["upload (2).txt"] }); + expect(readFileSync(join(project, "upload.txt"), "utf8")).toBe("other upload"); + expect(readFileSync(join(project, "upload (2).txt"), "utf8")).toBe("new upload"); + }); + + it.each([".gitignore", "archive.tar.txt"])( + "retries suffix races without changing extension rules: %s", + async (name) => { + const { app, project } = fixture(); + const dot = name.indexOf(".", name.startsWith(".") ? 1 : 0); + const base = dot > 0 ? name.slice(0, dot) : name; + const ext = dot > 0 ? name.slice(dot) : ""; + writeFileSync(join(project, name), "original"); + raceDuringRead(name, () => { + writeFileSync(join(project, `${base} (2)${ext}`), "second"); + writeFileSync(join(project, `${base} (3)${ext}`), "third"); + }); + const response = await upload(app, "", name); + expect(response.status).toBe(201); + expect(await response.json()).toMatchObject({ files: [`${base} (4)${ext}`] }); + expect(readFileSync(join(project, `${base} (2)${ext}`), "utf8")).toBe("second"); + expect(readFileSync(join(project, `${base} (3)${ext}`), "utf8")).toBe("third"); + }, + ); + + it("does not write through a symlink planted during the upload read", async (context) => { + const { app, project, outside } = fixture(); + // Establish symlink support before entering the mocked asynchronous read. + const probe = join(project, "probe-link"); + linkOrSkip(context, join(outside, "secret.txt"), probe, "file"); + rmSync(probe); + raceDuringRead("upload.txt", () => + symlinkSync(join(outside, "secret.txt"), join(project, "upload.txt")), + ); + const response = await upload(app); + expect(await response.json()).toMatchObject({ files: [] }); + expect(readFileSync(join(outside, "secret.txt"), "utf8")).toBe("outside secret"); + }); +}); diff --git a/packages/studio-server/src/routes/files.ts b/packages/studio-server/src/routes/files.ts index 4037c3b323..3c1afc351f 100644 --- a/packages/studio-server/src/routes/files.ts +++ b/packages/studio-server/src/routes/files.ts @@ -2204,13 +2204,14 @@ async function processUploadedFiles( // Don't overwrite — append (2), (3), etc. let finalPath = destPath; let finalName = name; + // Handle dotfiles correctly: .gitignore → ext="", base=".gitignore" + const dotIdx = name.indexOf(".", name.startsWith(".") ? 1 : 0); + const ext = dotIdx > 0 ? name.slice(dotIdx) : ""; + const base = dotIdx > 0 ? name.slice(0, dotIdx) : name; + const MAX_COPY_INDEX = 10000; + let n = 1; if (existsSync(finalPath)) { - // Handle dotfiles correctly: .gitignore → ext="", base=".gitignore" - const dotIdx = name.indexOf(".", name.startsWith(".") ? 1 : 0); - const ext = dotIdx > 0 ? name.slice(dotIdx) : ""; - const base = dotIdx > 0 ? name.slice(0, dotIdx) : name; - let n = 2; - const MAX_COPY_INDEX = 10000; + n = 2; while (n < MAX_COPY_INDEX && existsSync(resolve(targetDir, `${base} (${n})${ext}`))) n++; if (n >= MAX_COPY_INDEX) { skipped.push(name); @@ -2231,7 +2232,33 @@ async function processUploadedFiles( continue; } - writeFileSync(finalPath, buffer); + // Reading the upload yields: another request can claim the selected name. + // Only exclusive creation authorizes a write; retry collisions without + // following a newly planted link or overwriting another upload. + let written = false; + while (n < MAX_COPY_INDEX && isSafePath(projectDir, finalPath)) { + try { + const fd = openSync(finalPath, "wx"); + try { + writeFileSync(fd, buffer); + } finally { + closeSync(fd); + } + written = true; + break; + } catch (error) { + if (!(error instanceof Error) || !("code" in error) || error.code !== "EEXIST") { + throw error; + } + n++; + finalName = `${base} (${n})${ext}`; + finalPath = resolve(targetDir, finalName); + } + } + if (!written) { + if (n >= MAX_COPY_INDEX) skipped.push(name); + continue; + } const relativePath = subDir ? join(subDir, finalName) : finalName; uploaded.push(relativePath); if (isAudioFile(finalName)) {