Make WordPress Core

Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#40020 closed defect (bug) (fixed)

Customizer fails to load in Safari due to X-Origin Header mismatch

Reported by: nickkeenan's profile nickkeenan Owned by: pento's profile pento
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.7.2
Component: Customize Keywords: has-patch dev-feedback has-unit-tests
Focuses: Cc:

Description

Steps to Reproduce:

1) Using Safari (10.0.3, possibly other recent versions)
2) Plugins disabled, using TwentySeventeen theme, and WP 4.7.2
3) This is a site where the home and siteurl slightly differ. home is domain.com, and siteurl is domain.com/wp.
3) Open the Customizer.

Result: Blank Customizer Frame, with console errors:

[Error] Multiple 'X-Frame-Options' headers with conflicting values ('ALLOW-FROM http://archetype.gameflow.design/wp/wp-admin/customize.php, SAMEORIGIN') encountered when loading 'http://domain.com/?customize_changeset_uuid={{INSERT-UUID-HERE}}&customize_theme=twentyseventeen&customize_messenger_channel=preview-0'. Falling back to 'DENY'.

[Error] Refused to display 'http://archetype.gameflow.design/?customize_changeset_uuid={{INSERT-UUID-HERE}}&customize_theme=twentyseventeen&customize_messenger_channel=preview-0' in a frame because it set 'X-Frame-Options' to 'ALLOW-FROM http://archetype.gameflow.design/wp/wp-admin/customize.php, SAMEORIGIN'.

Potential Cause:
There are conflicting X-Frame-Headers which fallback to DENY in Safari 10.0.3.

wp-includes/class-wp-customize-manager.php line 1599:
public function filter_iframe_security_headers( $headers )

Conflicts with

wp-includes/functions.php line 5017:
function send_frame_options_header()

Which is loaded on default-filters.php on either login_init or admin_init.

Attachments (3)

Screen Shot 2017-03-02 at 12.42.39 PM.png (205.8 KB) - added by nickkeenan 8 years ago.
Screenshot of Safari Console Error
40020.diff (791 bytes) - added by fullyint 7 years ago.
40020.2.diff (1.8 KB) - added by fullyint 7 years ago.

Download all attachments as: .zip

Change History (20)

@nickkeenan
8 years ago

Screenshot of Safari Console Error

#1 @swissspidy
8 years ago

Hey there, thanks for your report!

This sounds like a duplicate of #39128. Can you verify that?

#2 @nickkeenan
8 years ago

No, I believe this is a distinct issue. While our configuration also has a different siteurl from home, This issue doesn't affect all browsers like #39128 - and we're not seeing the Non-existent changeset UUID error. Chrome and Firefox are operating normally, and Safari only is affected. IE is untested on my end.

#3 @westonruter
8 years ago

  • Keywords reporter-feedback added

@nickkeenan I cannot reproduce this issue.

The call to send_frame_options_header() happens before WP::send_headers() which is what invokes WP_Customize_Manager::filter_iframe_security_headers() via the wp_headers filter. Because WP::send_headers() uses header( $header ) and leaves off the second $replace parameter, the default value of true will be used. This means the last-sent X-Frame-Options header sent should be the one that ultimately gets sent to the client.

I set up a similar configuration as you, with WordPress installed in a subdirectory, the home being set to http://core-subdirectory.vvv and the siteurl being set to http://core-subdirectory.vvv/src.

When I go to http://core-subdirectory.vvv/src/wp-admin/customize.php, I get this response header back:

X-Frame-Options: SAMEORIGIN

When I look at the network console for the document loaded into the iframe, here http://core-subdirectory.vvv/?customize_changeset_uuid=92126aad-72f7-4c15-a7b1-e73ed23fb7a4&customize_theme=twentyseventeen&customize_messenger_channel=preview-0, I get these headers in the response:

X-Frame-Options: ALLOW-FROM http://core-subdirectory.vvv/src/wp-admin/customize.php
Content-Security-Policy: frame-ancestors http://core-subdirectory.vvv

So I'm not sure why your X-Frame-Options headers are showing multiple combined values that conflict. My best guess is that your web server is configured to add this additional value to the X-Frame-Options response header, or you have a plugin that is calling something like:

<?php
add_action( 'send_headers', function() {
    header( 'X-Frame-Options: SAMEORIGIN', false );
} );

#4 follow-up: @endortrails
8 years ago

