diff --git a/lib/utils/content-disposition.ts b/lib/utils/content-disposition.ts new file mode 100644 index 0000000..8748b6a --- /dev/null +++ b/lib/utils/content-disposition.ts @@ -0,0 +1,6 @@ +export function getAttachmentContentDisposition(filePath: string): string { + const rawFileName = filePath.split(/[\\/]/).filter(Boolean).pop() || "file" + const safeFileName = rawFileName.replace(/["\r\n\\]/g, "_").trim() || "file" + + return `attachment; filename="${safeFileName}"` +} diff --git a/lib/utils/resolve-file-proxy.ts b/lib/utils/resolve-file-proxy.ts index 3145b4e..aaa252e 100644 --- a/lib/utils/resolve-file-proxy.ts +++ b/lib/utils/resolve-file-proxy.ts @@ -2,6 +2,7 @@ import type { FileProxy } from "../db/schema" import { readFile } from "node:fs/promises" import { join } from "node:path" import { normalizePath } from "./normalize-path" +import { getAttachmentContentDisposition } from "./content-disposition" export async function resolveFileProxy( proxy: FileProxy, @@ -36,7 +37,7 @@ async function resolveDiskProxy( return new Response(content, { headers: { "Content-Type": contentType, - "Content-Disposition": `attachment; filename="${fileName}"`, + "Content-Disposition": getAttachmentContentDisposition(fileName), "Content-Length": content.byteLength.toString(), }, }) @@ -79,7 +80,10 @@ async function resolveHttpProxy( headers.set("Content-Length", contentLength) } const fileName = relativePath.split("/").pop() || "file" - headers.set("Content-Disposition", `attachment; filename="${fileName}"`) + headers.set( + "Content-Disposition", + getAttachmentContentDisposition(fileName), + ) return new Response(response.body, { status: response.status, diff --git a/routes/files/download.ts b/routes/files/download.ts index 4bb47d3..c2414df 100644 --- a/routes/files/download.ts +++ b/routes/files/download.ts @@ -5,6 +5,7 @@ import { uint8ArrayToArrayBuffer, } from "lib/utils/decode-base64" import { resolveFileProxy } from "lib/utils/resolve-file-proxy" +import { getAttachmentContentDisposition } from "lib/utils/content-disposition" export default withRouteSpec({ methods: ["GET"], @@ -34,9 +35,7 @@ export default withRouteSpec({ return new Response(responseBody, { headers: { "Content-Type": "application/octet-stream", - "Content-Disposition": `attachment; filename="${file.file_path - .split("/") - .pop()}"`, + "Content-Disposition": getAttachmentContentDisposition(file.file_path), "Content-Length": binaryBody.byteLength.toString(), }, }) @@ -45,9 +44,7 @@ export default withRouteSpec({ return new Response(file.text_content!, { headers: { "Content-Type": "text/plain", - "Content-Disposition": `attachment; filename="${file.file_path - .split("/") - .pop()}"`, + "Content-Disposition": getAttachmentContentDisposition(file.file_path), }, }) }) diff --git a/routes/files/download/[[...file_path]].ts b/routes/files/download/[[...file_path]].ts index 3e8fa02..3531887 100644 --- a/routes/files/download/[[...file_path]].ts +++ b/routes/files/download/[[...file_path]].ts @@ -5,6 +5,7 @@ import { uint8ArrayToArrayBuffer, } from "lib/utils/decode-base64" import { resolveFileProxy } from "lib/utils/resolve-file-proxy" +import { getAttachmentContentDisposition } from "lib/utils/content-disposition" export default withRouteSpec({ methods: ["GET"], @@ -35,9 +36,7 @@ export default withRouteSpec({ return new Response(responseBody, { headers: { "Content-Type": "application/octet-stream", - "Content-Disposition": `attachment; filename="${file.file_path - .split("/") - .pop()}"`, + "Content-Disposition": getAttachmentContentDisposition(file.file_path), "Content-Length": binaryBody.byteLength.toString(), }, }) @@ -46,9 +45,7 @@ export default withRouteSpec({ return new Response(file.text_content!, { headers: { "Content-Type": "text/plain", - "Content-Disposition": `attachment; filename="${file.file_path - .split("/") - .pop()}"`, + "Content-Disposition": getAttachmentContentDisposition(file.file_path), }, }) }) diff --git a/tests/routes/file-proxy02.test.ts b/tests/routes/file-proxy02.test.ts index c1314e8..7d27878 100644 --- a/tests/routes/file-proxy02.test.ts +++ b/tests/routes/file-proxy02.test.ts @@ -91,6 +91,38 @@ test("disk proxy with query param download", async () => { } }) +test("disk proxy download sanitizes unsafe attachment filename", async () => { + const { axios } = await getTestServer() + + const { mkdtemp, writeFile, rm } = await import("node:fs/promises") + const { tmpdir } = await import("node:os") + const { join } = await import("node:path") + + const tempDir = await mkdtemp(join(tmpdir(), "file-proxy-header-test-")) + + try { + await writeFile(join(tempDir, 'bad"name.txt'), "Sanitized proxy header") + + await axios.post("/file_proxies/create", { + proxy_type: "disk", + disk_path: tempDir, + matching_pattern: "header-test/*", + }) + + const downloadRes = await axios.get( + '/files/download/header-test/bad"name.txt', + ) + + expect(downloadRes.status).toBe(200) + expect(downloadRes.data).toBe("Sanitized proxy header") + expect(downloadRes.headers.get("content-disposition")).toBe( + 'attachment; filename="bad_name.txt"', + ) + } finally { + await rm(tempDir, { recursive: true, force: true }) + } +}) + test("disk proxy binary file", async () => { const { axios } = await getTestServer() diff --git a/tests/routes/files.test.ts b/tests/routes/files.test.ts index 196c354..3dbedd6 100644 --- a/tests/routes/files.test.ts +++ b/tests/routes/files.test.ts @@ -112,6 +112,26 @@ test("file download operations2", async () => { }) }) +test("download content disposition sanitizes unsafe filenames", async () => { + const { axios } = await getTestServer() + + const createRes = await axios.post("/files/upsert", { + file_path: '/quoted/bad"name\r\nx-injected: 1.txt', + text_content: "safe header", + }) + + const downloadRes = await axios.get("/files/download", { + params: { file_id: createRes.data.file.file_id }, + }) + + expect(downloadRes.status).toBe(200) + expect(downloadRes.data).toBe("safe header") + expect(downloadRes.headers.get("content-disposition")).toBe( + 'attachment; filename="bad_name__x-injected: 1.txt"', + ) + expect(downloadRes.headers.get("x-injected")).toBeNull() +}) + test("file delete operations", async () => { const { axios } = await getTestServer()