Skip to content

Review 5524

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

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

bhamodi · open · latest review R1e at b6184fe1a2c · view on GitHub

Verdict: approve and merge — R1e at b6184fe1a2c. Rounds 1–2 requested changes at 2ca50dfda6d; everything below down to the end of Rounds is their record, kept verbatim.

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.

Round 3 — R1e re-review at b6184fe1a2c

Loop 1.6.0 · audit 1.13 · full lane · two critic passes · 21m.

The author amended and force-pushed at 2026-08-27T09:12:49Z and rebased onto newer main, so this round is the first review of b6184fe1a2c26b8eaf0b60c6c5806e3c64d9f498. Rounds 1 and 2 both reviewed 2ca50dfda6d.

Both prior asks are satisfied, and the second one was checked two-sided rather than read. to is destructured at useLinkComponent.ts:39, ...rest now spreads before href and to at :57, and both values pass the scheme rule at :53-:54 while to keeps its precedence over the href-derived value. Restoring the pre-fix file from 2ca50dfda6d into a worktree at head makes exactly one test fail — "an explicit to passes the same check instead of riding through ...rest" — and nothing else, so the new case is a real regression test and not a passing decoration. At head the three changed test files are 23/23 green.

Nothing round 1 waived has got worse. The native <a> branch (useLinkComponent.ts:98 returns the bare string before the wrapper exists), the interactiveRef.click() proxy, and the second copy of the rule in Markdown/parser.ts:424 are all still there — and each is now stated in the PR body with a reason. The two copies were diffed with comments stripped and are identical, so nothing behaves differently until one is edited. React DOM 19.2.7 rewrites a javascript: href on any rendered anchor, which is what makes the "the rendered anchor is React-vetted" reason true for the one scheme that executes.

Visual: not applicable, and both endpoints were checked rather than assumed. The changed code writes no DOM of the repo's own — it sits inside onClick/onMouseUp callbacks, or sets props on a consumer-supplied component. The two repo-owned surfaces that do wrap a LinkProvider (the sandbox polymorphic-link page and the docsite's own provider) render ordinary URLs, and for an ordinary URL the vetted value is the input, so their pixels cannot move. The one endpoint whose pixels could differ — a rejected URL drawn by a custom router component — has no story anywhere in the repo, and what that component draws is DOM this repo does not contain.

Zero Effects added or changed; the rule is one predicate run at click time, never during render. No API change, no theme target, no new export — the helper is module-private with two importers.

One nit, not blocking. packages/core/src/utils/safeUrl.test.ts:29 carries a raw NUL byte where its two sibling assertions on the lines above use \t and \n escapes. The file parses, its assertions run and CI is green — but git files the whole new file as binary, so git show --stat reports it as Bin 0 -> 1282 bytes with zero insertions, the diff renders as "Binary files … differ", and git grep answers "Binary file … matches" with no line. The next person to review, blame or grep the test that proves this security rule gets a blob instead of the five assertions. '\x00javascript:...' reads the same and restores the file to text.

What the round cost itself

Two minutes of waste, worth recording because it generalises to every re-review: the commits endpoint showed one commit with an author date of 2026-08-25, which read as the author never pushed. A force-push preserves the author date, so that endpoint alone cannot tell you the head moved — the timeline and a base/head compare are what showed the 09:12:49Z force-push. Reach for the timeline first.

Status

Round 3 verdict: approve and merge. Everything rounds 1 and 2 asked for is done at b6184fe1a2c, the one remaining item is a nit with its fix named, and nothing is withheld from the author.

Rounds 1 and 2 were both posted to the PR as CHANGES_REQUESTED (2026-08-26 and 2026-08-27) — the round-1 record above, written before posting, says otherwise and is left as written. Those two reviews are now the only thing blocking the PR: reviewDecision is CHANGES_REQUESTED and the merge state is blocked, and only the account that filed them can clear them, so posting the approval from that account is what unblocks it. Round 3 itself made no change to the PR: no review, no comment, no inline, nothing pushed.

Round 2 (2026-08-27) has no record of its own on this page; only its ask — the explicit-to regression case — is captured, above, as satisfied.

CI on b6184fe1a2c: 24 check runs, with build, test, lint, pr-a11y, pr-rtl, build-storybook, build-sandbox, docsite-test, theme-layers, check-components, check-scope, dependency-check, smoke-test, the CLA check and Stable visual regression all green. The Vercel commit status is red here, and is also red on other open PRs and on main's own canary context.

Round 4 — Superseding self-contained R1e re-review at b6184fe1a2c

Supersedes Round 3. That round used delegated review work and is invalid under the loop's one-agent requirement. This round independently reread the current kit, reviewed the complete current diff, reran both test arms, and performed both critic passes in one review session.

