Skip to content

fix(core): prevent cross-origin Web Audio capture from silencing audio - #3481

Open
miga-heygen wants to merge 2 commits into
mainfrom
fix/web-audio-cross-origin-silence-v2
Open

fix(core): prevent cross-origin Web Audio capture from silencing audio#3481
miga-heygen wants to merge 2 commits into
mainfrom
fix/web-audio-cross-origin-silence-v2

Conversation

@miga-heygen

Copy link
Copy Markdown
Contributor

Summary

  • Detect cross-origin <audio> elements that would be silently muted by Web Audio's CORS policy
  • Route them to decode-only or native fallback automatically — no author opt-in needed
  • Surface bypass diagnostics via hyperframes check

Takes over #3459 with the data-native-audio escape hatch removed per review feedback — automatic detection covers the use cases.

Closes #3458

Original-Author: desenmeng
Co-Authored-By: desenmeng desenmeng@users.noreply.github.com
Co-Authored-By: Miga noreply@anthropic.com

Classify each <audio> element before Web Audio capture: same-origin,
CORS-opted-in, or a non-http(s) scheme stays on the primary
createMediaElementSource() path; cross-origin media without a
crossorigin opt-in withholds that call (the Web Audio spec makes such a
node output silence without throwing) and falls back to fetch +
decodeAudioData, preserving the FX graph whenever the server allows
CORS. Recheck the route at the transport's irreversible capture
boundary, and account for currentSrc, src, and <source> candidates the
same way the HTML resource-selection algorithm does.

Emit a stable preview diagnostic (`runtime_web_audio_bypass`) at media
discovery time, not only from playback scheduling, so `hyperframes
check` — which seeks but never plays — can surface it as a
`web_audio_bypass` finding. Diagnostics are suppressed during export
rendering, where the producer mixes audio offline and already applies
the FX chain. The existing non-unit-rate fail-closed rule stays scoped
to fx-chain/automation so this fix does not newly mute grouped or
above-unity tracks.

Takes over #3459 with the data-native-audio escape hatch removed per
review feedback: the automatic cross-origin detection already covers
the cases that mattered, so the extra per-element opt-in attribute,
its route-classifier branch, and its diagnostic path are dropped in
favor of a single automatic behavior.

Fixes #3458

Original-Author: desenmeng
Co-Authored-By: desenmeng <desenmeng@users.noreply.github.com>
Co-Authored-By: Miga <noreply@anthropic.com>

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES

Reasoning: The guard only protects an element before its first MediaElementAudioSourceNode is created. acquireMediaElementSource() returns the cached node before reclassifying the element. If a reused <audio> node first plays a same-origin source and later its src or <source> changes to cross-origin without CORS, direct callers reconnect that cached node and get the exact spec-mandated silence this PR fixes. The runtime caller classifies first and skips the transport, but the element was already permanently rerouted by the cached node; if decode fails, its claimed native fallback is still silent. Dynamic source updates on an existing DOM node therefore remain broken.

Please cover the same-element same-origin-to-cross-origin transition and make the route safe after a node has already been created (or explicitly replace or recreate the media element before native fallback). The current tests only exercise fresh elements, so they cannot see the one-way cached-node case. No merge action.

— Magi

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concur with @magi-bot CHANGES_REQUESTED at cce17da5. The cached MediaElementAudioSourceNode reuse is the primary blocker. Adding orthogonal concerns.

Concur with @magi-bot (verified at head):

  • WebAudioTransport.acquireMediaElementSource (packages/core/src/runtime/webAudioTransport.ts:263-273) returns _mediaElementSources.get(el) before the classifier runs. Grepped: _mediaElementSources is only invalidated in destroy() (line 753) — no emptied/abort/loadedmetadata-driven cache clear. Same-origin → cross-origin src mutation permanently holds the cached node; the element's native output stays hijacked per WebAudio spec; the classifier's "native fallback" is spec-impossible for that element.
  • decodeAudioElement becomes the ONLY working audio; if decode fails the element is silent — the same failure #3458 is meant to fix, now blessed by the runtime.
  • No test exercises reused elements (init.test.ts / webAudioRoute.test.ts construct fresh per case).

