Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customizer / Posts: load the Customizer and post previews on sites that disallow iframes #3291

Merged
merged 1 commit into from
Feb 16, 2016

Conversation

mattwiebe
Copy link
Contributor

We have some higher-security sites that always force the x-frame-options: SAMEORIGIN
HTTP header, breaking the Customizer and post previewing. We allow unsetting this header
through a nonce, which is now included in REST API responses for sites.

This may pave the way for increasing the security for everyone by enabling said HTTP
header on all WP.com sites.

To Test

Someone from Automattic will need to try post previews and/or the Customizer with an internal site. You may need to clear out your localStorage to ensure that your site responses are fresh and contain the nonce (site.options.frame_nonce)

@mattwiebe mattwiebe added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Customizer The site customizer for traditional, non-block themes. labels Feb 12, 2016
@ryanboren
Copy link
Contributor


query.return = protocol + '//' + host + this.getPreviousPath();
query.calypso = true;
query.calypsoOrigin = protocol + '//' + host;
if ( site.options.frame_nonce ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need to guard against missing options attribute with site.options && site.options.frame_nonce.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt we get site responses without options, but can't hurt.

@mtias
Copy link
Member

mtias commented Feb 15, 2016

Thanks for tackling this one!

@lancewillett
Copy link
Contributor

Testing this pull, fixes the Customizer iframe issue for me — testing with an a8c private P2 site in local Calypso install on this branch. w00t!

@mtias mtias added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 15, 2016
parsed.query['frame-nonce'] = site.options.frame_nonce;
delete parsed.search;
previewUrl = url.format( parsed );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function desperately needs unit tests, though understandably non-existent at the moment. I might suggest that you consider adding one to the existing utils test suite:

describe( '#getPreviewURL', function() {
    it( 'should include a frame nonce when post has site defined', function() {
        // ...
    } );
} );

Not a blocker, but we should be more diligent about testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as it pains me to say, I'm gonna punt on that, since fixing everything here has already turned into a giant interdimensional rabbit hole.

We have some higher-security sites that always force the `x-frame-options: SAMEORIGIN`
HTTP header, breaking the Customizer and post previewing. We allow unsetting this header
through a nonce, which is now included in REST API responses for sites.

This may pave the way for increasing the security for everyone by enabling said HTTP
header on all WP.com sites.
@mattwiebe mattwiebe added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Feb 16, 2016
@mtias
Copy link
Member

mtias commented Feb 16, 2016

👍

@mtias mtias added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 16, 2016
mattwiebe added a commit that referenced this pull request Feb 16, 2016
Customizer / Posts: load the Customizer and post previews on sites that disallow iframes
@mattwiebe mattwiebe merged commit 247e012 into master Feb 16, 2016
@mattwiebe mattwiebe deleted the fix/secure-iframe-embeds branch February 16, 2016 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Customizer The site customizer for traditional, non-block themes. [Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants