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

Add CSP frame-ancestors, make X-Frame-Options conditional #977

Merged
merged 4 commits into from
May 19, 2018

Conversation

fullyint
Copy link
Contributor

This PR adds a workaround to https://discourse.roots.io/t/safari-conflicting-multiple-x-frame-options/10077 until https://core.trac.wordpress.org/ticket/40020 is resolved.

frame-ancestors CSP

This PR adds the frame-ancestors CSP directive which has obsoleted the X-Frame-Options header.

Note that it is acceptable to have multiple CSPs (i.e., in case users add other CSP headers).

CSP is designed to be fully backward compatible... Browsers that don't support it still work with servers that implement it, and vice-versa: browsers that don't support CSP simply ignore it, functioning as usual, defaulting to the standard same-origin policy for web content. --reference

Dynamic X-Frame-Options

This PR retains the X-Frame-Options header for the sake of older browsers, but returns an empty X-Frame-Options value for WordPress Customizer content. This prevents the Safari browser's conflict between SAMEORIGIN and the ALLOW-FROM option that WordPress adds on its own.

@swalkinshaw
Copy link
Member

Where is arg_customize_changeset_uuid from? Does that come from the customizer?

This looks good, but I think we should just move this out of the h5bp directory now and make it ours. If we customize these templates, they belong to us :)

@fullyint
Copy link
Contributor Author

arg_customize_changeset_uuid

Where is arg_customize_changeset_uuid from? Does that come from the customizer?

Yep, from the Customizer's embedded content:

<iframe src="https://example.com/?customize_changeset_uuid=8c0f9f64-2daf..."></iframe>

Nginx supports variables such as $arg_name for the "argument name in the request line." Nginx allows checking the presence of query parameters with if ($arg_name) {}.

I figured the presence of that query parameter was the most convenient indicator of whether a request was for the Customizer's embedded content. Although a malicious embed could spoof this strategy by adding a fake customize_changeset_uuid query parameter, it should fail.

  • An attempt to embed a real customize_changeset_uuid on some other domain should fail because WP code adds X-Frame-Options and frame-ancestors headers for embedded content. Also, embed content will fail with non existing changeset UUID unless the embed and parent ORIGIN are the same, and the user is logged in to wp-admin. Discussion at trac-39128.
  • An attempt to use a fake customize_changeset_uuid query parameter outside the Customizer context just to fool this PR's proposed conditional X-Frame-Options will fail for most modern browsers because this PR's added Content-Security-Policy: frame-ancestors 'self' usually overrides X-Frame-Options anyway.

Move from h5bp to Trellis core

I think we should just move this out of the h5bp directory now and make it ours.

I added a commit commenting out the X-Frame-Options in the h5bp file to prevent duplication, then adding this PR's revised headers to a new {% block embed_security -%} in wordpress-site.conf.j2. The headers restricting embeds are enabled by default but users could define nginx_embed_security: false globally in a group_vars file or per site in wordpress_sites, the latter location taking precedence.

My choice of wordpress-site.conf.j2 as the home for these headers, vs. a new include file, was in spirit of "Fewer templates/includes, but still DRY," a section in #740's comments. I chose against trying to add the headers as a new Trellis core file in includes.d because I suspect users see nginx-includes and includes.d as exact parallels, and the user's province only. I also didn't want to figure out coordination with the includes.d cleanup feature.

If Trellis begins to have or desire multiple Trellis core Nginx include files, perhaps the Copy h5bp configs task could become Copy Nginx configs and use with_items: [h5bp, trellis] where h5bp is the same directory as currently and trellis is a new directory with Trellis core confs such as embed-security-headers.conf. In such a case of non-templated files (copied only), these directories could be in a roles/nginx/files directory instead of roles/nginx/templates where h5bp is currently.

The X-Frame-Options header has been obsoleted by the frame-ancestors
directive. Retain the X-Frame-Options header for older browsers.

Return empty X-Frame-Options header for WordPress Customizer content
to prevent the conflict that SAMEORIGIN would have with the ALLOW-FROM
option that WordPress adds on its own (Safari browser).
Discussion in https://core.trac.wordpress.org/ticket/40020
@fullyint fullyint merged commit e3315fe into master May 19, 2018
@fullyint fullyint deleted the framing-headers branch May 19, 2018 23:31
greatislander pushed a commit to pressbooks/trellis that referenced this pull request Jun 7, 2018
* Add xdebug.remote_autostart to simplify xdebug sessions

* Update logrotate doc URL [ci skip]

* Update WP-CLI to 1.5.1.

* Update changelog. [ci skip]

* Update geerlingguy.composer 1.6.1->1.7.0 (roots#983)

Update from `1.6.1` -> `1.7.0` which addresses roots#943 ([DEPRECATION WARNING]: The use of 'include' for tasks has been deprecated.)

* Update geerlingguy.ntp 1.5.2->1.6.0 (roots#984)

Avoids deprecation warnings introduced in Ansible 2.4:
"The use of 'include' for tasks has been deprecated."

* Enable nginx to start on boot (roots#980)

* update changelog

* 'yarn run' -> 'yarn' [ci skip]

* Issue warning for all Ubuntu releases that are not Xenial (roots#986)

* Clarify that changelog entry indicates Trellis version (roots#987)

* Validate python version on control machine (roots#988)

* Common: Install `git` instead of `git-core`

Because `git-core` is now a dummy package of `git`.

See: http://git.661346.n2.nabble.com/git-core-vs-git-package-on-ubuntu-tp7576083p7576085.html

* Add CSP frame-ancestors, make X-Frame-Options conditional (roots#977)

The X-Frame-Options header has been obsoleted by the frame-ancestors
directive. Retain the X-Frame-Options header for older browsers.

Return empty X-Frame-Options header for WordPress Customizer content
to prevent the conflict that SAMEORIGIN would have with the ALLOW-FROM
option that WordPress adds on its own (Safari browser).
Discussion in https://core.trac.wordpress.org/ticket/40020

* Improve failed_when rule for Wordpress Installed check (roots#991)

In rare cases the wp_installed registered var may be missing the
stderr attribute, so add a default to avoid related error.

The `wp core  is-installed` command return code is 1 if WP is simply
not installed. However, in rare cases the command may return some
other return code indicative of true failure, so fail if rc > 1.

* deploy.sh: Return non-zero exit code when misuse (roots#990)

- Exit with `127` when not enough arguments
- Exit with `1` when hosts file not exist

See: http://www.tldp.org/LDP/abs/html/exitcodes.html

* Skip Acme Challenge failure message for non-failed sites (roots#993)

* Bump Ansible version_tested_max to 2.5.3 (roots#981)

* Bump Ansible version_tested_max to 2.5.3

Convert Jinja2 tests from filter format to `var is testname` format.
Encourage users on Ansible 2.5.0 to upgrade to avoid erroneous warnings
fixed in ansible/ansible 37538

* Add option to enable FastCGI background updates (roots#962)

Enabled by default

* Add quotes to nginx_cache_background_update value "on"

Quotes prevent Ansible from interpolating the variable value as True.
True is an invalid value for fastcgi_cache_background_update and would
would make Nginx unable to reload.

* Verify `wp-cli.phar` checksum
@strarsis
Copy link
Contributor

@fullyint: Can this config block/option key nginx_embed_security also
be used for allowing to embed a specific WordPress site?
For example, the site example.com should be embeddable by others,
specifically everything that ends with ?embed=1 in the URL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants