From e6a335bb0fcb4c09889abc009825cf4e93f7cc72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Krzeslak?= Date: Fri, 2 Oct 2026 15:27:37 +0200 Subject: [PATCH] test: apply suggested changes from review --- tests/converters/assimp.test.ts | 4 +++- tests/converters/calibre.test.ts | 7 +++--- tests/converters/libreoffice.test.ts | 2 +- tests/converters/main.test.ts | 36 +++++++++++++--------------- tests/converters/pandoc.test.ts | 8 ------- tests/db/db.test.ts | 16 +++++++++---- tests/pages/deleteFile.test.ts | 31 ++++++++++++++---------- tests/pages/results.test.ts | 16 ++++++++++++- tests/pages/root.test.ts | 9 ++++--- tests/preload.ts | 18 ++++++++++++++ 10 files changed, 94 insertions(+), 53 deletions(-) diff --git a/tests/converters/assimp.test.ts b/tests/converters/assimp.test.ts index 6face8cb..ff8bb244 100644 --- a/tests/converters/assimp.test.ts +++ b/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"] }, + ]); }); diff --git a/tests/converters/calibre.test.ts b/tests/converters/calibre.test.ts index 6d47baf3..021dd5c7 100644 --- a/tests/converters/calibre.test.ts +++ b/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) => { diff --git a/tests/converters/libreoffice.test.ts b/tests/converters/libreoffice.test.ts index b6304509..532b55ef 100644 --- a/tests/converters/libreoffice.test.ts +++ b/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)"]); }); diff --git a/tests/converters/main.test.ts b/tests/converters/main.test.ts index 11199a09..ceeaf0e5 100644 --- a/tests/converters/main.test.ts +++ b/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 () => { diff --git a/tests/converters/pandoc.test.ts b/tests/converters/pandoc.test.ts index d70642fd..7904ac85 100644 --- a/tests/converters/pandoc.test.ts +++ b/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/); - }); }); diff --git a/tests/db/db.test.ts b/tests/db/db.test.ts index 29a5f903..21f4ce58 100644 --- a/tests/db/db.test.ts +++ b/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 { diff --git a/tests/pages/deleteFile.test.ts b/tests/pages/deleteFile.test.ts index 93c6dbe8..1da8f61f 100644 --- a/tests/pages/deleteFile.test.ts +++ b/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); + }, + ); }); diff --git a/tests/pages/results.test.ts b/tests/pages/results.test.ts index 0550db78..44e8d8ec 100644 --- a/tests/pages/results.test.ts +++ b/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", () => { diff --git a/tests/pages/root.test.ts b/tests/pages/root.test.ts index f06a7e6c..9fcc11a8 100644 --- a/tests/pages/root.test.ts +++ b/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 () => { diff --git a/tests/preload.ts b/tests/preload.ts index f5aef700..7fd7470e 100644 --- a/tests/preload.ts +++ b/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