I'm receiving the exact same error as @nickkeenan. I'm adding some additional details in case it helps others replicate the issue. This only seems to happen in Safari. Chrome and FF are fine.

How to replicate:

  1. Safari version 10.0.1
  2. WordPress network using subdomains
  3. Wildcard SSL cert to secure all subdomains
  4. NGINX with a single add_header X-Frame-Options SAMEORIGIN; set in configs.

#5 in reply to: ↑ 4 @westonruter
8 years ago

Replying to endortrails:

  1. NGINX with a single add_header X-Frame-Options SAMEORIGIN; set in configs.

So if you remove this Nginx rule, the Customizer loads as expected in Safari?

The resolution here may be that the Nginx config should be removed in favor of adding the X-Frame-Options header via PHP in WordPress only.

  1. WordPress network using subdomains
  2. Wildcard SSL cert to secure all subdomains

To confirm, the home and siteurl on each of the sites match each other, right?

Related: #38571

#6 @jeremyfelt
7 years ago

I'm able to reproduce this in Safari 11.0.1 when Nginx has a add_header X-Frame-Options SAMEORIGIN always; directive applied. Safari sees conflicting rules and then falls back to DENY.

When the directive is removed in Nginx, the Customizer frame loads, but Safari still reports an error that ALLOW-FROM http://wp.wsu.dev/wp-admin/customize.php is not a recognized directive for X-Frame-Options and ignores the header. Safari and Chrome do not support ALLOW-FROM

In my case, I *believe* it's safe (in custom code) to remove the ALLOW-FROM header and rely on the SAMEORIGIN provided by Nginx and the frame-ancestors CSP provided by core Customizer code.

I'm not sure that it makes sense as a change in core, so it may be okay to close this ticket as a config conflict that's best handled on a case by case basis.

FWIW, X-Frame-Options is deprecated and frame-ancestors is a well supported replacement. Once IE11 fades off some more, it may be possible to rely on frame-ancestors alone.

#7 @Oclair
7 years ago

Hey all, so from my understanding using CSP with nginx (an appropriate security measure to undertake) breaks customizer for safari (in my case Safari 11.1). This creates an incentive to make wordpress installs insecure.

Closing this ticket sends the wrong message to everyone.

I am a server admin and am affected by this bug, CSP is very important, please make sure features do not break security.

thanks for your contributions to opensource!
OC

@fullyint
7 years ago

#8 @fullyint
7 years ago

  • Keywords has-patch dev-feedback added; reporter-feedback removed

@westonruter thank you for your huge work on the Customizer.

What do you think about changing ALLOW-FROM to SAMEORIGIN in WP_Customize_Manager::filter_iframe_security_headers()? I added an example patch with the change.

With broader browser support, SAMEORIGIN should offer broader protection. A switch to SAMEORIGIN would also prevent this ticket's original issue where a supported X-Frame-Options value from the server conflicts with an unsupported value from the code (Safari not supporting ALLOW-FROM). The ALLOW-FROM option is probably typically ineffectual anyway as I think modern browsers give priority to the frame-ancestors CSP that the Customizer adds to the embedded content.

As discussed in #39128, the Customizer currently only functions if the embedded content is served from the SAMEORIGIN as the wp-admin/customize.php's siteurl. Thus the current ALLOW-FROM siteurl is functionally the same as the proposed SAMEORIGIN. This remains true when your workaround for #39128 is applied (https://gist.github.com/westonruter/a9c6841ba7f28192e3eb7d90c9316e75).

Following this same reasoning, the proposed patch switches the frame-ancestors siteurl CSP to the simpler frame-ancestors 'self'.

Until the proposed switch to SAMEORIGIN is applied (if approved), sysadmins dealing with Nginx could avoid applying SAMEORIGIN to the embedded Customizer content:

# Avoid SAMEORIGIN conflict with ALLOW-FROM in Safari with WordPress Customizer
# until https://core.trac.wordpress.org/ticket/40020 is resolved

set $x_frame_options SAMEORIGIN;
if ($arg_customize_changeset_uuid) {
  set $x_frame_options "";
}
add_header X-Frame-Options $x_frame_options always;

add_header Content-Security-Policy "frame-ancestors 'self'" always;

Edit: Added always parameter to example Nginx code block.

Last edited 7 years ago by fullyint (previous) (diff)

#9 @westonruter
7 years ago