PR

#5524 fix(core): apply one URL scheme rule wherever a link leaves React's hands by bhamodi (bucket: external contributor — permission: read; absent from .github/ENGOWNERS and .github/DESIGNOWNERS)

HEAD REVIEWED

b6184fe1a2c26b8eaf0b60c6c5806e3c64d9f498

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: two unresolved prior CHANGES_REQUESTED reviews exist, the head moved, and the change is security-sensitive runtime behavior.

STEP 0: safe to run. The seven changed files are source, tests, and a changeset. No dependency, lockfile, workflow, package script, or executable tooling changed.

PRIOR REVIEW: cixzhang first concluded that explicit to remained in ...rest and overwrote the checked value, then asked for an explicit-to regression test. This round extends those reviews: both asks are satisfied at the new head.

MAIN: no commit on origin/main changed the six touched source/test paths after the latest prior review at 2026-08-27T04:09:15Z. The reviewed mechanism still exists.

PROBLEM

WHY 1: Core has navigation paths that act on caller-provided URLs without React DOM vetting them first. WHY 2: A script-scheme value can run code when a person activates a card or row whose destination came from app data. WHY 3: The design system owns those navigation exits, so an app cannot consistently repair the guarantee outside the component.

USER-FACING PROBLEM: a person activates an Astryx click surface whose destination came from untrusted data, and script runs in their page session instead of navigation being refused. PROBLEM SEVERITY: broken task — the person cannot safely use the navigation path. This round independently traced caller-controlled href to window.open / window.location.href before the guard and ran the focused tests proving rejected schemes no longer reach those calls.

VERDICT: clear

SOLUTION

A URL is checked at each core-owned exit that React does not vet: direct browser navigation and the handoff to a custom router component. Ordinary URLs pass unchanged; script schemes become a no-op or an omitted router prop. The shared rule lives in a pure utility, while each navigation owner invokes it at its boundary.

SOLUTION (1 decision · about 30 runtime lines of a 270-line addition)

  1. Apply one shared URL-scheme rule to imperative navigation and custom-router href/to handoff. [serves the stated problem]

BURDEN: low — one pure utility and event/render-boundary guards; no state, Effect, timer, observer, listener, dependency, or public export. BURDEN MATCH: proportionate — the small mechanism closes a demonstrated unsafe navigation path.

VERDICT: clear

ARCHITECTURE

OWNER: the URL-scheme policy, implemented by the internal safeUrl utility and invoked by the two hooks that own the exits. TIER 1: useLinkComponent for router adaptation; useClickableContainer for enlarged click surfaces. TIER 2: none. SEAMS: LinkProvider custom component, per-call as, native <a>, and the interactiveRef proxy. BEHAVIOR UNIT: pure utility — deterministic string classification with focused unit tests; each sink has focused integration coverage.

seam result
custom component, generated to safe and unsafe values covered at head
custom component, explicit to unsafe value withheld at head; same test fails on the old implementation
explicit ordinary to keeps precedence over href on both arms
native <a> / click proxy unchanged; still routed through React-rendered anchor behavior

The prior blocker is closed at its owner: to is destructured, checked separately, and written after ...rest. A non-blocking duplication remains: safeUrl.ts and Markdown/parser.ts carry the same rule, already disclosed in the PR body and waived in the first review as “worth knowing rather than fixing here.”

VERDICT: note — two copies of the URL rule remain, disclosed and outside the prior blocking ask.

IMPACT

People using ordinary relative, HTTP(S), hash, mail, telephone, and image-data URLs see no change. People activating rejected script-scheme destinations through imperative click surfaces no longer execute them. Builders using a custom LinkProvider receive undefined for rejected href/to values; explicit ordinary to values still win over href.

VERDICT: clear

API

No API change. No export, prop, signature, accepted value, or default is added or removed; isSafeUrl is internal and absent from the package barrel.

OSSIFICATION: nothing new becomes public. The behavior change restores the existing navigation-safety contract rather than adding a capability.

VERDICT: clear

THEMING

No style, token, target, variable, or painting element changes. Added-line style grep across the three runtime files returned zero.

VERDICT: clear

BREAKING

BEHAVIOR: intentional — rejected script-scheme URLs stop navigating at the imperative sinks and are withheld from custom routers. The far side of that condition is covered by focused tests; ordinary explicit to remains unchanged. API: no signature, export, prop, or default changes. VISUAL: no Astryx-owned DOM, style, geometry, or text changes; nothing grows. THEME: no target, token, variable, or override changes.

VERDICT: note — one intentional behavior correction, stated in the PR body and changeset.

PERFORMANCE & RESOURCES

