From c229cfccb49dd93ac050e27017c7d153e98ab618 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Thu, 1 Feb 2018 08:04:54 +0200 Subject: [PATCH 01/22] handle general form requests around comment post logic --- amp.php | 5 +--- includes/amp-helper-functions.php | 41 +++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/amp.php b/amp.php index b837d2a6efa..9ce15914401 100644 --- a/amp.php +++ b/amp.php @@ -106,7 +106,6 @@ function amp_after_setup_theme() { * @global string $pagenow */ function amp_init() { - global $pagenow; /** * Triggers on init when AMP plugin is active. @@ -128,9 +127,7 @@ function amp_init() { if ( class_exists( 'Jetpack' ) && ! ( defined( 'IS_WPCOM' ) && IS_WPCOM ) ) { require_once AMP__DIR__ . '/jetpack-helper.php'; } - if ( isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) { - amp_prepare_comment_post(); - } + amp_prepare_post(); } // Make sure the `amp` query var has an explicit value. diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 505a4d1d03e..9b9d6e474cd 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -238,22 +238,27 @@ function amp_get_content_sanitizers( $post = null ) { } /** - * Hook into a comment submission of an AMP XHR post request. - * - * This only runs on wp-comments-post.php. + * Hook into a submission of an AMP XHR post request. * * @since 0.7.0 */ -function amp_prepare_comment_post() { - if ( ! isset( $_GET['__amp_source_origin'] ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars(). +function amp_prepare_post() { + global $pagenow; + if ( ! isset( $_GET['__amp_source_origin'] ) || ! isset( $pagenow ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars(). return; } - // Add amp comment hooks. - add_filter( 'comment_post_redirect', function() { - // We don't need any data, so just send a success. - wp_send_json_success(); - }, PHP_INT_MAX, 2 ); + if ( 'wp-comments-post.php' === $pagenow ) { + // This only runs on wp-comments-post.php. + add_filter( 'comment_post_redirect', function() { + // We don't need any data, so just send a success. + wp_send_json_success(); + }, PHP_INT_MAX, 2 ); + + } else { + // Add amp comment hooks. + add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); + } // Add die handler for AMP error display. add_filter( 'wp_die_handler', function() { @@ -275,4 +280,20 @@ function amp_prepare_comment_post() { // Send AMP header. $origin = esc_url_raw( wp_unslash( $_GET['__amp_source_origin'] ) ); // WPCS: CSRF ok. header( 'AMP-Access-Control-Allow-Source-Origin: ' . $origin, true ); + +} + +/** + * Handle a general, non comment AMP XHR post. + * + * @since 0.7.0 + * @param string $location The location to redirect to. + */ +function amp_handle_general_post( $location ) { + + $url = site_url( $location ); + header( 'AMP-Redirect-To: ' . $url ); + header( 'Access-Control-Expose-Headers: AMP-Redirect-To;' ); + // Send json success. + wp_send_json_success(); } From 96c0bacb655c013643c77f91716c713679a9d1b1 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Thu, 1 Feb 2018 08:47:46 +0200 Subject: [PATCH 02/22] record that action was converted to action-xhr and only handle converted submits. --- includes/amp-helper-functions.php | 10 +++++----- includes/sanitizers/class-amp-form-sanitizer.php | 2 ++ tests/test-amp-form-sanitizer.php | 4 ++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 3a01c329d76..9355fd26c3e 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -418,11 +418,12 @@ function amp_prepare_post() { // We don't need any data, so just send a success. wp_send_json_success(); }, PHP_INT_MAX, 2 ); - - } else { - // Add amp comment hooks. - add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); + } elseif ( ! isset( $_GET['_wp_amp_action_xhr_converted'] ) ) { // WPCS: CSRF ok. + // submission was from a set action-xhr, implying it's being handled already. + return; } + // Add amp comment hooks. + add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); // Add die handler for AMP error display. add_filter( 'wp_die_handler', function() { @@ -444,7 +445,6 @@ function amp_prepare_post() { // Send AMP header. $origin = esc_url_raw( wp_unslash( $_GET['__amp_source_origin'] ) ); // WPCS: CSRF ok. header( 'AMP-Access-Control-Allow-Source-Origin: ' . $origin, true ); - } /** diff --git a/includes/sanitizers/class-amp-form-sanitizer.php b/includes/sanitizers/class-amp-form-sanitizer.php index 2240906fa13..aba92e97c32 100644 --- a/includes/sanitizers/class-amp-form-sanitizer.php +++ b/includes/sanitizers/class-amp-form-sanitizer.php @@ -87,6 +87,8 @@ public function sanitize() { } elseif ( 'post' === $method ) { $node->removeAttribute( 'action' ); if ( ! $xhr_action ) { + // record that action was converted tp action-xhr. + $action_url = add_query_arg( '_wp_amp_action_xhr_converted', 1, $action_url ); $node->setAttribute( 'action-xhr', $action_url ); } elseif ( 'http://' === substr( $xhr_action, 0, 7 ) ) { $node->setAttribute( 'action-xhr', substr( $xhr_action, 5 ) ); diff --git a/tests/test-amp-form-sanitizer.php b/tests/test-amp-form-sanitizer.php index c0674d2ea6e..6f80abd7d0b 100644 --- a/tests/test-amp-form-sanitizer.php +++ b/tests/test-amp-form-sanitizer.php @@ -46,7 +46,7 @@ public function get_data() { ), 'form_with_post_method_http_action_and_no_target' => array( '
', - '
', + '
', ), 'form_with_post_method_http_action_and_blank_target' => array( '
', @@ -58,7 +58,7 @@ public function get_data() { ), 'form_with_post_method_https_action_and_custom_target' => array( '
', - '
', + '
', ), ); } From 024d4e0886b43be802fbf1141f9528e154102b0b Mon Sep 17 00:00:00 2001 From: David Cramer Date: Thu, 1 Feb 2018 08:54:08 +0200 Subject: [PATCH 03/22] rename function to be less ambiguous --- amp.php | 2 +- includes/amp-helper-functions.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/amp.php b/amp.php index 9ce15914401..9daa2d75c25 100644 --- a/amp.php +++ b/amp.php @@ -127,7 +127,7 @@ function amp_init() { if ( class_exists( 'Jetpack' ) && ! ( defined( 'IS_WPCOM' ) && IS_WPCOM ) ) { require_once AMP__DIR__ . '/jetpack-helper.php'; } - amp_prepare_post(); + amp_prepare_xhr_post(); } // Make sure the `amp` query var has an explicit value. diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 9355fd26c3e..2d35fa263b4 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -406,7 +406,7 @@ function amp_print_schemaorg_metadata() { * * @since 0.7.0 */ -function amp_prepare_post() { +function amp_prepare_xhr_post() { global $pagenow; if ( ! isset( $_GET['__amp_source_origin'] ) || ! isset( $pagenow ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars(). return; From 967a8c64a0620faf71e11c3e20fb95db6445a89d Mon Sep 17 00:00:00 2001 From: David Cramer Date: Thu, 1 Feb 2018 08:55:31 +0200 Subject: [PATCH 04/22] dont add wp_redirect filter on comment-handler --- includes/amp-helper-functions.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 2d35fa263b4..1630a62b3b3 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -421,10 +421,10 @@ function amp_prepare_xhr_post() { } elseif ( ! isset( $_GET['_wp_amp_action_xhr_converted'] ) ) { // WPCS: CSRF ok. // submission was from a set action-xhr, implying it's being handled already. return; + } else { + // Add amp comment hooks. + add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); } - // Add amp comment hooks. - add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); - // Add die handler for AMP error display. add_filter( 'wp_die_handler', function() { /** From 4b259df89e5621db30c45b26bd5914abb25b3b83 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Thu, 1 Feb 2018 10:12:10 +0200 Subject: [PATCH 05/22] add appending error template for converted forms --- .../sanitizers/class-amp-form-sanitizer.php | 41 +++++++++++++++++++ tests/test-amp-form-sanitizer.php | 4 +- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-form-sanitizer.php b/includes/sanitizers/class-amp-form-sanitizer.php index aba92e97c32..0e118a2f184 100644 --- a/includes/sanitizers/class-amp-form-sanitizer.php +++ b/includes/sanitizers/class-amp-form-sanitizer.php @@ -90,6 +90,8 @@ public function sanitize() { // record that action was converted tp action-xhr. $action_url = add_query_arg( '_wp_amp_action_xhr_converted', 1, $action_url ); $node->setAttribute( 'action-xhr', $action_url ); + // Append error handler if not found. + $this->error_handler( $node ); } elseif ( 'http://' === substr( $xhr_action, 0, 7 ) ) { $node->setAttribute( 'action-xhr', substr( $xhr_action, 5 ) ); } @@ -110,4 +112,43 @@ public function sanitize() { } } } + + /** + * Checks if the form has an error handler else create one if not. + * + * @link https://www.ampproject.org/docs/reference/components/amp-form + * @since 0.7 + * @param DOMElement $node The form node to check. + */ + public function error_handler( $node ) { + $templates = $node->getElementsByTagName( 'template' ); + if ( $templates->length ) { + for ( $i = $templates->length - 1; $i >= 0; $i-- ) { + if ( $templates->item( $i )->parentNode->hasAttribute( 'submit-error' ) ) { + return; // Found error template, do nothing. + } + } + } + $node->appendChild( $this->create_error_template() ); + } + + /** + * Creates a error handler element node. + * + * @link https://www.ampproject.org/docs/reference/components/amp-form + * @since 0.7 + * + * return DOMElement + */ + public function create_error_template() { + $node = $this->dom->createElement( 'div' ); + $template = $this->dom->createElement( 'template' ); + $mustache = $this->dom->createTextNode( '{{{error}}}' ); + $node->setAttribute( 'submit-error', '' ); + $template->setAttribute( 'type', 'amp-mustache' ); + $template->appendChild( $mustache ); + $node->appendChild( $template ); + + return $node; + } } diff --git a/tests/test-amp-form-sanitizer.php b/tests/test-amp-form-sanitizer.php index 6f80abd7d0b..071931eeaee 100644 --- a/tests/test-amp-form-sanitizer.php +++ b/tests/test-amp-form-sanitizer.php @@ -46,7 +46,7 @@ public function get_data() { ), 'form_with_post_method_http_action_and_no_target' => array( '
', - '
', + '
', ), 'form_with_post_method_http_action_and_blank_target' => array( '
', @@ -58,7 +58,7 @@ public function get_data() { ), 'form_with_post_method_https_action_and_custom_target' => array( '
', - '
', + '
', ), ); } From ac86bf2c72effb4489b948a28344f4181931052d Mon Sep 17 00:00:00 2001 From: David Cramer Date: Thu, 1 Feb 2018 10:13:16 +0200 Subject: [PATCH 06/22] redirect page if redirect-template hits --- includes/amp-helper-functions.php | 8 ++++++++ includes/class-amp-theme-support.php | 3 ++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 1630a62b3b3..0e90b4b1796 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -424,6 +424,14 @@ function amp_prepare_xhr_post() { } else { // Add amp comment hooks. add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); + add_action( 'template_redirect', function() { + /* + * Buffering starts here, so unlikely the form has a redirect, + * so force a redirect to the same page. + */ + $location = esc_url_raw( wp_unslash( $_SERVER['REQUEST_URI'] ) ); // WPCS: CSRF ok, input var ok. + amp_handle_general_post( $location ); + }, 0 ); } // Add die handler for AMP error display. add_filter( 'wp_die_handler', function() { diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 92c3646f3d7..407401a951f 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -102,7 +102,7 @@ public static function init() { self::register_paired_hooks(); } - self::purge_amp_query_vars(); // Note that amp_prepare_comment_post() still looks at $_GET['__amp_source_origin']. + self::purge_amp_query_vars(); // Note that amp_prepare_xhr_post() still looks at $_GET['__amp_source_origin']. self::register_hooks(); self::$embed_handlers = self::register_content_embed_handlers(); self::$sanitizer_classes = amp_get_content_sanitizers(); @@ -214,6 +214,7 @@ public static function register_hooks() { public static function purge_amp_query_vars() { $query_vars = array( '__amp_source_origin', + '_wp_amp_action_xhr_converted', 'amp_latest_update_time', ); From eca4789491b2c7cc2b887efb02d0d8c3905b6398 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Thu, 1 Feb 2018 10:17:46 +0200 Subject: [PATCH 07/22] update docs --- includes/amp-helper-functions.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 0e90b4b1796..6071fd4bcba 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -422,7 +422,7 @@ function amp_prepare_xhr_post() { // submission was from a set action-xhr, implying it's being handled already. return; } else { - // Add amp comment hooks. + // Add amp redirect hooks. add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); add_action( 'template_redirect', function() { /* @@ -466,6 +466,6 @@ function amp_handle_general_post( $location ) { $url = site_url( $location ); header( 'AMP-Redirect-To: ' . $url ); header( 'Access-Control-Expose-Headers: AMP-Redirect-To;' ); - // Send json success. + // Send json success as no data is required. wp_send_json_success(); } From 9c965ad9ac0482dfc2ba6a792415417fffc43de2 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 2 Feb 2018 07:05:00 +0200 Subject: [PATCH 08/22] rename handler function --- amp.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/amp.php b/amp.php index 9daa2d75c25..0acc2bfa9c9 100644 --- a/amp.php +++ b/amp.php @@ -127,7 +127,7 @@ function amp_init() { if ( class_exists( 'Jetpack' ) && ! ( defined( 'IS_WPCOM' ) && IS_WPCOM ) ) { require_once AMP__DIR__ . '/jetpack-helper.php'; } - amp_prepare_xhr_post(); + amp_handle_xhr_request(); } // Make sure the `amp` query var has an explicit value. From 83a0799235a36876a0dbc4369b0fedfe8435845e Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 2 Feb 2018 07:10:24 +0200 Subject: [PATCH 09/22] simplify handler function --- includes/amp-helper-functions.php | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 6071fd4bcba..9fd42acbaf2 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -406,7 +406,7 @@ function amp_print_schemaorg_metadata() { * * @since 0.7.0 */ -function amp_prepare_xhr_post() { +function amp_handle_xhr_request() { global $pagenow; if ( ! isset( $_GET['__amp_source_origin'] ) || ! isset( $pagenow ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars(). return; @@ -418,10 +418,8 @@ function amp_prepare_xhr_post() { // We don't need any data, so just send a success. wp_send_json_success(); }, PHP_INT_MAX, 2 ); - } elseif ( ! isset( $_GET['_wp_amp_action_xhr_converted'] ) ) { // WPCS: CSRF ok. - // submission was from a set action-xhr, implying it's being handled already. - return; - } else { + amp_handle_xhr_headers_output(); + } elseif ( isset( $_GET['_wp_amp_action_xhr_converted'] ) ) { // WPCS: CSRF ok. // Add amp redirect hooks. add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); add_action( 'template_redirect', function() { @@ -432,7 +430,17 @@ function amp_prepare_xhr_post() { $location = esc_url_raw( wp_unslash( $_SERVER['REQUEST_URI'] ) ); // WPCS: CSRF ok, input var ok. amp_handle_general_post( $location ); }, 0 ); + amp_handle_xhr_headers_output(); } + +} + +/** + * Handle the AMP XHR headers and output errors. + * + * @since 0.7.0 + */ +function amp_handle_xhr_headers_output() { // Add die handler for AMP error display. add_filter( 'wp_die_handler', function() { /** From 09de4203fb46ac92494af820c16853225ff59886 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 2 Feb 2018 07:19:54 +0200 Subject: [PATCH 10/22] correct phpdoc return tag --- includes/sanitizers/class-amp-form-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-form-sanitizer.php b/includes/sanitizers/class-amp-form-sanitizer.php index 0e118a2f184..7937d6f16fc 100644 --- a/includes/sanitizers/class-amp-form-sanitizer.php +++ b/includes/sanitizers/class-amp-form-sanitizer.php @@ -138,7 +138,7 @@ public function error_handler( $node ) { * @link https://www.ampproject.org/docs/reference/components/amp-form * @since 0.7 * - * return DOMElement + * @return DOMElement The div[submit-error] element. */ public function create_error_template() { $node = $this->dom->createElement( 'div' ); From 40934c1cbbb54aa3ed0c0e4c212bb3512e69592b Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 2 Feb 2018 07:21:44 +0200 Subject: [PATCH 11/22] update @link with correct url --- includes/sanitizers/class-amp-form-sanitizer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-form-sanitizer.php b/includes/sanitizers/class-amp-form-sanitizer.php index 7937d6f16fc..c3cdfe437b1 100644 --- a/includes/sanitizers/class-amp-form-sanitizer.php +++ b/includes/sanitizers/class-amp-form-sanitizer.php @@ -116,7 +116,7 @@ public function sanitize() { /** * Checks if the form has an error handler else create one if not. * - * @link https://www.ampproject.org/docs/reference/components/amp-form + * @link https://www.ampproject.org/docs/reference/components/amp-form#success/error-response-rendering * @since 0.7 * @param DOMElement $node The form node to check. */ @@ -135,7 +135,7 @@ public function error_handler( $node ) { /** * Creates a error handler element node. * - * @link https://www.ampproject.org/docs/reference/components/amp-form + * @link https://www.ampproject.org/docs/reference/components/amp-form#success/error-response-rendering * @since 0.7 * * @return DOMElement The div[submit-error] element. From 6dd4e9d1e37056d202bad75e68a9568c46d46a6f Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 2 Feb 2018 07:23:29 +0200 Subject: [PATCH 12/22] rename redirect function --- includes/amp-helper-functions.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 9fd42acbaf2..34ea9e2daac 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -421,7 +421,7 @@ function amp_handle_xhr_request() { amp_handle_xhr_headers_output(); } elseif ( isset( $_GET['_wp_amp_action_xhr_converted'] ) ) { // WPCS: CSRF ok. // Add amp redirect hooks. - add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 ); + add_filter( 'wp_redirect', 'amp_intercept_post_request_redirect', PHP_INT_MAX, 2 ); add_action( 'template_redirect', function() { /* * Buffering starts here, so unlikely the form has a redirect, @@ -469,7 +469,7 @@ function amp_handle_xhr_headers_output() { * @since 0.7.0 * @param string $location The location to redirect to. */ -function amp_handle_general_post( $location ) { +function amp_intercept_post_request_redirect( $location ) { $url = site_url( $location ); header( 'AMP-Redirect-To: ' . $url ); From a9d9bfdad23693eef28001e6b975f2df9cdf2ea6 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 2 Feb 2018 07:26:59 +0200 Subject: [PATCH 13/22] catch data for redirection and reload inclusion hackery --- includes/amp-helper-functions.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 34ea9e2daac..93109809304 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -408,6 +408,14 @@ function amp_print_schemaorg_metadata() { */ function amp_handle_xhr_request() { global $pagenow; + if ( isset( $_GET['__amp_redirect'] ) ) { // WPCS: CSRF ok. + add_action( 'template_redirect', function() { + // grab post data. + $transint_name = wp_unslash( $_GET['__amp_redirect'] ); // WPCS: CSRF ok, input var ok. + $_POST = get_transient( $transint_name ); + delete_transient( $transint_name ); + }, 0 ); + } if ( ! isset( $_GET['__amp_source_origin'] ) || ! isset( $pagenow ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars(). return; } @@ -423,12 +431,17 @@ function amp_handle_xhr_request() { // Add amp redirect hooks. add_filter( 'wp_redirect', 'amp_intercept_post_request_redirect', PHP_INT_MAX, 2 ); add_action( 'template_redirect', function() { + // grab post data. + $transient_name = uniqid(); + set_transient( $transient_name, wp_unslash( $_POST ), 60 ); // WPCS: CSRF ok, input var ok. + /* * Buffering starts here, so unlikely the form has a redirect, * so force a redirect to the same page. */ $location = esc_url_raw( wp_unslash( $_SERVER['REQUEST_URI'] ) ); // WPCS: CSRF ok, input var ok. - amp_handle_general_post( $location ); + $location = add_query_arg( '__amp_redirect', $transient_name, $location ); + amp_intercept_post_request_redirect( $location ); }, 0 ); amp_handle_xhr_headers_output(); } From 17ab3fd5ff7ce3fad95d863c58e07a259cc6ba7e Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 2 Feb 2018 07:36:53 +0200 Subject: [PATCH 14/22] check redirect location has a host first --- includes/amp-helper-functions.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 93109809304..1f8cdb911a3 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -483,9 +483,11 @@ function amp_handle_xhr_headers_output() { * @param string $location The location to redirect to. */ function amp_intercept_post_request_redirect( $location ) { - - $url = site_url( $location ); - header( 'AMP-Redirect-To: ' . $url ); + $host = wp_parse_url( $location, PHP_URL_HOST ); + if ( is_null( $host ) ) { + $location = home_url( $location ); + } + header( 'AMP-Redirect-To: ' . $location ); header( 'Access-Control-Expose-Headers: AMP-Redirect-To;' ); // Send json success as no data is required. wp_send_json_success(); From 92f3a761236553ef09584c12d81bb3d1a68f3029 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 2 Feb 2018 14:26:42 -0800 Subject: [PATCH 15/22] Allow all formatting tags to be returned in submit error response --- includes/amp-helper-functions.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 1f8cdb911a3..bf25bd7ec4d 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -466,8 +466,10 @@ function amp_handle_xhr_headers_output() { if ( is_wp_error( $error ) ) { $error = $error->get_error_message(); } - $error = strip_tags( $error, 'strong' ); - wp_send_json( compact( 'error' ) ); + $amp_mustache_allowed_html_tags = array( 'strong', 'b', 'em', 'i', 'u', 's', 'small', 'mark', 'del', 'ins', 'sup', 'sub' ); + wp_send_json( array( + 'error' => wp_kses( $error, array_fill_keys( $amp_mustache_allowed_html_tags, array() ) ), + ) ); }; } ); From 1593f882256d6d835db9a0e73ec9d0128f297a8c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 2 Feb 2018 14:33:01 -0800 Subject: [PATCH 16/22] Remove support for handling non-redirecting post requests, for now --- includes/amp-helper-functions.php | 31 ++-------------- .../sanitizers/class-amp-form-sanitizer.php | 37 ++++++------------- 2 files changed, 16 insertions(+), 52 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index bf25bd7ec4d..fc09db8580d 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -400,27 +400,18 @@ function amp_print_schemaorg_metadata() { } /** - * Hook into a comment submission of an AMP XHR post request. - * - * This only runs on wp-comments-post.php. + * Hook into a form submissions, such as comment the form or some other . * * @since 0.7.0 + * @global string $pagenow */ function amp_handle_xhr_request() { global $pagenow; - if ( isset( $_GET['__amp_redirect'] ) ) { // WPCS: CSRF ok. - add_action( 'template_redirect', function() { - // grab post data. - $transint_name = wp_unslash( $_GET['__amp_redirect'] ); // WPCS: CSRF ok, input var ok. - $_POST = get_transient( $transint_name ); - delete_transient( $transint_name ); - }, 0 ); - } - if ( ! isset( $_GET['__amp_source_origin'] ) || ! isset( $pagenow ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars(). + if ( ! isset( $_GET['__amp_source_origin'] ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars(). return; } - if ( 'wp-comments-post.php' === $pagenow ) { + if ( isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) { // This only runs on wp-comments-post.php. add_filter( 'comment_post_redirect', function() { // We don't need any data, so just send a success. @@ -428,21 +419,7 @@ function amp_handle_xhr_request() { }, PHP_INT_MAX, 2 ); amp_handle_xhr_headers_output(); } elseif ( isset( $_GET['_wp_amp_action_xhr_converted'] ) ) { // WPCS: CSRF ok. - // Add amp redirect hooks. add_filter( 'wp_redirect', 'amp_intercept_post_request_redirect', PHP_INT_MAX, 2 ); - add_action( 'template_redirect', function() { - // grab post data. - $transient_name = uniqid(); - set_transient( $transient_name, wp_unslash( $_POST ), 60 ); // WPCS: CSRF ok, input var ok. - - /* - * Buffering starts here, so unlikely the form has a redirect, - * so force a redirect to the same page. - */ - $location = esc_url_raw( wp_unslash( $_SERVER['REQUEST_URI'] ) ); // WPCS: CSRF ok, input var ok. - $location = add_query_arg( '__amp_redirect', $transient_name, $location ); - amp_intercept_post_request_redirect( $location ); - }, 0 ); amp_handle_xhr_headers_output(); } diff --git a/includes/sanitizers/class-amp-form-sanitizer.php b/includes/sanitizers/class-amp-form-sanitizer.php index c3cdfe437b1..05c6252f0cc 100644 --- a/includes/sanitizers/class-amp-form-sanitizer.php +++ b/includes/sanitizers/class-amp-form-sanitizer.php @@ -91,7 +91,7 @@ public function sanitize() { $action_url = add_query_arg( '_wp_amp_action_xhr_converted', 1, $action_url ); $node->setAttribute( 'action-xhr', $action_url ); // Append error handler if not found. - $this->error_handler( $node ); + $this->ensure_submit_error_element( $node ); } elseif ( 'http://' === substr( $xhr_action, 0, 7 ) ) { $node->setAttribute( 'action-xhr', substr( $xhr_action, 5 ) ); } @@ -118,37 +118,24 @@ public function sanitize() { * * @link https://www.ampproject.org/docs/reference/components/amp-form#success/error-response-rendering * @since 0.7 - * @param DOMElement $node The form node to check. + * + * @param DOMElement $form The form node to check. */ - public function error_handler( $node ) { - $templates = $node->getElementsByTagName( 'template' ); - if ( $templates->length ) { - for ( $i = $templates->length - 1; $i >= 0; $i-- ) { - if ( $templates->item( $i )->parentNode->hasAttribute( 'submit-error' ) ) { - return; // Found error template, do nothing. - } + public function ensure_submit_error_element( $form ) { + $templates = $form->getElementsByTagName( 'template' ); + for ( $i = $templates->length - 1; $i >= 0; $i-- ) { + if ( $templates->item( $i )->parentNode->hasAttribute( 'submit-error' ) ) { + return; // Found error template, do nothing. } } - $node->appendChild( $this->create_error_template() ); - } - /** - * Creates a error handler element node. - * - * @link https://www.ampproject.org/docs/reference/components/amp-form#success/error-response-rendering - * @since 0.7 - * - * @return DOMElement The div[submit-error] element. - */ - public function create_error_template() { - $node = $this->dom->createElement( 'div' ); + $div = $this->dom->createElement( 'div' ); $template = $this->dom->createElement( 'template' ); $mustache = $this->dom->createTextNode( '{{{error}}}' ); - $node->setAttribute( 'submit-error', '' ); + $div->setAttribute( 'submit-error', '' ); $template->setAttribute( 'type', 'amp-mustache' ); $template->appendChild( $mustache ); - $node->appendChild( $template ); - - return $node; + $div->appendChild( $template ); + $form->appendChild( $div ); } } From 3791cd8b54a0a7851a4b002e56773c490f2ed68d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 2 Feb 2018 15:01:57 -0800 Subject: [PATCH 17/22] Ensure relative redirect URLs get made absolute for AMP --- includes/amp-helper-functions.php | 34 +++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index fc09db8580d..4694b6953ce 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -412,7 +412,7 @@ function amp_handle_xhr_request() { } if ( isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) { - // This only runs on wp-comments-post.php. + // We don't need any data, so just send a success. add_filter( 'comment_post_redirect', function() { // We don't need any data, so just send a success. wp_send_json_success(); @@ -422,7 +422,6 @@ function amp_handle_xhr_request() { add_filter( 'wp_redirect', 'amp_intercept_post_request_redirect', PHP_INT_MAX, 2 ); amp_handle_xhr_headers_output(); } - } /** @@ -456,18 +455,37 @@ function amp_handle_xhr_headers_output() { } /** - * Handle a general, non comment AMP XHR post. + * Intercept the response to a non-comment POST request. * * @since 0.7.0 * @param string $location The location to redirect to. */ function amp_intercept_post_request_redirect( $location ) { - $host = wp_parse_url( $location, PHP_URL_HOST ); - if ( is_null( $host ) ) { - $location = home_url( $location ); + + // Make sure relative redirects get made absolute. + $parsed_location = array_merge( + array( + 'scheme' => 'https', + 'host' => wp_parse_url( home_url(), PHP_URL_HOST ), + 'path' => strtok( wp_unslash( $_SERVER['REQUEST_URI'] ), '?' ), + ), + wp_parse_url( $location ) + ); + + $absolute_location = $parsed_location['scheme'] . '://' . $parsed_location['host']; + if ( isset( $parsed_location['port'] ) ) { + $absolute_location .= ':' . $parsed_location['port']; + } + $absolute_location .= $parsed_location['path']; + if ( isset( $parsed_location['query'] ) ) { + $absolute_location .= '?' . $parsed_location['query']; } - header( 'AMP-Redirect-To: ' . $location ); - header( 'Access-Control-Expose-Headers: AMP-Redirect-To;' ); + if ( isset( $parsed_location['fragment'] ) ) { + $absolute_location .= '#' . $parsed_location['fragment']; + } + + header( 'AMP-Redirect-To: ' . $absolute_location ); + header( 'Access-Control-Expose-Headers: AMP-Redirect-To' ); // Send json success as no data is required. wp_send_json_success(); } From 7a4b398a801e0fa6a475f60f588b5be14c6e4b4c Mon Sep 17 00:00:00 2001 From: David Cramer Date: Mon, 5 Feb 2018 16:14:45 +0200 Subject: [PATCH 18/22] add process isolation --- phpunit.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/phpunit.xml b/phpunit.xml index a2b80c5bb36..625934afd4a 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,4 +1,5 @@ Date: Mon, 5 Feb 2018 16:31:06 +0200 Subject: [PATCH 19/22] remove isolation tag --- phpunit.xml | 1 - 1 file changed, 1 deletion(-) diff --git a/phpunit.xml b/phpunit.xml index 625934afd4a..a2b80c5bb36 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,5 +1,4 @@ Date: Tue, 6 Feb 2018 07:28:34 +0200 Subject: [PATCH 20/22] add test for amp_intercept_post_request_redirect --- tests/test-amp-helper-functions.php | 49 +++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/test-amp-helper-functions.php b/tests/test-amp-helper-functions.php index f7cde3d0e7a..f8b3bd4a5c9 100644 --- a/tests/test-amp-helper-functions.php +++ b/tests/test-amp-helper-functions.php @@ -362,4 +362,53 @@ public function test_amp_get_schemaorg_metadata() { $this->assertArrayHasKey( 'did_amp_schemaorg_metadata', $metadata ); $this->assertEquals( 'George', $metadata['author']['name'] ); } + + /** + * Test amp_intercept_post_request_redirect(). + * + * @runInSeparateProcess + * @preserveGlobalState disabled + * @group amp-submissions + */ + public function test_amp_intercept_post_request_redirect() { + if ( ! function_exists( 'xdebug_get_headers' ) ) { + $this->markTestSkipped( 'xdebug is required for this test' ); + } + + add_theme_support( 'amp' ); + $url = get_home_url(); + + add_filter( 'wp_doing_ajax', '__return_true' ); + add_filter( 'wp_die_ajax_handler', function () { + return '__return_false'; + } ); + + ob_start(); + amp_intercept_post_request_redirect( $url ); + ob_get_clean(); + $this->assertContains( 'AMP-Redirect-To: ' . $url, xdebug_get_headers() ); + $this->assertContains( 'Access-Control-Expose-Headers: AMP-Redirect-To', xdebug_get_headers() ); + + ob_start(); + amp_intercept_post_request_redirect( '/new-location/' ); + ob_get_clean(); + $this->assertContains( 'AMP-Redirect-To: https://example.org/new-location/', xdebug_get_headers() ); + + ob_start(); + amp_intercept_post_request_redirect( '//example.com/new-location/' ); + ob_get_clean(); + $headers = xdebug_get_headers(); + $this->assertContains( 'AMP-Redirect-To: https://example.com/new-location/', $headers ); + + ob_start(); + amp_intercept_post_request_redirect( '' ); + ob_get_clean(); + $this->assertContains( 'AMP-Redirect-To: https://example.org', xdebug_get_headers() ); + + remove_filter( 'wp_doing_ajax', '__return_true' ); + remove_filter( 'wp_die_ajax_handler', function () { + return '__return_false'; + } ); + + } } From 4090e4e52c05d90d076dcfc8518400dfdad952a5 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Tue, 6 Feb 2018 07:48:17 +0200 Subject: [PATCH 21/22] add test for amp_handle_xhr_request --- tests/test-amp-helper-functions.php | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tests/test-amp-helper-functions.php b/tests/test-amp-helper-functions.php index f8b3bd4a5c9..bdd46354a44 100644 --- a/tests/test-amp-helper-functions.php +++ b/tests/test-amp-helper-functions.php @@ -368,7 +368,6 @@ public function test_amp_get_schemaorg_metadata() { * * @runInSeparateProcess * @preserveGlobalState disabled - * @group amp-submissions */ public function test_amp_intercept_post_request_redirect() { if ( ! function_exists( 'xdebug_get_headers' ) ) { @@ -411,4 +410,25 @@ public function test_amp_intercept_post_request_redirect() { } ); } + + /** + * Test amp_handle_xhr_request(). + * + * @runInSeparateProcess + * @preserveGlobalState disabled + * @covers amp_handle_xhr_headers_output() + */ + public function test_amp_handle_xhr_request() { + global $pagenow; + if ( ! function_exists( 'xdebug_get_headers' ) ) { + $this->markTestSkipped( 'xdebug is required for this test' ); + } + + $_GET['__amp_source_origin'] = 'https://example.org'; + $pagenow = 'wp-comments-post.php'; + + amp_handle_xhr_request(); + $this->assertContains( 'AMP-Access-Control-Allow-Source-Origin: https://example.org', xdebug_get_headers() ); + + } } From ee75de2759833f55c1bdf295fd2af746992eb83b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Feb 2018 23:42:17 -0800 Subject: [PATCH 22/22] Augment amp_intercept_post_request_redirect test with checking output --- tests/test-amp-helper-functions.php | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/test-amp-helper-functions.php b/tests/test-amp-helper-functions.php index bdd46354a44..4eed7183a7f 100644 --- a/tests/test-amp-helper-functions.php +++ b/tests/test-amp-helper-functions.php @@ -368,6 +368,7 @@ public function test_amp_get_schemaorg_metadata() { * * @runInSeparateProcess * @preserveGlobalState disabled + * @covers amp_intercept_post_request_redirect() */ public function test_amp_intercept_post_request_redirect() { if ( ! function_exists( 'xdebug_get_headers' ) ) { @@ -384,31 +385,26 @@ public function test_amp_intercept_post_request_redirect() { ob_start(); amp_intercept_post_request_redirect( $url ); - ob_get_clean(); + $this->assertEquals( '{"success":true}', ob_get_clean() ); + $this->assertContains( 'AMP-Redirect-To: ' . $url, xdebug_get_headers() ); $this->assertContains( 'Access-Control-Expose-Headers: AMP-Redirect-To', xdebug_get_headers() ); ob_start(); amp_intercept_post_request_redirect( '/new-location/' ); - ob_get_clean(); + $this->assertEquals( '{"success":true}', ob_get_clean() ); $this->assertContains( 'AMP-Redirect-To: https://example.org/new-location/', xdebug_get_headers() ); ob_start(); amp_intercept_post_request_redirect( '//example.com/new-location/' ); - ob_get_clean(); + $this->assertEquals( '{"success":true}', ob_get_clean() ); $headers = xdebug_get_headers(); $this->assertContains( 'AMP-Redirect-To: https://example.com/new-location/', $headers ); ob_start(); amp_intercept_post_request_redirect( '' ); - ob_get_clean(); + $this->assertEquals( '{"success":true}', ob_get_clean() ); $this->assertContains( 'AMP-Redirect-To: https://example.org', xdebug_get_headers() ); - - remove_filter( 'wp_doing_ajax', '__return_true' ); - remove_filter( 'wp_die_ajax_handler', function () { - return '__return_false'; - } ); - } /**