fix: trimFilenameAndExt should not be passed full path (#12101)

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<string, string>,
+  postfix = ".zip"
+): Promise<string> {
+  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);
+  });
+});

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) => {

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();
   });
 });

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);