fix(viewer): complete exhibit markdown rendering (styling, in-block scroll, link-scheme XSS guard) (#32) - #45
Merged
Conversation
Add a client-side markdown renderer for exhibit bodies (figures/tables) in the viewer's RESULT block, using pulldown-cmark with the GFM tables + strikethrough extensions. Raw HTML in a body is neutralised (re-emitted as escaped text) so the rendered string is safe to mount via Leptos `inner_html`. Math ($…$) stays literal, consistent with D3. Plumb `Exhibit.body` (already carried by ara-core) through `ExhibitView` and emit the rendered HTML under the evidence chips. Bundle-size spike (wasm-release, wired so it is not DCE'd) passes the CI gate: +160 KB uncompressed / +56 KB brotli, landing at 65% / 64% of the 1 MB / 350 KB budgets. Includes the plan doc with the measured numbers. Remaining: responsive .exhibit-body scroll CSS, web tests, embed regen, version bump + changelog.
The initial #32 render shipped the exhibit-body <table> markup but not the CSS/responsive layer (plan steps 5-7), so exhibit tables were unstyled and could overflow the 70ch detail column. Add the .exhibit-body scroll container and share the markdown data-table styling with the rendered tables, correct the stale RESULT-block comment, and add a web test asserting the table renders inside the scroll wrapper. Bump to 0.1.11, roll the changelog, and regenerate the embedded viewer bundle.
…down (#32) The client-side exhibit-body renderer escaped raw HTML but passed markdown link/image destinations to push_html unfiltered, so a downloaded artifact could smuggle a clickable [x](javascript:...) or  sink through inner_html. Add an is_safe_url scheme allowlist (http/https/mailto + relative) and rewrite unsafe Tag::Link/Tag::Image destinations to "#". Whitespace/control chars are stripped first (the <...> dest form delivers a tab into the parsed scheme) and entity encodings are decoded by pulldown-cmark before the guard runs, both covered by regression tests. Also add a blank-body web test and regenerate the embedded viewer bundle.
fenfenai
approved these changes
Jul 20, 2026
fenfenai
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
Reviewed 16 files (~620 additions) across bug-detection, error-handling, type-design, test-coverage, comment-quality, and guidelines-compliance aspects. The core change is a client-side GFM markdown renderer for exhibit bodies with an XSS link/image-scheme guard, plus the CSS/scroll layer that #32 left unshipped.
Verdict: Approve. No findings at confidence ≥75.
What I verified empirically (on the branch)
cargo test -p ara-viewer --lib markdown→ 9 passed.- Adversarially probed the guard beyond the included tests — all neutralised:
- Autolink
<javascript:alert(1)>→href="#" - Reference-style link (
[x][r]/[r]: javascript:…) →href="#" - Raw HTML inside a table cell (
<img onerror=…>) → escaped to text - Newline-in-scheme → link doesn't parse, rendered as literal text
- Autolink
is_safe_url's first-colon +/,?,#-in-scheme heuristic correctly classifies relative URLs (?q=a:b,#anchor,./x) as safe.- Embedded bundle is consistent:
index.htmlasset hashes (3b9263f1…js/wasm,e6588b2e…css) match the files on disk;viewer.source-hashpresent. - Version bumped
0.1.10 → 0.1.11; CHANGELOG has bothFixedandSecurityentries;pulldown-cmarkpulled withdefault-features = false.
Strengths
- The
Event::Html | Event::InlineHtml → Event::Textre-emission closes raw HTML, and the scheme guard covers allTag::Link/Tag::Imagestart events (inline, reference, and autolink forms alike) — no bypass found. - Control-char stripping in
is_safe_urlmatches browser URL-parsing behavior (ASCII tab/newline/controls), and the tests lock in the non-obvious cases (<…>tab evasion,:/:entity decoding). - Math extension deliberately left off so
$…$stays inert — consistent with the existinglatex_viewposture, and documented. - Empty-body guard (
!body.trim().is_empty()) is tested to skip the container.
Minor note (below threshold, non-blocking)
- The allowlist rewrites all
data:URLs to#, includingdata:image/*. That's a defensible security posture (blocksdata:text/html), but a figure exhibit whose markdown embeds a legitimate base64 inline image would silently render a brokensrc="#". Exhibit figures typically reference image files by relative path, so this is unlikely to bite — just flagging in case inline data-image figures are expected. If they are, consider allowingdata:image/*specifically.
Collaborator
Author
|
CC @AmberLJC demo markdown.mp4 |
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.
Summary
Completes the CSS/responsive layer and closes a residual XSS vector in the
client-side exhibit-body markdown renderer from #32. The initial #32 merge
shipped the
<table>markup but not the styling/scroll layer, and neutralizedraw HTML but not markdown's own link/image sinks.
Two commits on this branch:
data-table styling (borders, cell padding, header shading) and sit in an
.exhibit-body { overflow-x:auto }container so wide tables scroll insidetheir block instead of overflowing the 70ch detail column. Corrects the stale
RESULT-block comment. (Acceptance criteria Add real-ARA no-panic regression coverage (vendored subset + opt-in submodule sweep) #3, Real-ARA no-panic regression coverage (#3) #4.)
is_safe_urlallowlistshttp/https/mailto(plus relative URLs) and rewrites any other scheme (
javascript:,data:,vbscript:, …) to#. Whitespace/control chars are stripped first (the<...>destination form can deliver a tab into the parsed scheme) and entityencodings (
:,:) are decoded by pulldown-cmark before the guardruns. Both behaviors were verified empirically and are locked in by tests.
Why it matters
ara servecan render a downloaded artifact, so exhibit bodies are notalways self-authored. Without the guard, a hostile
evidence/tables/*.mdwith[click](javascript:...)becomes click-to-execute in the viewer origin.Tests
markdown.rsunit tests (js link, data image, control-char<...>evasion, entity-encoded scheme, safe/relative links preserved,
is_safe_urlclassification).
web.rstests: table renders inside.exhibit-body; empty body rendersthe chip but no body container.
cargo test -p ara-core -p ara-viewer --lib→ 278 passed. Web tests compileclean for wasm32; the headless-Chrome run is exercised by the
viewer-web-testCI job.
embed-viewer.sh --checkgreen.Version bumped to 0.1.11; CHANGELOG updated (
Fixed+Security).Closes #32.