engine fix: synch user assets folder better#4
Merged
Conversation
…der into build/
Real-use bug from the v1.0.0 release: a deck whose content.md
referenced `images/1-0.jpg` rendered with broken <img> 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a v1.0.0 engine/publish issue where decks that reference local assets (e.g. images/...) render broken links because only build/index.html was written and the build output lacked the referenced files.
Changes:
- Add an asset-sync step in the reveal engine build that copies non-excluded top-level deck entries into
build/before writingindex.html. - Add unit tests asserting
images/(and nested assets) are mirrored intobuild/, while deckmark internals (e.g.annotations/, agent files) are excluded.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
runtime/engines/reveal.ts |
Introduces syncUserAssetsToBuild() and calls it during buildDeck to mirror deck assets into build/. |
test/unit/engines-reveal.test.ts |
Adds unit tests covering asset mirroring and exclusion behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…multi-file publish
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <script src>
inliner — single helper, applied to both replaceLinkStylesheets and
replaceScripts.
4. multi-file symlink copy (Medium): cp(buildDir, outDir) preserved any
symlinks in buildDir verbatim. Symlink-following web servers (Apache,
nginx default) would then expose whatever those links point at when
the published folder is hosted. Pass the same rejectSymlink filter
used by the engine's asset sync; applied to both the buildDir copy
and the reveal dist copy as defense in depth.
Regression tests:
- buildDeck rejects outDir = deck dir (verifies content.md survives)
- buildDeck rejects outDir = filesystem root
- inlineHtml ignores tampered traversal hrefs and srcs (logs prove the
guard fires before readFile)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cp(buildDir → outDir) would happily place a plain file at <outDir>/vendor
if buildDir contained one, and the subsequent mkdir(vendor/reveal, {
recursive: true }) would throw ENOTDIR with no context — leaving the
caller to debug which path was the problem.
The engine's asset sync already excludes 'vendor' so deckmark itself
never produces this shape, but multiFile is callable with any buildDir
(other tooling, manual builds, tests). Add an assertDirOrAbsent() probe
on both <outDir>/vendor and <outDir>/vendor/reveal before mkdir, so a
collision fails with a descriptive message pointing at the offending
path.
Regression test covers the file-at-vendor case.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
#65 (High) — replaceImages() in inline-html.ts had no containment check.
Stripping leading '/' and '.' chars doesn't remove internal '..' segments,
so an <img src="images/../../../../etc/passwd"> would, after
resolve(buildDir, ...), escape buildDir and get base64-inlined into the
single-file HTML. Generalize isUnderRevealDist() → isUnder(root, path)
and apply it to image inlining too. Same attack class as the reveal
sinks, just a different root.
#66 (High) — buildDeck's unconditional rm(opts.outDir) was guarded only
against fs roots and outDir-contains-contentPath. A mis-specified
outDir like ~/Documents would still get wiped. Add a marker-file
ratchet: drop '.deckmark-build' inside outDir on each build; on the
next build, refuse to clean if outDir is non-empty and the marker is
missing. First-build flow (outDir doesn't exist or is empty) still
works. Catches catastrophic data loss without breaking legitimate
repeated builds. Multi-file publish strips the marker via a new
rejectSymlinkAndMarker cp filter so it doesn't leak into published/.
Regression tests:
- replaceImages ignores ../-escaping img srcs (logs prove guard fires)
- buildDeck rejects non-empty outDir without marker (and pre-existing
user files survive the rejected call)
- buildDeck round-trips the marker across two consecutive builds
- Pre-existing stale-symlink test updated to plant the marker so it
still simulates a previous-deckmark-build state
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Real-use bug from the v1.0.0 release: a deck whose content.md referenced![]()
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.
images/1-0.jpgrendered with brokenbuild_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:
33/33 tests pass on Node 22.