From 13e0f9b4c8bc3389016b7e4d6c172f9e8e2ec986 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 29 May 2026 07:14:52 -0700 Subject: [PATCH] harden(make-pdf): emoji gate + font install per adversarial review Codex adversarial pass on the implementation diff flagged five robustness gaps, all fixed here: - emoji-gate skipped green in CI when poppler/font prerequisites were absent, which could let the tofu regression ship behind a green build. Missing prerequisites are now a HARD FAILURE when process.env.CI is set; local dev still skips cleanly. - execFileSync children (make-pdf, pdffonts, pdftoppm, fc-match) had no timeout; a wedged binary or hostile GSTACK_*_BIN override could hang the job past Bun's test timeout. Each child now has a 25s ceiling. - PPM parser trusted header tokens blindly; malformed/variant output gave a silently-wrong count. Now validates magic/dimensions/maxval and pixel-buffer length, handles header comments, throws a hard diagnostic on mismatch. - predictable /tmp paths were collision/symlink-prone; now mkdtempSync under /tmp (kept under /tmp for browse's validateOutputPath allowlist). - only apt-get update was timeout-wrapped; dnf/pacman/apk installs and apt install can hang on locks/mirrors. All package installs now timeout-bound. Co-Authored-By: Claude Opus 4.8 (1M context) --- make-pdf/test/e2e/emoji-gate.test.ts | 73 ++++++++++++++++++++++------ setup | 10 ++-- test/setup-emoji-font.test.ts | 6 +++ 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/make-pdf/test/e2e/emoji-gate.test.ts b/make-pdf/test/e2e/emoji-gate.test.ts index ab9713a9c..0e3a42c29 100644 --- a/make-pdf/test/e2e/emoji-gate.test.ts +++ b/make-pdf/test/e2e/emoji-gate.test.ts @@ -24,7 +24,10 @@ * * Gating: runs only when the compiled binary + browse + pdffonts + pdftoppm are * available AND a color-emoji font is installed for Chromium to fall back to. - * Skipped cleanly otherwise (local dev before ./setup installs the font). + * In CI (process.env.CI set) missing prerequisites are a HARD FAILURE, not a + * skip — CI is expected to install poppler-utils + fonts-noto-color-emoji, so a + * silent skip there would let the tofu regression ship behind a green build. + * Local dev without those tools skips cleanly. */ import { describe, expect, test } from "bun:test"; @@ -45,6 +48,11 @@ const SATURATED_PIXEL_FLOOR = 200; // A pixel is "colored" when its max-min channel spread exceeds this. Black text, // gray rules, and white background all stay near 0; color emoji spike high. const SATURATION_DELTA = 40; +// Per-child wall-clock bound. Bun's test timeout doesn't reliably interrupt a +// synchronous execFileSync, so each child gets its own ceiling — a wedged +// browser/poppler binary (or a hostile GSTACK_*_BIN override) fails instead of +// hanging the whole job. +const CHILD_TIMEOUT_MS = 25_000; /** Is a color-emoji font available for Chromium to fall back to? */ function emojiFontAvailable(): boolean { @@ -58,7 +66,7 @@ function emojiFontAvailable(): boolean { const out = execFileSync( fcMatch, ["-f", "%{color}\n", ":lang=und-zsye:charset=1F600"], - { encoding: "utf8" }, + { encoding: "utf8", timeout: CHILD_TIMEOUT_MS }, ); return /true/i.test(out); } catch { @@ -78,24 +86,49 @@ function prerequisitesAvailable(): { ok: true } | { ok: false; reason: string } return { ok: true }; } -/** Count pixels in a P6 (binary) PPM whose RGB channel spread exceeds delta. */ +/** + * Count pixels in a P6 (binary) PPM whose RGB channel spread exceeds delta. + * Validates the header and buffer length so malformed/variant output is a hard + * diagnostic (thrown), never a silently-wrong count. + */ function countSaturatedPixels(ppmPath: string, delta: number): number { const b = fs.readFileSync(ppmPath); let i = 0; + const skipWhitespaceAndComments = () => { + for (;;) { + while (i < b.length && (b[i] === 0x20 || b[i] === 0x0a || b[i] === 0x09 || b[i] === 0x0d)) i++; + if (b[i] === 0x23) { // '#': comment runs to end of line + while (i < b.length && b[i] !== 0x0a) i++; + continue; + } + break; + } + }; const token = (): string => { - while (b[i] === 0x20 || b[i] === 0x0a || b[i] === 0x09 || b[i] === 0x0d) i++; + skipWhitespaceAndComments(); const s = i; while (i < b.length && b[i] !== 0x20 && b[i] !== 0x0a && b[i] !== 0x09 && b[i] !== 0x0d) i++; return b.slice(s, i).toString("ascii"); }; const magic = token(); - if (magic !== "P6") throw new Error(`expected P6 PPM, got ${magic}`); + if (magic !== "P6") throw new Error(`expected P6 PPM, got "${magic}"`); const w = Number(token()); const h = Number(token()); - token(); // maxval - i++; // single whitespace byte after the maxval precedes the pixel block - let sat = 0; + const maxval = Number(token()); + if (!Number.isInteger(w) || w <= 0 || !Number.isInteger(h) || h <= 0) { + throw new Error(`invalid PPM dimensions: ${w}x${h}`); + } + if (maxval !== 255) { + // pdftoppm emits 8-bit P6 (maxval 255). 16-bit would be 2 bytes/channel and + // would break the byte math below — fail loudly rather than miscount. + throw new Error(`unexpected PPM maxval ${maxval} (expected 255)`); + } + i++; // single whitespace byte after maxval precedes the pixel block const total = w * h; + if (b.length - i < total * 3) { + throw new Error(`PPM pixel buffer too short: have ${b.length - i}, need ${total * 3}`); + } + let sat = 0; for (let p = 0; p < total; p++) { const o = i + p * 3; const r = b[o], g = b[o + 1], bl = b[o + 2]; @@ -109,21 +142,26 @@ describe("emoji render gate", () => { test.skipIf(!avail.ok)("emoji render as color glyphs, not tofu", () => { if (!avail.ok) return; // type narrowing - const outputPdf = `/tmp/make-pdf-emoji-gate-${process.pid}.pdf`; - const ppmPrefix = `/tmp/make-pdf-emoji-gate-${process.pid}`; + // Private temp dir under /tmp: browse's validateOutputPath only allows + // /tmp and /private/tmp (not os.tmpdir()'s /var/folders), and mkdtemp + // dodges the predictable-path symlink/collision risk. + const workDir = fs.mkdtempSync("/tmp/make-pdf-emoji-gate-"); + const outputPdf = path.join(workDir, "out.pdf"); + const ppmPrefix = path.join(workDir, "page"); const ppmPath = `${ppmPrefix}.ppm`; try { execFileSync(PDF_BIN, ["generate", FIXTURE, outputPdf, "--quiet"], { encoding: "utf8", env: { ...process.env, BROWSE_BIN }, stdio: ["ignore", "pipe", "pipe"], + timeout: CHILD_TIMEOUT_MS, }); expect(fs.existsSync(outputPdf)).toBe(true); // 1. An emoji family must be embedded — the cascade found a real emoji // font instead of falling through to .notdef. const pdffonts = resolvePopplerTool("pdffonts")!; - const fontList = execFileSync(pdffonts, [outputPdf], { encoding: "utf8" }); + const fontList = execFileSync(pdffonts, [outputPdf], { encoding: "utf8", timeout: CHILD_TIMEOUT_MS }); if (!/emoji/i.test(fontList)) { process.stderr.write(`\n--- pdffonts ---\n${fontList}\n--- END ---\n`); } @@ -133,6 +171,7 @@ describe("emoji render gate", () => { const pdftoppm = resolvePopplerTool("pdftoppm")!; execFileSync(pdftoppm, ["-r", "100", "-singlefile", outputPdf, ppmPrefix], { stdio: ["ignore", "pipe", "pipe"], + timeout: CHILD_TIMEOUT_MS, }); expect(fs.existsSync(ppmPath)).toBe(true); const saturated = countSaturatedPixels(ppmPath, SATURATION_DELTA); @@ -141,13 +180,17 @@ describe("emoji render gate", () => { } expect(saturated).toBeGreaterThanOrEqual(SATURATED_PIXEL_FLOOR); } finally { - try { fs.unlinkSync(outputPdf); } catch { /* ignore */ } - try { fs.unlinkSync(ppmPath); } catch { /* ignore */ } + try { fs.rmSync(workDir, { recursive: true, force: true }); } catch { /* ignore */ } } - }, 30000); + }, 60000); if (!avail.ok) { - test("prerequisites check", () => { + // In CI, missing prerequisites are a hard failure — a silent skip would let + // the Linux tofu regression ship behind a green build. Locally, just warn. + test("emoji gate prerequisites are present (hard-required in CI)", () => { + if (process.env.CI) { + throw new Error(`emoji gate prerequisites missing in CI: ${avail.reason}`); + } console.warn(`[skip] ${avail.reason}`); }); } diff --git a/setup b/setup index abd1c5969..063fe5b77 100755 --- a/setup +++ b/setup @@ -296,19 +296,21 @@ ensure_emoji_font() { sudo="sudo -n" fi + # Every package-manager call is wrapped in `timeout` so a stuck dpkg/rpm lock + # or a wedged mirror fails fast into the warn path instead of hanging setup. if command -v apt-get >/dev/null 2>&1; then echo "Installing color-emoji font (fonts-noto-color-emoji) so make-pdf emoji render (set GSTACK_SKIP_FONTS=1 to skip)..." DEBIAN_FRONTEND=noninteractive timeout 30 $sudo apt-get update -qq >/dev/null 2>&1 || true - DEBIAN_FRONTEND=noninteractive $sudo apt-get install -y -qq fonts-noto-color-emoji >/dev/null 2>&1 || return 1 + DEBIAN_FRONTEND=noninteractive timeout 120 $sudo apt-get install -y -qq fonts-noto-color-emoji >/dev/null 2>&1 || return 1 elif command -v dnf >/dev/null 2>&1; then echo "Installing color-emoji font (google-noto-color-emoji-fonts)..." - $sudo dnf install -y google-noto-color-emoji-fonts >/dev/null 2>&1 || return 1 + timeout 120 $sudo dnf install -y google-noto-color-emoji-fonts >/dev/null 2>&1 || return 1 elif command -v pacman >/dev/null 2>&1; then echo "Installing color-emoji font (noto-fonts-emoji)..." - $sudo pacman -Sy --noconfirm noto-fonts-emoji >/dev/null 2>&1 || return 1 + timeout 120 $sudo pacman -Sy --noconfirm noto-fonts-emoji >/dev/null 2>&1 || return 1 elif command -v apk >/dev/null 2>&1; then echo "Installing color-emoji font (font-noto-emoji)..." - $sudo apk add --no-cache font-noto-emoji >/dev/null 2>&1 || return 1 + timeout 120 $sudo apk add --no-cache font-noto-emoji >/dev/null 2>&1 || return 1 else return 1 fi diff --git a/test/setup-emoji-font.test.ts b/test/setup-emoji-font.test.ts index 83475387d..7e8668c2d 100644 --- a/test/setup-emoji-font.test.ts +++ b/test/setup-emoji-font.test.ts @@ -52,6 +52,12 @@ describe('setup: ensure_emoji_font static invariants', () => { test('install path is non-interactive and timeout-guarded', () => { expect(helper).toContain('DEBIAN_FRONTEND=noninteractive'); expect(helper).toMatch(/timeout 30 .*apt-get update/); + // Every package-manager INSTALL (not just apt update) must be timeout-bound + // so a stuck lock/mirror fails fast instead of hanging setup. + expect(helper).toMatch(/timeout \d+ .*apt-get install/); + expect(helper).toMatch(/timeout \d+ .*dnf install/); + expect(helper).toMatch(/timeout \d+ .*pacman -Sy/); + expect(helper).toMatch(/timeout \d+ .*apk add/); }); test('covers all four package managers with the correct package names', () => {