Additional (differentiated):

  1. Bind-time false-positive diagnostic on <source> fallback selection. init.ts:1871-1890 calls reportWebAudioRoute(mediaEl) synchronously at bind. webAudioRoute.ts:801-813 routeCandidates(): when both currentSrc and src attr are empty, walks <source> children and returns "decode-only" on the FIRST cross-origin URL. Scenario: <audio><source src="https://cdn.example.com/a.mp3"><source src="/assets/fallback.mp3"></audio>. Bind fires with no committed resource → conservative decode-only verdict → reportWebAudioMediaRoute emits [hyperframes] runtime_web_audio_bypass + latches diagnosedElements.add(el). Later loadedmetadata fires with currentSrc = /assets/fallback.mp3 (same-origin, browser-selected). Classifier now returns web-audio, no report. But the false-positive diagnostic already fired and the CLI check gate reports a phantom bypass. Fix: emit only from the loadedmetadata handler, or only latch after currentSrc is set.

  2. hasCorsOptIn secondary IDL check is over-permissive. webAudioRoute.ts:78-82:

    if (hasAttr(el, "crossorigin")) return true;
    return typeof el.crossOrigin === "string";

    In Chromium/Firefox/Safari, el.crossOrigin for an element with no attribute returns null (typeof "object"), so the fallback is inert. But in jsdom variants (and any host that returns "" for absent-attribute IDL), the fallback returns true for ALL cross-origin audio, silently disabling the entire guard in test envs. A genuine IDL-set el.crossOrigin = "anonymous" reflects to the attribute → primary check catches it. Recommend return typeof el.crossOrigin === "string" && el.crossOrigin.length > 0 — the empty-string fallback buys nothing and risks fail-open.

  3. Comment overstates enforcement reach. webAudioTransport.ts docstring: "init.ts routes on the same verdict before ever calling in; this stays the enforcement point so a direct caller (studio, player) cannot reopen the one-way door." But packages/studio/src/components/sidebar/AudioRow.tsx:149 calls actxRef.current.createMediaElementSource(el) DIRECTLY, bypassing WebAudioTransport entirely. Same-origin serveUrl in dev makes this benign today, but the enforcement claim is factually wrong. Either correct the comment or route AudioRow through the transport.

  4. srcObject / MediaStream binding unclassified. routeCandidates only walks currentSrc/src/<source>. el.srcObject = mediaStream leaves all three empty → classifier returns web-audio → MediaElementSource proceeds. For MediaStream, spec-fine. For MediaSource attached via srcObject (streaming HLS/DASH from a cross-origin CDN), MediaElementSource may still be silenced. Low prevalence in current HF compositions; document in the module preamble.

  5. Redirect chains bypass classification. isCorsSilenced (webAudioRoute.ts:822-833) judges the raw URL string, not the resolved fetch. <audio src="/proxy/track.mp3"> where /proxy 302s to https://cdn.example.com/track.mp3 → same-origin verdict → capture proceeds → spec silences. Fundamental gap without a HEAD probe or fetch-hook; document.

Softer-floor suggestion on @magi-bot's remediation: since the DOM <audio> node is composition-owned (not runtime-owned), a hard "explicitly recreate the element" is expensive. Softer: (a) listen for emptied/loadstart on bound media, _mediaElementSources.delete(el) + disconnect the stale node before new src loads; (b) if verdict flips to decode-only on a cached element, log a distinct DIAGNOSTIC_CACHE_POISONED code — since native recovery is spec-impossible, at minimum the failure has to be OBSERVABLE (matches the whole point of this PR).

— Review by tai (pr-review)

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES at acc6898

R2 delta verification (base cce17da5 → head acc6898).

