Make WordPress Core

Opened 6 weeks ago

Closed 5 weeks ago

#64803 closed defect (bug) (fixed)

Media: Skip cross-origin isolation for third-party page builders

Reported by: adamsilverstein's profile adamsilverstein Owned by: adamsilverstein's profile adamsilverstein
Milestone: 7.0 Priority: normal
Severity: normal Version: trunk
Component: Media Keywords: has-patch commit has-unit-tests
Focuses: Cc:

Description

In 7.0, WordPress is adding headers to enable cross-origin isolation for the block editor, unlocking SharedArrayBuffer for client-side media processing.

DIP or COEP/COOP headers isolate the document into its own agent cluster, which blocks same-origin iframe access. Third-party page builders (e.g. Elementor, Beaver Builder, Divi) that override the block editor typically rely on same-origin iframes and would break under DIP.

These page builders register custom action query parameters (e.g. action=elementor) to load their editor instead of the block editor. This ticket adds a check to client side media when a non-edit action parameter is detected on the post editing screen.

Open question: Some editors like Web Stories use action=edit but replace the block editor entirely. The current check would still apply DIP in that case. We may need a filter or an alternative detection mechanism to handle this edge case.

PR: PR #11170

See also: discussion on PR #11098

Change History (28)

This ticket was mentioned in PR #11170 on WordPress/wordpress-develop by @adamsilverstein.


6 weeks ago
#1

  • Keywords has-patch added

## Summary

  • Adds a check to skip Document-Isolation-Policy (DIP) when a third-party page builder overrides the block editor via a custom action query parameter
  • DIP isolates the document into its own agent cluster, which blocks same-origin iframe access that page builders rely on
  • Split out from #11098 per review feedback to allow separate discussion of edge cases (e.g. Web Stories uses action=edit but replaces the block editor entirely)

See discussion: https://github.com/WordPress/wordpress-develop/pull/11098#discussion_r2064009989

## Test plan

  • [ ] Verify the block editor still gets cross-origin isolation headers on standard post/page edit screens
  • [ ] Verify that a page builder using a custom action parameter (e.g. action=elementor) does NOT get DIP headers
  • [ ] Discuss edge cases where action=edit is used but the block editor is replaced (e.g. Web Stories)

#2 @adamsilverstein
6 weeks ago

  • Milestone changed from Awaiting Review to 7.0

#3 @manhar
6 weeks ago

Tested with WordPress trunk and Elementor.

Before applying the patch:
Cross-Origin-Embedder-Policy and Cross-Origin-Opener-Policy headers were present when opening the editor with action=elementor.
https://postimg.cc/HctzBfPB

After applying the patch:
These headers are no longer sent for post.php?post=68&action=elementor.
https://postimg.cc/Jt45RYF0

Patch works as expected.

#4 follow-up: @westonruter
6 weeks ago

I tried Beaver Builder and it seems editing a post takes me to the frontend and I'm not in the admin. For example, I'm taken to /?page_id=1939&fl_builder&fl_builder_ui.

#5 in reply to: ↑ 4 @adamsilverstein
5 weeks ago

Replying to westonruter:

I tried Beaver Builder and it seems editing a post takes me to the frontend and I'm not in the admin. For example, I'm taken to /?page_id=1939&fl_builder&fl_builder_ui.

Did you notice any issues using it though? Can you test uploading an image?

This ticket was mentioned in Slack in #meta by adamsilverstein. View the logs.


5 weeks ago

@adamsilverstein commented on PR #11170:


5 weeks ago
#7

In my testing with Elementor _without_ this PR things seemed to be fixed, perhaps because we switched to using Document Isolation Policy. I will confirm this with their team, then I think we can close this PR as "working as expected".

@louiswol94 commented on PR #11170:


5 weeks ago
#8

In my testing with Elementor _without_ this PR things seemed to be fixed, perhaps because we switched to using Document Isolation Policy. I will confirm this with their team, then I think we can close this PR as "working as expected".

@adamsilverstein Can you test by removing this filter? https://github.com/elementor/elementor/pull/34900/changes

