-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5522
bhamodi · open, approve posted · reviewed at 428cc07f216 · view on GitHub
Verdict: approve — the presentation's hold was the repo's own review-required gate, which needs an eng owner's signature; Cindy is that owner and has given it
Someone reading rendered markdown they did not write — an AI response, a comment, a document — gets an image whose src is a scheme the component promises to reject. On main, ![logo][l] with [l]: <java\tscript:alert(1)> puts a literal <img src="java\tscript:alert(1)"> into their page: the parser never ran its URL rule on reference-image or block-image srcs, and the render-side guard trimmed but did not strip control characters, so the tab hid the scheme from both anchored checks. Verified in Chromium — the before frame shows the broken-image icon, and the DOM read returns that exact src.
Honest bound on the harm: a javascript:/vbscript: URL in a native <img src> does not execute in any current browser, so this is a bypass of a documented rule rather than a live script execution. It matters because the same value also reaches a components.image override and the public parseMarkdown AST, where a consumer may put it somewhere that does navigate.
(2 decisions · ~14 runtime lines of 58)
- Every image emit site requires
isSafeUrl(src)— reference full/collapsed (parser.ts:372), reference shortcut (:382), standalone block (:1425); an unsafe block image falls through to the paragraph path. -
sanitizeUrlstrips[\x00-\x1f\x7f]before testing and returns the normalized value (Markdown.tsx:563).
Markdown lets you write an image several different ways, and each way is turned into a picture by a different piece of code; only some of those pieces stopped to ask whether the address was safe. This makes every one of them ask the same question, and it makes the second, drawing-time check tidy up invisible characters before it looks — so an address wearing a disguise cannot walk past a check that only recognises the undisguised spelling. Decision 1 is one judgement applied at three sites, not three. Both trace to sentences in the PR body, and the fix(Markdown) type is honest: consumer-visible behaviour changed, and the changeset is a patch on @astryxdesign/core.
Markdown/parser.ts owns which text becomes a link or image node and Markdown/Markdown.tsx owns what reaches the DOM; the implementation lives with both owners and survives every seam that could be enumerated — onLinkClick receives safeHref (Markdown.tsx:894), components.link receives safeHref (:874), and components.image receives safeSrc at both call sites (:934, :1574). Every path a URL takes to the DOM goes through sanitizeUrl; all three call sites were read.
The seam the diff does not close is the review's one note. The rule now exists twice in this folder with different scheme lists: parser.ts:429-433 rejects javascript:, vbscript: and data:text/html; Markdown.tsx:561 rejects javascript:, vbscript: and every data:. That drift has already happened, and the same author's #5524 is landing a third copy in packages/core/src/utils/safeUrl.ts — its body names unification as a deliberate follow-up "once in-flight Markdown work lands", i.e. this PR. Known, owned, and not this diff's to fix.
Everyone on the next release, but only inside a document that contains an image URL the rule rejects. For an ordinary document nothing changes at all — the two control cases in the frames, an https:// image and an https:// link, are byte-identical before and after.
Two people notice:
-
A reader of untrusted markdown. A disguised
javascript:image src stops being written into their page. Onmainit was: the before frame shows a broken-image icon where nothing should have rendered. -
A reader of any document containing a rejected image. Where they used to see
[alt]they now see the raw markdown line —. The author's alt text is still in there, with the source around it. Only reachable for content the rule rejects, and it matches what a rejected inline image already did onmain, so it is a consistency change rather than a new appearance.
Landing it newly exposes nothing: the one adjacent gap, the data: divergence, renders identically before and after.
No API change. sanitizeUrl and isSafeUrl are both module-private — neither carries an export — parseMarkdown's return type is untouched, and no prop, export, type or default moves. Nothing new for a consumer to write:
<Markdown>{markdownSource}</Markdown>No new theme targets; structural only, and the style grep over the added lines returns 0. One narrow consequence, since a target is a promise about which element exists: an unsafe standalone image used to render the astryx-markdown-image div and now renders astryx-markdown-paragraph. A theme selecting markdown-image stops matching — for rejected content only. Every safe image still emits the target (Markdown.tsx:1563, :1582).
Nothing ossifies. No new export, no new helper, no new internal abstraction beside a shipped one — decision 2 is a normalization added inside an existing private function rather than a new one. The one thing worth naming: this diff deliberately does not add a shared util, which is what keeps it out of #5524's way.
- API — no. Nothing exported changes; no call site stops compiling or changes meaning.
-
Visual — yes, three cases. An unsafe image renders the raw markdown line instead of
[alt], and the previously-rendered<img>for the control-character case disappears. Deliberate, and the PR body says so: "unsafe srcs fall through to the paragraph path and render as literal text — the same behavior as an unsafe inline image or link." - Theme — yes, narrowly: the element a rejected image renders changes class, as above.
-
Behaviour — the diff touches a conditional and adds an early-return path, so both sides of the boundary were driven, plus the far side of the normalization: safe src (unchanged), unsafe src inline / reference-full / reference-shortcut / standalone block (all now literal text), and the control-character-disguised definition, now rejected where it was rendering an
<img>on main. Empty, loading, error, disabled and controlled-vs-uncontrolled are not reachable — the diff adds no state, no default, no prop.
@astryxdesign/core is published and not private, so the audience is real; every changed case requires markdown that the rule rejects.
Zero effects added, moved or deleted; no listeners or observers; no forced reflow — no getComputedStyle, offsetWidth or getBoundingClientRect anywhere in the diff; no new dependency. Downstream memoization is untouched: parseMarkdownIncremental's block cache keys off the source text and the link-definition signature, and parser.perf.test.ts's 14 cache-invariant tests pass unchanged at this head.
Measured as a count rather than a clock — isSafeUrl invocations per parseMarkdown, on a synthetic document of N units where each unit is one inline image, one inline link, one block image and one reference image:
| N | before | after | ratio |
|---|---|---|---|
| 1 | 2 | 4 | 2.0× |
| 20 | 40 | 80 | 2.0× |
| 200 | 400 | 800 | 2.0× |
Flat 2×, linear in image count — no shape change and no new N-squared. One invocation is a regex .replace over the URL plus .trim(), .toLowerCase() and three startsWith, so O(url length). Before numbers taken by checking out the parent's parser.ts in the same worktree, same install, same run. The library pays, at parse time, and the work is the fix — there is no cheaper spelling that still applies the rule.
Both frames captured in real Chromium at 620×900 against a local Storybook and both opened with the read tool. Before and after are the same worktree, same install, same browser: the parent's parser.ts and Markdown.tsx checked out, HMR, re-probe, restore.
| # | case | before | after | intentional? |
|---|---|---|---|---|
| 1 | standalone image, vbscript:
|
[alt] |
 |
