Sanitize rendered HTML from content, and require review on content changes - #29
Merged
Merged
Conversation
Content is rendered with raw HTML passed through verbatim: parse.ts uses
`remarkRehype({ allowDangerousHtml: true })` and ofm.ts then runs `rehypeRaw`
unconditionally, turning whatever a page contains into real markup. Nothing
sanitized it, and the result is emitted as static HTML to the published site.
That is a bigger deal here than in a hand-written wiki. Per the README these
pages are drafted and refreshed by an LLM agent reading upstream repositories
and specifications, so a page is untrusted input -- a hostile upstream README,
or a single unreviewed content change, is enough. Checked against a scratch
page: an `onerror` handler, a `<script>` element, `javascript:` hrefs from both
markdown and raw links, and an `<iframe srcdoc>` all reached the output intact.
`rehype-sanitize` now runs immediately after `rehypeRaw`, so every later plugin
only ever sees vetted markup. The schema starts from the default (GitHub's
rules) and adds exactly what Quartz itself emits before the sanitizer runs:
callouts with arbitrary types and fold state, wikilinks, aliases, tag links and
transclusions, rehype-pretty-code's figures and shiki custom properties,
mermaid with its clipboard payload, the media the content uses, footnotes, task
lists and highlights. KaTeX, heading anchors, link crawling, YouTube embeds and
block references all run after the sanitizer and are untouched by it.
Two choices worth noting: `<style>` is stripped with its contents rather than
unwrapped, so CSS cannot become visible text; and `clobber` is narrowed to
`name`, because remark-rehype already prefixes footnote ids with
`user-content-` and prefixing them twice breaks the footnote links.
Output was compared page by page against a build of the previous behaviour.
One page of 42 changes, by 13 bytes: coordinated-releases.md contains the
literal text "Tagged at the main-branch HEAD on <date>", and `<date>` is not an
HTML element, so an empty unknown element was being emitted where the
placeholder was meant to read. It rendered as nothing before and is absent now.
Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
Adds fixture pages and a test so the sanitizer cannot quietly regress. The payload page carries the shapes worth worrying about: event handlers, `script` and `style` elements, `javascript:` in a markdown link, a raw link and an entity-encoded href, `iframe srcdoc`, `svg onload`, `form` with `formaction`, `object`, `embed`, `meta refresh`, `base`, `ontoggle`, and payloads smuggled through highlights, wikilink aliases and callout titles. A second page exercises the markup Quartz legitimately produces, and a third is a tag page, since tag pages render through the same path as page bodies. The test builds those fixtures with the real pipeline and fails if any script-capable markup survives. It also fails if Quartz's own markup stops surviving, so a schema change that over-blocks is caught by the same run rather than discovered on the live site. The detector is itself asserted against known-bad markup, so it cannot silently stop detecting anything. There was no workflow running the test suite, so add one for pushes to v4 and for pull requests, with its actions pinned to commit SHAs. The fixtures are excluded from prettier: their markup is deliberately malformed and has to reach the parser exactly as written. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
Sanitizing the rendered output handles the markup; this handles the provenance. Content here is drafted by an agent from upstream sources, and with no CODEOWNERS a content change could reach the published site without a human looking at it. Requires review from the maintainers repository-wide, from maintainers and admins together on content, and from admins on the workflows and repository configuration. Both teams are the ones already granted access to this repository in OpenVTC/governance. Signed-off-by: Glenn Gore <glenn.g@affinidi.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.
Targets
v4.The problem
Markdown in
content/is rendered with raw HTML passed straight through:quartz/processors/parse.tsusesremarkRehype({ allowDangerousHtml: true })and
quartz/plugins/transformers/ofm.tsthen runsrehypeRawunconditionally,which parses that raw HTML into real markup. Nothing sanitized it anywhere in
the pipeline, and the result is emitted as static HTML to the published site.
That matters here more than it would in a hand-written wiki. Per the README this
is an LLM-maintained wiki: pages are drafted and refreshed by an agent reading
upstream repositories and specifications. A page is therefore untrusted input —
a hostile upstream README, or one unreviewed content change, is enough. And
there was no
CODEOWNERS, so no human review was required on the way in.Confirmed on
v4with a scratch page (not committed) containing four payloads.All four reached the emitted HTML intact:
The frontmatter
titleanddescriptionwere reflected into<meta>tags aswell, though escaped there — see below.
The fix
rehype-sanitizenow runs immediately afterrehypeRaw, so every plugin afterit only ever sees vetted markup. The schema lives in
quartz/util/sanitize.tsand starts from
rehype-sanitize's default (GitHub's rules), adding exactly themarkup Quartz itself produces before the sanitizer runs, and nothing else:
data-callouttypes, metadata and fold staterehype-pretty-codeoutput —figure/figcaption/pre/spanwith the--shiki-light/--shiki-darkcustom properties, matched by a pattern ratherthan allowing
stylegenerallydata-clipboardpayloadimg,video,audio, andiframerestricted to
srcplus thepdfclassmarkhighlights,abbrTwo details worth a reviewer's eye:
<style>is stripped with its contentsrather than unwrapped, so CSS never becomes visible text; and
clobberisnarrowed to
nameonly, becauseremark-rehypealready prefixes footnote idswith
user-content-and prefixing them twice breaks footnote links.KaTeX, heading anchors, link crawling, YouTube embeds and block references all
run after the sanitizer and are unaffected by it.
Head.tsxandTagContent.tsxwere both checked.Head.tsxpasses thefrontmatter description through
unescapeHTMLinto<meta content=…>, butPreact escapes attribute values on render, so the payload lands inert — the
fixture page carries payloads in both
titleanddescriptionand theregression test scans the whole document,
<head>included, so this is nowcovered by a test rather than by argument.
TagContent.tsxrenders through thesame
htmlToJsxpath as page bodies, so the sanitizer covers it; the fixtureset includes a tag page with payloads to prove it.
No content loss
Built
v4and this branch from the same 42 content files, normalized away theonly per-build variation (the
explorer-NN/toc-NNinstance counters — notimestamps or build ids reach the output), and compared all 165 emitted HTML and
XML files. The normalizer was validated first by confirming two separate
v4builds normalize to byte-identical output.
One page of 42 differs, by 13 bytes:
content/concepts/coordinated-releases.mdcontains the literal textTagged at the main-branch HEAD on <date>.—<date>is not an HTML element,so
rehypeRawparsed it into an unknown empty element andv4emitted<date></date>into the page. The sanitizer drops it. Rendering is identicaleither way, since an unknown inline element with no children displays nothing;
the placeholder was already invisible on the live site. Every other
<…>-shaped placeholder in the content (<hex>,<did>,<scid>,<crate>and friends) sits inside backticks, so it is code, never parsed as HTML, and is
untouched.
Separately, the whole feature fixture was built with and without the sanitizer
and all 27 expected markup snippets — callouts, aliases, transclusions, block
references, sized image embeds, PDF/video/audio/YouTube embeds, shiki output,
mermaid, KaTeX inline and display, footnotes, task lists, highlights — are
present in both. The sanitizer removes none of them.
Regression check
quartz/util/fixtures/sanitize/holds a payload page (22 payloads: eventhandlers,
script,style,javascript:in markdown and raw links andentity-encoded form,
iframe srcdoc,svg onload,form/formaction,object,embed,meta refresh,base,ontoggle, payloads insidehighlights, wikilink aliases and callout titles), a feature page exercising
Quartz's legitimate output, and a tag page.
quartz/util/sanitize.test.tsbuilds those fixtures with the real pipeline andfails if any event handler,
srcdoc,meta refresh, script-capable URL orembedding element survives — and separately fails if Quartz's own markup stops
surviving, so a future schema change that over-blocks is caught too. The checker
itself is tested against known-bad markup so it cannot silently stop detecting.
The repository had no workflow running
npm test, so.github/workflows/ci.yamladds one on pushes to
v4and on pull requests, with its actions pinned tocommit SHAs. The fixtures are added to
.prettierignore, since their markup isdeliberately malformed and must reach the parser exactly as written.
CODEOWNERS
.github/CODEOWNERSrequires review from@OpenVTC/openvtc-maintainersrepository-wide, from both that team and
@OpenVTC/openvtc-adminson/content/, and from@OpenVTC/openvtc-adminson/.github/. Both teams existand are the ones granted access to this repository in
OpenVTC/governance.Sanitizing the output and requiring a human on content changes address the same
finding from two directions.
Verification
npx quartz buildsucceeds; 216 files emitted from 42 input files.npx tsx --test: 73 tests, all passing (69 pre-existing plus 4 new).tsc --noEmitandprettier --checkmatchv4exactly — one pre-existingmoduleResolution=node10deprecation and 45 pre-existing Markdown formattingwarnings, neither introduced here.
npm run checktherefore fails onv4today and fails identically here; worth a separate cleanup.
Divergence from upstream
ofm.ts,package.jsonand the newquartz/util/sanitize.tsdiverge fromjackyzha0/quartz, which ships no sanitizer at all, sonpx quartz updatewillconflict in
ofm.ts. Upstream would likely want this behind a plugin option;this repository wants it on unconditionally, which is a reasonable place to
differ.
CODEOWNERS,ci.yamland.prettierignoreare this repository's ownfiles and carry no upstream cost.
quartz/plugins/transformers/oxhugofm.tsalso callsrehypeRawand is leftalone deliberately: it is not in
quartz.config.ts, so it renders nothing onthis site. It would need the same treatment before anyone enabled it.