From 1d130e2446ef2e8599575a160b5364623631650c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 12 Jun 2026 09:38:52 -0700 Subject: [PATCH] feat(make-pdf): out-of-tree image reads warn; --strict makes them fatal (D8.1) Local CLI semantics stay (absolute paths and ../ still inline, like pandoc), but never silently: an agent PDF-ing untrusted markdown can't quietly embed a file from outside the input directory into a shareable document without a visible warning, and --strict pipelines hard-fail. Two unit tests. Also: TODOS.md gains the deferred e2e-harness dedup entry (D8.2). Co-Authored-By: Claude Fable 5 --- TODOS.md | 18 +++++++++++++++ make-pdf/src/diagram-prepass.ts | 10 +++++++++ make-pdf/test/diagram-prepass.test.ts | 32 ++++++++++++++++++++++++++- 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/TODOS.md b/TODOS.md index 02fc88565..52e806af3 100644 --- a/TODOS.md +++ b/TODOS.md @@ -2397,3 +2397,21 @@ building once users hit multi-diagram docs; wedge perf is fine without it. **Effort:** S (human ~1d, CC ~30min). **Depends on:** diagram engine wedge shipping (lib/diagram-render bundle versioning). + +### P3: Dedupe the make-pdf e2e gate-test harness + +**What:** Five e2e files (`combined-gate`, `emoji-gate`, `diagram-gate`, +`landscape-gate`, `format-gate`) each hand-roll the same prerequisite probe +(binary/browse/poppler checks with CI hard-fail vs local skip), mkdtemp/rm +lifecycle, and child-timeout constants. Extract a shared +`make-pdf/test/e2e/helpers.ts` (prerequisites(), withWorkDir(), runGenerate()). + +**Why:** Review-army maintainability finding on v1.58.0.0 — the boilerplate +diverges a little more with each new gate (diagram-gate now captures stderr +via Bun.spawnSync while the others use execFileSync), and a future fix to the +CI-hard-fail contract has to land five times. + +**Context:** Deferred at ship time (D8.2) because it's test-only churn across +five green files at the tail of a release. Zero user-facing value; pure DRY. + +**Effort:** S (human ~3h, CC ~20min). **Depends on:** None. diff --git a/make-pdf/src/diagram-prepass.ts b/make-pdf/src/diagram-prepass.ts index 361806799..8d6c6a48c 100644 --- a/make-pdf/src/diagram-prepass.ts +++ b/make-pdf/src/diagram-prepass.ts @@ -619,6 +619,16 @@ export function inlineLocalImages(html: string, opts: PrepassImageOptions): stri const cached = memo.get(filePath); if (cached !== undefined) return rewriteImgTag(tag, cached); + // Out-of-tree reads are legal (local CLI semantics — like pandoc) but + // never silent: an agent PDF-ing untrusted markdown should not quietly + // embed ~/.ssh/config into a shareable document. --strict makes it fatal. + const inputRoot = path.resolve(opts.inputDir) + path.sep; + if (!filePath.startsWith(inputRoot)) { + const msg = `image resolves OUTSIDE the input directory: ${src} → ${filePath}`; + if (opts.strict) throw new StrictModeError(msg + " — move it under the markdown's directory or drop --strict"); + opts.warn(msg); + } + if (!fs.existsSync(filePath)) { const msg = `image not found: ${src} (resolved to ${filePath})`; if (opts.strict) throw new StrictModeError(msg); diff --git a/make-pdf/test/diagram-prepass.test.ts b/make-pdf/test/diagram-prepass.test.ts index 4d49485cb..c589dbdf1 100644 --- a/make-pdf/test/diagram-prepass.test.ts +++ b/make-pdf/test/diagram-prepass.test.ts @@ -306,6 +306,35 @@ describe("inlineLocalImages", () => { expect(out).toContain('data-gstack-px-height="44"'); }); + test("out-of-tree image reads warn (never silent) and still inline", () => { + const outside = fs.mkdtempSync(path.join(os.tmpdir(), "prepass-outside-")); + fs.writeFileSync(path.join(outside, "ext.png"), tinyPng(10, 10)); + try { + const warnings: string[] = []; + const out = inlineLocalImages(``, { + ...base, warn: (m) => warnings.push(m), + }); + expect(out).toContain("data:image/png;base64,"); + expect(warnings.some((w) => w.includes("OUTSIDE the input directory"))).toBe(true); + } finally { + fs.rmSync(outside, { recursive: true, force: true }); + } + }); + + test("out-of-tree image + --strict → StrictModeError", () => { + const outside = fs.mkdtempSync(path.join(os.tmpdir(), "prepass-outside-")); + fs.writeFileSync(path.join(outside, "ext.png"), tinyPng(10, 10)); + try { + expect(() => + inlineLocalImages(``, { + ...base, strict: true, warn: () => {}, + }), + ).toThrow(StrictModeError); + } finally { + fs.rmSync(outside, { recursive: true, force: true }); + } + }); + test("Windows drive-letter src is treated as a local path, not a URL scheme", () => { // C:/x.png matches the single-letter-scheme regex — it must reach the // local-path branch (and the missing-file placeholder), never silently @@ -313,7 +342,8 @@ describe("inlineLocalImages", () => { const warnings: string[] = []; const out = inlineLocalImages(``, { ...base, warn: (m) => warnings.push(m) }); expect(out).toContain("image-missing"); - expect(warnings.length).toBe(1); + // Two warnings: it's out-of-tree (resolved outside inputDir) AND missing. + expect(warnings.some((w) => w.includes("image not found"))).toBe(true); }); test("indented fences inside lists replay byte-for-byte (no list splitting)", () => {