Addressed cleanly (4/4 R2 claims verified):

  1. Bind-time false-positive (tai#1). isRouteSelectionSettled() gate added in webAudioRoute.ts; init.ts:1847-1852 skips the discovery-time report when both currentSrc is empty and src attr is absent. The loadedmetadata listener still fires with a settled currentSrc and calls reportWebAudioMediaRoute (latched per-element, so single report). No race: gate + classify are both synchronous with no microtask boundary between them, and routeCandidates reads the same currentSrc/src the gate just read.
  2. hasCorsOptIn (tai#2). Reduced to el.crossOrigin != null. Two new regression tests via withUnreflectedCrossOrigin (Object.defineProperty) cover both directions. Miga's != null correctly respects <audio crossorigin> — the bare-attribute anonymous opt-in whose IDL fallback is "". tai's suggested .length > 0 would have wrongly rejected that valid opt-in; Miga's trade-off is more spec-faithful. Untouched element (crossOrigin === null) still classifies as no-opt-in.
  3. AudioRow bypass (tai#3). AudioRow.tsx:172 now sets el.src = serveUrl BEFORE the classifier read (necessary — classifier reads currentSrc/src), then gates createMediaElementSource(el) behind classifyWebAudioMediaRoute(el).kind === "web-audio". Falls through to native <audio> playback (no visualizer) when the verdict blocks capture. Grep confirms AudioRow was the only other direct createMediaElementSource call site outside the transport (the hit in packages/lint/src/rules/media.ts is a rule reference, not a call). Public subpath @hyperframes/core/runtime/web-audio-route wired via package-subpaths.json + package.json exports.
  4. srcObject (tai#4). Documented in webAudioRoute.ts module docstring as "recorded as a boundary rather than fixed" — defensible: no current codepath feeds createMediaElementSource from a srcObject element.

BLOCKER unresolved — Magi's R1 primary finding:

WebAudioTransport.acquireMediaElementSource (webAudioTransport.ts:262-274) is untouched in this delta:

private acquireMediaElementSource(el) {
  const cached = this._mediaElementSources.get(el);
  if (cached) return cached;   // ← returns before reclassifying
  ...
  const route = classifyWebAudioMediaRoute(el);
  if (route.kind !== "web-audio") { reportWebAudioMediaRoute(el, route); return null; }
  const sourceNode = this._ctx.createMediaElementSource(el);
  this._mediaElementSources.set(el, sourceNode);
  return sourceNode;
}

_mediaElementSources is still only cleared in destroy() (line 753) — no emptied / loadstart / abort-driven eviction. The same-element same-origin-then-cross-origin src mutation Magi described therefore remains broken: the cached MediaElementAudioSourceNode created against the original same-origin src is returned on the reused-element path, permanently rerouting the element's native output per the Web Audio spec even though a fresh classify would now say decode-only.

The AudioRow classifier gate does not cover this case — that fix protects the studio preview player, not runtime element reuse in the transport. And no transport-level test exercises reused elements: webAudioTransport.test.ts:167 covers a fresh cross-origin element only, and line 150's "cached native source reusable" test is same-origin throughout.

Also unresolved (soft):

  • tai#5 (redirect chains). isCorsSilenced still judges the raw URL string; not documented in the delta.
  • webAudioTransport.ts docstring on acquireMediaElementSource still claims to be "the enforcement point so a direct caller (studio, player) cannot reopen the one-way door." AudioRow (a studio direct caller) demonstrates it is one of two co-equal enforcement points; the softened claim in webAudioRoute.ts acknowledges this, but the transport's own docstring does not. Minor.

Minimum ask:

Either (a) invalidate _mediaElementSources on emptied / loadstart and disconnect the stale node before the next classify, or (b) if out of scope, document the cached-node hazard in the module docstring alongside the srcObject boundary and add a distinct DIAGNOSTIC_CACHE_POISONED observable code so failures are at least surfaced (per tai's soft-floor suggestion). Either resolves Magi's block; leaving it silent-and-undocumented does not.

CI at time of review: Producer unit tests SUCCESS, Lint SUCCESS, Fallow audit SUCCESS, Typecheck in progress; regression + windows-render + player-perf shards still running. No merge action.

— Via

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 at head acc68982 after Miga's fix-up. Items 1, 3, 4 + @magi-bot's cached-node reuse are addressed well; Item 5 (redirect chains) is explicitly deferred (acceptable). One residual defense-in-depth gap on the transport worth flagging, but no code-level blocker at head.

Retraction on my R1 Item 2 — I was wrong.

My R1 recommendation to tighten hasCorsOptIn to typeof el.crossOrigin === "string" && el.crossOrigin.length > 0 is spec-wrong. Per HTML spec crossorigin is an enumerated attribute whose invalid-value default is anonymous; therefore crossorigin="" IS a valid opt-in equivalent to anonymous, and its IDL fallback reads as "". Miga's != null at webAudioRoute.ts:80-83 is the correct predicate. My length > 0 suggestion would have falsely rejected legitimate opt-ins for hosts that expose the value only via the IDL property (not reflected to the attribute), reintroducing the silent-audio bug this PR is meant to fix. The module comment at webAudioRoute.ts:61-79 calls this out explicitly, and the tests at webAudioRoute.test.ts:60-68, 114-138 assert both the reflected-attribute path and the unreflected empty-IDL path correctly. Withdrawing the R1 recommendation.

Concur on the fixes (verified at head):

  • Item 1 (bind-time false-positive)isRouteSelectionSettled(el) at webAudioRoute.ts:150-154 returns true only when currentSrc or src attr is set. init.ts:1845-1855's discovery-time reportWebAudioRoute skips when unsettled; init.ts:1898 binds loadedmetadata for the deferred report. Latch in reportWebAudioMediaRoute (webAudioRoute.ts:238-240) only fires on non-web-audio verdicts, so the early-skip does not consume the latch. Edge case: if <source> selection genuinely never settles (all candidates fail to load), discovery-time won't fire, BUT the schedule path at init.ts:3170-3171 still calls reportWebAudioMediaRoute(rawEl, route) when the classifier's <source>-walk produces a decode-only verdict — so an authored composition that ever tries to schedule the audio still emits. Acceptable trade-off.
  • Item 3 (AudioRow bypass)AudioRow.tsx:174 sets el.src = serveUrl before classifying; :184 calls classifyWebAudioMediaRoute(el) and only wires createMediaElementSource on .kind === "web-audio". Non-web-audio verdict is a graceful no-op: analyser/visualizer bar skipped, but native <audio> playback below (:194 audioRef.current.play()) still runs. New public subpath @hyperframes/core/runtime/web-audio-route correctly declared in packages/core/package-subpaths.json (import, browser export, CommonJS export all present).
  • Item 4 (srcObject / MediaStream) — module docstring at webAudioRoute.ts:30-36 is explicit and useful: "a srcObject element always reads as web-audio here, correctly or not. Nothing in this codebase feeds createMediaElementSource from a srcObject element today, so this is recorded as a boundary rather than fixed." Documented deferral is fine.
  • @magi-bot's cached-node reuse — closed at the caller side. init.ts:3170 calls classifyWebAudioMediaRoute(rawEl) fresh on every schedule invocation and only invokes scheduleMediaElementPlayback on .kind === "web-audio", so a src flip from same-origin (cached node exists) to cross-origin can no longer trigger the poisoned-cache path via the timeline scheduler.

One residual defense-in-depth gap (non-blocking, scope call):

WebAudioTransport.acquireMediaElementSource at webAudioTransport.ts:262-274 still returns this._mediaElementSources.get(el) unconditionally when cached, WITHOUT re-classifying — the classifyWebAudioMediaRoute call at :266 only runs on cache miss. So the transport's docstring at :258-260 ("this stays the enforcement point so a direct caller … cannot reopen the one-way door") overclaims: the enforcement only holds on FIRST bind. init.ts:3170's pre-classify closes the observed path today, but any future direct caller of acquireMediaElementSource (or a refactor of init.ts that stops pre-classifying) touching an element whose src was flipped after first cache would hand back a stale silent node.

Two ways to close it:

  • (a) Invalidate the _mediaElementSources entry when src changes — element listener for emptied/loadstart or comparing currentSrc against the value cached at first bind.
  • (b) Move the classifyWebAudioMediaRoute check ahead of the cache return inside acquireMediaElementSource — reject cached nodes for elements whose current verdict is no longer web-audio.

Scope call — happy to see either in this PR or a follow-up.

Minor:

  • Redirect-chain boundary (Item 5) is deferred but not documented in isCorsSilenced — a parallel doc line matching the srcObject boundary note (webAudioRoute.ts:30-36) would be honest.
  • Transport docstring at :258-260 still says "enforcement point"; Miga's Slack described "softened to caller contract," but that exact phrase isn't in the code. Downstream of the defense-in-depth call above — if you close the cached-node re-classify, the "enforcement point" claim becomes literally true and the comment can stay.

CI status at head: MANY required checks still IN_PROGRESS (Build, CLI smoke, Typecheck aggregate, Tests, Tests on windows-latest, all 9 regression-shards, Render on windows, Preview parity, Producer integration, Perf suite). Not stamping until CI settles.

Test coverage on the new work is strong — webAudioRoute.test.ts has 12 tests including both crossorigin-attr-reflection paths and the <source> walk. Good.

— Review by tai (pr-review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

createMediaElementSource silently mutes cross-origin media that has no CORS opt-in

4 participants