Skip to content

Review 5524

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 4 revisions

#5524 — fix(core): apply one URL scheme rule wherever a link leaves React's hands

bhamodi · open, request changes drafted · reviewed at 2ca50dfda6d · view on GitHub

Verdict: request changes — one line, and then it is an approve

The review asks for to to be destructured before ...rest: the wrapper sets to: safeHref, and ...rest puts the raw value back one line later, so the one prop LinkWithTo exists to serve walks around the new check. It did not merge because the same URL is vetted as href and unvetted as to, in the same wrapper — one spelling works and another silently does not.

Problem

A person clicks a card whose href came from data the app did not write, and the URL runs as script in their session.

useClickableContainer.ts:200   window.location.href = href;

Verified live in Chromium rather than inferred: assigning a javascript: URL there executes — the probe recorded __hit = ["EXEC-loc"]. That sink was unguarded and it is the real hole this PR closes.

The review has to say what closes the other exits, because the PR's framing implies it is doing all of it. React 19.2.7 rewrites a javascript: href in the production build, not only in dev — the measured attribute is "javascript:throw new Error('React has blocked a javascript: URL as a security precaution.')". The rewrite lives in the attribute, so the interactiveRef.current.click() proxy inherits it too. React does not touch vbscript: or data:text/html; both were written verbatim.

Solution

(3 decisions · ~30 runtime lines of 216)

  1. A shared isSafeUrl predicate lands at utils/safeUrl.ts, copied from Markdown/parser.ts:424.
  2. useClickableContainer's two window.open sinks and its location.href sink call it.
  3. LinkWithTo withholds an unsafe href from a custom link component — href and to both become undefined.

A URL only becomes dangerous at the moment something acts on it. React already refuses to act on a script URL it wrote itself; Astryx has three places where it acts on a URL without asking React — it opens a window, it sets the address bar, and it hands the URL to somebody else's link component. Those places now ask one shared question first, and if the answer is no, nothing happens: no navigation, no error, no warning. All three decisions trace to a problem the body states.

What the body does not record is decision 3's choice of remedy — withhold as undefined, rather than coerce to about:blank the way richtext/linkUtils.ts:59 does for the same job. One line in the body would close it; it is noted rather than raised, because building the argument the PR did not make is not the reviewer's job.

The rule itself has no owner, and that is the shape everything else is a child of. A grep for the rule's spellings across packages/*/src returns four, two of them byte-identical, and they do not agree:

where shape blocks
Markdown/Markdown.tsx:561 blocklist javascript: vbscript: all data:
Markdown/parser.ts:424 blocklist javascript: vbscript: data:text/html
utils/safeUrl.ts:23 (new) blocklist body byte-identical to parser.ts:424
richtext/linkUtils.ts:32 allowlist, returns a value everything but http/https/mailto/tel

linkUtils.ts's own docblock is the sentence worth citing: "Sanitization is the security-critical part: it blocks javascript:, data:, and other non-http(s)/mailto/tel schemes from being written into a LinkNode's href, so an inserted link can never smuggle in an executable URL." The shape it names — a function that returns the URL you may use, about:blank at linkUtils.ts:59 — is the thing worth borrowing. The caveat matters, because recommending the wrong half would be wrong advice: richtext's list does not transplant, since SAFE_PROTOCOLS plus its withDefaultScheme helper would reject core's relative hrefs (/docs/button) or rewrite them to https://. Only the return-a-value shape carries over.

The body's stated reason for the fourth copy does not hold. It says unifying is "a follow-up once in-flight Markdown work lands"; the in-flight work is the author's own #5522, whose diff to parser.ts adds three isSafeUrl(...) call sites and does not touch the function's body. Nothing blocks parser.ts importing from utils/safeUrl today. Whether the two PRs are one PR was decided rather than skipped: they are not — disjoint files, disjoint sinks, and each stands alone if the other is closed. #5522 is cited here as evidence that this failure has already happened three times, and for nothing else.

Impact

