test: apply suggested changes from review

tests/converters/assimp.test.ts

@@ -21,5 +21,7 @@ test("passes format ids that are not file extensions through unchanged", async (
 
   await convert("in/scene.obj", "obj", "glb2", "out/scene.glb", undefined, execFile);
 
-  expect(calls[0]?.args).toEqual(["export", "in/scene.obj", "out/scene.glb", "-fglb2"]);
+  expect(calls).toEqual([
+    { cmd: "assimp", args: ["export", "in/scene.obj", "out/scene.glb", "-fglb2"] },
+  ]);
 });

tests/converters/calibre.test.ts

@@ -14,9 +14,10 @@ test("invokes ebook-convert with input and target path", async () => {
 });
 
 test.each([
-  ["recipe file type", "in/news.recipe", "recipe"],
-  ["downloaded_recipe file type", "in/news.downloaded_recipe", "downloaded_recipe"],
-  ["upper-case file type", "in/news.RECIPE", "RECIPE"],
+  // the extension is harmless, so only the file type triggers the rejection
+  ["recipe file type", "in/news.txt", "recipe"],
+  ["downloaded_recipe file type", "in/news.txt", "downloaded_recipe"],
+  ["upper-case file type", "in/news.txt", "RECIPE"],
   ["recipe extension with another file type", "in/news.recipe", "txt"],
   ["upper-case downloaded_recipe extension", "in/news.DOWNLOADED_RECIPE", "txt"],
 ])("rejects recipe input (%s) without invoking ebook-convert", async (_, filePath, fileType) => {

tests/converters/libreoffice.test.ts

@@ -270,7 +270,7 @@ test("getFilters returns text filters when both formats are text formats", () =>
   expect(getFilters("doc", "odt")).toEqual(["MS Word 97", "writer8"]);
 });
 
-test("getFilters falls back to calc filters when the target is not a text format", () => {
+test("getFilters falls back to calc filters when the source is not a text format", () => {
   expect(getFilters("xls", "csv")).toEqual(["MS Excel 97", "Text - txt - csv (StarCalc)"]);
 });
 

tests/converters/main.test.ts

@@ -5,18 +5,19 @@ import { tmpdir } from "node:os";
 import { join } from "node:path";
 import type { Cookie } from "elysia";
 import { properties as libreofficeProperties } from "../../src/converters/libreoffice";
-import {
-  chunks,
-  getAllInputs,
-  getAllTargets,
-  getPossibleTargets,
-  handleConvert,
-  mainConverter,
-} from "../../src/converters/main";
-// tests/preload.ts points DB_PATH at a temporary database, so production data is never used
-import db from "../../src/db/db";
 import { captureConsole } from "./helpers/converters";
 
+if (!process.env.CONVERTX_TEST_ROOT) {
+  // Without tests/preload.ts the db module would open ./data/mydb.sqlite and these tests
+  // would write their rows into it.
+  throw new Error("Converter tests must run via `bun test` so that tests/preload.ts is loaded.");
+}
+
+// dynamic imports ensure that the guard above runs before the db module is loaded
+const { default: db } = await import("../../src/db/db");
+const { chunks, getAllInputs, getAllTargets, getPossibleTargets, handleConvert, mainConverter } =
+  await import("../../src/converters/main");
+
 const testRoot = mkdtempSync(join(tmpdir(), "convertx-main-test-"));
 
 // Every row written by these tests uses this prefix, so cleanup never touches other rows.
@@ -38,10 +39,6 @@ function rowsFor(name: string, orderBy: "id" | "file_name" = "id"): FileRow[] {
     .all(`${JOB_PREFIX}${name}`) as FileRow[];
 }
 
-function countAllRows(): number {
-  return (db.query("SELECT COUNT(*) AS count FROM file_names").get() as { count: number }).count;
-}
-
 const vcard = (fullName: string) => `BEGIN:VCARD\nFN:${fullName}\nEND:VCARD\n`;
 
 let caseCounter = 0;
@@ -297,13 +294,14 @@ describe("handleConvert", () => {
   });
 
   test("converts but does not record rows when the job id cookie has no value", async () => {
-    await writeFile(`${uploadsDir}contact.vcf`, vcard("No Job"));
-    const rowsBefore = countAllRows();
+    // without a job id there is no job_id to look for, so look for the file name instead
+    const fileName = `${JOB_PREFIX}no-job.vcf`;
+    await writeFile(`${uploadsDir}${fileName}`, vcard("No Job"));
 
-    await handleConvert(["contact.vcf"], uploadsDir, outputDir, "csv", "vcf", jobCookie());
+    await handleConvert([fileName], uploadsDir, outputDir, "csv", "vcf", jobCookie());
 
-    expect(countAllRows()).toBe(rowsBefore);
-    expect(await readFile(`${outputDir}contact.csv`, "utf-8")).toContain("No Job");
+    expect(db.query("SELECT * FROM file_names WHERE file_name = ?").all(fileName)).toEqual([]);
+    expect(await readFile(`${outputDir}${JOB_PREFIX}no-job.csv`, "utf-8")).toContain("No Job");
   });
 
   test("replaces only the last occurrence of the extension", async () => {

tests/converters/pandoc.test.ts

@@ -51,12 +51,4 @@ describe("convert", () => {
 
     expect(calls[0]?.args.some((arg) => arg.startsWith("--pdf-engine"))).toBe(false);
   });
-
-  test("should reject if execFile returns an error", async () => {
-    const { execFile } = createMockExecFile({ error: new Error("fail") });
-
-    await expect(
-      convert("input.md", "markdown", "html", "output.html", undefined, execFile),
-    ).rejects.toMatch(/error: Error: fail/);
-  });
 });

tests/db/db.test.ts

@@ -1,8 +1,15 @@
 import { test, expect, beforeEach, afterEach } from "bun:test";
 import { Database } from "bun:sqlite";
-import { unlinkSync, existsSync, mkdirSync } from "node:fs";
+import { unlinkSync, existsSync } from "node:fs";
+import { join } from "node:path";
 
-// tests/preload.ts points DB_PATH at a temporary database, so production data is never used
+const testRoot = process.env.CONVERTX_TEST_ROOT;
+if (!testRoot) {
+  // Without tests/preload.ts the db module would open and migrate ./data/mydb.sqlite.
+  throw new Error("Database tests must run via `bun test` so that tests/preload.ts is loaded.");
+}
+
+// the dynamic import ensures that the guard above runs before the db module is loaded
 let initializeDatabase: (db: Database) => void;
 let defaultDb: Database | undefined;
 await import("../../src/db/db").then((mod) => {
@@ -58,8 +65,7 @@ let testDbPath: string;
 let testDb: Database;
 
 beforeEach(() => {
-  mkdirSync("./data", { recursive: true });
-  testDbPath = `./data/test-db-${Date.now()}.sqlite`;
+  testDbPath = join(testRoot, `test-db-${Date.now()}.sqlite`);
   testDb = new Database(testDbPath, { create: true });
   // Now uses the real initialization logic from db.ts
   initializeDatabase(testDb);
@@ -105,7 +111,7 @@ test("db initializes and creates tables on first run", () => {
 
 test("db handles migration from version 0 to version 1", () => {
   testDb.close();
-  const migrateDbPath = `./data/test-db-migrate-${Date.now()}.sqlite`;
+  const migrateDbPath = join(testRoot, `test-db-migrate-${Date.now()}.sqlite`);
   const migrateDb = new Database(migrateDbPath, { create: true });
 
   try {

tests/pages/deleteFile.test.ts

@@ -68,26 +68,33 @@ describe("POST /delete", () => {
     const { user, job } = await userWithUploads();
     const other = await userWithUploads("keep.txt");
 
-    await request("/delete", {
+    const response = await request("/delete", {
       cookies: { auth: user.token, jobId: job },
       json: { filename: `../../${other.user.id}/${other.job}/keep.txt` },
     });
 
+    // the request passed the session and job checks, so the file name was really used;
+    // the sanitized name does not exist, which runs into the bug tracked below
+    expect(response.status).not.toBeOneOf([302, 401]);
     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. Remove
-  // `.failing` once deleteFile.tsx handles the missing file.
-  test.failing("answers a missing file without a server error or server paths", async () => {
-    const { user, job } = await userWithUploads();
+  // 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"])(
+    "answers a missing file (%s) without a server error or server paths",
+    async (filename) => {
+      const { user, job } = await userWithUploads();
 
-    const response = await request("/delete", {
-      cookies: { auth: user.token, jobId: job },
-      json: { filename: "missing.txt" },
-    });
+      const response = await request("/delete", {
+        cookies: { auth: user.token, jobId: job },
+        json: { filename },
+      });
 
-    expect(response.status).toBeLessThan(500);
-    expect(await response.text()).not.toContain(uploadsDir);
-  });
+      expect(response.status).toBeLessThan(500);
+      expect(await response.text()).not.toContain(uploadsDir);
+    },
+  );
 });

tests/pages/results.test.ts

@@ -143,7 +143,7 @@ describe.each([
     expect(body).toContain(`href="/convertx/archive/${job}"`);
   });
 
-  test("does not set cookies", async () => {
+  test("keeps the session and the current job", async () => {
     const user = await createUser();
     const job = createJob(user.id);
 
@@ -155,6 +155,20 @@ describe.each([
     expect(setCookie(response, "jobId")).toBeUndefined();
     expect(setCookie(response, "auth")).toBeUndefined();
   });
+
+  // no page sets job_id anymore (the job cookie is called jobId), but the handlers still
+  // clear it for browsers that kept it
+  test("clears the legacy job_id cookie", async () => {
+    const user = await createUser();
+    const job = createJob(user.id);
+
+    const response = await request(`${route}/${job}`, {
+      method,
+      cookies: { auth: user.token, job_id: job },
+    });
+
+    expect(setCookie(response, "job_id")).toContain("Max-Age=0");
+  });
 });
 
 describe("GET /results/:jobId", () => {

tests/pages/root.test.ts

@@ -8,7 +8,6 @@ import {
   setCookie,
   setCookieValue,
   setEnv,
-  signToken,
 } from "./helpers/app";
 
 afterEach(() => {
@@ -42,12 +41,16 @@ describe("GET / with accounts", () => {
   });
 
   test("clears the session of an account that no longer exists", async () => {
-    const response = await request("/", { cookies: { auth: await signToken(999_999) } });
+    const user = await createUser();
+    db.query("DELETE FROM users WHERE id = ?").run(user.id);
+
+    const response = await request("/", { cookies: { auth: user.token } });
 
     expect(response.status).toBe(302);
     expect(response.headers.get("location")).toBe("/login");
     expect(setCookie(response, "auth")).toContain("Max-Age=0");
-    expect(jobsOf(999_999)).toEqual([]);
+    // no job is started for the deleted account
+    expect(jobsOf(user.id)).toEqual([]);
   });
 
   test("starts a new job and remembers it in the jobId cookie", async () => {

tests/preload.ts

@@ -10,6 +10,24 @@ const testRoot = mkdtempSync(join(tmpdir(), "convertx-tests-"));
 process.env.DB_PATH = join(testRoot, "test.sqlite");
 process.env.CONVERTX_TEST_ROOT = testRoot;
 
+// Settings from the developer's shell or a .env file (which Bun loads automatically) must
+// not change what the tests see, e.g. WEBROOT would move every page. src/helpers/env.ts
+// reads them once at import; tests switch them with setEnv() from tests/pages/helpers/app.ts.
+for (const name of [
+  "ACCOUNT_REGISTRATION",
+  "HTTP_ALLOWED",
+  "ALLOW_UNAUTHENTICATED",
+  "AUTO_DELETE_EVERY_N_HOURS",
+  "HIDE_HISTORY",
+  "BRANDING",
+  "WEBROOT",
+  "LANGUAGE",
+  "MAX_CONVERT_PROCESS",
+  "UNAUTHENTICATED_USER_SHARING",
+]) {
+  delete process.env[name];
+}
+
 // Several pages import uploadsDir/outputDir from src/index.tsx. Importing the real module
 // would start the server, spawn every converter's --version and schedule the job cleanup,
 // and its relative ./data paths would mix test files with real conversions. Provide the