@fullyint your reasoning seems sound to me. Part of the reason for using ALLOW-FROM was the idea that the iframe could be limited to be embedded from just customize.php. But apparently that's not how ALLOW-FROM works and this granular usage of allowing from specific URL paths isn't supported.

I'd like to get +1 from someone else who is more familiar with the security implications of these headers.

Last edited 7 years ago by westonruter (previous) (diff)

#10 @fullyint
7 years ago

Summary

Currently, if the Customizer's embed gives an ALLOW-FROM header, it must be same domain/origin due to #39128 and any specified path has no effect (discussed below). Thus, switching to SAMEORIGIN loses nothing. It achieves broader browser support (see added green boxes in table below).

The path should probably remain absent from frame-ancestors, due to browsers' inconsistent treatment. If there is ever a day of cross-domain Customizer embeds, frame-ancestors can accept multiple sources.

Path in ALLOW-FROM

It would be appealing to add the path/to/customize.php specificity to ALLOW-FROM but as you suspected, the path appears to be ignored. I find ALLOW-FROM discussed only as some other origin, like an opposite to SAMEORIGIN, never with a path. Also see this note from the X-Frame-Options spec:

The meaning of the term "serialized-origin" is given in RFC6454... Any data beyond the domain address (i.e., any data after the "/" separator) is to be ignored."

Perhaps the path specificity never worked but the perceived loss is limited because ALLOW-FROM would only apply to IE 11 and other older browser versions. Most modern browsers support frame-ancestors.

If a resource has both policies, the frame-ancestors policy SHOULD be enforced and the X-Frame-Options policy SHOULD be ignored.

Path in frame-ancestors

My impression is that browsers' implementation of frame-ancestors has not settled on the treatment of paths. In my tests (see table below), Firefox and Edge seem to ignore the path. So long as the origin is correct, all paths succeed, e.g.,

frame-ancestors https://example.com/nonexistent-path/

On the other hand, it seems that any path, even if correct, causes Chrome and Safari to block the embed.

The fact that Firefox ignores the path in frame-ancestors (embed succeeds) but Chrome evaluates the path in some way (embed fails) may be an example of this discussion at Firefox for how to implement frame-ancestors.

It seems safest for frame-ancestors to avoid paths until cross-browser treatment becomes more consistent.

Tests

I did some informal manual testing related to ALLOW-FROM and frame-ancestors, their interaction, and the effect of adding a path to the source:

IE Edge Firefox Chrome Safari
ALLOW-FROM supported? _ Y _ _ Y _ _ Y _ _ N _ _ N _
SAMEORIGIN supported? _ Y _ _ Y _ _ Y _ _ Y _ _ Y _
frame-ancestors supported? _ N _ _ Y _ _ Y _ _ Y _ _ Y _
frame-ancestors override ALLOW-FROM? n/a _ Y _ _ Y _ _ Y _ _ Y _
ALLOW-FROM honors path? _ N _ _ N _ _ N _ n/a n/a
when frame-ancestors
specifies a source with a path
n/a embed succeeds
(path ignored)
embed succeeds
(path ignored)
embed fails embed fails
browser version tested 11.371.16299.0 41.16299.371.0 65.0.3325.181 59.0.2 11605.1.33.1.3

#11 @westonruter
7 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.0

Amazing research 🎉

@fullyint I'm pretty sure that the unit tests will need to be updated based on this change: https://github.com/WordPress/wordpress-develop/blob/ede824e/tests/phpunit/tests/customize/manager.php#L845-L858

@fullyint
7 years ago

#12 @fullyint
7 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#13 @danlockcuff
6 years ago

@fullyint I have been seeing this with Chrome for a while (running WP 4.9.6). I applied your diff and Chrome was able to view the page again. When will this be added to prod?
Thanks

#14 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

#15 @pento
6 years ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

In 44580:

Customizer: Improve browser compatibility of the preview iframe.

When home and siteurl are different, the customizer preview iframe will be blank in Chrome and Safari, due to their X-Frame-Options implementation quirks.

Changing this to SAMEORIGIN and adding the frame-ancestors Content Security Policy gives the correct behaviour.

Props fullyint.
Fixes #40020.

#16 @pento
6 years ago

In 44582:

Customizer: Fix a coding standards issue introduced in [44580].

See #40020.

This ticket was mentioned in Slack in #core-editor by adamsilverstein. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.