From 0dbc60708e9f3922ddad25495b2eeae463fb8f55 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 6 Nov 2026 22:53:27 -0800 Subject: [PATCH] fix(make-pdf): read local images only from the document's directory, and only image files (GST-1747) (#4113) * fix(make-pdf): read local images only from the document's directory, and only image files (GST-1747) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(make-pdf): inline a local file only when its bytes match its image format A file with an image extension now needs that format's signature (or a recognized size header) before it is inlined; anything else gets the blocked placeholder, and --strict throws. The docx ICNS test now sees the local file blocked by the prepass and the inline data URI dropped by the docx guard. Review-Finding: p1/specialist-batched#1 reader=security Review-Finding: p1/specialist-batched#2 reader=testing Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(make-pdf): compare image paths with path.relative so a root-level document keeps its images A markdown file at the filesystem root made the containment prefix '//' (or 'C:\\'), which no path matched. Review-Finding: v1/adversarial-native#5 reader=adversarial-native Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- .../capy-gst-1747-make-pdf-local-images.md | 4 + docs/howto-diagrams-and-formats.md | 14 ++-- make-pdf/SKILL.md | 18 +++-- make-pdf/SKILL.md.tmpl | 18 +++-- make-pdf/src/diagram-prepass.ts | 47 ++++++++++-- make-pdf/test/diagram-prepass.test.ts | 74 ++++++++++++++++++- make-pdf/test/docx-image-size.test.ts | 3 +- 7 files changed, 145 insertions(+), 33 deletions(-) create mode 100644 changelog.d/capy-gst-1747-make-pdf-local-images.md diff --git a/changelog.d/capy-gst-1747-make-pdf-local-images.md b/changelog.d/capy-gst-1747-make-pdf-local-images.md new file mode 100644 index 000000000..4b50f72b6 --- /dev/null +++ b/changelog.d/capy-gst-1747-make-pdf-local-images.md @@ -0,0 +1,4 @@ +--- +important: security +--- +- **make-pdf reads local images only from the markdown's own directory, and only image files.** An image reference that resolves outside the input markdown's directory (a `../` path, an absolute path, a `file:` URL or a symlink pointing out) is no longer read: it renders as a visible `[image blocked (outside the document directory): …]` placeholder with a warning, and `--strict` still fails the run. A local file that is not a png, jpeg, gif, webp or svg image renders as an `[image blocked (not an image): …]` placeholder instead of being inlined, and fails the run under `--strict`. The make-pdf skill and docs/howto-diagrams-and-formats.md describe the new defaults. (GST-1747) diff --git a/docs/howto-diagrams-and-formats.md b/docs/howto-diagrams-and-formats.md index fa31cf240..788e1222e 100644 --- a/docs/howto-diagrams-and-formats.md +++ b/docs/howto-diagrams-and-formats.md @@ -59,9 +59,11 @@ so a phone photo doesn't bloat the document. Image safety defaults: remote (http/https) images are **blocked with a visible placeholder** unless you pass `--allow-network`. An image path that -resolves outside the markdown's directory (even through a symlink) still -inlines but warns loudly. Files over 64MB and non-regular files (fifos, -devices) degrade to a placeholder instead of hanging the render. +resolves outside the markdown's directory (even through a symlink) is never +read: it renders as a visible placeholder with a warning. Only image files +(png, jpeg, gif, webp, svg) inline; other files, files over 64MB and +non-regular files (fifos, devices) degrade to a placeholder instead of +hanging the render. Per-image directives go immediately after the image: @@ -130,9 +132,9 @@ make-pdf generate docs.md --strict ``` Missing local images, blocked remote images, out-of-tree image reads (a path -or symlink resolving outside the markdown's directory), oversized files -(>64MB), and non-regular files all exit non-zero instead of degrading to a -warning or placeholder — for docs pipelines where a broken image should +or symlink resolving outside the markdown's directory), non-image files, +oversized files (>64MB), and non-regular files all exit non-zero instead of +degrading to a warning or placeholder — for docs pipelines where a broken image should break the build. ## Troubleshooting diff --git a/make-pdf/SKILL.md b/make-pdf/SKILL.md index 813061d1e..4c164a68d 100644 --- a/make-pdf/SKILL.md +++ b/make-pdf/SKILL.md @@ -285,10 +285,11 @@ small with no visible quality loss. Remote (http/https) images are **blocked with a visible placeholder** by default — offline posture; pass `--allow-network` to fetch them. An image -that resolves outside the markdown's directory (even via symlink) still -inlines, but warns loudly; `--strict` makes it fatal. Files over 64MB or -non-regular files (fifos, devices) degrade to a placeholder instead of -hanging the run. +that resolves outside the markdown's directory (even via symlink) is never +read: it renders as a visible placeholder with a warning, and `--strict` +makes it fatal. Only image files (png, jpeg, gif, webp, svg) inline; other +files, files over 64MB and non-regular files (fifos, devices) degrade to a +placeholder instead of hanging the run. Per-image directives, written immediately after the image: @@ -322,8 +323,8 @@ promoted page is vertically centered. When the heuristic guesses wrong, ### CI mode — fail loud on missing assets ```bash -"$P" generate docs.md --strict # missing, remote, out-of-tree, oversized, - # and non-regular-file images exit non-zero +"$P" generate docs.md --strict # missing, remote, out-of-tree, non-image, + # oversized and non-regular-file images exit non-zero # instead of warn + placeholder ``` @@ -348,8 +349,9 @@ Branding: Output: --to pdf|html|docx Output format (default: pdf). html = single self-contained file; docx = content fidelity. - --strict Missing, remote, out-of-tree, oversized, or - non-regular-file images fail the run (CI mode). + --strict Missing, remote, out-of-tree, non-image, + oversized, or non-regular-file images fail the + run (CI mode). --page-numbers "N of M" footer (default on) --tagged Accessible PDF (default on) --outline PDF bookmarks from headings (default on) diff --git a/make-pdf/SKILL.md.tmpl b/make-pdf/SKILL.md.tmpl index b55cf768a..27bed9e7a 100644 --- a/make-pdf/SKILL.md.tmpl +++ b/make-pdf/SKILL.md.tmpl @@ -133,10 +133,11 @@ small with no visible quality loss. Remote (http/https) images are **blocked with a visible placeholder** by default — offline posture; pass `--allow-network` to fetch them. An image -that resolves outside the markdown's directory (even via symlink) still -inlines, but warns loudly; `--strict` makes it fatal. Files over 64MB or -non-regular files (fifos, devices) degrade to a placeholder instead of -hanging the run. +that resolves outside the markdown's directory (even via symlink) is never +read: it renders as a visible placeholder with a warning, and `--strict` +makes it fatal. Only image files (png, jpeg, gif, webp, svg) inline; other +files, files over 64MB and non-regular files (fifos, devices) degrade to a +placeholder instead of hanging the run. Per-image directives, written immediately after the image: @@ -170,8 +171,8 @@ promoted page is vertically centered. When the heuristic guesses wrong, ### CI mode — fail loud on missing assets ```bash -"$P" generate docs.md --strict # missing, remote, out-of-tree, oversized, - # and non-regular-file images exit non-zero +"$P" generate docs.md --strict # missing, remote, out-of-tree, non-image, + # oversized and non-regular-file images exit non-zero # instead of warn + placeholder ``` @@ -196,8 +197,9 @@ Branding: Output: --to pdf|html|docx Output format (default: pdf). html = single self-contained file; docx = content fidelity. - --strict Missing, remote, out-of-tree, oversized, or - non-regular-file images fail the run (CI mode). + --strict Missing, remote, out-of-tree, non-image, + oversized, or non-regular-file images fail the + run (CI mode). --page-numbers "N of M" footer (default on) --tagged Accessible PDF (default on) --outline PDF bookmarks from headings (default on) diff --git a/make-pdf/src/diagram-prepass.ts b/make-pdf/src/diagram-prepass.ts index de84b6d38..b3e2c8e95 100644 --- a/make-pdf/src/diagram-prepass.ts +++ b/make-pdf/src/diagram-prepass.ts @@ -609,19 +609,20 @@ export async function inlineLocalImages(html: string, opts: PrepassImageOptions) return buildMissingImagePlaceholder(src); } - // 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. + // Out-of-tree files are never read: rendering untrusted markdown must not + // copy files from elsewhere on disk into a shareable document. Visible + // placeholder by default; --strict makes it fatal. // Compare REAL paths — a symlink inside the input dir pointing outside // would otherwise pass a string-prefix check (Codex adversarial finding). // Runs after the existence check: realpath of a missing file can't // resolve, and on macOS /var vs /private/var would false-positive. - const inputRoot = safeRealpath(path.resolve(opts.inputDir)) + path.sep; + const inputRoot = safeRealpath(path.resolve(opts.inputDir)); const realFilePath = safeRealpath(filePath); - if (!realFilePath.startsWith(inputRoot)) { + if (!isInsideDir(realFilePath, inputRoot)) { const msg = `image resolves OUTSIDE the input directory: ${src} → ${realFilePath}`; - if (opts.strict) throw new StrictModeError(msg + " — move it under the markdown's directory or drop --strict"); + if (opts.strict) throw new StrictModeError(msg + " — move it under the markdown's directory"); opts.warn(msg); + return buildBlockedLocalPlaceholder("outside the document directory", src); } // Bound the read BEFORE reading: a markdown image pointing at a special @@ -649,7 +650,13 @@ export async function inlineLocalImages(html: string, opts: PrepassImageOptions) const buf = fs.readFileSync(filePath); const dims = imageDims(buf); - const mime = dims?.mime ?? mimeFromExtension(filePath); + const mime = dims?.mime ?? signedExtensionMime(buf, filePath); + if (mime === "application/octet-stream") { + const msg = `not a recognized image (png, jpeg, gif, webp, svg): ${src}`; + if (opts.strict) throw new StrictModeError(msg); + opts.warn(msg); + return buildBlockedLocalPlaceholder("not an image", src); + } // Print-resolution normalization (D4): rasters only — SVG scales free. if (dims && mime !== "image/svg+xml" && dims.width > maxPx && opts.run) { @@ -735,6 +742,18 @@ function buildBlockedRemotePlaceholder(src: string): string { ); } +function buildBlockedLocalPlaceholder(reason: string, src: string): string { + return ( + `` + + `[image blocked (${reason}): ${escapeHtml(src)}]` + ); +} + +function isInsideDir(p: string, dir: string): boolean { + const rel = path.relative(dir, p); + return rel !== "" && rel !== ".." && !rel.startsWith(".." + path.sep) && !path.isAbsolute(rel); +} + /** realpath that degrades to the input path when resolution fails. */ function safeRealpath(p: string): string { try { @@ -744,6 +763,20 @@ function safeRealpath(p: string): string { } } +const IMAGE_SIGNATURES: Record boolean> = { + "image/png": (b) => b.length >= 8 && b.readUInt32BE(0) === 0x89504e47 && b.readUInt32BE(4) === 0x0d0a1a0a, + "image/jpeg": (b) => b.length >= 3 && b[0] === 0xff && b[1] === 0xd8 && b[2] === 0xff, + "image/gif": (b) => /^GIF8[79]a/.test(b.toString("ascii", 0, 6)), + "image/webp": (b) => b.toString("ascii", 0, 4) === "RIFF" && b.toString("ascii", 8, 12) === "WEBP", + "image/svg+xml": (b) => /]/i.test(b.toString("utf8", 0, 65536)), +}; + +/** The extension's image MIME, only when the bytes carry that format's signature. */ +function signedExtensionMime(buf: Buffer, p: string): string { + const mime = mimeFromExtension(p); + return IMAGE_SIGNATURES[mime]?.(buf) ? mime : "application/octet-stream"; +} + function mimeFromExtension(p: string): string { switch (path.extname(p).toLowerCase()) { case ".png": return "image/png"; diff --git a/make-pdf/test/diagram-prepass.test.ts b/make-pdf/test/diagram-prepass.test.ts index 52115b621..075ffa760 100644 --- a/make-pdf/test/diagram-prepass.test.ts +++ b/make-pdf/test/diagram-prepass.test.ts @@ -348,21 +348,89 @@ describe("inlineLocalImages", () => { expect(out).toContain('data-gstack-px-height="44"'); }); - test("out-of-tree image reads warn (never silent) and still inline", async () => { + test("out-of-tree image is not inlined: visible placeholder + warning", async () => { const outside = fs.mkdtempSync(path.join(os.tmpdir(), "prepass-outside-")); - fs.writeFileSync(path.join(outside, "ext.png"), tinyPng(10, 10)); + const png = tinyPng(10, 10); + fs.writeFileSync(path.join(outside, "ext.png"), png); try { const warnings: string[] = []; const out = await inlineLocalImages(``, { ...base, warn: (m) => warnings.push(m), }); - expect(out).toContain("data:image/png;base64,"); + expect(out).not.toContain(png.toString("base64")); + expect(out).not.toContain("data:image/png;base64,"); + expect(out).toContain("image-missing"); expect(warnings.some((w) => w.includes("OUTSIDE the input directory"))).toBe(true); } finally { fs.rmSync(outside, { recursive: true, force: true }); } }); + test("relative reference to a file outside the document directory leaves its bytes out of the output", async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "prepass-tree-")); + const docDir = path.join(root, "doc"); + const outsideDir = path.join(root, "outside"); + fs.mkdirSync(docDir); + fs.mkdirSync(outsideDir); + const secret = Buffer.from("PRIVATE-MARKER-7f3c9a\n"); + fs.writeFileSync(path.join(outsideDir, "secret.txt"), secret); + try { + const warnings: string[] = []; + const out = await inlineLocalImages(`x`, { + ...base, inputDir: docDir, warn: (m) => warnings.push(m), + }); + expect(out).not.toContain(secret.toString("base64")); + expect(out).not.toContain("PRIVATE-MARKER-7f3c9a"); + expect(out).not.toContain("data:"); + expect(out).toContain("image-missing"); + expect(warnings.some((w) => w.includes("OUTSIDE the input directory"))).toBe(true); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + test("non-image file inside the input dir is not inlined", async () => { + const text = Buffer.from("plain text, not an image\n"); + fs.writeFileSync(path.join(dir, "notes.txt"), text); + const warnings: string[] = []; + const out = await inlineLocalImages(``, { ...base, warn: (m) => warnings.push(m) }); + expect(out).not.toContain(text.toString("base64")); + expect(out).not.toContain("data:"); + expect(out).toContain("image-missing"); + expect(warnings.some((w) => w.includes("not a recognized image"))).toBe(true); + }); + + test("markdown at the filesystem root still inlines images beneath it", async () => { + const file = path.join(dir, "rooted.png"); + fs.writeFileSync(file, tinyPng(12, 12)); + const out = await inlineLocalImages(``, { ...base, inputDir: path.parse(dir).root, warn: () => {} }); + expect(out).toContain("data:image/png;base64,"); + }); + + test("non-image bytes under an image extension are not inlined", async () => { + const text = Buffer.from("plain text named like an image\n"); + fs.writeFileSync(path.join(dir, "fake.png"), text); + fs.writeFileSync(path.join(dir, "fake.svg"), text); + for (const name of ["fake.png", "fake.svg"]) { + const warnings: string[] = []; + const out = await inlineLocalImages(``, { ...base, warn: (m) => warnings.push(m) }); + expect(out).not.toContain(text.toString("base64")); + expect(out).not.toContain("data:"); + expect(out).toContain("image-missing"); + expect(warnings.some((w) => w.includes("not a recognized image"))).toBe(true); + await expect( + inlineLocalImages(``, { ...base, strict: true, warn: () => {} }), + ).rejects.toThrow(StrictModeError); + } + }); + + test("non-image file + --strict → StrictModeError", async () => { + fs.writeFileSync(path.join(dir, "notes2.txt"), "plain text\n"); + await expect( + inlineLocalImages(``, { ...base, strict: true, warn: () => {} }), + ).rejects.toThrow(StrictModeError); + }); + test("out-of-tree image + --strict → StrictModeError", async () => { const outside = fs.mkdtempSync(path.join(os.tmpdir(), "prepass-outside-")); fs.writeFileSync(path.join(outside, "ext.png"), tinyPng(10, 10)); diff --git a/make-pdf/test/docx-image-size.test.ts b/make-pdf/test/docx-image-size.test.ts index 8301303c9..41ac60a2f 100644 --- a/make-pdf/test/docx-image-size.test.ts +++ b/make-pdf/test/docx-image-size.test.ts @@ -104,7 +104,8 @@ await generate({ input: ${JSON.stringify(path.join(workDir, "doc.md"))}, output: const run = Bun.spawnSync([process.execPath, "-e", script], { timeout: 30_000, stdout: "pipe", stderr: "pipe" }); const stderr = run.stderr.toString(); expect(run.exitCode).toBe(0); - expect(stderr.match(/docx: dropped an ICNS image/g)).toHaveLength(2); + expect(stderr).toContain("not a recognized image (png, jpeg, gif, webp, svg): evil.png"); + expect(stderr.match(/docx: dropped an ICNS image/g)).toHaveLength(1); expect(unzip(["-p", out, "word/document.xml"]).toString("utf8")).not.toContain(""); } finally { fs.rmSync(workDir, { recursive: true, force: true });