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

Projects
None yet
2 participants
@westonruter
Copy link
Member

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.

westonruter added some commits Aug 29, 2018

Fix handling of CORS requests
* 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

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 30, 2018

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

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 30, 2018

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

@westonruter westonruter requested review from hellofromtonya and amedina and removed request for hellofromtonya Aug 30, 2018

@amedina
Copy link
Member

amedina left a comment

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' ) ) );

This comment has been minimized.

Copy link
@amedina

amedina Aug 30, 2018

Member

Eliminate duplicate

@westonruter westonruter merged commit 64cad60 into develop Aug 30, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/cors-requests branch Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.