fix(player/runtime): rebind timelines and bound paused seeks - #3489
Conversation
|
@claude review |
|
@claude review |
|
Post-push verification update:
No code changes are pending; awaiting the fresh review approval. |
1b7e265 to
87a62ea
Compare
|
CPU-stability update at
@claude review |
|
Final-head automation update for
The remaining gate is human review. |
c89f5d7 to
75cd47c
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Read the five changed files in full at head 75cd47c588159dbd66a1f1f315970e3fb14ad413, plus runtime-in-srcdoc.ts, runtime-url.ts and the sandbox-policy path, rather than reviewing from the diff. The ordering work is sound and the _onIframeLoad reasoning is right. Two things I would want changed first.
Blocking: runtime-src is an any-origin remote script load, and the default sandbox is same-origin
runtimeSrcFromElement accepts any URL whose scheme is http: or https::
return url.protocol === "http:" || url.protocol === "https:" ? url.href : RUNTIME_CDN_URL;and that value is interpolated straight into a script tag in runtime-in-srcdoc.ts:
const tag = `<script src="${runtimeUrl}"></script>`;The scheme check is the only constraint, so the host is unrestricted. Meanwhile _applySandboxOriginPolicy adds allow-same-origin whenever sandbox-origin is absent, which is the default:
if (this.hasAttribute(SANDBOX_ORIGIN_ATTR)) this.iframe.sandbox.remove("allow-same-origin");
else this.iframe.sandbox.add("allow-same-origin");A srcdoc document with allow-same-origin inherits the embedder's origin, so a runtime-src value that reaches this component is arbitrary script execution in the embedding page's origin, not inside a sandboxed frame. The component already loads remote code, but until now from a pinned constant. This makes that origin caller-controlled. It matters because this is a published element that third parties embed, and React spreads unknown props onto custom elements, so runtime-src can arrive from a prop bag without the embedder ever naming it.
The description scopes the feature to "an authorized local rig", and the tests agree: the one positive case is http://127.0.0.1:8900/hyperframe.runtime.iife.js, and the only rejection asserted is javascript:alert("no"). The suite's own notion of "unsafe" is scheme-only. Restricting the accepted set to loopback and same-origin serves the stated purpose exactly, keeps both of those tests passing unchanged, and closes the hole:
const url = new URL(configured, document.baseURI);
const okScheme = url.protocol === "http:" || url.protocol === "https:";
const loopback = url.hostname === "127.0.0.1" || url.hostname === "localhost" || url.hostname === "[::1]";
return okScheme && (loopback || url.origin === location.origin) ? url.href : RUNTIME_CDN_URL;If a foreign origin really is needed, that is worth naming as a deliberate opt-in rather than falling out of a scheme check.
Blocking: _reloadShaderOptions navigates without clearing bridge readiness, and this PR removes the net that covered it
_reloadForSandboxOriginPolicy clears state before it navigates:
this._ready = false;
this._runtimeBridgeReady = false;
this._rejectAllRuntimeDataDeliveries("Sandbox policy changed before runtime data was applied");_reloadShaderOptions does none of that. It just assigns srcdoc or src. Until this PR, _onIframeLoad set _runtimeBridgeReady = false, which closed that window at load. Dropping that line is right for the handshake reason you give, but it was also the only thing resetting readiness for this path, and this PR adds RUNTIME_SRC_ATTR as a third trigger into it.
Consequence: across a runtime-src, shader-capture-scale or shader-loading change, _runtimeBridgeReady stays true through the navigation and past load. _deliverRuntimeData and _deliverRuntimeDataClear gate on nothing else, so a caption apply issued in that window registers a pending delivery, posts into a document that is being replaced, and surfaces as a RUNTIME_DATA_DELIVERY_TIMEOUT_MS timeout instead of the immediate, explanatory rejection the sandbox path gives. The same three lines in _reloadShaderOptions fix it.
Nit
RUNTIME_SRC_ATTR routes into _reloadShaderOptions, but runtimeSrcFromElement is only consulted by prepareSrcdocForElement. On the src path, changing runtime-src therefore tears down and rebuilds the document for a value that is never read.
Verified rather than taken
- The connection-deferral claim holds.
connectedCallbackapplies bothsrcdoc(line 187) andsrc(line 189) through theprepare*ForElementhelpers, so attributes present before connection are folded into that first navigation and nothing is dropped by the newisConnectedguards. _withDirectTimelinenow prefers a freshly resolved adapter over the cached one and republishes duration when the adapter changes, which is the rebind the description claims.- It depends on #3472, which is merged, so the base is in place.
CI
Slightly ahead of the evidence. At this head 49 checks pass, 7 skip, and Tests on windows-latest is still pending, so "Windows rendering ... green" is not yet true. Nothing is failing.
— Review by Rames
miga-heygen
left a comment
There was a problem hiding this comment.
Clean fix across two critical interaction surfaces — the runtime timeline rebind and the player lifecycle ordering.
Runtime (init.ts): The late-bound reconcileTimelineAfterRuntimeData callback is the right pattern — it lets the reporter invoke the reconciler without a forward reference to resolveRootTimelineFromDocument, and the teardown nulls it cleanly. The capturedTimeline identity check (line ~1575) correctly avoids needless rebinds for in-place mutations while catching full replacement objects. The postTimeline() call before runtime-data-applied is the real fix for the frozen-first-segment bug — the parent would seek against the bootstrap duration otherwise.
Paused-seek deduplication (init.ts ~3136): The lastTransportSeekTime / lastTransportSeekTimeline identity guard is correct. Playing always re-seeks (short-circuit), paused only re-seeks when time or timeline identity changes. The pausedSeekDeferredByManualGesture flag ensures exactly one reconciliation after drop/cancel. Tests cover all four transitions (initial frame, steady-state no-op, explicit seek, and timeline replacement).
Player (hyperframes-player.ts): The !this.isConnected guards on attributeChangedCallback for src, srcdoc, and runtime-src fix the React attribute-before-insertion ordering hazard correctly. The _onIframeLoad change to preserve _runtimeBridgeReady is well-reasoned — DOMContentLoaded fires the runtime ready handshake before iframe load, and clearing readiness there strands retained data. Source setters and _reloadShaderOptions already clear readiness on actual navigations.
runtime-src origin validation (shader-options.ts): Loopback + same-origin check is the right security boundary for srcdoc's allow-same-origin constraint. Unsafe schemes and foreign origins fall back to the CDN runtime. Test coverage includes the javascript: case.
_withDirectTimeline re-probe: Correctly re-resolves the timeline adapter on each call and updates duration when the adapter identity changes. This closes the stale-adapter path for the legacy same-origin case.
No blocking issues. Thorough test coverage with 4 new runtime tests and 6 new player tests covering the exact ordering hazards described.
— Miga
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head 1fbaa0a3d712f7f018b7997240cc0a7de8a9e489. This clears my own CHANGES_REQUESTED (review 5061792527, at 75cd47c588159dbd66a1f1f315970e3fb14ad413). Both findings are addressed, and I checked the fixes rather than the commit message.
Blocking finding: runtime-src accepted any origin
Fixed, and fixed more narrowly than I asked for:
const okScheme = url.protocol === "http:" || url.protocol === "https:";
const loopback =
url.hostname === "127.0.0.1" || url.hostname === "localhost" || url.hostname === "[::1]";
return okScheme && (loopback || url.origin === location.origin) ? url.href : RUNTIME_CDN_URL;A foreign origin now falls back to the pinned CDN URL instead of being interpolated into the srcdoc as a <script src>. The reasoning holds up: runtimeSrcFromElement runs in the parent while building the srcdoc, so location.origin is the embedding page's own origin, which means the same-origin branch grants no privilege the page does not already have. Loopback is a real dev affordance and reaching it requires a malicious server on the victim's own machine, which is a categorically higher bar than an arbitrary host. Browsers treat http://localhost as potentially trustworthy, so it still resolves from an HTTPS embedder.
The javascript: case stays rejected by the scheme check, and the existing test for it still passes unchanged.
Second finding: _reloadShaderOptions navigated without dropping readiness
Fixed, and it now matches _reloadForSandboxOriginPolicy exactly:
this._ready = false;
this._runtimeBridgeReady = false;
this._rejectAllRuntimeDataDeliveries("Shader options changed before runtime data was applied");Placed before the navigation, so a delivery issued afterwards gets the immediate explanatory rejection rather than a timeout against a document being replaced.
I audited every iframe navigation site in the file rather than just this one:
srcattribute change (234) clears at 229-231srcdocattribute change (244) clears at 239-241_reloadForSandboxOriginPolicy(324, 328) clears at 319-321_reloadShaderOptions(795, 799) clears at 790-792, the new codeconnectedCallback(187, 189) does not clear, correctly: both fields are initializedfalseat 97 and 115, anddisconnectedCallbackclears them at 214-216, so a freshly connected element is already in that state
That accounts for all eight. RUNTIME_SRC_ATTR routes into _reloadShaderOptions at 302-304, which was the specific concern in my original review, since this PR added it as a third trigger into a path that then did not reset readiness. It does now.
Tests
Both regressions are present and both would fail on a revert. The origin test asserts negatively and positively, not.toContain("evil.example.com") alongside toContain("hyperframe.runtime.iife.js"), so it catches a silent change in the fallback target as well as the leak. The readiness test drives the real attribute path rather than calling the private method.
CI
Test is green at this head, which is the job that runs these two regressions, alongside Preflight (lint + format), Render on windows-latest, Smoke: global install, Preview parity and preview-regression. Tests on windows-latest was still running when I wrote this; it is the same suite on another platform, not additional evidence about either finding.
— Review by Rames
Part 3 of 3 in the HyperFrames caption convergence stack. Depends on #3472.
What changes
runtime-data-applied, so an immediate seek cannot clamp against the initial timeline.runtime-srcoverride for pairing a branch-tip player with its matching branch-tip core in an authorized local rig; unsafe schemes fall back to the pinned CDN runtime.Why
Data-driven caption attach rebuilds the GSAP timeline. The direct player adapter and the injected runtime could retain the killed initial object, leaving playback on segment one. Real-browser validation exposed three additional ordering hazards: React can assign
srcdocbefore connection, the runtime ready message can precede iframeload, and a replacement could be acknowledged before its new duration was published.Verification
HyperFrames core: 124 test files / 2,542 tests passed; focused runtime tests, typecheck, build, oxlint, and oxfmt passed.
Player: 12 test files / 360 tests passed; build, typecheck, oxlint, and oxfmt passed.
Historical exact-browser proof against catalog 0.6.0 and the matching branch-tip core: create with React's
srcdocthenruntime-srcordering; apply three segments; switchcaption-highlighttocaption-pill-karaoke; immediately seek to 0.2s, 1.7s, and 3.2s after each apply. Both styles reported duration 3.9s and landed all seeks exactly; the old player disconnected, the bridge stayed ready, and no runtime-data error fired.Current-head CI, Windows rendering, player performance, preview regression, CodeQL, and all nine regression shards are green.
Review size
390 additions and 9 deletions across 5 files, below the 1,500-line stack limit.