yes — body: "unsafe srcs fall through to the paragraph path and render as literal text" |
| 2 | reference image, javascript: definition |
[logo] |
![logo][l] |
yes — same rule; the body states the check, not the rendering |
| 3 | shortcut reference image | [logo] |
![logo] |
yes — as above |
| 4 | definition hiding the scheme behind a tab |
<img src="java\tscript:alert(1)"> (broken-image icon) |
![logo][l] |
yes — the defect the PR exists to close |
| 5 | standalone data:image/png (real 8×8 PNG) |
[chart] |
[chart] |
unchanged — the divergence: the parser allows it, the renderer rejects it. Pre-existing |
| 6 | inline data:image/png
|
before [chart] after |
before [chart] after |
unchanged, same divergence |
| 7 | ordinary https:// image (control) |
<img src="https://…"> |
identical | unchanged — no regression |
| 8 | ordinary https:// link (control) |
<a>astryx</a> |
identical | unchanged — no regression |
Rows dropped and why: RTL, 200% zoom, forced colors, hug-fill and narrow-viewport do not apply — the diff writes no CSS and moves no box; the only rendered difference is which text node exists. CI's pr-rtl and pr-visual are green on this head. The two frames are published alongside the posted comment, on the fork only.
Nothing here is touched, and this is what was checked. aria-|role=|useTranslator|t('@astryx over the added lines → 0.
-
Auto-covered.
pr-a11yis green on428cc07, and.github/a11y-baseline.jsonis not in the diff — the four files are the changeset,Markdown.tsx,parser.tsandparser.test.ts— so no violation was bought with a baseline entry.pr-rtlgreen. -
Checked in a browser. The one thing the diff can reach is what a reader, and an AT, get for a rejected image. Before:
[alt]. After: the raw markdown line, which still contains the alt text with the source around it. Nothing becomes unreachable, no focus moves, no live region fires, no control appears or disappears. - Strings — no user-visible or AT-visible string is added, so no catalog key and nothing to translate. The literal text that now renders is the author's own markdown source, not a library string.
- Direction — not reachable; no property, logical or physical, is written.
approve. The strongest candidate for a block was the divergence between the two copies of the rule, and it was chased to a rendered consequence — an inline data:image/* image shows [chart] instead of the picture — then confirmed by frame to be identical before and after, so it is inherited debt rather than this PR's. No slot's prose contains a sentence a user would call a bug.
1. The `data:` claim in the summary and the changeset is not true yet
→ whoever reads the changelog is told the two layers now apply one rule; they
still differ, and a builder who embeds ``
gets the text `[chart]` with no error
· Markdown.tsx:561 vs parser.ts:433
The anchored lines at this head, so the next reader can string-compare rather than fetch:
Markdown.tsx:561 const DANGEROUS_URL_PATTERN = /^(javascript|data|vbscript):/i;
parser.ts:433 lower.startsWith('data:text/html')
The two breaking axes that are not "no", each answered rather than stepped over:
-
Visual — not worth raising. Three rendering cases change and all three require markdown the rule rejects. A reader of an ordinary document sees nothing different (frame cases 7 and 8, byte-identical), and the new appearance is the one a rejected inline image already had on
main, so the diff removes an inconsistency rather than creating one. -
Theme — not worth raising. A rejected standalone image renders
astryx-markdown-paragraphwhere it used to renderastryx-markdown-image, so a theme selectingmarkdown-imagestops matching for rejected content only. No target is added, removed or renamed, and every safe image still emits it (Markdown.tsx:1563,:1582). A theme author cannot reach the difference without feeding the component ajavascript:src.
Nothing blocks. The one finding was confirmed a second way: found by reading the two patterns, confirmed by rendering a real 8×8 PNG data URI in Chromium and reading back an empty imgs array with the text [chart].
CI on 428cc07: 19 checks, all green except Vercel, which is failing on every open PR sampled (#5521, #5523, #5524, #5515, #5444) and succeeding on main — not this PR's, and not something to tell the author to fix.
Three things found and not spent on the author: the parser's isSafeUrl normalizes for the test and stores the raw value, so the public AST still carries control characters — parseMarkdown('') returns src: "/fo\u0001o.png" — which is not a hole, since anything dangerous after stripping is rejected outright and the renderer now normalizes before writing the attribute; all four linkDefs.get consumers and all four direct emit paths are now guarded, and a fifth same-shape site inside the parser does not exist, the next instance of the shape being in a different file; and the new test comments are the kind that get cut, three lines, not worth a contributor's round trip.
Not verified: whether a components.image override or a downstream consumer of the public parseMarkdown AST actually does anything dangerous with an image src — that is consumer DOM, unverifiable by reading, which is why the harm is framed as a bypass of the rule rather than as script execution.
Thanks — the
[l]: <java\tscript:…>definition was actually reaching the DOM as an<img>on main, good catch. One for the description: parser and renderer still differ ondata:.[Reviewed by Robohands]
Inline: packages/core/src/Markdown/Markdown.tsx:561 — Blocks every data:; the parser only blocks data:text/html. A data-URI image renders as [alt].
One review, two gate passes.
-
Gate 1 — failed on three counts. Breaking answered "yes" on Visual and "yes" on Theme and the judgement never said whether either was worth raising — a general paragraph about "the notes" is not that answer. The posted comment asserted the
<img>was reaching the DOM onmainand the inline asserted a data-URI image renders as[alt], both sentences a reader has to see: the frames existed and had been opened, but the draft never told whoever posts to publish and embed them. And the anchored line's text was not pasted in forMarkdown.tsx:561, which is what turns the next check into a string comparison. One borderline note: "the layers" is a definite-article phrase, and naming them costs one word. -
Gate 2 — clean. The judgement now carries a named line for Visual and one for Theme, each with the frame cases and the
file:linethat make it checkable; the evidence slot carries the publish-to-the-fork instruction and what to do if the upload fails; both anchors carry their literal text; "the layers" became "parser and renderer".