From 4533ed993490e27dab018fc7c38344d6b2781dcb Mon Sep 17 00:00:00 2001 From: Hong Zhang Date: Mon, 25 May 2026 15:00:00 -0700 Subject: [PATCH 1/8] fix(engine): mirror user assets (images/, fonts/, etc.) from deck folder into build/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real-use bug from the v1.0.0 release: a deck whose content.md referenced `images/1-0.jpg` rendered with broken tags because the engine only wrote build/index.html and ignored every other file in the deck folder. The local server serves URLs from build/, so /images/1-0.jpg 404'd. Same bug broke publish_deck — single-file mode couldn't base64-inline images it couldn't find, multi-file mode copied build/ to published/ without them. build_deck now runs a syncUserAssetsToBuild step after mkdir-ing the output dir and before writing index.html. It mirrors every top-level entry from the deck folder into build/ EXCEPT a known exclusion list (content.md, deckmark.config.json, annotations/, build/, published/, node_modules/, agent instruction files, hidden files, .git/, .github/, .claude-plugin/, .mcp.json, and top-level .html/.tgz publish artifacts). Result: a deck folder of melody/ content.md images/ 1-0.jpg, ... now builds to melody/build/ index.html images/ 1-0.jpg, ... so /images/1-0.jpg resolves under the served build/ root, and publish_deck in either mode picks up the assets correctly. Idempotent: re-running overwrites files but doesn't delete extras (removed source files linger in build/ until a clean rebuild). Acceptable for MVP; a future flag could enable strict-sync. Adds two unit tests: 1. images/ subtree (including nested) is mirrored under build/. 2. AGENTS.md, deckmark.config.json, .gitignore, annotations/ are NOT copied; a regular user folder (assets/) IS copied. 33/33 tests pass on Node 22. --- runtime/engines/reveal.ts | 63 +++++++++++++++++++++++++++++++- test/unit/engines-reveal.test.ts | 42 +++++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/runtime/engines/reveal.ts b/runtime/engines/reveal.ts index 96dcfd2..111f3ff 100644 --- a/runtime/engines/reveal.ts +++ b/runtime/engines/reveal.ts @@ -1,11 +1,65 @@ -import { mkdir, readFile, writeFile } from 'node:fs/promises'; -import { dirname, join } from 'node:path'; +import { mkdir, readFile, writeFile, readdir, cp } from 'node:fs/promises'; +import { dirname, join, basename } from 'node:path'; import { fileURLToPath } from 'node:url'; import { marked } from 'marked'; const REVEAL_PREFIX = '/vendor/reveal'; const __dirname = dirname(fileURLToPath(import.meta.url)); +/** + * Names at the deck root that are deckmark/agent internals, NOT user assets. + * Everything else (images/, fonts/, etc.) gets copied into build/ so the + * rendered deck can reference assets via relative paths like + * `` and the local server can serve them. + */ +const EXCLUDED_FROM_ASSET_SYNC = new Set([ + 'content.md', + 'deckmark.config.json', + 'annotations', + 'build', + 'published', + 'node_modules', + '.git', + '.github', + '.gitignore', + '.claude-plugin', + '.mcp.json', + 'AGENTS.md', + 'CLAUDE.md', + 'GEMINI.md', + '.codex', + '.cursor' +]); + +/** + * Mirror non-internal files from the deck folder into build/ so the rendered + * HTML can reference user assets (images, fonts, etc.) via stable relative + * paths. Idempotent: re-running overwrites files in build/ but never deletes + * extras (a removed asset in deckDir will still be served from build/ until + * the next clean rebuild — acceptable for MVP). + */ +async function syncUserAssetsToBuild(deckDir: string, buildDir: string): Promise { + let entries; + try { + entries = await readdir(deckDir, { withFileTypes: true }); + } catch { + return; + } + const buildBase = basename(buildDir); + for (const e of entries) { + // Hidden / dotfiles never sync. + if (e.name.startsWith('.')) continue; + if (EXCLUDED_FROM_ASSET_SYNC.has(e.name)) continue; + // Don't sync the build dir itself (handles unusual outDir locations too). + if (e.name === buildBase) continue; + // Top-level .html and .tgz files are likely publish artifacts; skip. + if (!e.isDirectory() && (e.name.endsWith('.html') || e.name.endsWith('.tgz'))) continue; + const src = join(deckDir, e.name); + const dst = join(buildDir, e.name); + await cp(src, dst, { recursive: true, force: true }); + } +} + export type DeckStyle = 'professional' | 'academic' | 'fashion' | 'technical' | 'fun'; export type DeckMode = 'light' | 'dark'; export type DeckMotion = 'slide-transitions' | 'fragment-reveals' | 'auto-animate'; @@ -134,6 +188,11 @@ ${sections.join('\n')} `; await mkdir(opts.outDir, { recursive: true }); + // Mirror user assets (images/, fonts/, etc.) from the deck folder into + // build/ before writing index.html, so the rendered deck can reference + // them via relative paths and so publish_deck (which reads from build/) + // can find them for inlining or for multi-file deploy. + await syncUserAssetsToBuild(dirname(opts.contentPath), opts.outDir); await writeFile(join(opts.outDir, 'index.html'), htmlDocument, 'utf8'); return { outDir: opts.outDir, slideCount: sections.length, style, mode, motion, slideNumbers: slideNumberValue }; } diff --git a/test/unit/engines-reveal.test.ts b/test/unit/engines-reveal.test.ts index 1c35c58..2fa417b 100644 --- a/test/unit/engines-reveal.test.ts +++ b/test/unit/engines-reveal.test.ts @@ -101,3 +101,45 @@ test('buildDeck with motion=[] disables transitions', async () => { assert.match(html, /transition:\s*"none"/); await rm(dir, { recursive: true }); }); + +test('buildDeck mirrors user assets (images/) from deck folder into build/', async () => { + const { mkdir, writeFile: w } = await import('node:fs/promises'); + const { existsSync } = await import('node:fs'); + const dir = await tmpDir(); + // Set up a deck with an images/ subfolder containing a couple of files + await mkdir(join(dir, 'images'), { recursive: true }); + await w(join(dir, 'images', '1-0.jpg'), 'fake-jpeg-bytes'); + await w(join(dir, 'images', 'nested', 'deep.png'), '', 'utf8').catch(async () => { + await mkdir(join(dir, 'images', 'nested'), { recursive: true }); + await w(join(dir, 'images', 'nested', 'deep.png'), ''); + }); + await w(join(dir, 'content.md'), SAMPLE_CONTENT); + await buildDeck({ contentPath: join(dir, 'content.md'), outDir: join(dir, 'build') }); + // After build, images/ should be mirrored inside build/ + assert.ok(existsSync(join(dir, 'build', 'images', '1-0.jpg')), 'images/1-0.jpg should be copied to build/'); + assert.ok(existsSync(join(dir, 'build', 'images', 'nested', 'deep.png')), 'nested image should be copied'); + await rm(dir, { recursive: true }); +}); + +test('buildDeck does NOT sync deckmark internals (AGENTS.md, annotations/, .gitignore, etc.) into build/', async () => { + const { mkdir, writeFile: w } = await import('node:fs/promises'); + const { existsSync } = await import('node:fs'); + const dir = await tmpDir(); + // Internals that should NOT be copied + await w(join(dir, 'AGENTS.md'), '# agent instructions'); + await w(join(dir, 'deckmark.config.json'), '{}'); + await w(join(dir, '.gitignore'), 'build/\n'); + await mkdir(join(dir, 'annotations'), { recursive: true }); + await w(join(dir, 'annotations', 'session-stub.json'), '{}'); + // A user asset that SHOULD be copied + await mkdir(join(dir, 'assets'), { recursive: true }); + await w(join(dir, 'assets', 'logo.svg'), ''); + await w(join(dir, 'content.md'), SAMPLE_CONTENT); + await buildDeck({ contentPath: join(dir, 'content.md'), outDir: join(dir, 'build') }); + assert.equal(existsSync(join(dir, 'build', 'AGENTS.md')), false); + assert.equal(existsSync(join(dir, 'build', 'deckmark.config.json')), false); + assert.equal(existsSync(join(dir, 'build', '.gitignore')), false); + assert.equal(existsSync(join(dir, 'build', 'annotations')), false); + assert.ok(existsSync(join(dir, 'build', 'assets', 'logo.svg')), 'user asset should be copied'); + await rm(dir, { recursive: true }); +}); From 8bda45dd7d33458fe3ecc5b9ca3fb7dee77a6a5e Mon Sep 17 00:00:00 2001 From: Hong Zhang Date: Mon, 25 May 2026 15:35:44 -0700 Subject: [PATCH 2/8] fix(publish): emit relative vendor paths so file:// open works after multi-file publish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Engine previously emitted absolute `/vendor/reveal/...` URLs. That works when served from a web server (it resolves against the origin root), but breaks when a user double-clicks the published index.html — the browser resolves `/vendor/reveal/reveal.js` against the file:// scheme root (filesystem drive root), not the published folder. End result: blank page because reveal.js never loads. Switch REVEAL_PREFIX to a relative path ('vendor/reveal') so the same emitted HTML works in both modes: - served: http://host/.../ → relative resolves under the deck origin - file:// C:/.../published/ → relative resolves inside the folder The dev server's '/vendor/reveal/*' route still matches because the browser resolves the relative path against the served page URL. Also: - inline-html.ts: regex now accepts both '/vendor/reveal/...' and 'vendor/reveal/...' so single-file publish keeps inlining even if legacy HTML appears (defense in depth). - multi-file.ts: copy order — buildDir first, reveal.js dist last — so a user-named 'vendor/' subfolder can never shadow the official reveal.js dist that publish_deck writes. - engines-reveal.test.ts: assert relative path (no leading slash) and assert no absolute /vendor/reveal appears in emitted HTML. - Per Copilot review on the asset-sync PR: type Dirent[] explicitly, skip symlinks during user-asset sync, and exclude 'vendor' from sync so a user 'vendor/' folder never clobbers reveal dist. Co-Authored-By: Claude Opus 4.7 --- runtime/engines/reveal.ts | 23 +++++++++++++++++++++-- runtime/publish/inline-html.ts | 12 ++++++++---- runtime/publish/multi-file.ts | 12 ++++++++---- test/unit/engines-reveal.test.ts | 15 +++++++-------- 4 files changed, 44 insertions(+), 18 deletions(-) diff --git a/runtime/engines/reveal.ts b/runtime/engines/reveal.ts index 111f3ff..ad17614 100644 --- a/runtime/engines/reveal.ts +++ b/runtime/engines/reveal.ts @@ -1,9 +1,15 @@ import { mkdir, readFile, writeFile, readdir, cp } from 'node:fs/promises'; +import type { Dirent } from 'node:fs'; import { dirname, join, basename } from 'node:path'; import { fileURLToPath } from 'node:url'; import { marked } from 'marked'; -const REVEAL_PREFIX = '/vendor/reveal'; +// Relative (no leading slash) so the emitted HTML works both as served +// from the local review server AND as a standalone file:// open after +// publish_deck multi-file mode. The dev server's /vendor/reveal/* route +// still matches because the browser resolves the relative path against +// the page URL, which is the server root '/'. +const REVEAL_PREFIX = 'vendor/reveal'; const __dirname = dirname(fileURLToPath(import.meta.url)); /** @@ -11,6 +17,10 @@ const __dirname = dirname(fileURLToPath(import.meta.url)); * Everything else (images/, fonts/, etc.) gets copied into build/ so the * rendered deck can reference assets via relative paths like * `` and the local server can serve them. + * + * `vendor` is excluded so a user-provided `vendor/` folder cannot collide + * with — or overwrite — the official reveal.js dist that publish_deck + * (multi-file mode) places at `/vendor/reveal/`. */ const EXCLUDED_FROM_ASSET_SYNC = new Set([ 'content.md', @@ -18,6 +28,7 @@ const EXCLUDED_FROM_ASSET_SYNC = new Set([ 'annotations', 'build', 'published', + 'vendor', 'node_modules', '.git', '.github', @@ -37,9 +48,15 @@ const EXCLUDED_FROM_ASSET_SYNC = new Set([ * paths. Idempotent: re-running overwrites files in build/ but never deletes * extras (a removed asset in deckDir will still be served from build/ until * the next clean rebuild — acceptable for MVP). + * + * Symlinks at the deck root are skipped on purpose: copying a symlink like + * `secret -> /etc/passwd` into build/ would let the review server (or + * publish_deck) serve arbitrary local files via the deck. The static server + * has its own containment check, but skipping symlinks here is defense in + * depth. */ async function syncUserAssetsToBuild(deckDir: string, buildDir: string): Promise { - let entries; + let entries: Dirent[]; try { entries = await readdir(deckDir, { withFileTypes: true }); } catch { @@ -54,6 +71,8 @@ async function syncUserAssetsToBuild(deckDir: string, buildDir: string): Promise if (e.name === buildBase) continue; // Top-level .html and .tgz files are likely publish artifacts; skip. if (!e.isDirectory() && (e.name.endsWith('.html') || e.name.endsWith('.tgz'))) continue; + // Symlinks: skip, to avoid copying links that point outside the deck. + if (e.isSymbolicLink()) continue; const src = join(deckDir, e.name); const dst = join(buildDir, e.name); await cp(src, dst, { recursive: true, force: true }); diff --git a/runtime/publish/inline-html.ts b/runtime/publish/inline-html.ts index 7d1f72f..0624b56 100644 --- a/runtime/publish/inline-html.ts +++ b/runtime/publish/inline-html.ts @@ -57,8 +57,11 @@ async function replaceLinkStylesheets(html: string): Promise { dbg(` link matches: ${matches.length}`); for (const m of matches) { const href = m[1]; - if (!href.startsWith('/vendor/reveal/')) continue; - const file = href.replace('/vendor/reveal/', ''); + // Accept both relative ('vendor/reveal/...') and absolute ('/vendor/reveal/...') + // forms — engine now emits relative but old-published HTML may have absolute. + const VENDOR_RE = /^\/?vendor\/reveal\//; + if (!VENDOR_RE.test(href)) continue; + const file = href.replace(VENDOR_RE, ''); const src = resolve(REVEAL_DIST, file); if (!existsSync(src)) { dbg(` skip missing: ${src}`); continue; } const css = await readFile(src, 'utf8'); @@ -74,8 +77,9 @@ async function replaceScripts(html: string): Promise { dbg(` script matches: ${matches.length}`); for (const m of matches) { const src = m[1]; - if (!src.startsWith('/vendor/reveal/')) continue; - const file = src.replace('/vendor/reveal/', ''); + const VENDOR_RE = /^\/?vendor\/reveal\//; + if (!VENDOR_RE.test(src)) continue; + const file = src.replace(VENDOR_RE, ''); const filePath = resolve(REVEAL_DIST, file); if (!existsSync(filePath)) { dbg(` skip missing: ${filePath}`); continue; } const js = await readFile(filePath, 'utf8'); diff --git a/runtime/publish/multi-file.ts b/runtime/publish/multi-file.ts index b6b442d..3a2bd3b 100644 --- a/runtime/publish/multi-file.ts +++ b/runtime/publish/multi-file.ts @@ -16,13 +16,17 @@ export interface MultiFileOpts { export async function multiFile(opts: MultiFileOpts): Promise<{ outDir: string; files: string[] }> { await mkdir(opts.outDir, { recursive: true }); - // Copy reveal.js dist contents to outDir/vendor/reveal/ - await mkdir(join(opts.outDir, 'vendor', 'reveal'), { recursive: true }); - await cp(REVEAL_DIST, join(opts.outDir, 'vendor', 'reveal'), { recursive: true }); - // Copy build/ contents (index.html and any embedded images) to outDir/ + // Order matters: copy buildDir FIRST (user assets, including any + // user-named vendor/ that may have made it in), then overlay the + // official reveal.js dist LAST. Whatever path collisions exist, + // /vendor/reveal/* always reflects the real reveal.js — never user + // content masquerading under the same path. await cp(opts.buildDir, opts.outDir, { recursive: true, force: true }); + await mkdir(join(opts.outDir, 'vendor', 'reveal'), { recursive: true }); + await cp(REVEAL_DIST, join(opts.outDir, 'vendor', 'reveal'), { recursive: true, force: true }); + // Walk the output to list what landed const { readdir } = await import('node:fs/promises'); async function walk(dir: string, base = ''): Promise { diff --git a/test/unit/engines-reveal.test.ts b/test/unit/engines-reveal.test.ts index 2fa417b..0676a81 100644 --- a/test/unit/engines-reveal.test.ts +++ b/test/unit/engines-reveal.test.ts @@ -32,7 +32,9 @@ test('buildDeck produces index.html with both slides', async () => { assert.match(html, /]*>[\s\S]*Slide One/); assert.match(html, /]*>[\s\S]*Slide Two/); assert.match(html, /reveal\.js/); - assert.match(html, /\/vendor\/reveal\/reveal\.js/); + // Relative (no leading slash) so file:// open also works after publish_deck. + assert.match(html, /["']vendor\/reveal\/reveal\.js["']/); + assert.doesNotMatch(html, /["']\/vendor\/reveal/); await rm(dir, { recursive: true }); }); @@ -75,8 +77,8 @@ test('buildDeck sets data-mode on and embeds the style sheet', async () = assert.match(html, /]+data-mode="dark"/); assert.match(html, /data-deckmark-style="academic"/); assert.match(html, /data-deckmark-mode="dark"/); - // dark mode loads the dark reveal base theme - assert.match(html, /\/vendor\/reveal\/theme\/black\.css/); + // dark mode loads the dark reveal base theme (relative path) + assert.match(html, /["']vendor\/reveal\/theme\/black\.css["']/); await rm(dir, { recursive: true }); }); @@ -107,12 +109,9 @@ test('buildDeck mirrors user assets (images/) from deck folder into build/', asy const { existsSync } = await import('node:fs'); const dir = await tmpDir(); // Set up a deck with an images/ subfolder containing a couple of files - await mkdir(join(dir, 'images'), { recursive: true }); + await mkdir(join(dir, 'images', 'nested'), { recursive: true }); await w(join(dir, 'images', '1-0.jpg'), 'fake-jpeg-bytes'); - await w(join(dir, 'images', 'nested', 'deep.png'), '', 'utf8').catch(async () => { - await mkdir(join(dir, 'images', 'nested'), { recursive: true }); - await w(join(dir, 'images', 'nested', 'deep.png'), ''); - }); + await w(join(dir, 'images', 'nested', 'deep.png'), ''); await w(join(dir, 'content.md'), SAMPLE_CONTENT); await buildDeck({ contentPath: join(dir, 'content.md'), outDir: join(dir, 'build') }); // After build, images/ should be mirrored inside build/ From a0c479d022920e3e0d81cc4abfa2278105e0efed Mon Sep 17 00:00:00 2001 From: Hong Zhang Date: Mon, 25 May 2026 15:44:32 -0700 Subject: [PATCH 3/8] fix(security): reject symlinks at every depth during asset sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior fix only skipped symlinks at the deck root via Dirent.isSymbolicLink(). cp() with { recursive: true } still copied nested symlinks verbatim — e.g. images/secret -> /etc/passwd would land in build/, and the static server's stat()/readFile() (which follow symlinks) would happily serve it. Pass a `filter` callback to cp() that lstat's each visited path and returns false for symlinks. The top-level Dirent check stays as a fast path; the filter is the actual security boundary. Add an integration test that symlinks a file *outside* the deck into images/ and asserts the symlink does not appear in build/. The test self-skips on platforms where symlink() returns EPERM/ENOSYS (Windows without Developer Mode or admin), so it stays green in CI but proves the security guarantee when symlinks are creatable. Co-Authored-By: Claude Opus 4.7 --- runtime/engines/reveal.ts | 35 ++++++++++++++++++++++------- test/unit/engines-reveal.test.ts | 38 ++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/runtime/engines/reveal.ts b/runtime/engines/reveal.ts index ad17614..3741ec9 100644 --- a/runtime/engines/reveal.ts +++ b/runtime/engines/reveal.ts @@ -1,4 +1,4 @@ -import { mkdir, readFile, writeFile, readdir, cp } from 'node:fs/promises'; +import { mkdir, readFile, writeFile, readdir, cp, lstat } from 'node:fs/promises'; import type { Dirent } from 'node:fs'; import { dirname, join, basename } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -42,6 +42,24 @@ const EXCLUDED_FROM_ASSET_SYNC = new Set([ '.cursor' ]); +/** + * cp() filter that rejects symlinks at any depth. Required because + * `cp(..., { recursive: true })` without this would copy nested symlinks + * verbatim (e.g. `images/secret -> /etc/passwd`); the static server then + * follows them via stat()/readFile(), exposing arbitrary local files. + * + * `lstat` (not `stat`) is mandatory here — `stat` follows symlinks and + * would report the target's type, defeating the check. + */ +async function rejectSymlink(src: string): Promise { + try { + const st = await lstat(src); + return !st.isSymbolicLink(); + } catch { + return false; + } +} + /** * Mirror non-internal files from the deck folder into build/ so the rendered * HTML can reference user assets (images, fonts, etc.) via stable relative @@ -49,11 +67,10 @@ const EXCLUDED_FROM_ASSET_SYNC = new Set([ * extras (a removed asset in deckDir will still be served from build/ until * the next clean rebuild — acceptable for MVP). * - * Symlinks at the deck root are skipped on purpose: copying a symlink like - * `secret -> /etc/passwd` into build/ would let the review server (or - * publish_deck) serve arbitrary local files via the deck. The static server - * has its own containment check, but skipping symlinks here is defense in - * depth. + * Symlinks are skipped at every depth (top-level entries + the `filter` + * passed to cp()) to keep links like `images/secret -> /etc/passwd` out of + * the published output. The static server has its own containment check, + * but this is defense in depth. */ async function syncUserAssetsToBuild(deckDir: string, buildDir: string): Promise { let entries: Dirent[]; @@ -71,11 +88,13 @@ async function syncUserAssetsToBuild(deckDir: string, buildDir: string): Promise if (e.name === buildBase) continue; // Top-level .html and .tgz files are likely publish artifacts; skip. if (!e.isDirectory() && (e.name.endsWith('.html') || e.name.endsWith('.tgz'))) continue; - // Symlinks: skip, to avoid copying links that point outside the deck. + // Fast path: skip top-level symlinks before recursing. if (e.isSymbolicLink()) continue; const src = join(deckDir, e.name); const dst = join(buildDir, e.name); - await cp(src, dst, { recursive: true, force: true }); + // `filter` is applied to every src path cp() visits during recursion, + // so nested symlinks are rejected too — not just top-level ones. + await cp(src, dst, { recursive: true, force: true, filter: rejectSymlink }); } } diff --git a/test/unit/engines-reveal.test.ts b/test/unit/engines-reveal.test.ts index 0676a81..9ad3e31 100644 --- a/test/unit/engines-reveal.test.ts +++ b/test/unit/engines-reveal.test.ts @@ -120,6 +120,44 @@ test('buildDeck mirrors user assets (images/) from deck folder into build/', asy await rm(dir, { recursive: true }); }); +test('buildDeck skips nested symlinks during asset sync (security: no /etc/passwd via images/secret)', async () => { + // Symlink creation on Windows usually requires either admin rights or + // Developer Mode. If symlink() fails with EPERM we skip — the test is a + // security regression check, not a portability test. + const { mkdir, writeFile: w, symlink } = await import('node:fs/promises'); + const { existsSync, lstatSync } = await import('node:fs'); + const dir = await tmpDir(); + await mkdir(join(dir, 'images'), { recursive: true }); + await w(join(dir, 'images', 'real.jpg'), 'jpeg-bytes'); + // Drop a target file outside the deck and try to symlink to it from inside. + const outsideTarget = join(dir, '..', 'sensitive.txt'); + await w(outsideTarget, 'SECRET'); + try { + await symlink(outsideTarget, join(dir, 'images', 'leak')); + } catch (err: unknown) { + const e = err as NodeJS.ErrnoException; + if (e.code === 'EPERM' || e.code === 'ENOSYS') { + await rm(dir, { recursive: true }); + await rm(outsideTarget, { force: true }); + return; + } + throw err; + } + await w(join(dir, 'content.md'), SAMPLE_CONTENT); + await buildDeck({ contentPath: join(dir, 'content.md'), outDir: join(dir, 'build') }); + assert.ok(existsSync(join(dir, 'build', 'images', 'real.jpg')), 'real file should still be copied'); + assert.equal( + existsSync(join(dir, 'build', 'images', 'leak')), + false, + 'nested symlink must NOT be copied into build/' + ); + // Sanity: the source symlink itself is in fact a symlink (so the test + // would meaningfully fail if rejectSymlink stopped working). + assert.ok(lstatSync(join(dir, 'images', 'leak')).isSymbolicLink()); + await rm(dir, { recursive: true }); + await rm(outsideTarget, { force: true }); +}); + test('buildDeck does NOT sync deckmark internals (AGENTS.md, annotations/, .gitignore, etc.) into build/', async () => { const { mkdir, writeFile: w } = await import('node:fs/promises'); const { existsSync } = await import('node:fs'); From 3f4c9f0cbb67cf8c299cbe39235739dd7bc72f5a Mon Sep 17 00:00:00 2001 From: Hong Zhang Date: Mon, 25 May 2026 15:54:10 -0700 Subject: [PATCH 4/8] fix(engine): four asset-sync hardenings from Copilot review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Custom content filenames (Medium): the MCP layer accepts an arbitrary contentPath, so a deck with `slides.md` (not `content.md`) would have its source markdown copied verbatim into build/ and become web-accessible. Pass basename(opts.contentPath) into the sync and skip that entry explicitly; 'content.md' stays in the static set as a belt-and-suspenders default. 2. outDir basename collision (Medium): the skip used basename(buildDir) compared to entry names, which would drop a legitimate deck folder like `output/` on the floor whenever outDir happened to be named the same. Compare resolved absolute paths instead — only the literal buildDir is excluded. 3. Stale build/ entries (High): the sync explicitly does not delete extras, so a symlink placed in build/ before the symlink-rejection filter landed would persist across rebuilds and stay reachable via the static server's stat()/readFile(). Clean opts.outDir (rm + mkdir) at the top of buildDeck so each build starts from a known-clean tree. Comment makes the caller-trust assumption explicit (always invoked with an outDir under deckmark's own control). 4. Test path collision (Medium): the symlink security test used a target under the OS temp dir without per-run uniqueness, which would race under node:test's concurrent runner. Include basename(dir) in the outside-target filename. Plus three new regression tests covering #1, #2, #3 directly. Tests that need symlink() self-skip with EPERM/ENOSYS so CI stays green on Windows without Developer Mode. Co-Authored-By: Claude Opus 4.7 --- runtime/engines/reveal.ts | 40 ++++++++++++++--- test/unit/engines-reveal.test.ts | 75 +++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 9 deletions(-) diff --git a/runtime/engines/reveal.ts b/runtime/engines/reveal.ts index 3741ec9..297e54d 100644 --- a/runtime/engines/reveal.ts +++ b/runtime/engines/reveal.ts @@ -1,6 +1,6 @@ -import { mkdir, readFile, writeFile, readdir, cp, lstat } from 'node:fs/promises'; +import { mkdir, readFile, writeFile, readdir, cp, lstat, rm } from 'node:fs/promises'; import type { Dirent } from 'node:fs'; -import { dirname, join, basename } from 'node:path'; +import { dirname, join, basename, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { marked } from 'marked'; @@ -23,6 +23,10 @@ const __dirname = dirname(fileURLToPath(import.meta.url)); * (multi-file mode) places at `/vendor/reveal/`. */ const EXCLUDED_FROM_ASSET_SYNC = new Set([ + // The content markdown is excluded dynamically by basename(opts.contentPath) + // — it may be named anything (slides.md, talk.md, etc.), and the MCP layer + // lets callers override it. 'content.md' stays here as a belt-and-suspenders + // default for the common case. 'content.md', 'deckmark.config.json', 'annotations', @@ -72,20 +76,30 @@ async function rejectSymlink(src: string): Promise { * the published output. The static server has its own containment check, * but this is defense in depth. */ -async function syncUserAssetsToBuild(deckDir: string, buildDir: string): Promise { +async function syncUserAssetsToBuild( + deckDir: string, + buildDir: string, + contentBase: string +): Promise { let entries: Dirent[]; try { entries = await readdir(deckDir, { withFileTypes: true }); } catch { return; } - const buildBase = basename(buildDir); + // Compare resolved absolute paths — not basenames — so a deck that happens + // to contain a folder whose name matches the buildDir basename (e.g. an + // `output/` assets folder when outDir=/tmp/out/output/) doesn't get + // accidentally skipped. Only the literal buildDir is excluded. + const buildResolved = resolve(buildDir); for (const e of entries) { // Hidden / dotfiles never sync. if (e.name.startsWith('.')) continue; if (EXCLUDED_FROM_ASSET_SYNC.has(e.name)) continue; - // Don't sync the build dir itself (handles unusual outDir locations too). - if (e.name === buildBase) continue; + // Skip the content markdown (any filename — caller passes its basename). + if (e.name === contentBase) continue; + // Don't sync the build dir itself, even if it sits inside deckDir. + if (resolve(join(deckDir, e.name)) === buildResolved) continue; // Top-level .html and .tgz files are likely publish artifacts; skip. if (!e.isDirectory() && (e.name.endsWith('.html') || e.name.endsWith('.tgz'))) continue; // Fast path: skip top-level symlinks before recursing. @@ -225,12 +239,24 @@ ${sections.join('\n')} `; + // Clean the build dir before each build. Without this, stale entries from + // a previous build (especially symlinks placed there before the asset-sync + // hardening landed) would persist and remain reachable through the static + // server's stat()/readFile() — defeating the symlink-rejection filter. + // `rm` + `mkdir` is cheap and produces deterministic, byte-equal output for + // the same input. Callers always pass an outDir under their own control + // (e.g. /build/); never invoke with an outDir you don't own. + await rm(opts.outDir, { recursive: true, force: true }); await mkdir(opts.outDir, { recursive: true }); // Mirror user assets (images/, fonts/, etc.) from the deck folder into // build/ before writing index.html, so the rendered deck can reference // them via relative paths and so publish_deck (which reads from build/) // can find them for inlining or for multi-file deploy. - await syncUserAssetsToBuild(dirname(opts.contentPath), opts.outDir); + await syncUserAssetsToBuild( + dirname(opts.contentPath), + opts.outDir, + basename(opts.contentPath) + ); await writeFile(join(opts.outDir, 'index.html'), htmlDocument, 'utf8'); return { outDir: opts.outDir, slideCount: sections.length, style, mode, motion, slideNumbers: slideNumberValue }; } diff --git a/test/unit/engines-reveal.test.ts b/test/unit/engines-reveal.test.ts index 9ad3e31..2e36126 100644 --- a/test/unit/engines-reveal.test.ts +++ b/test/unit/engines-reveal.test.ts @@ -2,7 +2,7 @@ import { test } from 'node:test'; import { strict as assert } from 'node:assert'; import { mkdtemp, writeFile, readFile, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join, basename } from 'node:path'; import { buildDeck } from '../../runtime/engines/reveal.ts'; async function tmpDir() { @@ -130,7 +130,9 @@ test('buildDeck skips nested symlinks during asset sync (security: no /etc/passw await mkdir(join(dir, 'images'), { recursive: true }); await w(join(dir, 'images', 'real.jpg'), 'jpeg-bytes'); // Drop a target file outside the deck and try to symlink to it from inside. - const outsideTarget = join(dir, '..', 'sensitive.txt'); + // Name the target uniquely per run so concurrent tests don't collide on + // a shared path under the OS temp dir. + const outsideTarget = join(dir, '..', `sensitive-${basename(dir)}.txt`); await w(outsideTarget, 'SECRET'); try { await symlink(outsideTarget, join(dir, 'images', 'leak')); @@ -158,6 +160,75 @@ test('buildDeck skips nested symlinks during asset sync (security: no /etc/passw await rm(outsideTarget, { force: true }); }); +test('buildDeck does NOT copy a custom-named content file (e.g. slides.md) into build/', async () => { + const { writeFile: w } = await import('node:fs/promises'); + const { existsSync } = await import('node:fs'); + const dir = await tmpDir(); + // Caller passes an arbitrary contentPath via the MCP layer — make sure the + // sync excludes whatever basename was used, not just the literal 'content.md'. + await w(join(dir, 'slides.md'), SAMPLE_CONTENT); + await buildDeck({ contentPath: join(dir, 'slides.md'), outDir: join(dir, 'build') }); + assert.equal( + existsSync(join(dir, 'build', 'slides.md')), + false, + 'custom-named content markdown should NOT be copied into build/' + ); + // sanity: index.html still produced from it + assert.ok(existsSync(join(dir, 'build', 'index.html'))); + await rm(dir, { recursive: true }); +}); + +test('buildDeck does not accidentally skip a deck folder whose name matches the outDir basename', async () => { + const { mkdir, writeFile: w } = await import('node:fs/promises'); + const { existsSync } = await import('node:fs'); + const deckDir = await tmpDir(); + // The deck legitimately has a folder called "output/" — it must be copied. + await mkdir(join(deckDir, 'output'), { recursive: true }); + await w(join(deckDir, 'output', 'chart.png'), 'PNG'); + await w(join(deckDir, 'content.md'), SAMPLE_CONTENT); + // Choose an outDir whose basename collides with the deck's 'output/' folder + // but lives in a different parent — the old basename-based skip would have + // dropped the deck's output/ on the floor. + const outParent = await tmpDir(); + const outDir = join(outParent, 'output'); + await buildDeck({ contentPath: join(deckDir, 'content.md'), outDir }); + assert.ok( + existsSync(join(outDir, 'output', 'chart.png')), + "deck's output/ folder should be copied even when outDir basename is also 'output'" + ); + await rm(deckDir, { recursive: true }); + await rm(outParent, { recursive: true }); +}); + +test('buildDeck removes stale symlinks left in build/ from a prior build', async () => { + const { mkdir, writeFile: w, symlink } = await import('node:fs/promises'); + const { existsSync } = await import('node:fs'); + const dir = await tmpDir(); + await mkdir(join(dir, 'build', 'images'), { recursive: true }); + const outsideTarget = join(dir, '..', `stale-${basename(dir)}.txt`); + await w(outsideTarget, 'STALE_SECRET'); + try { + await symlink(outsideTarget, join(dir, 'build', 'images', 'leak')); + } catch (err: unknown) { + const e = err as NodeJS.ErrnoException; + if (e.code === 'EPERM' || e.code === 'ENOSYS') { + await rm(dir, { recursive: true }); + await rm(outsideTarget, { force: true }); + return; + } + throw err; + } + await w(join(dir, 'content.md'), SAMPLE_CONTENT); + await buildDeck({ contentPath: join(dir, 'content.md'), outDir: join(dir, 'build') }); + assert.equal( + existsSync(join(dir, 'build', 'images', 'leak')), + false, + 'stale symlink from a previous build must not survive a rebuild' + ); + await rm(dir, { recursive: true }); + await rm(outsideTarget, { force: true }); +}); + test('buildDeck does NOT sync deckmark internals (AGENTS.md, annotations/, .gitignore, etc.) into build/', async () => { const { mkdir, writeFile: w } = await import('node:fs/promises'); const { existsSync } = await import('node:fs'); From 5991c5b11aa8aeb4296a586359dcbaa427b71580 Mon Sep 17 00:00:00 2001 From: Hong Zhang Date: Mon, 25 May 2026 16:10:11 -0700 Subject: [PATCH 5/8] fix(security): four more hardenings from Copilot review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Destructive outDir clean guard (High): the new `rm(opts.outDir, { recursive: true })` in buildDeck could wipe the drive root or the deck source if outDir was mis-specified. Refuse two pathological shapes before invoking rm: - outDir is a filesystem root (dirname(resolved) === resolved) - opts.contentPath lives inside opts.outDir Both cases throw a descriptive error so misuse fails loud. 2. Inline CSS path traversal (Medium): a tampered HTML reference like `vendor/reveal/../../../etc/passwd` survived the regex prefix check and, after `resolve(REVEAL_DIST, file)`, landed outside the reveal dist. Added isUnderRevealDist() containment check before readFile so the inliner only reads files actually inside the reveal package. 3. Inline JS path traversal (Medium): same fix on the `); diff --git a/runtime/publish/multi-file.ts b/runtime/publish/multi-file.ts index 3a2bd3b..33129f0 100644 --- a/runtime/publish/multi-file.ts +++ b/runtime/publish/multi-file.ts @@ -1,5 +1,5 @@ // runtime/publish/multi-file.ts -import { mkdir, cp } from 'node:fs/promises'; +import { mkdir, cp, lstat } from 'node:fs/promises'; import { dirname, join } from 'node:path'; import { createRequire } from 'node:module'; @@ -9,6 +9,22 @@ const require = createRequire(import.meta.url); // node_modules when deckmark is installed via npx. See static-overlay.ts. const REVEAL_DIST = dirname(require.resolve('reveal.js/dist/reveal.js')); +/** + * cp() filter that rejects symlinks at every depth. The engine's asset sync + * already rejects symlinks before they reach buildDir, but multi-file may + * also be invoked with a buildDir produced by other tooling. A + * symlink-following web server (Apache/nginx default) would expose whatever + * the symlink points at, so we belt-and-suspenders it here too. + */ +async function rejectSymlink(src: string): Promise { + try { + const st = await lstat(src); + return !st.isSymbolicLink(); + } catch { + return false; + } +} + export interface MultiFileOpts { buildDir: string; outDir: string; @@ -22,10 +38,10 @@ export async function multiFile(opts: MultiFileOpts): Promise<{ outDir: string; // official reveal.js dist LAST. Whatever path collisions exist, // /vendor/reveal/* always reflects the real reveal.js — never user // content masquerading under the same path. - await cp(opts.buildDir, opts.outDir, { recursive: true, force: true }); + await cp(opts.buildDir, opts.outDir, { recursive: true, force: true, filter: rejectSymlink }); await mkdir(join(opts.outDir, 'vendor', 'reveal'), { recursive: true }); - await cp(REVEAL_DIST, join(opts.outDir, 'vendor', 'reveal'), { recursive: true, force: true }); + await cp(REVEAL_DIST, join(opts.outDir, 'vendor', 'reveal'), { recursive: true, force: true, filter: rejectSymlink }); // Walk the output to list what landed const { readdir } = await import('node:fs/promises'); diff --git a/test/unit/engines-reveal.test.ts b/test/unit/engines-reveal.test.ts index 2e36126..b4004b7 100644 --- a/test/unit/engines-reveal.test.ts +++ b/test/unit/engines-reveal.test.ts @@ -59,6 +59,35 @@ test('buildDeck de-duplicates slugs when titles collide', async () => { await rm(dir, { recursive: true }); }); +test('buildDeck refuses to clean an outDir that contains the deck source', async () => { + const dir = await tmpDir(); + await writeFile(join(dir, 'content.md'), SAMPLE_CONTENT); + // outDir = the deck dir itself. Without the guard, rm({ recursive: true }) + // would wipe content.md before reading it. The guard must catch it before + // any destructive call. + await assert.rejects( + () => buildDeck({ contentPath: join(dir, 'content.md'), outDir: dir }), + /refusing to clean outDir/i + ); + // Source survived the rejected call. + const stillThere = await readFile(join(dir, 'content.md'), 'utf8'); + assert.match(stillThere, /Slide One/); + await rm(dir, { recursive: true }); +}); + +test('buildDeck refuses to clean filesystem root', async () => { + const dir = await tmpDir(); + await writeFile(join(dir, 'content.md'), SAMPLE_CONTENT); + // Pick a platform-appropriate root. The check is path-based so no actual + // rm is attempted — we only need to confirm the guard fires. + const root = process.platform === 'win32' ? 'C:\\' : '/'; + await assert.rejects( + () => buildDeck({ contentPath: join(dir, 'content.md'), outDir: root }), + /refusing to clean filesystem root/i + ); + await rm(dir, { recursive: true }); +}); + test('buildDeck throws when content has no slides', async () => { const dir = await tmpDir(); await writeFile(join(dir, 'content.md'), '\n\n---\n\n'); diff --git a/test/unit/publish.test.ts b/test/unit/publish.test.ts index 024c426..11a67c4 100644 --- a/test/unit/publish.test.ts +++ b/test/unit/publish.test.ts @@ -36,3 +36,27 @@ test('multiFile writes deploy folder with index.html and vendor/reveal/', async assert.ok(r.files.some(f => f.startsWith('vendor/reveal/'))); await rm(dir, { recursive: true }); }); + +test('inlineHtml refuses path-traversal references like vendor/reveal/../../etc', async () => { + const dir = await setupDeck(); + // Tamper with the built HTML to inject a traversal reference. The + // resolve() against REVEAL_DIST would land outside the reveal dist, and + // without the containment guard the inliner would happily readFile() + // whatever local file the user pointed at. + const indexPath = join(dir, 'build', 'index.html'); + const orig = await readFile(indexPath, 'utf8'); + const tampered = orig + .replace('href="vendor/reveal/reveal.css"', 'href="vendor/reveal/../../../../../../../etc/passwd"') + .replace('src="vendor/reveal/reveal.js"', 'src="vendor/reveal/../../../../../../../etc/hosts"'); + await writeFile(indexPath, tampered, 'utf8'); + const outFile = join(dir, 'tampered.html'); + await inlineHtml({ buildDir: join(dir, 'build'), outFile }); + const html = await readFile(outFile, 'utf8'); + // The tampered hrefs should remain as plain /`); @@ -114,6 +116,11 @@ async function replaceImages(html: string, buildDir: string): Promise { } const file = src.replace(/^[/.]+/, ''); const filePath = resolve(buildDir, file); + // Containment guard: stripping leading '/' and '.' chars does NOT remove + // internal `..` segments, so `images/../../../etc/passwd` would still + // escape buildDir after resolve(). Reject anything that lands outside. + const buildResolved = resolve(buildDir); + if (!isUnder(buildResolved, filePath)) { dbg(` skip traversal: ${filePath}`); continue; } if (!existsSync(filePath)) { dbg(` skip missing local img: ${filePath}`); continue; } const ext = extname(filePath); const buf = await readFile(filePath); diff --git a/runtime/publish/multi-file.ts b/runtime/publish/multi-file.ts index 112af8e..56f211a 100644 --- a/runtime/publish/multi-file.ts +++ b/runtime/publish/multi-file.ts @@ -1,6 +1,6 @@ // runtime/publish/multi-file.ts import { mkdir, cp, lstat } from 'node:fs/promises'; -import { dirname, join } from 'node:path'; +import { dirname, join, basename } from 'node:path'; import { createRequire } from 'node:module'; const require = createRequire(import.meta.url); @@ -25,6 +25,16 @@ async function rejectSymlink(src: string): Promise { } } +/** + * cp() filter for buildDir → outDir: rejects symlinks AND the internal + * .deckmark-build marker so the published folder doesn't ship deckmark's + * "this dir is ours to clean" signal. + */ +async function rejectSymlinkAndMarker(src: string): Promise { + if (basename(src) === '.deckmark-build') return false; + return rejectSymlink(src); +} + /** * Throw a clear error if `path` exists and is not a directory. Used before * `mkdir({ recursive: true })` calls that would otherwise fail with ENOTDIR @@ -59,7 +69,7 @@ export async function multiFile(opts: MultiFileOpts): Promise<{ outDir: string; // official reveal.js dist LAST. Whatever path collisions exist, // /vendor/reveal/* always reflects the real reveal.js — never user // content masquerading under the same path. - await cp(opts.buildDir, opts.outDir, { recursive: true, force: true, filter: rejectSymlink }); + await cp(opts.buildDir, opts.outDir, { recursive: true, force: true, filter: rejectSymlinkAndMarker }); // After the buildDir copy, the reveal dist is overlaid at /vendor/reveal/. // If buildDir happened to contain a *file* (not a directory) at either diff --git a/test/unit/engines-reveal.test.ts b/test/unit/engines-reveal.test.ts index b4004b7..00a89d8 100644 --- a/test/unit/engines-reveal.test.ts +++ b/test/unit/engines-reveal.test.ts @@ -75,6 +75,40 @@ test('buildDeck refuses to clean an outDir that contains the deck source', async await rm(dir, { recursive: true }); }); +test('buildDeck refuses to clean a non-empty outDir without the .deckmark-build marker', async () => { + const { mkdir, writeFile: w } = await import('node:fs/promises'); + const { existsSync } = await import('node:fs'); + const dir = await tmpDir(); + await writeFile(join(dir, 'content.md'), SAMPLE_CONTENT); + // Pretend outDir is a user-owned folder with existing data — no marker. + const outDir = join(dir, 'random-user-folder'); + await mkdir(outDir, { recursive: true }); + await w(join(outDir, 'family-photo.jpg'), 'JPEG_BYTES'); + await w(join(outDir, 'taxes.pdf'), 'PDF_BYTES'); + await assert.rejects( + () => buildDeck({ contentPath: join(dir, 'content.md'), outDir }), + /no \.deckmark-build marker/i + ); + // Pre-existing user data survived. + assert.ok(existsSync(join(outDir, 'family-photo.jpg'))); + assert.ok(existsSync(join(outDir, 'taxes.pdf'))); + await rm(dir, { recursive: true }); +}); + +test('buildDeck cleans an outDir on repeated builds (marker round-trip)', async () => { + const { existsSync } = await import('node:fs'); + const dir = await tmpDir(); + await writeFile(join(dir, 'content.md'), SAMPLE_CONTENT); + // First build: outDir doesn't exist → succeeds, drops marker. + await buildDeck({ contentPath: join(dir, 'content.md'), outDir: join(dir, 'build') }); + assert.ok(existsSync(join(dir, 'build', '.deckmark-build'))); + // Second build: outDir exists with marker → rm + rebuild → succeeds. + await buildDeck({ contentPath: join(dir, 'content.md'), outDir: join(dir, 'build') }); + assert.ok(existsSync(join(dir, 'build', '.deckmark-build'))); + assert.ok(existsSync(join(dir, 'build', 'index.html'))); + await rm(dir, { recursive: true }); +}); + test('buildDeck refuses to clean filesystem root', async () => { const dir = await tmpDir(); await writeFile(join(dir, 'content.md'), SAMPLE_CONTENT); @@ -234,6 +268,9 @@ test('buildDeck removes stale symlinks left in build/ from a prior build', async const { existsSync } = await import('node:fs'); const dir = await tmpDir(); await mkdir(join(dir, 'build', 'images'), { recursive: true }); + // Marker tells buildDeck "this dir is yours to clean" — simulates the + // state left behind by a previous successful build. + await w(join(dir, 'build', '.deckmark-build'), ''); const outsideTarget = join(dir, '..', `stale-${basename(dir)}.txt`); await w(outsideTarget, 'STALE_SECRET'); try { diff --git a/test/unit/publish.test.ts b/test/unit/publish.test.ts index 774f42b..c54ae24 100644 --- a/test/unit/publish.test.ts +++ b/test/unit/publish.test.ts @@ -51,6 +51,29 @@ test('multiFile fails clearly when buildDir has a FILE at vendor (not a director await rm(dir, { recursive: true }); }); +test('inlineHtml refuses traversal img src like images/../../../../etc/passwd', async () => { + const dir = await setupDeck(); + // Inject an with a traversal src. Stripping leading '/' and '.' + // characters doesn't kill internal `..` segments, so resolve(buildDir, ...) + // walks out of buildDir without the new containment guard. + const indexPath = join(dir, 'build', 'index.html'); + const orig = await readFile(indexPath, 'utf8'); + const tampered = orig.replace( + '
', + '
\n' + ); + await writeFile(indexPath, tampered, 'utf8'); + const outFile = join(dir, 'tampered-img.html'); + await inlineHtml({ buildDir: join(dir, 'build'), outFile }); + const html = await readFile(outFile, 'utf8'); + // The img tag should still reference the traversal path (untouched), NOT + // a base64 data URI of /etc/passwd contents. + assert.match(html, /]+src="images\/\.\.\//); + assert.doesNotMatch(html, /data:application\/octet-stream;base64,/); + assert.doesNotMatch(html, /root:x:/); + await rm(dir, { recursive: true }); +}); + test('inlineHtml refuses path-traversal references like vendor/reveal/../../etc', async () => { const dir = await setupDeck(); // Tamper with the built HTML to inject a traversal reference. The