-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5527
bhamodi · open, approve · reviewed at 3bf1dae4069 · view on GitHub
Verdict: approve — the loop merges it
Two reachable states, both measured before and after: a sandbox embed link with a prototype-chain theme name white-screened the page, and a page on another origin could recolour an embedded sandbox. Both are closed, nothing else moves, and the one ask — guard where the name becomes a theme, not only at the entrances — rides as a question rather than a condition.
Two, and one of them is real today.
A person opening a sandbox embed link with a prototype-chain theme name gets a blank error page. ?embed=1&theme=constructor — and toString, __proto__, valueOf, hasOwnProperty — passed the paramTheme in themes validation, so themes['constructor'], the Object constructor, was handed to <Theme>. Measured at the parent commit: the page is replaced by "Application error: a client-side exception has occurred", with TypeError: Cannot convert undefined or null to object and no [data-astryx-theme] element at all.
A page on another origin that embeds the sandbox could change what the sandbox shows. The embed's message handler acted on any sender. Measured: a page served from a second local origin flipped the embedded sandbox from Neutral/light to Matcha/dark. The sandbox ships as a static export to GitHub Pages, so it carries no frame-ancestors header to stop the embedding either.
Honestly framed: nobody has hit either. The first is a defect a person reaches only by typing an odd URL or being handed one; the second is a demo site whose colours a hostile embedder can pick. Neither has a reported case. Both are reachable states with a real outcome, which is what makes them problems rather than tidiness.
(3 decisions · 21 added / 5 removed lines, all runtime)
- The embed's
messagehandler ignores anything not fromwindow.location.origin, and requires the payload's theme to be a string. - The shell addresses its
postMessageto its own origin instead of'*'. - Theme-name lookups use
Object.hasOwninstead ofin, at four sites.
The sandbox trusted two things it should not have: who a message came from, and whether a name was really one of its themes. JavaScript's in says yes to names every object secretly has — constructor, toString — so a made-up name resolved to something that was not a theme, and the page died trying to render it. Now the embed only listens to messages from its own site, and a name only counts if the theme list itself owns it. Anything else quietly falls back to the default theme, exactly like a typo does.
Three decisions sits at the split-it threshold, so the question is asked and answered: no split. The three are one hardening of one channel — a receiver check, its matching sender change, and the validation the receiver depends on — at 21 lines, and the first PR of any split would leave the receiver checking an origin while the sender still broadcasts to '*', a state nobody would ship.
The receiver check lives with its owner. The name validation does not: it lives at each entrance rather than at the one place that turns a name into a theme. Driven, seam by seam — shell to embed on the same origin works, with the iframe's theme attribute going neutral to matcha; a foreign origin is correctly ignored at head and obeyed at the parent commit; the URL-param entry falls back to neutral at head and crashes at the parent; the localStorage entry is guarded at head (providers.tsx:114) with the same one-line guard as the driven URL path; the docsite playground's own param entry is guarded at head (PlaygroundClient.tsx:260), read rather than driven; and the docsite preview iframe's message entry is still msg.theme in themeByValue at preview/page.tsx:187, untouched by this PR.
The repo already owns this shape twice. Object.hasOwn is the house answer to prototype keys — a CLI codemod uses it with the comment "Uses Object.hasOwn to avoid prototype pollution", and there are 6 non-test call sites repo-wide — and isTrustedPreviewMessage (apps/docsite/src/app/playground/previewChannel.ts:36) is the house answer to an untrusted message. This PR matches the first and half of the second.
- Someone opening a sandbox embed link with an odd theme name — today they get a blank page reading "Application error"; after this, they get the sandbox in the default theme, the same as any misspelled name. That is the whole visible change, and it only happens to someone who was handed such a link.
- Someone reading a third-party page that embeds our sandbox — after this, what they see is what we chose; before, the host page could recolour it. Invisible to anyone who has not been embedded that way.
-
Everyone else — nothing. Both apps are private demos (
apps/sandbox/package.json:4,apps/docsite/package.json:4), so no npm consumer can be broken and no builder writes against any of this. - The next builder in these files — one more place where a name is validated at the door and trusted at the point of use.
Landing it makes nothing new visible. The unfixed fifth site was broken before this PR and remains exactly as broken — and the docsite path that could reach it is now closed one level up, so this makes it less reachable, not more.
No API change. Nothing exported, no prop, no default, no signature; three app files, none of them a package.
n/a — structural only. The style grep over the three changed files returns 0: no new theme targets, no token touched, no astryx-* class added or renamed. Existing targets, tokens and overrides are untouched — what changes is which theme object gets selected, never what a theme can express.
Nothing new ossifies, and no helper is added either: the PR inlines Object.hasOwn at each site rather than introducing an isKnownTheme() the repo would then have to own. Given the repo already treats Object.hasOwn as the idiom, inlining is the right call, and it is why there is nothing to escalate.
No consumer can be broken: both apps are private, and nothing is published from either.
- API — no. Nothing exported changed.
- Visual — no, except the fix itself: the only pixels that move are a crashed page becoming a rendered one.
- Theme — no. Existing targets and overrides resolve unchanged.
-
Behaviour — valid theme names are unchanged, driven; an unknown name still falls back to
neutral; a prototype-chain name goes from crash toneutral, which is the fix; a live theme switch through the channel still syncs, driven; and a foreign-origin sender goes from obeyed to ignored, which is the other half of the fix. Empty, loading, error, disabled and boundary states are not reachable — the diff adds no state, no default, no prop.
The far side of the new bound is an embed whose origin is not ours, driven both ways above. The reason the same-origin case survives is that the iframe is sandbox="allow-scripts allow-same-origin" (PreviewShell.tsx:606) — drop allow-same-origin and the frame becomes an opaque origin and this channel dies silently. Worth knowing; not a defect in this diff.
The diff adds no effect, no memo and no callback. Two existing effects keep the same deps: the embed's message effect gains three lines inside the handler body (providers.tsx:151), and the shell's sync effect changes only its target-origin argument (PreviewShell.tsx:307). Nothing moved into render, no new subscription, no dependency identity changed. The same single message listener is added and removed by the same effect (providers.tsx:168-169) — none added, none leaked. No forced reflow, no getComputedStyle, offsetWidth or getBoundingClientRect anywhere in the diff, no dependency added, and nothing changed about what any hook returns.
One change worth stating rather than hiding: the origin comparison now runs for every message event the embed window receives, not just theme-sync ones, because it is the first statement in the handler. Not measured. The count is one string comparison per message event, and the only sender on this channel is the shell's theme picker, driven above — one message per theme or mode switch.
The diff touches no style file, so the render matrix's RTL, zoom, forced-colors, hug-fill and icon-only rows are not owed. What is owed is the before and after of the two states the PR changes, and seven frames were captured in real Chromium against a dev server in the review worktree and opened with the read tool: the prototype key in the sandbox embed, before and after; the foreign-origin embedder, before and after; a valid theme as an unchanged control; and the shell-to-iframe live sync, initial and after switching.
| difference | intentional? | source |
|---|---|---|
?embed=1&theme=constructor: blank "Application error" page → the sandbox rendered in Neutral |
intentional | the PR body: "in walks the prototype chain, so a value like constructor passed validation and fed a non-theme object (Object itself) into <Theme>, corrupting the render." The measured outcome is stronger than "corrupting" — it is a white screen |
| foreign embedder's message: embed turns Matcha/dark → embed stays Neutral/light | intentional | the PR body: "the embed-mode message handler now requires event.origin === window.location.origin before acting"
|
| valid names and the shell's live switch | no difference — the theme attribute is identical before and after, and the picker still drives the iframe | — |
The before frame carries the proof in the dev overlay's own code line: <Theme theme={function Object} mode="light">. Both probes were banked in the review kit, and BEFORE was taken in the same worktree, same install, same browser session, by reverting the three files to the parent commit and letting HMR recompile — not from a different checkout.
Nothing here is touched, and this is what was checked: aria-|role=|useTranslator|t('@astryx over the added lines of the three changed files → 0.
- Automated coverage — the a11y job skips on this PR, correctly: it scopes to changed components and this diff changes none. No baseline entry was added; the baseline file is untouched.
- Strings — no string reaches a user or AT from this diff, and no catalog key is added or removed. Checked by reading all 21 added lines as well as by grep.
- Rendered checks — not touched: the diff renders nothing, adds no element, changes no attribute on any element. Not looked at in a browser for a11y specifically, and not owed — the DOM the sandbox produces is byte-identical for every valid theme name, driven against the parent commit's render.
- Direction — no logical or physical property, no glyph, no translation; the RTL job skips for the same reason.
One thing worth stating plainly because it is an accessibility-adjacent outcome: at the parent commit the crash path left a user with a page containing 127 characters and no landmarks, and after, the full page renders. The fix improves this; nothing in it regresses.
approve. Both halves were measured, before and after, in one session. Prototype key: parent commit gives "Application error", a TypeError, and no theme attribute; head gives data-astryx-theme="neutral" and a full page of 2,357 characters of text. Foreign origin: parent commit obeys and goes Matcha/dark; head ignores it and stays Neutral/light. Valid names and the shell's live sync are unchanged in both directions, so the fix costs nothing it was not meant to cost.
1. The four Object.hasOwn guards sit at the entrances; providers.tsx:181
and preview/page.tsx:117 still turn a name into a theme without
checking
→ the fifth entrance the sweep missed (preview/page.tsx:187, still
`in`) resolves `constructor` to the Object constructor, and the next
entrance anyone adds inherits the same trap
· apps/sandbox/src/app/providers.tsx:181
2. The handler checks the origin but not the sender, where the pattern it
cites checks both
→ any other window on this origin can still drive the embed's theme,
and the next person copying this handler copies half a check
· providers.tsx:159 vs previewChannel.ts:45
Finding 1 is asked in the review and does not block: nothing regresses without it, the reachable path is closed one level up, and the ask is one line at each of two lines of code, so the author chooses this PR or a follow-up. Finding 2 is accepted, and Cindy can overrule — nobody pays today, since the only other windows on this origin are the sandbox's own, and what would remove it is event.source === window.parent, one line, the same shape previewChannel.ts:45 already uses. It is in the review as a note so it does not evaporate.
The strongest user-harm sentence anywhere in these slots — the white screen and the TypeError — describes the parent commit, not this PR. It is a sentence this PR removes, and nothing in the slots describes harm the PR introduces or leaves newly reachable, so approve is compatible with it. No design call is involved: no API surface, no new concept, two private apps.
On the merge: the four required checks are green, and the repo's review gate is the only thing holding it, which this verdict clears. Vercel is red and does not gate — it is not in the required list, and it is red on every fork PR from this author while merged internal PRs are green, so it is a fork deployment restriction. Two things to watch before treating the automatic merge as armed, neither observable from the review: the review gate flips only when the review-signal workflow re-runs on the review event, and on a fork PR that re-run can sit waiting for a maintainer to approve workflows. Every run on this head is a completed success, so nothing is stalled today, but the flip itself has not happened yet and cannot. Check the check after approving, and sweep by hand if the gate stays pending.
Two things found and not spent on the author: the sandbox embed already logs a hydration mismatch for any non-default ?theme=, pre-existing at the parent commit and not this PR's to carry; and Object.hasOwn is already the repo's stated answer to this exact hazard, which made the first finding shorter rather than being an argument to make.
Not verified: the docsite playground's own paths — its param guard and the preview iframe's fifth in — are read at source rather than driven, because the run drove the sandbox and not the docsite. The localStorage entry point is likewise read rather than driven; it is the same one-line guard as the URL path, which is driven.
Thanks — good catch: before this,
?embed=1&theme=constructorwhite-screened the sandbox. Worth also guarding atproviders.tsx:181, where the name becomes a theme — want that here, or a follow-up?[Reviewed by Robohands]
Inline: apps/sandbox/src/app/providers.tsx:181 — Hmm, themes[themeName] || neutralTheme still trusts the name; Object.hasOwn here covers every entrance. · apps/docsite/src/app/playground/preview/page.tsx:187 — Same in lookup here — the fifth site, and the sweep missed it. · providers.tsx:159 — Might also check event.source === window.parent; previewChannel.ts does both halves.
One review, three gate passes, and the posted text never changed a word — every round graded the record rather than the comment.
- Gate 1 — failed on three counts, none of them in the comment. An unmeasured cost was priced with an adjective in disguise: "one string compare on a channel that carries a handful of messages per session" is the banned shape whether or not the rule is named beside it. The merge trailer said the merge would arm while the run's own evidence recorded a red check, without ever saying whether that check gates — an armed automatic merge that quietly never fires is the documented failure. And the architecture block was missing its behaviour-unit line.
- Gate 2 — three more, same kind. The behaviour-unit line now named the behaviour's subject rather than the boundary and why it is testable, so the question — is this inline on purpose — was still unanswered. The trailer asserted an outcome nobody had observed, on the one PR shape documented as failing silently: a fork contributor whose review-gate workflow can itself sit waiting for approval, so the armed merge waits forever looking healthy. And the time total added a waste figure the round before had treated as a subset — either total is defensible, not both.
- Gate 3 — clean. Nothing binding the posted text, any slot or the verdict. The trailer now asserts only what was observed, names what has not happened and cannot yet, and carries an exit: check the check after approving, and sweep by hand if the gate stays pending. The one slip was a table cell in the change log that showed the new text in both the "replaced" and "written" columns, corrected in place rather than spent as a round.
Approve, with the merge armed to follow the approval. Nothing has changed in the drafted comment: it goes up as written, with the [Full review](https://github.com/cixzhang/astryx/wiki/Review-5527) line appended before the attribution — this page is published first so the link resolves. The merge trailer is the loop's instruction to itself and is never part of the comment.
Recorded plainly, because this page is the record: at the time it was written the review was not yet on the PR — the PR was open with no reviews — and this publication leg posts nothing to facebook/astryx. The approval and the merge are the run's next step, and the gate to watch after posting is the repo's own review-required status on a fork PR.