Merge pull request #684 from C4illin/FixBugsWithMarkedUnitTests

fix: bugs with marked unit tests

src/converters/ffmpeg.ts

@@ -770,9 +770,15 @@ export async function convert(
   }
 
   // Parse FFMPEG_ARGS environment variable into array
-  const ffmpegArgs = process.env.FFMPEG_ARGS ? process.env.FFMPEG_ARGS.split(/\s+/) : [];
+  const ffmpegArgs = process.env.FFMPEG_ARGS
+    ? process.env.FFMPEG_ARGS.split(/\s+/)
+        .filter(Boolean)
+        .map((arg) => arg.trim())
+    : [];
   const ffmpegOutputArgs = process.env.FFMPEG_OUTPUT_ARGS
     ? process.env.FFMPEG_OUTPUT_ARGS.split(/\s+/)
+        .filter(Boolean)
+        .map((arg) => arg.trim())
     : [];
 
   return new Promise((resolve, reject) => {

src/converters/main.ts

@@ -85,6 +85,11 @@ const properties: Record<
     properties: propertiesxelatex,
     converter: convertxelatex,
   },
+  // djvu before calibre so the dedicated ddjvu converter wins for DjVu files
+  djvu: {
+    properties: propertiesDjvu,
+    converter: convertDjvu,
+  },
   calibre: {
     properties: propertiesCalibre,
     converter: convertCalibre,
@@ -93,10 +98,6 @@ const properties: Record<
     properties: propertiesDasel,
     converter: convertDasel,
   },
-  djvu: {
-    properties: propertiesDjvu,
-    converter: convertDjvu,
-  },
   libreoffice: {
     properties: propertiesLibreOffice,
     converter: convertLibreOffice,
@@ -246,8 +247,8 @@ async function mainConverter(
     converterFunc = properties[converterName]?.converter;
   } else {
     // Iterate over each converter in properties
-    for (converterName in properties) {
-      const converterObj = properties[converterName];
+    for (const name in properties) {
+      const converterObj = properties[name];
 
       if (!converterObj) {
         break;
@@ -259,9 +260,13 @@ async function mainConverter(
           converterObj?.properties?.to[key]?.includes(convertTo)
         ) {
           converterFunc = converterObj.converter;
+          converterName = name;
           break;
         }
       }
+      if (converterFunc) {
+        break;
+      }
     }
   }
 
@@ -296,6 +301,7 @@ const possibleTargets: Record<string, Record<string, string[]>> = {};
 
 for (const converterName in properties) {
   const converterProperties = properties[converterName]?.properties;
+
   if (!converterProperties) continue;
 
   for (const key in converterProperties.from) {
@@ -307,7 +313,7 @@ for (const converterName in properties) {
     for (const ext of fromList) {
       if (!possibleTargets[ext]) possibleTargets[ext] = {};
 
-      possibleTargets[ext][converterName] = toList;
+      possibleTargets[ext][converterName] = [...toList];
     }
   }
 }
@@ -322,9 +328,7 @@ const possibleInputs: string[] = [];
 for (const converterName in properties) {
   const converterProperties = properties[converterName]?.properties;
 
-  if (!converterProperties) {
-    continue;
-  }
+  if (!converterProperties) continue;
 
   for (const key in converterProperties.from) {
     for (const extension of converterProperties.from[key] ?? []) {
@@ -341,15 +345,16 @@ const allTargets: Record<string, string[]> = {};
 for (const converterName in properties) {
   const converterProperties = properties[converterName]?.properties;
 
-  if (!converterProperties) {
-    continue;
-  }
+  if (!converterProperties) continue;
 
   for (const key in converterProperties.to) {
     if (allTargets[converterName]) {
-      allTargets[converterName].push(...(converterProperties.to[key] || []));
+      allTargets[converterName] = [
+        ...allTargets[converterName],
+        ...(converterProperties.to[key] || []),
+      ];
     } else {
-      allTargets[converterName] = converterProperties.to[key] || [];
+      allTargets[converterName] = [...(converterProperties.to[key] || [])];
     }
   }
 }
@@ -362,13 +367,14 @@ const allInputs: Record<string, string[]> = {};
 for (const converterName in properties) {
   const converterProperties = properties[converterName]?.properties;
 
-  if (!converterProperties) {
-    continue;
-  }
+  if (!converterProperties) continue;
 
   for (const key in converterProperties.from) {
     if (allInputs[converterName]) {
-      allInputs[converterName].push(...(converterProperties.from[key] || []));
+      allInputs[converterName] = [
+        ...allInputs[converterName],
+        ...(converterProperties.from[key] || []),
+      ];
     } else {
       allInputs[converterName] = converterProperties.from[key] || [];
     }

src/converters/vcf.ts

@@ -46,12 +46,14 @@ export function parseVCF(data: string): Record<string, string>[] {
 
 export function toCSV(data: Record<string, string>[]): string {
   if (!data.length) return "";
-  const first = data[0];
-  if (!first) return "";
-  const headers = Object.keys(first);
+  const headers = new Set<string>();
+  for (const row of data) {
+    Object.keys(row).forEach((key) => headers.add(key));
+  }
+  const headerArray = Array.from(headers);
   const escape = (str: string) => `"${str.replace(/"/g, '""')}"`;
-  const rows = data.map((row) => headers.map((h) => escape(row[h] || "")).join(","));
-  return [headers.join(","), ...rows].join("\n");
+  const rows = data.map((row) => headerArray.map((h) => escape(row[h] || "")).join(","));
+  return [headerArray.join(","), ...rows].join("\n");
 }
 
 export async function convert(

src/pages/convert.tsx

@@ -56,16 +56,25 @@ export const convert = new Elysia().use(userService).post(
       return redirect(`${WEBROOT}/`, 302);
     }
 
-    const fileNames = JSON.parse(body.file_names) as string[];
-
-    for (let i = 0; i < fileNames.length; i++) {
-      fileNames[i] = sanitize(fileNames[i] || "");
+    let fileNames: string[];
+    try {
+      fileNames = JSON.parse(body.file_names) as string[];
+    } catch {
+      return redirect(`${WEBROOT}/`, 302);
     }
 
     if (!Array.isArray(fileNames) || fileNames.length === 0) {
       return redirect(`${WEBROOT}/`, 302);
     }
 
+    if (!fileNames.every((name) => typeof name === "string")) {
+      return redirect(`${WEBROOT}/`, 302);
+    }
+
+    for (let i = 0; i < fileNames.length; i++) {
+      fileNames[i] = sanitize(fileNames[i] || "");
+    }
+
     db.query("UPDATE jobs SET num_files = ?1, status = 'pending' WHERE id = ?2").run(
       fileNames.length,
       jobId.value,

src/pages/deleteFile.tsx

@@ -9,7 +9,7 @@ import path from "node:path";
 
 export const deleteFile = new Elysia().use(userService).post(
   "/delete",
-  async ({ body, redirect, cookie: { jobId }, user }) => {
+  async ({ body, redirect, set, cookie: { jobId }, user }) => {
     if (!jobId?.value) {
       return redirect(`${WEBROOT}/`, 302);
     }
@@ -27,7 +27,15 @@ export const deleteFile = new Elysia().use(userService).post(
     const sanitized = sanitize(body.filename);
     const targetPath = path.join(userUploadsDir, sanitized);
 
-    await unlink(targetPath);
+    try {
+      await unlink(targetPath);
+    } catch (e) {
+      if ((e as NodeJS.ErrnoException).code === "ENOENT") {
+        set.status = 404;
+        return { message: "File not found." };
+      }
+      throw e;
+    }
 
     return {
       message: "File deleted successfully.",

src/pages/download.tsx

@@ -21,8 +21,8 @@ export const download = new Elysia()
         return redirect(`${WEBROOT}/results`, 302);
       }
       // parse from URL encoded string
-      const jobId = decodeURIComponent(params.jobId);
-      const fileName = sanitize(decodeURIComponent(params.fileName));
+      const jobId = params.jobId;
+      const fileName = sanitize(params.fileName);
 
       const filePath = `${outputDir}${userId}/${jobId}/${fileName}`;
       const file = Bun.file(filePath);

src/pages/root.tsx

@@ -33,30 +33,54 @@ export const root = new Elysia().use(userService).get(
     // validate jwt
     let user: ({ id: string } & JWTPayloadSpec) | false = false;
     if (ALLOW_UNAUTHENTICATED) {
-      const newUserId = String(
-        UNAUTHENTICATED_USER_SHARING
-          ? 0
-          : randomInt(2 ** 24, Math.min(2 ** 48 + 2 ** 24 - 1, Number.MAX_SAFE_INTEGER)),
-      );
-      const accessToken = await jwt.sign({
-        id: newUserId,
-      });
-
-      user = { id: newUserId };
-      if (!auth) {
-        return {
-          message: "No auth cookie, perhaps your browser is blocking cookies.",
-        };
+      // Check if the current cookie is already a valid session
+      if (auth?.value) {
+        try {
+          const verifiedUser = await jwt.verify(auth.value);
+          if (verifiedUser && verifiedUser.id) {
+            const numericId = Number.parseInt(verifiedUser.id);
+            // IDs below the anonymous range (excluding shared 0) are registered users
+            const isRegistered = numericId > 0 && numericId < 2 ** 24;
+            if (
+              !isRegistered ||
+              db.query("SELECT * FROM users WHERE id = ?").as(User).get(verifiedUser.id)
+            ) {
+              user = verifiedUser;
+            }
+          }
+        } catch {
+          // Invalid cookie, fall through to anonymous creation
+        }
       }
 
-      // set cookie
-      auth.set({
-        value: accessToken,
-        httpOnly: true,
-        secure: !HTTP_ALLOWED,
-        maxAge: 24 * 60 * 60,
-        sameSite: "strict",
-      });
+      // If no valid session found, create an anonymous user
+      if (!user) {
+        const newUserId = String(
+          UNAUTHENTICATED_USER_SHARING
+            ? 0
+            : randomInt(2 ** 24, Math.min(2 ** 48 + 2 ** 24 - 1, Number.MAX_SAFE_INTEGER)),
+        );
+        const accessToken = await jwt.sign({
+          id: newUserId,
+        });
+
+        user = { id: newUserId };
+
+        // Use existing cookie if it exists, otherwise set a new one
+        if (auth) {
+          auth.set({
+            value: accessToken,
+            httpOnly: true,
+            secure: !HTTP_ALLOWED,
+            maxAge: 24 * 60 * 60,
+            sameSite: "strict",
+          });
+        } else {
+          return {
+            message: "No auth cookie, perhaps your browser is blocking cookies.",
+          };
+        }
+      }
     } else if (auth?.value) {
       user = await jwt.verify(auth.value);
 

tests/converters/ffmpeg.test.ts

@@ -181,10 +181,7 @@ test("splits FFMPEG_ARGS on any whitespace", async () => {
   expect(calls[0]?.slice(0, 3)).toEqual(["-hide_banner", "-y", "-nostdin"]);
 });
 
-// BUG: split(/\s+/) keeps empty strings for leading/trailing whitespace
-// (" -y ".split(/\s+/) is ["", "-y", ""]), so ffmpeg receives "" as an argument
-// and fails. Remove `.failing` once src/converters/ffmpeg.ts drops empty entries.
-test.failing("ignores leading and trailing whitespace in FFMPEG_ARGS", async () => {
+test("ignores leading and trailing whitespace in FFMPEG_ARGS", async () => {
   process.env.FFMPEG_ARGS = " -y ";
   process.env.FFMPEG_OUTPUT_ARGS = " -preset veryfast ";
 

tests/converters/main.test.ts

@@ -148,11 +148,7 @@ describe("converter registry", () => {
     expect(getAllInputs("doesnotexist")).toEqual([]);
   });
 
-  // BUG: building allTargets/allInputs stores the first category's array by
-  // reference and then pushes the other categories into it. That mutates the
-  // converters' own properties, so e.g. LibreOffice offers docx -> xlsx and
-  // libjxl offers jxl -> jxl. Remove `.failing` once main.ts copies the arrays.
-  test.failing("listing all targets and inputs does not leak formats across categories", () => {
+  test("listing all targets and inputs does not leak formats across categories", () => {
     expect(getPossibleTargets("docx").libreoffice).not.toContain("xlsx");
     expect(getPossibleTargets("jxl").libjxl).not.toContain("jxl");
     expect(libreofficeProperties.from.text).not.toContain("xlsx");
@@ -251,10 +247,7 @@ describe("mainConverter", () => {
     },
   );
 
-  // BUG: discovery loops with `for (converterName in properties)` and only breaks
-  // the inner loop, so the variable always ends on the last registered converter.
-  // Remove `.failing` once mainConverter stops at the first match.
-  test.failing("logs the name of the auto-discovered converter", async () => {
+  test("logs the name of the auto-discovered converter", async () => {
     const inputPath = `${uploadsDir}named.vcf`;
     await writeFile(inputPath, vcard("Named"));
 
@@ -263,11 +256,7 @@ describe("mainConverter", () => {
     expect(output.logs.join("\n")).toContain("successfully using vcf");
   });
 
-  // BUG: same root cause - later matches overwrite earlier ones, so emf -> png
-  // uses ImageMagick even though Inkscape is registered first on purpose
-  // ("Prioritize Inkscape for EMF files"). The input does not exist, so whichever
-  // converter is picked fails fast; only the logged converter name matters.
-  test.failing("auto-discovery prefers the first registered converter", async () => {
+  test("auto-discovery prefers the first registered converter", async () => {
     await mainConverter(`${uploadsDir}missing.emf`, "emf", "png", `${outputDir}missing.png`);
 
     expect(output.errors.join("\n")).toContain("using inkscape");

tests/converters/vcf.test.ts

@@ -181,10 +181,7 @@ describe("toCSV", () => {
     expect(toCSV(contacts)).toBe('Full Name,Email\n"John Doe","john@example.com"\n"Jane Smith",""');
   });
 
-  // BUG: the header is built from the first contact only, so fields that only
-  // later contacts have are silently dropped from the export. Remove `.failing`
-  // once toCSV builds the header from all contacts.
-  test.failing("should not drop fields that only later contacts have", () => {
+  test("should not drop fields that only later contacts have", () => {
     const contacts = [{ "Full Name": "John Doe" }, { "Full Name": "Jane Smith", Email: "j@x.com" }];
 
     expect(toCSV(contacts)).toContain("j@x.com");

tests/pages/convert.test.ts

@@ -154,10 +154,7 @@ describe("POST /convert", () => {
     ]);
   });
 
-  // BUG: file_names is parsed and indexed before it is validated, so a value that is not a
-  // JSON array ends in a 500 instead of the redirect used for every other invalid input.
-  // Remove `.failing` once convert.tsx validates the parsed value first.
-  test.failing.each([
+  test.each([
     ["is not JSON", "not json"],
     ["is a JSON string", '"a.vcf"'],
   ])("redirects home when file_names %s", async (_, fileNames) => {

tests/pages/deleteFile.test.ts

@@ -79,11 +79,7 @@ describe("POST /delete", () => {
     expect(existsSync(`${other.dirs.uploads}keep.txt`)).toBe(true);
   });
 
-  // BUG: unlink errors are not handled, so deleting a file that does not exist answers
-  // with a 500 whose body contains the absolute upload path on the server. This also
-  // happens for every sanitized traversal attempt. Remove `.failing` once deleteFile.tsx
-  // handles the missing file.
-  test.failing.each(["missing.txt", "../../1/1/keep.txt"])(
+  test.each(["missing.txt", "../../1/1/keep.txt"])(
     "answers a missing file (%s) without a server error or server paths",
     async (filename) => {
       const { user, job } = await userWithUploads();

tests/pages/download.test.ts

@@ -111,11 +111,7 @@ describe("GET /download/:userId/:jobId/:fileName", () => {
     expect(response.status).toBe(404);
   });
 
-  // BUG: Elysia already decodes path parameters and download.tsx decodes them a second
-  // time, so file names containing "%" fail with a 500 "URI error" (and a literal "%20"
-  // would be turned into a space). The results page links to such files. Remove
-  // `.failing` once the second decodeURIComponent is dropped.
-  test.failing("serves file names containing a percent sign", async () => {
+  test("serves file names containing a percent sign", async () => {
     const { user, job } = await userWithOutputs({ "100%.csv": "percent" });
 
     const response = await request(`/download/${user.id}/${job}/100%25.csv`, {

tests/pages/root.test.ts

@@ -142,12 +142,7 @@ describe("GET / without accounts (ALLOW_UNAUTHENTICATED)", () => {
     expect(body).not.toContain('href="/logoff"');
   });
 
-  // BUG: the ALLOW_UNAUTHENTICATED branch runs before the session check, so every visit
-  // replaces the existing session (even a registered account's) with a new anonymous id
-  // and the user loses access to their previous jobs. This regressed the fix for #114
-  // ("keep unauthenticated user logged in if allowed", bc4ad49) in 538c5b6. Remove
-  // `.failing` once root.tsx verifies an existing session first.
-  test.failing("keeps an existing session", async () => {
+  test("keeps an existing session", async () => {
     setEnv({ ALLOW_UNAUTHENTICATED: true });
     const user = await createUser();