EFFECTS: zero added, changed, moved, or deleted. The pre-existing useClickableContainer Effect is outside the hunks.

RENDER: no added render pass; the custom-router path performs bounded string checks once per wrapper render, and imperative paths do so once per activation. LISTENERS/OBSERVERS: none added or changed. LAYOUT: no layout/style read or write. BUNDLE: one 35-line internal module, no dependency; CI build is green.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: the diff changes navigation decisions and custom-component props, but writes no Astryx-owned DOM, style, geometry, or text. The repo contains no story or app surface with a rejected script-scheme URL; custom router output is consumer-owned DOM. Stable visual regression, build-storybook, build-sandbox, and docsite-test all passed at the exact head.

No frames. A frame from an Astryx story cannot exercise the rejected custom-router endpoint, and no visual claim is used in the verdict.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

Added-line grep found zero ARIA, role, translation, locale, direction, or style changes. pr-a11y and pr-rtl passed at the exact head with no baseline edit. Withholding a rejected navigation prop changes only consumer-supplied custom-router DOM; no Astryx-owned accessible output changes.

VERDICT: clear

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION clear
ARCHITECTURE note — duplicate rule remains, already disclosed
IMPACT clear
API clear
THEMING clear
BREAKING note — intended behavior correction
PERFORMANCE clear
VISUAL clear
A11Y & I18N clear

GOAL: met — at head all 23 focused tests pass; replacing only useLinkComponent.ts with the prior reviewed version makes exactly the explicit-to regression test fail while 14 sibling tests pass. DISPOSITION: prior to bypass → resolved at head; requested explicit-to regression → resolved and mutation-proved; duplicated rule → accepted as a disclosed follow-up already waived by the prior review; raw NUL in safeUrl.test.ts → non-blocking inline nit. ADVICE: proven local pattern — encode the raw NUL as \x00, matching the escaped control-character cases directly above it while preserving the test input. AUTHOR CAN PROCEED: yes — both blocking asks are satisfied; the remaining item is a readable-source nit. WORST OUTCOME: “a person activates an Astryx click surface whose destination came from untrusted data, and script runs in their page session” → this is what the PR now prevents, consistent with approval.

JUDGEMENT NEEDED: none — defect fix against an existing contract; no new public concept or design choice.

approve and merge

  1. [not blocking] safeUrl.test.ts contains one raw NUL byte, so Git classifies the entire new test as binary and hides its line diff and blame. → the next maintainer reviewing the security regression sees a binary blob instead of the assertions; \x00 preserves the case as text · packages/core/src/utils/safeUrl.test.ts:29 — expect(isSafeUrl('\x00javascript:alert(1)')).toBe(false); (the displayed escape represents the raw byte currently in the file)

REVIEW

Thanks — we asked for explicit to to pass the same URL check; it now does, and the regression test fails on the old implementation. One nit inline.

[Reviewed by Robohands]

INLINE (1)

  • packages/core/src/utils/safeUrl.test.ts:29 — Raw NUL makes this test binary in Git; \x00 keeps the same case readable.

EVIDENCE I DID NOT SPEND

  • #5522, the related Markdown fix, merged before this head; there is no competing open safe-URL PR or issue.
  • The exact-head full remote test, lint, build, pr-a11y, pr-rtl, Storybook, sandbox, and docsite jobs are green; Vercel is red and visual acceptance remains pending outside the code-review evidence.

TIME

TIME total 11m setup/rules 4m full current kit, repo guidance, fresh official/fork wiki clones, isolated worktree; warm main reused: yes build/server 0m fast-install completed in 4s; no build or server started browser/a11y 0m static/code-only slot; read exact-head remote pr-a11y, pr-rtl, and visual results focused tests 1m 23/23 at head; pre-fix arm 1 expected failure / 14 pass; one capture rerun code/history 2m complete diff, source, owner bucket, prior reviews, range-diff, main movement, related work critique/wiki 3m draft, critic pass, rewrite, clean critic pass, serialized wiki publication CI wait 0m full remote CI was already complete; no waiting cleanup 1m restored arm, verified clean worktree, removed worktree and fresh clones waste 1m repeated the pre-fix arm after using a shell-specific pipeline-status variable

WHAT I COULD NOT VERIFY

  • Whether a real third-party router throws or renders inert DOM when it receives to={undefined}; that consumer-owned output is not represented in Astryx stories and does not change the resolved prior blocker.
  • Whether the red Vercel deployment is caused by external preview infrastructure; the repo's exact-head build and docsite checks passed.

What changed before posting

Posted as an approval at the recorded head. The raw-NUL nit moved from an inline into the 30-word summary because Git classifies that new test file as binary and GitHub exposes no line anchor.

Clone this wiki locally