fix(doc-engine): keep protect-pdf passwords out of qpdf's argv (#708)

qpdf expands argument files before parsing options, so a bare positional password
beginning with @ was resolved as a path and the file's contents became the
encryption key: exit 0, and the user's own password no longer opened the PDF.

Drive the encrypt through a job-JSON file so neither password reaches argv. The
=-joined flag form needs qpdf 11.7+, and the released image carries 11.3.0 which
rejects it; job JSON works on both (verified 11.3.0 and 12.1.0, R = 6 each).

packages/doc-engine/src/pdf-ops.ts

@@ -1,3 +1,7 @@
+import { randomUUID } from "node:crypto";
+import { rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
 import { runQpdf } from "./qpdf.js";
 
 // qpdf page ranges: digits, commas, hyphens, r-prefixed (r1 = last), and z (last page).
@@ -41,14 +45,31 @@ export async function qpdfRotate(
 }
 
 /*
- * Security note: passwords are passed as argv elements to spawn() (no shell).
- * They are visible in /proc/<pid>/cmdline for the ~1s process lifetime. This
- * is acceptable for the single-tenant container threat model. If multi-tenant
- * isolation is ever needed, switch to qpdf's --password-file or @argfile
- * syntax with a 0600 temp file in the scratch dir, deleted in a finally block.
+ * Security note: qpdfEncrypt passes its passwords through a 0600 job-JSON file that
+ * is unlinked in a finally block, so they stay out of argv. qpdfDecrypt still passes
+ * one as an argv element to spawn() (no shell), visible in /proc/<pid>/cmdline for
+ * the ~1s process lifetime, which is acceptable for the single-tenant container
+ * threat model. Its `--password=` form is a single token, so the argument-file
+ * pre-pass described below cannot fire on it.
  */
 
-/** AES-256 encrypt with user + owner passwords (qpdf --encrypt user owner 256 --). */
+/**
+ * AES-256 encrypt with user + owner passwords, via a qpdf job-JSON file.
+ *
+ * The passwords deliberately never appear in argv. qpdf runs an argument-file
+ * pre-pass over every argv element before it parses options, so a bare positional
+ * password beginning with `@` is resolved as a path and that file's lines are
+ * spliced into qpdf's own argv. A single-line file then encrypts the document under
+ * that file's contents at exit 0, handing the user a PDF their own password does not
+ * open, and a multi-line one shifts the arguments enough to surface part of the file
+ * in qpdf's error text.
+ *
+ * The `=`-joined flag form (`--encrypt --user-password=...`) also avoids this, but
+ * only on qpdf 11.7 and newer; the shipped image carries 11.3, which rejects it
+ * outright. Job JSON is accepted by both (verified against 11.3.0 and 12.1.0) and
+ * has the side benefit of keeping the passwords out of /proc/<pid>/cmdline, which
+ * the note above asks for.
+ */
 export async function qpdfEncrypt(
   inputPath: string,
   userPassword: string,
@@ -57,10 +78,23 @@ export async function qpdfEncrypt(
 ): Promise<void> {
   assertPassword(userPassword);
   assertPassword(ownerPassword);
-  await runQpdf(
-    [inputPath, "--encrypt", userPassword, ownerPassword, "256", "--", outPath],
-    60_000,
+
+  const jobPath = join(tmpdir(), `snapotter-qpdf-job-${randomUUID()}.json`);
+  await writeFile(
+    jobPath,
+    JSON.stringify({
+      inputFile: inputPath,
+      outputFile: outPath,
+      encrypt: { userPassword, ownerPassword, "256bit": {} },
+    }),
+    { mode: 0o600 },
   );
+
+  try {
+    await runQpdf([`--job-json-file=${jobPath}`], 60_000);
+  } finally {
+    await rm(jobPath, { force: true });
+  }
 }
 
 /** Decrypt with a known password; qpdf rejects wrong passwords with exit 2. */

packages/doc-engine/tests/pdf-ops.test.ts

@@ -1,5 +1,8 @@
 import { spawn } from "node:child_process";
+import { existsSync } from "node:fs";
+import { readFile } from "node:fs/promises";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { settleClose } from "./helpers/fake-child.js";
 import { makeSpawnHelpers } from "./helpers/spawn-capture.js";
 
 vi.mock("node:child_process", () => ({ spawn: vi.fn() }));
@@ -137,20 +140,54 @@ describe("qpdfRotate", () => {
 });
 
 describe("qpdfEncrypt", () => {
-  it("builds <in> --encrypt <user> <owner> 256 -- <out>", async () => {
+  it("drives qpdf through a job-JSON file rather than argv", async () => {
     h.nextClose({ code: 0 });
     await import("../src/pdf-ops.js").then((m) =>
       m.qpdfEncrypt("/in.pdf", "userpw", "ownerpw", "/out.pdf"),
     );
-    expect(h.lastArgs()).toEqual([
-      "/in.pdf",
-      "--encrypt",
-      "userpw",
-      "ownerpw",
-      "256",
-      "--",
-      "/out.pdf",
-    ]);
+    const args = h.lastArgs();
+    expect(args).toHaveLength(1);
+    expect(args[0]).toMatch(/^--job-json-file=.*\.json$/);
+  });
+
+  // qpdf runs an argument-file pre-pass over every argv element before it parses
+  // options, so a bare positional value starting with `@` is read as a path and that
+  // file's lines are spliced into qpdf's own argv. A one-line file then encrypts the
+  // document under the file's contents, at exit 0, instead of the chosen password.
+  // Keeping passwords out of argv entirely is what closes it.
+  it.each([
+    ["argument-file sigil", "@/etc/hostname"],
+    ["leading dashes", "--allow-insecure"],
+  ])("never puts a password with %s into argv", async (_label, password) => {
+    h.nextClose({ code: 0 });
+    await import("../src/pdf-ops.js").then((m) =>
+      m.qpdfEncrypt("/in.pdf", password, "ownerpw", "/out.pdf"),
+    );
+    expect(JSON.stringify(h.lastArgs())).not.toContain(password);
+  });
+
+  it("writes the literal passwords into the job file and removes it afterwards", async () => {
+    // A child that never settles on its own, so the job file can be read while qpdf
+    // is notionally still running. It is unlinked once the promise resolves.
+    const child = h.nextManual();
+    const { qpdfEncrypt } = await import("../src/pdf-ops.js");
+    const pending = qpdfEncrypt("/in.pdf", "@/etc/hostname", "ownerpw", "/out.pdf");
+
+    await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalled());
+    const jobPath = h.lastArgs()[0].replace("--job-json-file=", "");
+    const contents = JSON.parse(await readFile(jobPath, "utf8"));
+
+    settleClose(child, { code: 0 });
+    await pending;
+
+    expect(contents.inputFile).toBe("/in.pdf");
+    expect(contents.outputFile).toBe("/out.pdf");
+    expect(contents.encrypt).toMatchObject({
+      userPassword: "@/etc/hostname",
+      ownerPassword: "ownerpw",
+      "256bit": {},
+    });
+    expect(existsSync(jobPath)).toBe(false);
   });
 
   it("rejects an empty user password before spawning", async () => {

tests/integration/tools/document/protect-pdf.test.ts

@@ -1,3 +1,7 @@
+import { randomUUID } from "node:crypto";
+import { rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
 import { qpdfAvailable } from "@snapotter/doc-engine";
 import { eq } from "drizzle-orm";
 import { afterAll, beforeAll, describe, expect, it } from "vitest";
@@ -80,4 +84,49 @@ describe.skipIf(!qpdfAvailable())("protect-pdf (requires qpdf)", () => {
     const res = await runTool({ userPassword: "" });
     expect(res.statusCode).toBe(400);
   }, 60_000);
+
+  // Round-trips a password whose first character is qpdf's argument-file sigil.
+  // Passed as a bare positional, qpdf resolves it as a path and encrypts under
+  // that file's contents instead, so the user's own password no longer opens the
+  // document. Unlocking with the literal string is what proves it stayed literal.
+  it("treats a password starting with the argument-file sigil as a literal", async () => {
+    const canaryPath = join(tmpdir(), `snapotter-qpdf-canary-${randomUUID()}.txt`);
+    await writeFile(canaryPath, "CANARY_FILE_CONTENTS\n");
+
+    try {
+      const literalPassword = `@${canaryPath}`;
+      const res = await runTool({ userPassword: literalPassword });
+      expect(res.statusCode).toBe(200);
+
+      const dl = await testApp.app.inject({
+        method: "GET",
+        url: JSON.parse(res.body).downloadUrl,
+      });
+      expect(dl.statusCode).toBe(200);
+
+      const unlockWith = async (password: string) => {
+        const { body, contentType } = createMultipartPayload([
+          {
+            name: "file",
+            filename: "locked.pdf",
+            contentType: "application/pdf",
+            content: dl.rawPayload,
+          },
+          { name: "settings", content: JSON.stringify({ password }) },
+        ]);
+        return testApp.app.inject({
+          method: "POST",
+          url: "/api/v1/tools/pdf/unlock-pdf",
+          headers: { authorization: `Bearer ${adminToken}`, "content-type": contentType },
+          body,
+        });
+      };
+
+      expect((await unlockWith(literalPassword)).statusCode).toBe(200);
+      // The canary's contents must never have become the encryption key.
+      expect((await unlockWith("CANARY_FILE_CONTENTS")).statusCode).not.toBe(200);
+    } finally {
+      await rm(canaryPath, { force: true });
+    }
+  }, 90_000);
 });