VideoPress: Clean up postMessage bridge code#47436
Conversation
Extract duplicated allowed-origins arrays into a shared module with a type guard, add debug logging to origin checks, and improve error handling in the token bridge and media token flows.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
There was a problem hiding this comment.
Pull request overview
Security-focused cleanup of VideoPress postMessage bridges to reduce cross-origin risk (notably avoiding targetOrigin: '*' when sending JWTs), centralize origin allowlisting, and add unit tests around the bridge behaviors.
Changes:
- Introduce a shared
videopress-allowed-originsmodule and apply origin validation across token/player bridges and block editor listeners. - Update the token bridge to reply using the validated
event.origin(instead of*) and remove the legacyvideopress-token-bridge.js+ webpack entry. - Add new Jest test suites for token bridge and player bridge; improve token fetch failure logging in
get-media-token.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/videopress/webpack.config.js | Removes the legacy token bridge webpack entry. |
| projects/packages/videopress/src/client/lib/videopress-token-bridge.js | Deletes unused legacy token bridge implementation. |
| projects/packages/videopress/src/client/lib/videopress-allowed-origins.ts | Adds shared allowlist + isAllowedOrigin() type guard. |
| projects/packages/videopress/src/client/lib/token-bridge/index.ts | Validates origin and uses validated targetOrigin for JWT delivery; adds try/catch around token fetch. |
| projects/packages/videopress/src/client/lib/token-bridge/test/index.test.ts | Adds coverage for origin validation and non-wildcard targetOrigin behavior. |
| projects/packages/videopress/src/client/lib/player-bridge/index.ts | Adds origin validation for listening events via shared allowlist. |
| projects/packages/videopress/src/client/lib/player-bridge/test/index.test.ts | Adds tests for listening/emitting behaviors and origin filtering. |
| projects/packages/videopress/src/client/lib/get-media-token/index.ts | Switches token request failure logging from console.warn to debug. |
| projects/packages/videopress/src/client/block-editor/hooks/use-video-player/index.ts | Adds origin validation to message listener in the editor hook. |
| projects/packages/videopress/src/client/block-editor/blocks/video/components/poster-panel/index.tsx | Makes postMessage target origin explicit ('*'). |
| projects/packages/videopress/src/client/block-editor/blocks/video/components/player/index.tsx | Adds origin validation for the player loading-state listener. |
| projects/packages/videopress/changelog/harden-videopress-postmessage-security | Adds a patch changelog entry describing the hardening. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/packages/videopress/src/client/block-editor/hooks/use-video-player/index.ts
Show resolved
Hide resolved
Code Coverage SummaryCoverage changed in 4 files.
1 file is newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
Browsers can deliver MessageEvents with null source (e.g., from closed windows or certain cross-origin scenarios). Add a null check alongside the existing MessagePort/ServiceWorker guards to prevent a TypeError when calling postMessage on a null source. Also strengthens the 'accepts messages from video.wordpress.com' test to verify call count and payload content instead of just toHaveBeenCalled.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/packages/videopress/src/client/lib/token-bridge/index.ts
Outdated
Show resolved
Hide resolved
Move the origin and source checks above getSubscriberPlanIdIfExists() so invalid messages are rejected immediately without awaiting async I/O.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #
Proposed changes:
targetOrigin: '*'with validatedevent.originin token bridge JWT deliveryvideopress-allowed-originsmodule withisAllowedOrigin()type guardvideopress-token-bridge.jsand its webpack entryOther information:
Security-adjacent cleanup of postMessage handlers. The token bridge was sending JWTs with
targetOrigin: '*'— now uses the validated origin. Message listeners in the block editor hooks lacked origin checks (low risk since they're on SandBox contentWindow, but added for defense-in-depth).No functional changes to the user-facing video player behavior.
Jetpack product discussion
N/A — security-adjacent cleanup, no product changes.
Does this pull request change what data or activity we track or use?
No. This PR does not change what data or activity we track or use. It only tightens origin validation on existing postMessage communication between the VideoPress player iframe and the host page.
Testing instructions:
localStorage.debug = 'videopress:*'and reload*)cd projects/packages/videopress && pnpm test— all 139 tests passChangelog