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

Fix handling responses to form submissions from an AMP Cache #1382

Merged
merged 3 commits into from
Aug 30, 2018

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 30, 2018

It turns out that submitting a form from a page on an AMP cache is currently failing with the plugin. An error

Failed to load https://example.com/contact/?_wp_amp_action_xhr_converted=1&__amp_source_origin=https%3A%2F%2Fexample.com#contact-form-6: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'https://example-com.cdn.ampproject.org' is therefore not allowed access. The response had HTTP status code 403. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.
Form submission failed: Error: XHR Failed fetching (https://example.com/...): Failed to fetch​​​

The reason is that the plugin was not sending back the proper CORS response headers as detailed in the CORS in AMP docs. In particular, the Access-Control-Allow-Origin header was not being sent, and the Origin checking was not taking into the AMP Cache domains.

In this PR:

  • Refactor code for handling HTTP requests and responses by extracting code from AMP_Theme_Support and the entirety of AMP_Response_Headers to form a new class AMP_HTTP.
  • Add the AMP cache origins to the allowed_redirect_hosts so that wp_validate_redirect() will allow such origins.
  • Send CORS response headers and handle AMP XHR requests even in Classic mode.
  • Add Access-Control-Allow-Credentials:true and Vary:origin response headers.
  • Prevent overriding any previously-sent Access-Control-Expose-Headers response headers.

To test this, you need to have the code deployed publicly and then access your site on the AMP cache, specifically a page that has a contact form on it. For an easy way to determine the AMP Cache URL for any given URL, see https://ampbyexample.com/advanced/using_the_google_amp_cache/#amp-cache-url-format

Fixes #1356.

* Include AMP caches among the allowed redirect hosts.
* Send CORS headers and handle XHR requests in classic mode in addition to native/paired mode.
* Add Access-Control-Allow-Credentials:true and Vary:origin headers
@westonruter westonruter added this to the v1.0 milestone Aug 30, 2018
@westonruter
Copy link
Member Author

For reviewing, you'll want to look at the commit after the first (i.e. 1870954), since the first commit is just the code re-org.

@westonruter
Copy link
Member Author

For testing: amp.zip (v1.0-beta2-18709544-20180830T041312Z)

Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

This is awesome. Minor fix required. Then ship it!

// Ensure that purge_amp_query_vars() didn't execute.
$this->assertTrue( isset( $_REQUEST['__amp_source_origin'] ) ); // WPCS: CSRF ok.
$this->assertFalse( has_action( 'widgets_init', array( self::TESTED_CLASS, 'register_widgets' ) ) );
$this->assertFalse( has_action( 'widgets_init', array( self::TESTED_CLASS, 'register_widgets' ) ) );
Copy link
Member

Choose a reason for hiding this comment

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

Eliminate duplicate

@westonruter westonruter merged commit 64cad60 into develop Aug 30, 2018
@westonruter westonruter deleted the fix/cors-requests branch August 30, 2018 23:00
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.

2 participants