Repository navigation
Conversation
Zburgers
left a comment
Zburgers
left a comment
There was a problem hiding this comment.
Reviewed head 75c16f9b896ea73527880b2010014f36917c0ce7.
Do not merge yet. The validation job is green, but I found three release blockers that are not covered end-to-end by the current tests:
- Semantic
==highlight==markup is removed by Preview's search pass. - Footnote refs/backrefs no longer navigate.
- Raw HTML can forge a task control that mutates + auto-saves another source line.
I left each failure path inline. Please fix these and add interaction-level regressions, then re-request review.
| useEffect(() => { | ||
| const root = containerRef.current; | ||
| if (!root || !result) return; | ||
| highlightText(root, searchQuery); |
There was a problem hiding this comment.
Blocker — this search pass destroys the new semantic ==highlight== feature. highlightText() begins by calling clearHighlights(), and that helper currently removes every <mark> under the preview. v1.3 now deliberately emits ordinary <mark> for ==highlight==, so even searchQuery="" strips the semantic mark immediately; searching/clearing strips it again.
Please distinguish search-created marks from document marks (for example mark[data-mdview-search]) and only clear those. Add a Preview-level regression that renders ==important== with an empty query, then searches and clears, and verifies the semantic mark survives.
| return; | ||
| } | ||
|
|
||
| if (classified.kind === "anchor") { |
There was a problem hiding this comment.
Blocker — footnote refs/backrefs cannot navigate. Marked-footnote emits #footnote-... / #footnote-ref-... links and the sanitizer intentionally preserves those IDs, but every anchor is intercepted here and anchor navigation is now resolved exclusively through result.headings. Footnote IDs are not heading metadata, so both directions become no-ops.
Keep generated-heading lookup, then fall back to a safely scoped lookup for the preserved internal footnote IDs and scroll that element. Please add interaction tests for reference → footnote and backlink → reference; the existing footnote tests only validate generated HTML.
| ], | ||
| ALLOWED_ATTR: [ | ||
| "align", "alt", "aria-describedby", "aria-label", "checked", "class", "data-block", "data-callout", "data-fold", | ||
| "data-footnote-backref", "data-footnote-ref", "data-footnotes", "data-heading", "data-language", "data-line", "data-math", |
There was a problem hiding this comment.
Blocker — untrusted raw HTML can forge mdview's interactive task metadata and mutate disk state. Because class, data-line, disabled, and checkbox inputs survive sanitization, a document can supply raw HTML like <li class="task-list-item"><input disabled type="checkbox" data-line="42">...</li>. Preview later enables every checkbox matching that reserved selector; clicking this forged control calls onToggleTask(42), and App toggles + auto-saves that real source line when it is a task.
Reserved interaction metadata/classes need provenance protection rather than being globally forgeable by raw HTML. Strip/rescope these reserved values from raw input and attach them only to renderer-generated task nodes (the pre-sanitize task HTML regex should be hardened for the same reason). Add a hostile-raw-HTML regression proving a forged checkbox cannot edit any source line.
Zburgers
left a comment
Zburgers
left a comment
There was a problem hiding this comment.
Additional blocker found while tracing renderer provenance and navigation.
| export function createRenderSession(markdown: string): RenderSession { | ||
| let mathPrefix = "__MDVIEW_KATEX_"; | ||
| while (markdown.includes(mathPrefix)) mathPrefix += "_"; | ||
| let headingMarkerPrefix = "__MDVIEW_HEADING_"; |
There was a problem hiding this comment.
Blocker — the heading marker can be forged through entity decoding, so raw HTML can steal generated heading IDs. The collision check only searches the raw Markdown for the literal __MDVIEW_HEADING_ prefix. A document can use raw HTML such as <h1 class="__MDVIEW_HEADING_0">Fake</h1> followed by a real Markdown heading. The literal prefix is absent from the source, but HTML parsing/sanitization decodes _ to _. assignHeadingIds() then sees the forged class, assigns it the real heading's ID, and the genuine generated heading receives the same ID too. Outline / same-document navigation can land on the forged node.
This also undermines the security-doc claim that untrusted input cannot choose heading IDs. Please make renderer provenance non-forgeable (for example a per-render nonce or a pipeline that strips/reserves renderer metadata before raw HTML can carry it through) and add a regression using entity-encoded marker text. The math placeholder prefix uses the same raw-source collision strategy and should be audited at the same time.
Markdown now compiles once into a sanitized render result shared by Preview and Outline. Search, theme, and Outline changes reuse it. Stable heading IDs support same-document anchors and Rust-authorized relative Markdown navigation.
This implements the merged v1.3 rendering plan: parent-relative local images and visible failures, image lightbox, syntax highlighting and copy controls, expanded callouts, highlights, accessible footnotes, bounded math, generic fenced Mermaid rendering with expanded/source-copy controls, and contained tables/reader styling. Markdown remains untrusted, Mermaid stays strict, remote images stay opt-in, and local resources cross the Rust authorization boundary.
Final review fixed desktop Outline docking, enhanced-DOM persistence across rerenders, equal-HTML copy-control cleanup, and SVG-native Mermaid labels protected against document configuration overrides. Independent plan and quality reviews found no remaining blockers.
Validation:
Version sources are prepared as 1.3.0 and the branch includes the latest main release pipeline. No tag, PR merge, or publication was performed. Native Windows/macOS package checks and signed release artifacts remain gates in the release workflow.
See v1.3.0 qualification notes for the evidence and platform matrix.