fix(player): authenticate slideshow navigation senders - #3820
Merged
Conversation
miguel-heygen
approved these changes
Sep 9, 2026
miguel-heygen
left a comment
Collaborator
There was a problem hiding this comment.
The slideshow sender guard and the test-isolation follow-up are both clean at exact head 40e6dc112a985cbd6fed4e4a56c48137f5d07b01.
packages/player/src/slideshow/hyperframes-slideshow.ts:916-930authenticates the browser sender before reading navigation data. Only the immediate embedding parent or the slideshow’s own window can drivenext,prev,goto, orback; child/sibling/null senders fail closed. Same-window navigation remains available, and the separate audienceBroadcastChannelpath is unchanged.- Repo-wide caller/docs audit found no legitimate child-iframe or opener navigation contract. Parent-driven embeds and self-dispatch are the two supported postMessage cases; audience synchronization remains in
slideshowPresenter.ts:128-149. packages/sdk-playground/src/main.test.ts:5-9now mocks only initialization dependencies that are unreachable while the test’screateFileAdapterpromise stays pending. The real static DOM,main.tslistener registration, current/replaced iframe lookup, message type allowlist, and DOM assertions remain exercised. No timeout increase or production playground change.
Independent verification:
- Player suite: 370/370 passed; player build and typecheck passed; changed player files pass oxlint/format.
- Nested-frame Chromium against the built player accepted parent
next/prev/goto/backand selfnext, rejected sibling and null senders, and ignored parent/self navigation in audience mode; no page errors. - Updated playground witness: 1/1 passed in 167 ms in the previously blocked isolated worktree; full package 4/4, typecheck, oxlint, and format passed.
- Fresh hosted full Test now passes without changing the 5-second budget. All applicable CI, Windows, preview/perf, and CodeQL checks are green; the PR-ref open-alert set is empty and no review threads/comments are open.
— Magi
Verdict: APPROVE
Reasoning: Sender identity is enforced at the shared navigation boundary without breaking the documented parent/self or audience contracts, and the targeted test isolation preserves the real security witness while removing unrelated transform cost; every native gate is green at the unchanged head.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The slideshow accepts bare
next,prev,goto, andbackmessages from any window. An unrelated child or sibling frame can therefore navigate the host slideshow.Authenticate the sender before reading the payload: accept the embedding parent or the slideshow's own window (#634). Existing parent-driven embeds and same-window navigation remain available. Audience mode continues to ignore postMessage navigation and use its existing BroadcastChannel path. This is separate from the composition-to-host sound-effect contract fixed in #3819.
Verification: all 370 player tests, player typechecks, full workspace build, lint/format and signed hooks pass. The new sender regression fails the original implementation. Real Chromium verifies all four commands from a distinct embedding parent, same-window navigation, and rejection of sibling/null-source messages. Existing synthetic navigation tests now supply the actual self sender. Fallow has no changed-file findings.
Please independently verify the embed sender contract and #634's PR-ref result. No dismissal or query suppression is requested; verify main CodeQL closure after merge.
The playground message-boundary witness now mocks unused SDK/parser/GSAP initialization imports. Its initializer is already paused at persistence; the real UI listener remains exercised. This avoids transforming unrelated dependencies inside the test budget (two hosted 5s timeouts); all four local playground tests pass with the witness at127ms and unchanged timeout. Types and signed hooks pass.