From 6d7d8b056ccedf253a44255248ef3cca4d7232cc Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 18 Apr 2026 08:10:30 -0400 Subject: [PATCH] fix: trimFilenameAndExt should not be passed full path (#12101) --- server/utils/ZipHelper.test.ts | 68 +++++++++++++++++++++++ server/utils/ZipHelper.ts | 25 ++++++--- server/utils/fs.test.ts | 99 ++++++++++++++++++---------------- server/utils/fs.ts | 12 ++++- 4 files changed, 151 insertions(+), 53 deletions(-) create mode 100644 server/utils/ZipHelper.test.ts diff --git a/server/utils/ZipHelper.test.ts b/server/utils/ZipHelper.test.ts new file mode 100644 index 0000000000..0e0cacf1ee --- /dev/null +++ b/server/utils/ZipHelper.test.ts @@ -0,0 +1,68 @@ +import path from "node:path"; +import fs from "fs-extra"; +import JSZip from "jszip"; +import tmp from "tmp"; +import ZipHelper from "./ZipHelper"; + +async function writeZip( + entries: Record, + postfix = ".zip" +): Promise { + const zip = new JSZip(); + for (const [name, content] of Object.entries(entries)) { + zip.file(name, content); + } + const buffer = await zip.generateAsync({ type: "nodebuffer" }); + const zipPath = tmp.fileSync({ postfix }).name; + await fs.writeFile(zipPath, buffer); + return zipPath; +} + +describe("ZipHelper.extract", () => { + it("extracts a simple nested file inside the output directory", async () => { + const outputDir = tmp.dirSync({ unsafeCleanup: true }).name; + const zipPath = await writeZip({ "a/b/hello.txt": "hi" }); + + await ZipHelper.extract(zipPath, outputDir); + + const content = await fs.readFile( + path.join(outputDir, "a", "b", "hello.txt"), + "utf8" + ); + expect(content).toBe("hi"); + }); + + it("does not escape the output directory when the joined path exceeds MAX_PATH_LENGTH", async () => { + // Build a nested path whose joined length exceeds the old MAX_PATH_LENGTH + // (4096) threshold. Previously, passing the full path to trimFileAndExt + // would drop every directory segment and write the file relative to the + // process CWD. + const filename = "poc_escape_target.txt"; + const outputDir = tmp.dirSync({ unsafeCleanup: true }).name; + + // Pick a segment count that pushes the total joined path past 4096 bytes + // while keeping the directory portion under Linux PATH_MAX (4096). + const overhead = outputDir.length + 1 + 1 + filename.length; + const segments = Math.ceil((4097 - overhead) / 2); + const entryName = "a/".repeat(segments) + filename; + + const zipPath = await writeZip({ [entryName]: "ZIP_ESCAPE_POC_CONTENT" }); + + // Run extraction from a clean working directory so we can detect an escape + // without polluting the repo root. + const cwd = process.cwd(); + const scratchCwd = tmp.dirSync({ unsafeCleanup: true }).name; + process.chdir(scratchCwd); + try { + await ZipHelper.extract(zipPath, outputDir).catch(() => { + // Some environments may reject long paths; the key assertion below + // still verifies no file escaped the output directory. + }); + } finally { + process.chdir(cwd); + } + + // The escaped filename must NOT appear in CWD. + expect(await fs.pathExists(path.join(scratchCwd, filename))).toBe(false); + }); +}); diff --git a/server/utils/ZipHelper.ts b/server/utils/ZipHelper.ts index d835e8326f..05a0dc46fe 100644 --- a/server/utils/ZipHelper.ts +++ b/server/utils/ZipHelper.ts @@ -7,10 +7,9 @@ import yauzl, { validateFileName } from "yauzl"; import { bytesToHumanReadable } from "@shared/utils/files"; import Logger from "@server/logging/Logger"; import { trace } from "@server/logging/tracing"; -import { trimFileAndExt } from "./fs"; +import { trimFilenameAndExt } from "./fs"; const MAX_FILE_NAME_LENGTH = 255; -const MAX_PATH_LENGTH = 4096; @trace() export default class ZipHelper { @@ -170,16 +169,30 @@ export default class ZipHelper { return processNext(mkErr); } - const fileName = trimFileAndExt( + const fileName = trimFilenameAndExt( path.basename(filePath), MAX_FILE_NAME_LENGTH ); - const location = trimFileAndExt( - path.join(outputDir, path.dirname(filePath), fileName), - MAX_PATH_LENGTH + const resolvedOutput = path.resolve(outputDir); + const location = path.resolve( + resolvedOutput, + path.dirname(filePath), + fileName ); + if ( + location !== resolvedOutput && + !location.startsWith(resolvedOutput + path.sep) + ) { + Logger.warn("Zip entry escapes extraction directory", { + filePath, + location, + }); + readStream.destroy(); + return processNext(); + } + const dest = fs .createWriteStream(location) .on("error", (error) => { diff --git a/server/utils/fs.test.ts b/server/utils/fs.test.ts index de96bd1c07..ea159cfc1a 100644 --- a/server/utils/fs.test.ts +++ b/server/utils/fs.test.ts @@ -1,7 +1,7 @@ import { serializeFilename, deserializeFilename, - trimFileAndExt, + trimFilenameAndExt, stringByteLength, } from "./fs"; @@ -46,92 +46,92 @@ describe("stringByteLength", () => { }); }); -describe("trimFileAndExt", () => { +describe("trimFilenameAndExt", () => { it("should trim filename", () => { - expect(trimFileAndExt("file.txt", 6)).toBe("fi.txt"); - expect(trimFileAndExt("file.txt", 8)).toBe("file.txt"); - expect(trimFileAndExt("file.md", 9)).toBe("file.md"); - expect(trimFileAndExt("你好.md", 9)).toBe("你好.md"); // No trimming needed - expect(trimFileAndExt("你好.md", 8)).toBe("你.md"); // Trim one character + expect(trimFilenameAndExt("file.txt", 6)).toBe("fi.txt"); + expect(trimFilenameAndExt("file.txt", 8)).toBe("file.txt"); + expect(trimFilenameAndExt("file.md", 9)).toBe("file.md"); + expect(trimFilenameAndExt("你好.md", 9)).toBe("你好.md"); // No trimming needed + expect(trimFilenameAndExt("你好.md", 8)).toBe("你.md"); // Trim one character }); it("should handle files with no extension", () => { - expect(trimFileAndExt("filename", 4)).toBe("file"); - expect(trimFileAndExt("verylongfilename", 8)).toBe("verylong"); - expect(trimFileAndExt("file", 10)).toBe("file"); + expect(trimFilenameAndExt("filename", 4)).toBe("file"); + expect(trimFilenameAndExt("verylongfilename", 8)).toBe("verylong"); + expect(trimFilenameAndExt("file", 10)).toBe("file"); }); it("should handle extensions longer than the limit", () => { - expect(trimFileAndExt("file.verylongextension", 10)).toBe("file.veryl"); - expect(trimFileAndExt("a.toolongext", 5)).toBe("a.too"); + expect(trimFilenameAndExt("file.verylongextension", 10)).toBe("file.veryl"); + expect(trimFilenameAndExt("a.toolongext", 5)).toBe("a.too"); }); it("should handle edge cases with very short limits", () => { - expect(trimFileAndExt("file.txt", 1)).toBe("f"); // Can only fit 1 byte - expect(trimFileAndExt("file.txt", 0)).toBe(""); - expect(trimFileAndExt("file.txt", -1)).toBe(""); + expect(trimFilenameAndExt("file.txt", 1)).toBe("f"); // Can only fit 1 byte + expect(trimFilenameAndExt("file.txt", 0)).toBe(""); + expect(trimFilenameAndExt("file.txt", -1)).toBe(""); }); it("should handle files with multiple dots", () => { - expect(trimFileAndExt("file.name.txt", 10)).toBe("file.n.txt"); - expect(trimFileAndExt("archive.tar.gz", 14)).toBe("archive.tar.gz"); // No trimming needed - expect(trimFileAndExt("archive.tar.gz", 12)).toBe("archive.t.gz"); // Trim to fit - expect(trimFileAndExt("my.file.backup.txt", 10)).toBe("my.fil.txt"); + expect(trimFilenameAndExt("file.name.txt", 10)).toBe("file.n.txt"); + expect(trimFilenameAndExt("archive.tar.gz", 14)).toBe("archive.tar.gz"); // No trimming needed + expect(trimFilenameAndExt("archive.tar.gz", 12)).toBe("archive.t.gz"); // Trim to fit + expect(trimFilenameAndExt("my.file.backup.txt", 10)).toBe("my.fil.txt"); }); it("should handle empty strings", () => { - expect(trimFileAndExt("", 10)).toBe(""); - expect(trimFileAndExt("", 0)).toBe(""); + expect(trimFilenameAndExt("", 10)).toBe(""); + expect(trimFilenameAndExt("", 0)).toBe(""); }); it("should handle multi-byte UTF-8 characters properly", () => { - expect(trimFileAndExt("🦄🌟.txt", 8)).toBe("🦄.txt"); // 🦄 is 4 bytes, 🌟 is 4 bytes, .txt is 4 bytes - expect(trimFileAndExt("файл.txt", 8)).toBe("фа.txt"); // Cyrillic characters (фа is 4 bytes + .txt is 4 bytes = 8 total) - expect(trimFileAndExt("测试文件.md", 10)).toBe("测试.md"); // Chinese characters + expect(trimFilenameAndExt("🦄🌟.txt", 8)).toBe("🦄.txt"); // 🦄 is 4 bytes, 🌟 is 4 bytes, .txt is 4 bytes + expect(trimFilenameAndExt("файл.txt", 8)).toBe("фа.txt"); // Cyrillic characters (фа is 4 bytes + .txt is 4 bytes = 8 total) + expect(trimFilenameAndExt("测试文件.md", 10)).toBe("测试.md"); // Chinese characters }); it("should not break UTF-8 character boundaries", () => { // Ensure we don't cut through multi-byte characters - expect(trimFileAndExt("🦄🦄.txt", 8)).toBe("🦄.txt"); // Should not cut through second emoji (🦄 is 4 bytes + .txt is 4 bytes = 8 total) - expect(trimFileAndExt("🦄🦄.txt", 7)).toBe(".txt"); // Should slice the whole filename when extension won't fit (but preserve UTF-8 boundaries) - expect(trimFileAndExt("你好世界.txt", 11)).toBe("你好.txt"); // Should cut at character boundary + expect(trimFilenameAndExt("🦄🦄.txt", 8)).toBe("🦄.txt"); // Should not cut through second emoji (🦄 is 4 bytes + .txt is 4 bytes = 8 total) + expect(trimFilenameAndExt("🦄🦄.txt", 7)).toBe(".txt"); // Should slice the whole filename when extension won't fit (but preserve UTF-8 boundaries) + expect(trimFilenameAndExt("你好世界.txt", 11)).toBe("你好.txt"); // Should cut at character boundary }); it("should handle extension-only files", () => { - expect(trimFileAndExt(".gitignore", 5)).toBe(".giti"); - expect(trimFileAndExt(".env", 3)).toBe(".en"); - expect(trimFileAndExt(".bashrc", 10)).toBe(".bashrc"); + expect(trimFilenameAndExt(".gitignore", 5)).toBe(".giti"); + expect(trimFilenameAndExt(".env", 3)).toBe(".en"); + expect(trimFilenameAndExt(".bashrc", 10)).toBe(".bashrc"); }); it("should handle files where extension equals or exceeds the limit", () => { - expect(trimFileAndExt("file.extension", 9)).toBe("file.exte"); // Extension is 10 bytes, limit is 9 - expect(trimFileAndExt("f.verylongextension", 10)).toBe("f.verylong"); // Slice whole filename when extension too long + expect(trimFilenameAndExt("file.extension", 9)).toBe("file.exte"); // Extension is 10 bytes, limit is 9 + expect(trimFilenameAndExt("f.verylongextension", 10)).toBe("f.verylong"); // Slice whole filename when extension too long }); it("should preserve behavior when no trimming needed", () => { - expect(trimFileAndExt("short.txt", 100)).toBe("short.txt"); - expect(trimFileAndExt("file.md", 50)).toBe("file.md"); + expect(trimFilenameAndExt("short.txt", 100)).toBe("short.txt"); + expect(trimFilenameAndExt("file.md", 50)).toBe("file.md"); }); it("should handle mixed ASCII and UTF-8 characters", () => { - expect(trimFileAndExt("file-测试.txt", 12)).toBe("file-测.txt"); - expect(trimFileAndExt("🦄unicorn.md", 10)).toBe("🦄uni.md"); - expect(trimFileAndExt("test-файл.doc", 11)).toBe("test-ф.doc"); + expect(trimFilenameAndExt("file-测试.txt", 12)).toBe("file-测.txt"); + expect(trimFilenameAndExt("🦄unicorn.md", 10)).toBe("🦄uni.md"); + expect(trimFilenameAndExt("test-файл.doc", 11)).toBe("test-ф.doc"); }); it("should handle very long filenames", () => { const longName = "a".repeat(200); - const result = trimFileAndExt(`${longName}.txt`, 50); + const result = trimFilenameAndExt(`${longName}.txt`, 50); expect(Buffer.byteLength(result, "utf8")).toBe(50); expect(result.endsWith(".txt")).toBe(true); }); it("should handle filesystem limit edge cases", () => { // Test around common filesystem limits - expect(trimFileAndExt("file.txt", 255)).toBe("file.txt"); // Common filename limit - expect(trimFileAndExt("file.txt", 4096)).toBe("file.txt"); // Common path limit + expect(trimFilenameAndExt("file.txt", 255)).toBe("file.txt"); // Common filename limit + expect(trimFilenameAndExt("file.txt", 4096)).toBe("file.txt"); // Common path limit - const result255 = trimFileAndExt("a".repeat(300) + ".txt", 255); + const result255 = trimFilenameAndExt("a".repeat(300) + ".txt", 255); expect(Buffer.byteLength(result255, "utf8")).toBeLessThanOrEqual(255); }); @@ -146,7 +146,7 @@ describe("trimFileAndExt", () => { testCases.forEach((filename) => { for (let limit = 1; limit <= 20; limit++) { - const result = trimFileAndExt(filename, limit); + const result = trimFilenameAndExt(filename, limit); expect(result).not.toContain("�"); expect(Buffer.byteLength(result, "utf8")).toBeLessThanOrEqual(limit); } @@ -154,8 +154,17 @@ describe("trimFileAndExt", () => { }); it("should handle special ASCII characters", () => { - expect(trimFileAndExt("file-name_123.txt", 10)).toBe("file-n.txt"); - expect(trimFileAndExt("file@domain.com.txt", 12)).toBe("file@dom.txt"); - expect(trimFileAndExt("file (copy).txt", 10)).toBe("file (.txt"); + expect(trimFilenameAndExt("file-name_123.txt", 10)).toBe("file-n.txt"); + expect(trimFilenameAndExt("file@domain.com.txt", 12)).toBe("file@dom.txt"); + expect(trimFilenameAndExt("file (copy).txt", 10)).toBe("file (.txt"); + }); + + it("should reject inputs containing path separators", () => { + expect(() => trimFilenameAndExt("a/b.txt", 10)).toThrow(); + expect(() => trimFilenameAndExt("a\\b.txt", 10)).toThrow(); + expect(() => trimFilenameAndExt("/leading.txt", 10)).toThrow(); + expect(() => + trimFilenameAndExt("/tmp/import-ABC/" + "a/".repeat(2000) + "x.txt", 255) + ).toThrow(); }); }); diff --git a/server/utils/fs.ts b/server/utils/fs.ts index f7d856a0e7..226ae4bb9c 100644 --- a/server/utils/fs.ts +++ b/server/utils/fs.ts @@ -62,13 +62,21 @@ function sliceStringToByteLength(str: string, maxBytes: number): string { } /** - * Trim a file name to a maximum length, retaining the extension. + * Trim a file name to a maximum length, retaining the extension. The input + * must be a filename only — passing a path (containing `/` or `\`) will throw. * * @param text The file name to trim. * @param length The maximum length of the file name in bytes. * @returns The trimmed file name. + * @throws If `text` contains a path separator. */ -export function trimFileAndExt(text: string, length: number): string { +export function trimFilenameAndExt(text: string, length: number): string { + if (text.includes("/") || text.includes("\\")) { + throw new Error( + "trimFilenameAndExt expects a filename without path separators" + ); + } + if (Buffer.byteLength(text, "utf8") > length) { const ext = path.extname(text); const name = path.basename(text, ext);