End users notice nothing. No ordinary URL changes behaviour — relative paths, hashes, mailto:, tel: and data:image/* all pass, and safeUrl.test.ts asserts each. The only URLs whose behaviour changes are ones nobody legitimately writes.

Two builders are reached, and the same input gets opposite treatment. A builder who has wired a LinkProvider now gets href={undefined} for a rejected URL; a builder who has not gets it rendered verbatim. Measured rather than reasoned: with no provider, <Link href="vbscript:msgbox(1)"> renders href="vbscript:msgbox(1)"; with a provider, the same URL arrives as undefined.

Landing it newly exposes nothing. Clicking a vbscript: or a data:text/html anchor in Chromium navigates nowhere; both are inert in the browser regardless, so the split above costs no user anything today. It is a consistency gap rather than an open hole, and calling it a security finding would be the manufactured kind.

API

No call site changes — this is exactly what a consumer already writes:

<ClickableCard href="/docs/button">…</ClickableCard>
<LinkProvider component={NextLink}><Link href="/docs" /></LinkProvider>

isSafeUrl(url: string): boolean is added and is not public: utils/index.ts does not re-export it, so the export * from './utils' at index.ts:172 never reaches it. What does change is useLinkComponent() — a public hook (Link/index.ts:20) whose returned wrapper now withholds href and to for a rejected URL, with nothing in Link.doc.mjs saying so. No new props, no change to which element takes ...rest, no new callbacks.

Theme targets

n/a — structural only. The style grep returns 0 on each of the three changed source files. No new theme targets, none removed, no value pinned on xstyle or style.

Ossification

Nothing new ossifies, and the opposite was expected. The search was for a new public export to escalate, and the grep killed it: isSafeUrl is not on the barrel, so there is no permanent surface here and no want-it decision to make.

Breaking

@astryxdesign/core is published and not private, so the audience is real.

  • API — no. No signature changes; the two reformatted function signatures in useLinkComponent.ts are prettier only.
  • Visual — no, and nothing grew. The diff adds no element to the flow and changes no geometry, so the outer box of every consumer is byte-identical.
  • Theme — no. No target, token or override is touched.
  • Behaviour — one change, and it is the block. The far side of the bound is where a guard lives, so a rejected URL was driven through every exit rather than an ordinary one through the middle:
exit before after how verified
window.open, _blank/ctrl (useClickableContainer.ts:192) opens no-op the PR's own test, run
window.open, middle-click (:248) opens no-op the PR's own test, run
location.href, plain click, no interactiveRef (:200) executes no-op Chromium probe
the interactiveRef proxy (:197) React-neutralised for javascript: only unchanged probe — .click() on the React anchor executed nothing
useLinkComponent, custom component (useLinkComponent.ts:50) raw URL forwarded undefined scratch test
useLinkComponent, native <a> (:95) raw URL raw URL scratch test
useLinkComponent, explicit to (:55 ...rest,) raw URL raw URL scratch test

href == null short-circuits before the guard, unchanged; disabled returns early, untouched; loading, error and controlled-vs-uncontrolled are not reachable, because the diff adds no state, no default and no prop.

Performance & resources

Zero effects added, moved or deleted — the diff contains no useEffect or useLayoutEffect at all. useClickableContainer's existing effect is untouched and both useCallback dep arrays are unchanged, so nothing downstream re-memoizes. No getComputedStyle, no offsetWidth, no layout read anywhere in the diff; no listeners or observers added or removed; no new dependency, and safeUrl.ts is 35 lines that tree-shake with the two hooks importing it.

The cost, as a count rather than an adjective: one String.replace, one trim and one toLowerCase per navigation attempt — three string operations on a string a person typed into a prop, inside an event handler, at most once per click. Zero additional renders, zero style recalcs, zero layout reads, zero listeners, zero bytes of dependency. A perf test is not owed: the work is O(1) in the URL's length and runs once per click, not per row or per keystroke.

Visual evidence

No frames, and the reason is the acceptable one: nothing rendered changes. What was compared rather than assumed — the three changed source files contain zero style declarations, the diff adds and removes no element, and every URL a real app passes returns true, so the rendered href attribute is byte-identical before and after. The one DOM difference is href becoming absent on a custom link component fed a script URL, reachable only by writing javascript: into your own markup.

The evidence this PR needed was execution rather than appearance, and it came from one run of one probe against the repo's own React 19.2.7, banked and reproducible:

1. React rewrote javascript:       → YES, in the PRODUCTION build
   React rewrote vbscript:/data:   → no, both written verbatim
2. click the React javascript: anchor   → nothing executed
3. programmatic a.click() on it         → nothing executed  (the interactiveRef path)
4. location.href = 'javascript:…'       → __hit = ["EXEC-loc"]   ← the hole this PR closes
5. window.open('javascript:…')          → inconclusive; noopener severs the handle. Not asserted.
6. click vbscript: / data:text/html     → no navigation in Chromium; both inert

A11y & i18n

Nothing here is touched, and this is what was checked. aria-|role=|useTranslator|t('@astryx over the added lines → 0; the 13 hits inside useClickableContainer.ts are its pre-existing interactive-element selector list, which the diff does not go near.

  • Strings — none added, changed or removed, so no catalog key moves.
  • Automated axe coverage — green in CI, and .github/a11y-baseline.json is not in the diff, so no violation is being bought silence.
  • Disabled state and input ARIA — no control state and no input wiring is touched.
  • Focus — not looked at, deliberately. An <a> that loses its href also loses its link role and its place in the tab order, and that is known by reading rather than by driving; a claim about consumer-supplied DOM is not established by a source read, so the honest label is "not looked at". It does not move the verdict, because reaching that state requires an author writing a script URL into their own app.
  • Direction — no logical or physical property, no directional glyph, no locale-dependent value.

Judgement

request changes, on one line.

The notes about the rule's ownership and the block are the same finding seen three times: the rule is a boolean each sink must remember to call rather than a value a sink cannot obtain without it. A rule you have to remember is a rule that gets forgotten, and this diff forgets it at three exits, one of them inside the wrapper the diff itself is editing.

1. `to` is not destructured, so `...rest` puts the raw value back after
   the guard set it
   → whoever wires React Router or TanStack — the exact case LinkWithTo
     exists for — passes `to` themselves and the unvetted URL reaches
     their router, with nothing to tell them
     · useLinkComponent.ts:55  ...rest,   (guard at :50, overwritten at :54)
     · verified: <L href="/safe" to="javascript:alert(1)"> → the custom
       component received to: "javascript:alert(1)"

2. isSafeUrl is a predicate every exit must remember, and three do not
   → the next person adding a navigation sink to core gets nothing from
     the type system and no failure when they forget; the author's own
     #5522 is that having already happened three times in Markdown.
     4 spellings in packages/*/src, 2 byte-identical
     · useLinkComponent.ts:95 · useClickableContainer.ts:197