This fix was added on our side in 3.35 (latest version). But all our users that have < 3.35 would be affected.

@adamsilverstein commented on PR #11170:


5 weeks ago
#9

In my testing with Elementor _without_ this PR things seemed to be fixed, perhaps because we switched to using Document Isolation Policy. I will confirm this with their team, then I think we can close this PR as "working as expected".

@adamsilverstein[[Image(chrome-extension://hgomfjikakokcbkjlfgodhklifiplmpg/images/wp-logo.png)]] Can you test by removing this filter? https://github.com/elementor/elementor/pull/34900/changes

This fix was added on our side in 3.35 (latest version). But all our users that have < 3.35 would be affected.

I was actually testing on an older version before upgrading and it seemed to work fine, but I will also try disabling the filter now that I have upgraded Elementor.

@westonruter commented on PR #11170:


5 weeks ago
#10

@louiswol94 I don't get any error with that line commented out.

However, I'm seeing something else unexpected here:

https://github.com/WordPress/wordpress-develop/blob/db24a903a5d4ed6ca98666e4dc6bcefdbcec1b30/src/wp-includes/default-filters.php#L682

I don't see that wp_set_up_cross_origin_isolation() is getting called at all when accessing the Elementor editor. However, if I change it to:

add_action( 'load-post.php', fn() => wp_set_up_cross_origin_isolation() );

Then it _does_ run and I get an error in the console:

Uncaught SecurityError: Failed to read a named property 'elementorFrontend' from 'Window': Blocked a frame with origin "http://localhost:8000" from accessing a cross-origin frame.

And I see the Elementor loading spinner indefinitely:

https://github.com/user-attachments/assets/f0b2831f-0d44-4b52-9139-9dc0fb63d63c

Why wouldn't add_action( 'load-post.php', 'wp_set_up_cross_origin_isolation' ) work in Elementor?

@adamsilverstein commented on PR #11170:


5 weeks ago
#11

@louiswol94 - I did verify that Elementor loads without error even with that filter removed, so I'm not sure this PR is needed. It would still be good to address Weston's question above.

@westonruter commented on PR #11170:


5 weeks ago
#12

I can confirm that the patch fixes the issue in Elementor. However, I can only reproduce the issue if I do this change as well:

  • src/wp-includes/default-filters.php

    a b add_filter( 'plupload_default_settings', 'wp_show_heic_upload_error' ); 
    679679// Client-side media processing.
    680680add_action( 'admin_init', 'wp_set_client_side_media_processing_flag' );
    681681// Cross-origin isolation for client-side media processing.
    682 add_action( 'load-post.php', 'wp_set_up_cross_origin_isolation' );
     682add_action( 'load-post.php', fn() => wp_set_up_cross_origin_isolation() );
    683683add_action( 'load-post-new.php', 'wp_set_up_cross_origin_isolation' );
    684684add_action( 'load-site-editor.php', 'wp_set_up_cross_origin_isolation' );
    685685add_action( 'load-widgets.php', 'wp_set_up_cross_origin_isolation' );

(Aside from also commenting out the change introduced in https://github.com/elementor/elementor/pull/34900.)

@louiswol94 commented on PR #11170:


5 weeks ago
#13

Hey guys...

Quick question:

Does DIP break same-origin contentWindow access?

According to this https://developer.chrome.com/blog/document-isolation-policy

Iframes isolated with Document Isolation Policy don't have synchronous DOM access to same-origin iframes that are not isolated.

I get this error on my console (on http://elementorlocal.local/):

The Document-Isolation-Policy header has been ignored because the URL's origin was untrustworthy. Please deliver the response using the HTTPS protocol. You can also use the 'localhost' origin instead. See https://www.w3.org/TR/powerful-features/#potentially-trustworthy-origin.

Which makes me think that if I was on a trusted origin, DIP would be enforced and I might get a security error.

Then also, is it possible you cannot reproduce due to the Gutenburg plugin doing:

remove_action( 'load-post.php', 'wp_set_up_cross_origin_isolation' );
remove_action( 'load-post-new.php', 'wp_set_up_cross_origin_isolation' );

Thanks for bearing with me here guys :D I am just trying my best to understand the full picture and make sure we reach the right conclusion.

@louiswol94 commented on PR #11170:


5 weeks ago
#14

Something else that might be important evidence...

I ran our test suite against this PR: https://github.com/elementor/elementor/pull/35068/changes

In this PR I removed the filter and made sure the WP nightly version is used.

From the logs I can see PlayWright used Chromium 139.0.7258.5 which is above 137.

And all the tests passed.

@louiswol94 commented on PR #11170:


5 weeks ago
#15

I will monitor this thread, but after my latest findings if you guys still do not think this PR is necessary, then I am in agreement not to merge it.

I really want to thank you guys for the work you are doing!

Kind regards
Louis

@adamsilverstein commented on PR #11170:


5 weeks ago
#16

I will monitor this thread, but after my latest findings if you guys still do not think this PR is necessary, then I am in agreement not to merge it.

@louiswol94 thanks for the continued testing here. One way to test Beta 3 on an actual site is to install the Beta Tester Plugin - https://wordpress.org/plugins/wordpress-beta-tester/. Install it and set it to download nightlies, then update. Installing the Gutenberg plugin is a good proxy, but doesn't represent the exact code WordPress will release with. If you have some sites running older Elementor versions you could test on that would provide empirical validation.

Hi, sorry to jump in like this - but if this PR doesn't get merged, millions of Elementor sites will become unusable.

@miriamelementor - don't worry, we still have time to get this _very small change_ into the release, but I want to make sure its really needed. We made an architectural change recently (switching the header approach) that may have already fixed the issue. The fix in this PR turns off the feature for Elementor, meaning you won't be able to use it or any of the new capabilities it provides, so i want to make sure we are turning it off for a reason!

@louiswol94 commented on PR #11170:


5 weeks ago
#17

Thanks @adamsilverstein - I forgot about the Beta Tester Plugin. I am glad you prompted me to test further.

I installed:

  1. WordPress 7.0-beta3-61869.
  2. Elementor 3.34.4

On a Live website. And get the following:

editor.min.js?ver=3.34.4:2 Uncaught SecurityError: Failed to read a named property 'elementorFrontend' from 'Window': Blocked a frame with origin "https://naharwuz.elementor.cloud" from accessing a cross-origin frame.
    at editor.min.js?ver=3.34.4:2:1259679
    at Editor.onPreviewLoaded (editor.min.js?ver=3.34.4:2:1260247)
    at HTMLIFrameElement.dispatch (load-scripts.php?c=1&load%5Bchunk_0%5D=jquery-core,jquery-migrate,jquery-ui-core,jquery-ui-mouse,underscore,backbone,wp-api-request,wp-hooks&ver=7.0-beta3-61869:2:40035)
    at v.handle (load-scripts.php?c=1&load%5Bchunk_0%5D=jquery-core,jquery-migrate,jquery-ui-core,jquery-ui-mouse,underscore,backbone,wp-api-request,wp-hooks&ver=7.0-beta3-61869:2:38006)

https://github.com/user-attachments/assets/f4c9d673-9d02-4ff6-b8ef-40518752612f
https://github.com/user-attachments/assets/67a6c069-bfd6-44d6-861a-c2db43239d84
https://github.com/user-attachments/assets/84dd3170-26b7-460e-a531-c3a68ca66586

@louiswol94 commented on PR #11170:


5 weeks ago
#18

Here is a snapshot of the DOM:

https://github.com/user-attachments/assets/ed5c267b-9fec-414a-8bc5-6d3b6720babf

@westonruter commented on PR #11170:


5 weeks ago
#19

@louiswol94:

Then also, is it possible you cannot reproduce due to the Gutenburg plugin doing:

remove_action( 'load-post.php', 'wp_set_up_cross_origin_isolation' );
remove_action( 'load-post-new.php', 'wp_set_up_cross_origin_isolation' );

Right you are! I deactivated Gutenberg and then I was able to get the error, and this PR fixes the issue.

@westonruter commented on PR #11170:


5 weeks ago
#20

I get this error on my console (on http://elementorlocal.local/):

The Document-Isolation-Policy header has been ignored because the URL's origin was untrustworthy. Please deliver the response using the HTTPS protocol. You can also use the 'localhost' origin instead. See https://www.w3.org/TR/powerful-features/#potentially-trustworthy-origin.

Which makes me think that if I was on a trusted origin, DIP would be enforced and I might get a security error. (This also aligns with the code comment in this PR).

@adamsilverstein This relates to what we were discussing with whether client-side media handling should be enabled on localhost. In addition to the error @louiswol94 got above, I also see in the console:

Client-side media processing unavailable: SharedArrayBuffer is not available. This may be due to missing cross-origin isolation headers.. Using server-side processing.

This is because SharedArrayBuffer is never available in non-secure or localhost context.

I think wp_set_up_cross_origin_isolation() should be updated to short-circuit if the ( ! is_ssl() && $_SERVER['HTTP_HOST'] !== 'localhost' ). Or I guess, rather, that this should be the default value of wp_is_client_side_media_processing_enabled():

  • src/wp-includes/media.php

    diff --git a/src/wp-includes/media.php b/src/wp-includes/media.php
    index 8e4f67a349..79e2dc5937 100644
    a b function wp_get_image_editor_output_format( $filename, $mime_type ) { 
    63716371 * @return bool Whether client-side media processing is enabled.
    63726372 */
    63736373function wp_is_client_side_media_processing_enabled(): bool {
     6374        $enabled = ( is_ssl() || 'localhost' === $_SERVER['HTTP_HOST'] );
     6375
    63746376        /**
    63756377         * Filters whether client-side media processing is enabled.
    63766378         *
    63776379         * @since 7.0.0
    63786380         *
    6379          * @param bool $enabled Whether client-side media processing is enabled. Default true.
     6381         * @param bool $enabled Whether client-side media processing is enabled. Default true if SSL or localhost.
    63806382         */
    6381         return (bool) apply_filters( 'wp_client_side_media_processing_enabled', true );
     6383        return (bool) apply_filters( 'wp_client_side_media_processing_enabled', $enabled );
    63826384}
    63836385
    63846386/**

This should fix the issue that Louis observed.

@louiswol94 I presume you were testing on a non-HTTPS site first?

@louiswol94 commented on PR #11170:


5 weeks ago
#21

@louiswol94 I presume you were testing on a non-HTTPS site first?

@westonruter Correct - http://elementorlocal.local/

@adamsilverstein commented on PR #11170:


5 weeks ago
#23

I added a couple of tests for this in https://github.com/WordPress/wordpress-develop/pull/11170/commits/85d6e62e24b40b10a6da49f98be932e0ba563913 and it is ready for another review @westonruter @mukeshpanchal27.

I'd like to get this committed asap.

@adamsilverstein commented on PR #11170:


5 weeks ago
#24

Ship it!

https://github.com/user-attachments/assets/a9adedc0-ca2d-4aae-a41b-e19b9869f819

I'm a little disappointed GitHub doesn't offer a way to extend the emojis I can respond with so i can use the ship.

@adamsilverstein commented on PR #11170:


5 weeks ago
#25

Checking test failures...

@adamsilverstein commented on PR #11170:


5 weeks ago
#26

Checking test failures...

Ah, since the CI runs on a non-SSL, non-localhost environment, wp_is_client_side_media_processing_enabled() returns false which is breaking some tests. Fixing.

#27 @adamsilverstein
5 weeks ago

  • Keywords commit has-unit-tests added

#28 @adamsilverstein
5 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 61947:

Editor: Skip cross-origin isolation for third-party page builders.

Document-Isolation-Policy (DIP) isolates the document and blocks same-origin iframe access that page builders rely on. Skip DIP setup when a third-party page builder overrides the block editor via a custom action query parameter.

Also gates wp_is_client_side_media_processing_enabled() on a secure context check, since SharedArrayBuffer requires a secure context (HTTPS or localhost).

Props adamsilverstein, westonruter, mukesh27, louiswol94, manhar, illuminea.
Fixes #64803.

Note: See TracTickets for help on using tickets.