Validate origin and restrict child command dispatch on postMessage #19020

[Vvanlaar]

Ticket

#19020 — the bridge routed on ev.source and never re-validated event.origin, and dispatched this[ev.data.methodName] dynamically.

Summary

• CHILD_COMMANDS allowlist replaces dynamic dispatch. setLocation (which assigns window.location.href — a redirect / javascript: primitive), exit, callParent and callChild are no longer reachable by name from the child. Direct instance calls are unaffected; verified the player invokes none of the four.
• _isTrustedChildMessage re-validates ev.origin against the iframe origin on every message, exact match. ev.source can't be forged but it survives a navigation, so this is what catches the post-load src mutation the ticket describes.
• _iframeOrigin derived with URL, not the old regex. That regex yielded '//host' for a protocol-relative src — not a legal targetOrigin — and matched no host with a port at all, silently disabling validation for every dev/staging/on-prem deployment.
• Parent origin learned from ev.origin, not document.referrer.
• callChildPromise's inner listener checked neither source nor origin, so any window on the page could post a return and decide what the promise resolved to. Now guarded, and unbound on both outcomes (it previously leaked one listener per unanswered call).
• Outgoing targetOrigin no longer '*' on the parent direction.
• Adds a pr-checks workflow — the repo had no CI job running any test suite.

Rollout

Origin enforcement is warn-only by default (_enforceOrigin): mismatches are logged but still handled. The command allowlist ships enforced.

The handshake is deliberately gated on ev.source only, not origin: gating it would leave _handshakeSucceededChild false forever under enforcement — every callChild queued indefinitely, fullscreen dead, and nothing in the log saying the bridge had died. The localStorage push that follows is protected by its targetOrigin regardless. Fullscreen and command dispatch keep the full check.

Self-review

Ran /code-review high plus four clean-context lenses. Findings fixed in e44d584; two were regressions the first commit introduced — the document.referrer parent-origin derivation (which would have made the browser drop every callParent after any external-link navigation) and moving the handshake onto the unvalidated _iframeOrigin.

Noted, not fixed: bootstrap's if (match !== null && match.length > 1 || true) makes the bbvms/mainroll iframe filter a no-op, so a bridge is constructed for every iframe on the page. Pre-existing; I only stopped the new warnings from firing for those. Worth its own ticket.

Test plan

• npm test — 27 tests, in a jest + jsdom harness built from zero ("test" was echo "Error: no test specified" && exit 1, with only a manual test.html).
• Covers: spoofed origin, the subdomain-prefix trick, post-load src mutation, protocol-relative and ported srcs, the excluded commands (setLocation specifically), malformed data, the callChildPromise hijack, handshake resilience under enforcement, and callParent targeting.
• Mutation-verified: removing the CHILD_COMMANDS check fails 5 tests.
• npm run build — parcel exits 0.

Paired with standardplayer #1401 for #19019 — the other half of the same channel.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR hardens BBIframeBridge’s postMessage surface by enforcing safer message routing: validating event.origin per message, restricting child→parent command dispatch to an allowlist, tightening targetOrigin usage, and adding a Jest + jsdom test suite plus CI to prevent regressions.

Changes:

• Add CHILD_COMMANDS allowlist to replace dynamic this[methodName] dispatch from child messages.
• Re-validate event.origin against a derived iframe origin on every child message (warn-only by default via _enforceOrigin), and learn _parentOrigin from ev.origin to avoid '*' targeting when possible.
• Add Jest tests and a PR workflow that runs npm test on every pull request.

Reviewed changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/BBIframeBridge.js	Implements origin re-validation, child command allowlist dispatch, safer targetOrigin selection, and callChildPromise listener hardening/unbinding.
src/BBIframeBridge.postMessage.test.js	Adds unit tests covering origin spoofing, allowlist enforcement, handshake behavior, and callChildPromise hijack prevention.
README.md	Documents the new child-command restrictions and origin re-validation behavior.
package.json	Switches npm test to run Jest and adds required dev dependencies.
jest.config.js	Adds Jest + jsdom configuration and coverage settings.
jest.babel.config.json	Adds Babel preset config for Jest transforms.
.github/workflows/pr-checks.yaml	Adds PR CI workflow that installs dependencies and runs the Jest suite.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Vvanlaar]

Confirmed — real leak. In the it.each(["exit", "callParent", "callChild"]) case exit is spied with a no-op implementation, so afterEach calling bridge.exit() hit the mock, skipped removeEventListener, and left the window/document listeners bound. That matters here specifically because the callChildPromise tests dispatch real message events, so a leaked listener from an earlier test would have been running against a torn-down bridge.

Fixed in e9232d2 by restoring mocks before exit() in all six teardowns, so the real implementation runs.

[Vvanlaar]

Right, and the docs were describing the old behaviour. The regex this PR replaced genuinely did fall back to * for a relative src; new URL(iframe.src, window.location.href) resolves it against the document, so relative and protocol-relative srcs now validate normally. I had already noticed this when writing the tests — the fixture changed from /relative/player.html to data:text/html for exactly this reason — but did not carry it back into the README or the constructor comment.

Both corrected in e9232d2: only an opaque or unparseable src disables validation now, plus a note that a src-less iframe skips it silently (a bridge is constructed for every iframe on the page, so warning there would be noise).

[Vvanlaar]

Agreed. The construction warning is gated on this._iframe.src being truthy — deliberately, since a bridge is built for every iframe on the page including src-less ad slots — so _iframeOrigin === "*" does not imply anything was logged. Comment reworded in e9232d2 to say the warning only fires when there was an src.

[sayahweb2-png]

Static allowlist is the right fix, origin derivation with URL() fixes the protocol-relative and port bugs, and the warn-only rollout is the safe path. LGTM.

[ghultink]

@sayahweb2-png this one looks like it just needs the last step — you approved it on 17 Aug and moved #19020 to Reviewed the same minute, but the PR was never merged. It's still MERGEABLE / CLEAN, so it should be a straight merge commit.

Flagging it because of how it hides: Reviewed auto-clears System.AssignedTo, so once the ticket moved it belonged to nobody, and the board reads it as done. It came up in a sweep for "Reviewed tickets whose PR is still open" — this was the only one out of 17.

Worth landing sooner rather than later given what's in it: origin validation on postMessage and restricting child command dispatch. It's been open 12 days while the ticket has read as closed out.

The three unresolved threads are all Copilot's and all marked outdated, so nothing looks like it's actually waiting on you — but you're the approver, so it's your call, not mine to merge.