3. The body's reason for leaving a fourth copy is checkably wrong
   → a reader of the changelog is told the copies are converging when
     nothing schedules it; #5522 never touches isSafeUrl's body
     · .changeset/core-imperative-href-rule.md

Only 1 blocks; 2 rides in the summary as something to know, and 3 stays in this record. No design call is involved — a defect fix against the existing contract, with no new public surface, prop, value, export or theme target.

The risk class is stated rather than implied, because this is the class a future run could post unattended. Of the four conditions, three pass — no new API surface, no perf or resource regression, nothing got bigger — and one fails: an href a builder passes as to reaches their router unvetted, and the same URL as href does not. Three of four pass and the PR is still not in the class, which is the point of writing it down; without the sentence, "no API change, nothing grew, no perf cost" reads like low-risk to whoever skims next.

Three things found and not spent on the author. Markdown.tsx:561 rejects all data: while parser.ts:424 allows data:image/*, so a data:image/png markdown image parses to an image node and then renders as nothing — pre-existing, and #5522 does not close it. Vercel is red here and on two of this author's other PRs while four other open PRs are green, and it changes no dependency or config, so it was not chased; 20 checks ran on this head and 14 are green, so CI is real signal here rather than a path-filtered silence. And the repo's own review gate is pending with "core change, community contribution", so a human is already required whatever the loop concludes.

Not verified: whether a real router throws on to={undefined} rather than rendering an inert link — React Router was not installed, and if it throws, finding 1's consequence is worse than written, a crash rather than a silent bypass, with the ask unchanged. And whether window.open('javascript:…') executes in Chromium: noopener severs the handle, so the probe cannot see it.

The review, as drafted

Thanks for this — I confirmed the location.href sink executes in Chrome today, so it's worth having.

One thing first. LinkWithTo exists for to-routers, and to is the one prop that walks around the new check:

createElement(Component, {ref, href: safeHref, to: safeHref, ...rest});

to isn't destructured, so ...rest puts the raw value back. Someone wiring React Router and passing to themselves gets the unvetted URL, and nothing tells them.

Worth knowing rather than fixing here: isSafeUrl is a predicate every sink must remember, and three don't — the native <a> branch, the interactiveRef.click() proxy, and the Markdown sites in your #5522. richtext/linkUtils.ts returns a safe URL instead of a boolean, which can't be forgotten.

Could you pull to out of rest before this lands?

If you'd rather talk it through with someone, we're in Discord.

Inline: packages/core/src/Link/useLinkComponent.ts:55 — to isn't destructured above, so this puts the raw value back over to: safeHref. · :95 — This branch skips the rule, so vbscript: and data:text/html render verbatim here. React only blocks javascript:.

Rounds

One review, three gate passes.

  • Gate 1 — failed on eight counts, three of them anchors pointing at the wrong code. A line cited for location.href was a comment, a line cited for the .click() proxy was a window.open call, and a line cited for the native-<a> branch was blank; all three were repointed and every anchor now carries its line text, so the next check is a string comparison rather than a fetch. The comment ran 167 words against a 150 cap. There was no sentence saying prior review had been checked. Finding 2 pointed at richtext/linkUtils.ts as "that shape" without quoting the sentence that is the whole argument. The grep count did not travel with the finding. An inline said "the other two" without naming them. And a PR number appeared unlinked.
  • Gate 2 — three new ones, all small. The risk class was implied and never stated, on the one PR where the class decides whether a future run could post it unattended. A closing adjective — "nothing here is a cost anyone pays" — was doing a measurement's job, above an enumeration that was genuinely good. And #5522 was cited three times as evidence while the question it raises for a same-author overlap — one PR or two — was never adjudicated on the page, so a reader could not tell whether it had been decided or skipped.
  • Gate 3 — clean. Every check run mechanically against the artifact: 15 of 15 anchors correct at head, the comment at 128 words and the inlines at 14 and 16, the risk class written out condition by condition, the perf answer as three string operations rather than an adjective, and the overlap decided with its reason. The one thing worth recording about the run itself is that the anchors were noted from diff hunks and repaired afterwards — pasting the line text at the moment a finding is written removes the whole class.

Status

Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inlines. The presentation's merge trailer is a hold whose whole content is finding 1 — one line, and then it is an approve. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5524) line goes in before the attribution.

Clone this wiki locally