From fb649cccda72c969fff083396dc03f254a9078d5 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Tue, 20 Mar 2018 14:22:51 +0200 Subject: [PATCH 1/6] add amp-redirect if amp-live-list is not declared --- includes/class-amp-theme-support.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 99820b1ab70..34de89a3128 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -324,10 +324,22 @@ public static function handle_xhr_request() { if ( isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) { // We don't need any data, so just send a success. - add_filter( 'comment_post_redirect', function() { + add_filter( 'comment_post_redirect', function( $url, $comment ) { + // Get theme support. + $theme_support = get_theme_support( 'amp' ); + + // Add the comment ID to the URL to force AMP to refresh the page. + $url = add_query_arg( 'comment', $comment->comment_ID, $url ); + + // Send redirect header if amp-live-list has been opted-out. + if ( empty( $theme_support['comments_live_list'] ) ) { + header( 'AMP-Redirect-To: ' . $url, true ); + } + // We don't need any data, so just send a success. wp_send_json_success(); - }, PHP_INT_MAX ); + + }, PHP_INT_MAX, 2 ); self::handle_xhr_headers_output(); } elseif ( ! empty( self::$purged_amp_query_vars['_wp_amp_action_xhr_converted'] ) ) { add_filter( 'wp_redirect', array( __CLASS__, 'intercept_post_request_redirect' ), PHP_INT_MAX ); From 6f00f20644f494008e432087feb14348e135bd56 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Tue, 20 Mar 2018 17:58:39 +0200 Subject: [PATCH 2/6] Add message to response based on comment approval --- includes/class-amp-theme-support.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 34de89a3128..acb2d6baaad 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -335,9 +335,17 @@ public static function handle_xhr_request() { if ( empty( $theme_support['comments_live_list'] ) ) { header( 'AMP-Redirect-To: ' . $url, true ); } + // Create a success message to display to the user. + if ( '1' === (string) $comment->comment_approved ) { + $message = __( 'Your comment has been posted and has been approved.', 'amp' ); + } else { + $message = __( 'Your comment has been posted, and is awaiting moderation.', 'default' ); + } + + $message = apply_filters( 'amp_comment_submitted_message', $message, $comment ); // We don't need any data, so just send a success. - wp_send_json_success(); + wp_send_json_success( $message ); }, PHP_INT_MAX, 2 ); self::handle_xhr_headers_output(); @@ -528,7 +536,7 @@ public static function add_amp_comment_form_templates() { ?>
From a0fce3fbd7d288fedf586220813c71960fd6ff50 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 20 Mar 2018 17:31:47 -0700 Subject: [PATCH 3/6] Fix and refactor XHR handling * Explicitly limit handle_xhr_request to POST requests. * Make sure original status header is sent back in wp_die() call. * Fix reading of amp theme support flag since get_theme_support() returns an array of args. * Add missing paragraph surrounding success message template. * Fix moderation comment to ensure default core string is re-used. * Fix PHP notice in regards to additional comment_live_list theme support flag being present. * Remove obsolete comments. * Simplify success message. * Add phpdoc for amp_comment_posted_message filter. * Pass back success message in same way as error message is returned as named variable. * Add missing tests and introduce \AMP_Theme_Support::send_header() to avoid having to runInSeparateProcess. --- includes/class-amp-theme-support.php | 212 ++++++++++++++++++------- tests/test-class-amp-theme-support.php | 206 ++++++++++++++++++++---- 2 files changed, 325 insertions(+), 93 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index acb2d6baaad..fbaa674613b 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -67,6 +67,15 @@ class AMP_Theme_Support { */ public static $purged_amp_query_vars = array(); + /** + * Headers sent (or attempted to be sent). + * + * @since 0.7 + * @see AMP_Theme_Support::send_header() + * @var array[] + */ + public static $headers_sent = array(); + /** * Initialize. */ @@ -87,7 +96,7 @@ public static function init() { $args = array_shift( $support ); if ( ! is_array( $args ) ) { trigger_error( esc_html__( 'Expected AMP theme support arg to be array.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error - } elseif ( count( array_diff( array_keys( $args ), array( 'template_dir', 'available_callback' ) ) ) !== 0 ) { + } elseif ( count( array_diff( array_keys( $args ), array( 'template_dir', 'available_callback', 'comments_live_list' ) ) ) !== 0 ) { trigger_error( esc_html__( 'Expected AMP theme support to only have template_dir and/or available_callback.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error } } @@ -310,85 +319,172 @@ public static function purge_amp_query_vars() { } } + /** + * Send an HTTP response header. + * + * This largely exists to facilitate unit testing but it also provides a better interface for sending headers. + * + * @since 0.7.0 + * + * @param string $name Header name. + * @param string $value Header value. + * @param array $args { + * Args to header(). + * + * @type bool $replace Whether to replace a header previously sent. Default true. + * @type int $status_code Status code to send with the sent header. + * } + * @return bool Whether the header was sent. + */ + public static function send_header( $name, $value, $args = array() ) { + $args = array_merge( + array( + 'replace' => true, + 'status_code' => null, + ), + $args + ); + + self::$headers_sent[] = array_merge( compact( 'name', 'value' ), $args ); + if ( headers_sent() ) { + return false; + } + + header( + sprintf( '%s: %s', $name, $value ), + $args['replace'], + $args['status_code'] + ); + return true; + } + /** * Hook into a form submissions, such as the comment form or some other form submission. * * @since 0.7.0 - * @global string $pagenow */ public static function handle_xhr_request() { - global $pagenow; - if ( empty( self::$purged_amp_query_vars['__amp_source_origin'] ) ) { + if ( empty( self::$purged_amp_query_vars['__amp_source_origin'] ) || empty( $_SERVER['REQUEST_METHOD'] ) || 'POST' !== $_SERVER['REQUEST_METHOD'] ) { return; } - if ( isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) { - // We don't need any data, so just send a success. - add_filter( 'comment_post_redirect', function( $url, $comment ) { - // Get theme support. - $theme_support = get_theme_support( 'amp' ); + // Send AMP response header. + $origin = esc_url_raw( self::$purged_amp_query_vars['__amp_source_origin'] ); + self::send_header( 'AMP-Access-Control-Allow-Source-Origin', $origin, array( 'replace' => true ) ); // @todo The $origin needs to be validated. - // Add the comment ID to the URL to force AMP to refresh the page. - $url = add_query_arg( 'comment', $comment->comment_ID, $url ); + // Intercept POST requests which redirect. + add_filter( 'wp_redirect', array( __CLASS__, 'intercept_post_request_redirect' ), PHP_INT_MAX ); - // Send redirect header if amp-live-list has been opted-out. - if ( empty( $theme_support['comments_live_list'] ) ) { - header( 'AMP-Redirect-To: ' . $url, true ); - } - // Create a success message to display to the user. - if ( '1' === (string) $comment->comment_approved ) { - $message = __( 'Your comment has been posted and has been approved.', 'amp' ); - } else { - $message = __( 'Your comment has been posted, and is awaiting moderation.', 'default' ); - } + // Add special handling for redirecting after comment submission. + add_filter( 'comment_post_redirect', array( __CLASS__, 'filter_comment_post_redirect' ), PHP_INT_MAX, 2 ); + + // Add die handler for AMP error display, most likely due to problem with comment. + add_filter( 'wp_die_handler', function() { + return array( __CLASS__, 'handle_wp_die' ); + } ); + + } - $message = apply_filters( 'amp_comment_submitted_message', $message, $comment ); + /** + * Strip tags that are not allowed in amp-mustache. + * + * @since 0.7.0 + * + * @param string $text Text to sanitize. + * @return string Sanitized text. + */ + protected static function wp_kses_amp_mustache( $text ) { + $amp_mustache_allowed_html_tags = array( 'strong', 'b', 'em', 'i', 'u', 's', 'small', 'mark', 'del', 'ins', 'sup', 'sub' ); + return wp_kses( $text, array_fill_keys( $amp_mustache_allowed_html_tags, array() ) ); + } + + /** + * Handle comment_post_redirect to ensure page reload is done when comments_live_list is not supported, while sending back a success message when it is. + * + * @since 0.7.0 + * + * @param string $url Comment permalink to redirect to. + * @param WP_Comment $comment Posted comment. + * @return string URL. + */ + public static function filter_comment_post_redirect( $url, $comment ) { + $theme_support = get_theme_support( 'amp' ); - // We don't need any data, so just send a success. - wp_send_json_success( $message ); + // Cause a page refresh if amp-live-list is not implemented for comments via add_theme_support( 'amp', array( 'comments_live_list' => true ) ). + if ( empty( $theme_support[0]['comments_live_list'] ) ) { - }, PHP_INT_MAX, 2 ); - self::handle_xhr_headers_output(); - } elseif ( ! empty( self::$purged_amp_query_vars['_wp_amp_action_xhr_converted'] ) ) { - add_filter( 'wp_redirect', array( __CLASS__, 'intercept_post_request_redirect' ), PHP_INT_MAX ); - self::handle_xhr_headers_output(); + // Add the comment ID to the URL to force AMP to refresh the page. + $url = add_query_arg( 'comment', $comment->comment_ID, $url ); + + // Pass URL along to wp_redirect(). + return $url; } + + // Create a success message to display to the user. + if ( '1' === (string) $comment->comment_approved ) { + $message = __( 'Your comment has been posted.', 'amp' ); + } else { + $message = __( 'Your comment is awaiting moderation.', 'default' ); // Note core string re-use. + } + + /** + * Filters the message when comment submitted success message when + * + * @since 0.7 + */ + $message = apply_filters( 'amp_comment_posted_message', $message, $comment ); + + // Message will be shown in template defined by AMP_Theme_Support::add_amp_comment_form_templates(). + wp_send_json( array( + 'message' => self::wp_kses_amp_mustache( $message ), + ) ); } /** - * Handle the AMP XHR headers and output errors. + * New error handler for AMP form submission. * * @since 0.7.0 + * @see wp_die() + * + * @param WP_Error|string $error The error to handle. + * @param string|int $title Optional. Error title. If `$message` is a `WP_Error` object, + * error data with the key 'title' may be used to specify the title. + * If `$title` is an integer, then it is treated as the response + * code. Default empty. + * @param string|array|int $args { + * Optional. Arguments to control behavior. If `$args` is an integer, then it is treated + * as the response code. Default empty array. + * + * @type int $response The HTTP response code. Default 200 for Ajax requests, 500 otherwise. + * } */ - public static function handle_xhr_headers_output() { - // Add die handler for AMP error display. - add_filter( 'wp_die_handler', function() { - /** - * New error handler for AMP form submission. - * - * @param WP_Error|string $error The error to handle. - */ - return function( $error ) { - status_header( 400 ); - if ( is_wp_error( $error ) ) { - $error = $error->get_error_message(); - } - $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() ) ), - ) ); - }; - } ); + public static function handle_wp_die( $error, $title = '', $args = array() ) { + $status_code = 500; + if ( is_int( $title ) ) { + $status_code = $title; + } elseif ( is_int( $args ) ) { + $status_code = $args; + } elseif ( is_array( $args ) && isset( $args['response'] ) ) { + $status_code = $args['response']; + } + status_header( $status_code ); - // Send AMP header. - $origin = esc_url_raw( self::$purged_amp_query_vars['__amp_source_origin'] ); - header( 'AMP-Access-Control-Allow-Source-Origin: ' . $origin, true ); + if ( is_wp_error( $error ) ) { + $error = $error->get_error_message(); + } + + // Message will be shown in template defined by AMP_Theme_Support::add_amp_comment_form_templates(). + wp_send_json( array( + 'error' => self::wp_kses_amp_mustache( $error ), + ) ); } /** - * Intercept the response to a non-comment POST request. + * Intercept the response to a POST request. * * @since 0.7.0 + * @see wp_redirect() + * * @param string $location The location to redirect to. */ public static function intercept_post_request_redirect( $location ) { @@ -398,7 +494,7 @@ public static function intercept_post_request_redirect( $location ) { array( 'scheme' => 'https', 'host' => wp_parse_url( home_url(), PHP_URL_HOST ), - 'path' => strtok( wp_unslash( $_SERVER['REQUEST_URI'] ), '?' ), + 'path' => isset( $_SERVER['REQUEST_URI'] ) ? strtok( wp_unslash( $_SERVER['REQUEST_URI'] ), '?' ) : '/', ), wp_parse_url( $location ) ); @@ -415,9 +511,9 @@ public static function intercept_post_request_redirect( $location ) { $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. + self::send_header( 'AMP-Redirect-To', $absolute_location ); + self::send_header( 'Access-Control-Expose-Headers', 'AMP-Redirect-To' ); + wp_send_json_success(); } @@ -536,7 +632,7 @@ public static function add_amp_comment_form_templates() { ?>
diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 44baeb8a294..b49163ec785 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -25,6 +25,9 @@ public function tearDown() { remove_theme_support( 'amp' ); $_REQUEST = array(); // phpcs:ignore WordPress.CSRF.NonceVerification.NoNonceVerification $_SERVER['QUERY_STRING'] = ''; + unset( $_SERVER['REQUEST_URI'] ); + unset( $_SERVER['REQUEST_METHOD'] ); + AMP_Theme_Support::$headers_sent = array(); } /** @@ -266,69 +269,202 @@ public function test_purge_amp_query_vars() { // phpcs:enable WordPress.Security.NonceVerification.NoNonceVerification } + /** + * Test handle_xhr_request(). + * + * @covers AMP_Theme_Support::handle_xhr_request() + */ + public function test_handle_xhr_request() { + AMP_Theme_Support::handle_xhr_request(); + $this->assertEmpty( AMP_Theme_Support::$headers_sent ); + + $_GET['__amp_source_origin'] = home_url(); + $_SERVER['REQUEST_METHOD'] = 'POST'; + + AMP_Theme_Support::purge_amp_query_vars(); + AMP_Theme_Support::handle_xhr_request(); + + $this->assertCount( 1, AMP_Theme_Support::$headers_sent ); + + $this->assertEquals( + array( + 'name' => 'AMP-Access-Control-Allow-Source-Origin', + 'value' => 'http://example.org', + 'replace' => true, + 'status_code' => null, + ), + AMP_Theme_Support::$headers_sent[0] + ); + + $this->assertEquals( PHP_INT_MAX, has_filter( 'wp_redirect', array( 'AMP_Theme_Support', 'intercept_post_request_redirect' ) ) ); + $this->assertEquals( PHP_INT_MAX, has_filter( 'comment_post_redirect', array( 'AMP_Theme_Support', 'filter_comment_post_redirect' ) ) ); + $this->assertEquals( + array( 'AMP_Theme_Support', 'handle_wp_die' ), + apply_filters( 'wp_die_handler', '__return_true' ) + ); + } + + /** + * Test filter_comment_post_redirect(). + * + * @covers AMP_Theme_Support::filter_comment_post_redirect() + */ + public function test_filter_comment_post_redirect() { + add_filter( 'wp_doing_ajax', '__return_true' ); + add_filter( 'wp_die_ajax_handler', function() { + return '__return_null'; + } ); + + add_theme_support( 'amp' ); + $post = $this->factory()->post->create_and_get(); + $comment = $this->factory()->comment->create_and_get( array( + 'comment_post_ID' => $post->ID, + ) ); + $url = get_comment_link( $comment ); + + // Test without comments_live_list. + $filtered_url = AMP_Theme_Support::filter_comment_post_redirect( $url, $comment ); + $this->assertNotEquals( + strtok( $url, '#' ), + strtok( $filtered_url, '#' ) + ); + + // Test with comments_live_list. + add_theme_support( 'amp', array( + 'comments_live_list' => true, + ) ); + add_filter( 'amp_comment_posted_message', function( $message, WP_Comment $filter_comment ) { + return sprintf( '(comment=%d,approved=%d)', $filter_comment->comment_ID, $filter_comment->comment_approved ); + }, 10, 2 ); + + // Test approved comment. + $comment->comment_approved = '1'; + ob_start(); + AMP_Theme_Support::filter_comment_post_redirect( $url, $comment ); + $response = json_decode( ob_get_clean(), true ); + $this->assertArrayHasKey( 'message', $response ); + $this->assertEquals( + sprintf( '(comment=%d,approved=1)', $comment->comment_ID ), + $response['message'] + ); + + // Test moderated comment. + $comment->comment_approved = '0'; + ob_start(); + AMP_Theme_Support::filter_comment_post_redirect( $url, $comment ); + $response = json_decode( ob_get_clean(), true ); + $this->assertArrayHasKey( 'message', $response ); + $this->assertEquals( + sprintf( '(comment=%d,approved=0)', $comment->comment_ID ), + $response['message'] + ); + } + + /** + * Test handle_wp_die(). + * + * @covers AMP_Theme_Support::handle_wp_die() + */ + public function test_handle_wp_die() { + add_filter( 'wp_doing_ajax', '__return_true' ); + add_filter( 'wp_die_ajax_handler', function() { + return '__return_null'; + } ); + + ob_start(); + AMP_Theme_Support::handle_wp_die( 'string' ); + $this->assertEquals( '{"error":"string"}', ob_get_clean() ); + + ob_start(); + $error = new WP_Error( 'code', 'The Message' ); + AMP_Theme_Support::handle_wp_die( $error ); + $this->assertEquals( '{"error":"The Message"}', ob_get_clean() ); + } + /** * Test intercept_post_request_redirect(). * - * @runInSeparateProcess - * @preserveGlobalState disabled * @covers AMP_Theme_Support::intercept_post_request_redirect() */ public function test_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(); + $url = home_url( '/' ); add_filter( 'wp_doing_ajax', '__return_true' ); add_filter( 'wp_die_ajax_handler', function () { return '__return_false'; } ); + // Test redirecting to full URL. + AMP_Theme_Support::$headers_sent = array(); ob_start(); AMP_Theme_Support::intercept_post_request_redirect( $url ); $this->assertEquals( '{"success":true}', ob_get_clean() ); + $this->assertContains( + array( + 'name' => 'AMP-Redirect-To', + 'value' => $url, + 'replace' => true, + 'status_code' => null, + ), + AMP_Theme_Support::$headers_sent + ); + $this->assertContains( + array( + 'name' => 'Access-Control-Expose-Headers', + 'value' => 'AMP-Redirect-To', + 'replace' => true, + 'status_code' => null, + ), + AMP_Theme_Support::$headers_sent + ); - $this->assertContains( 'AMP-Redirect-To: ' . $url, xdebug_get_headers() ); - $this->assertContains( 'Access-Control-Expose-Headers: AMP-Redirect-To', xdebug_get_headers() ); - + // Test redirecting to host-less location. + AMP_Theme_Support::$headers_sent = array(); ob_start(); AMP_Theme_Support::intercept_post_request_redirect( '/new-location/' ); $this->assertEquals( '{"success":true}', ob_get_clean() ); - $this->assertContains( 'AMP-Redirect-To: https://example.org/new-location/', xdebug_get_headers() ); + $this->assertContains( + array( + 'name' => 'AMP-Redirect-To', + 'value' => set_url_scheme( home_url( '/new-location/' ), 'https' ), + 'replace' => true, + 'status_code' => null, + ), + AMP_Theme_Support::$headers_sent + ); + // Test redirecting to scheme-less location. + AMP_Theme_Support::$headers_sent = array(); ob_start(); - AMP_Theme_Support::intercept_post_request_redirect( '//example.com/new-location/' ); + $url = home_url( '/new-location/' ); + AMP_Theme_Support::intercept_post_request_redirect( substr( $url, strpos( $url, ':' ) + 1 ) ); $this->assertEquals( '{"success":true}', ob_get_clean() ); - $headers = xdebug_get_headers(); - $this->assertContains( 'AMP-Redirect-To: https://example.com/new-location/', $headers ); + $this->assertContains( + array( + 'name' => 'AMP-Redirect-To', + 'value' => set_url_scheme( home_url( '/new-location/' ), 'https' ), + 'replace' => true, + 'status_code' => null, + ), + AMP_Theme_Support::$headers_sent + ); + // Test redirecting to empty location. + AMP_Theme_Support::$headers_sent = array(); ob_start(); AMP_Theme_Support::intercept_post_request_redirect( '' ); $this->assertEquals( '{"success":true}', ob_get_clean() ); - $this->assertContains( 'AMP-Redirect-To: https://example.org', xdebug_get_headers() ); - } - - /** - * Test handle_xhr_request(). - * - * @runInSeparateProcess - * @preserveGlobalState disabled - * @covers AMP_Theme_Support::handle_xhr_request() - */ - public function test_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_Theme_Support::purge_amp_query_vars(); - - AMP_Theme_Support::handle_xhr_request(); - $this->assertContains( 'AMP-Access-Control-Allow-Source-Origin: https://example.org', xdebug_get_headers() ); + $this->assertContains( + array( + 'name' => 'AMP-Redirect-To', + 'value' => set_url_scheme( home_url(), 'https' ), + 'replace' => true, + 'status_code' => null, + ), + AMP_Theme_Support::$headers_sent + ); } /** From 1e32d6c0f4f3a42affef4080cd80bd8a991de476 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 20 Mar 2018 17:48:55 -0700 Subject: [PATCH 4/6] Add validation for __amp_source_origin --- includes/class-amp-theme-support.php | 6 ++++-- tests/test-class-amp-theme-support.php | 15 ++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index fbaa674613b..095173e4ed1 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -369,8 +369,10 @@ public static function handle_xhr_request() { } // Send AMP response header. - $origin = esc_url_raw( self::$purged_amp_query_vars['__amp_source_origin'] ); - self::send_header( 'AMP-Access-Control-Allow-Source-Origin', $origin, array( 'replace' => true ) ); // @todo The $origin needs to be validated. + $origin = wp_validate_redirect( wp_sanitize_redirect( esc_url_raw( self::$purged_amp_query_vars['__amp_source_origin'] ) ) ); + if ( $origin ) { + self::send_header( 'AMP-Access-Control-Allow-Source-Origin', $origin, array( 'replace' => true ) ); // @todo The $origin needs to be validated. + } // Intercept POST requests which redirect. add_filter( 'wp_redirect', array( __CLASS__, 'intercept_post_request_redirect' ), PHP_INT_MAX ); diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index b49163ec785..fb2506b7823 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -275,27 +275,32 @@ public function test_purge_amp_query_vars() { * @covers AMP_Theme_Support::handle_xhr_request() */ public function test_handle_xhr_request() { + AMP_Theme_Support::purge_amp_query_vars(); AMP_Theme_Support::handle_xhr_request(); $this->assertEmpty( AMP_Theme_Support::$headers_sent ); - $_GET['__amp_source_origin'] = home_url(); + // Try bad source origin. + $_GET['__amp_source_origin'] = 'http://evil.example.com/'; $_SERVER['REQUEST_METHOD'] = 'POST'; - AMP_Theme_Support::purge_amp_query_vars(); AMP_Theme_Support::handle_xhr_request(); + $this->assertEmpty( AMP_Theme_Support::$headers_sent ); + // Try home source origin. + $_GET['__amp_source_origin'] = home_url(); + $_SERVER['REQUEST_METHOD'] = 'POST'; + AMP_Theme_Support::purge_amp_query_vars(); + AMP_Theme_Support::handle_xhr_request(); $this->assertCount( 1, AMP_Theme_Support::$headers_sent ); - $this->assertEquals( array( 'name' => 'AMP-Access-Control-Allow-Source-Origin', - 'value' => 'http://example.org', + 'value' => home_url(), 'replace' => true, 'status_code' => null, ), AMP_Theme_Support::$headers_sent[0] ); - $this->assertEquals( PHP_INT_MAX, has_filter( 'wp_redirect', array( 'AMP_Theme_Support', 'intercept_post_request_redirect' ) ) ); $this->assertEquals( PHP_INT_MAX, has_filter( 'comment_post_redirect', array( 'AMP_Theme_Support', 'filter_comment_post_redirect' ) ) ); $this->assertEquals( From 5c963247cf0538b786b3a9319caef8df05b853c7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 21 Mar 2018 09:21:20 -0700 Subject: [PATCH 5/6] Use scheme-less URL for redirect if non-HTTPS --- includes/class-amp-theme-support.php | 6 +++++- tests/test-class-amp-theme-support.php | 29 ++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 095173e4ed1..c774c71d480 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -501,7 +501,11 @@ public static function intercept_post_request_redirect( $location ) { wp_parse_url( $location ) ); - $absolute_location = $parsed_location['scheme'] . '://' . $parsed_location['host']; + $absolute_location = ''; + if ( 'https' === $parsed_location['scheme'] ) { + $absolute_location .= $parsed_location['scheme'] . ':'; + } + $absolute_location .= '//' . $parsed_location['host']; if ( isset( $parsed_location['port'] ) ) { $absolute_location .= ':' . $parsed_location['port']; } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index fb2506b7823..e672e5e0cdf 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -394,14 +394,14 @@ public function test_handle_wp_die() { public function test_intercept_post_request_redirect() { add_theme_support( 'amp' ); - $url = home_url( '/' ); + $url = home_url( '/', 'https' ); add_filter( 'wp_doing_ajax', '__return_true' ); add_filter( 'wp_die_ajax_handler', function () { return '__return_false'; } ); - // Test redirecting to full URL. + // Test redirecting to full URL with HTTPS protocol. AMP_Theme_Support::$headers_sent = array(); ob_start(); AMP_Theme_Support::intercept_post_request_redirect( $url ); @@ -425,6 +425,31 @@ public function test_intercept_post_request_redirect() { AMP_Theme_Support::$headers_sent ); + // Test redirecting to non-HTTPS URL. + AMP_Theme_Support::$headers_sent = array(); + ob_start(); + $url = home_url( '/', 'http' ); + AMP_Theme_Support::intercept_post_request_redirect( $url ); + $this->assertEquals( '{"success":true}', ob_get_clean() ); + $this->assertContains( + array( + 'name' => 'AMP-Redirect-To', + 'value' => preg_replace( '#^\w+:#', '', $url ), + 'replace' => true, + 'status_code' => null, + ), + AMP_Theme_Support::$headers_sent + ); + $this->assertContains( + array( + 'name' => 'Access-Control-Expose-Headers', + 'value' => 'AMP-Redirect-To', + 'replace' => true, + 'status_code' => null, + ), + AMP_Theme_Support::$headers_sent + ); + // Test redirecting to host-less location. AMP_Theme_Support::$headers_sent = array(); ob_start(); From 8da1a3633c5d141a463ad4d7c3dc97c7307ddb5e Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 21 Mar 2018 11:25:58 -0700 Subject: [PATCH 6/6] Improve comments regarding XHR handling Improve default status_code handling --- includes/class-amp-theme-support.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index c774c71d480..52b13f98111 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -359,7 +359,7 @@ public static function send_header( $name, $value, $args = array() ) { } /** - * Hook into a form submissions, such as the comment form or some other form submission. + * Hook into a POST form submissions, such as the comment form or some other form submission. * * @since 0.7.0 */ @@ -371,7 +371,7 @@ public static function handle_xhr_request() { // Send AMP response header. $origin = wp_validate_redirect( wp_sanitize_redirect( esc_url_raw( self::$purged_amp_query_vars['__amp_source_origin'] ) ) ); if ( $origin ) { - self::send_header( 'AMP-Access-Control-Allow-Source-Origin', $origin, array( 'replace' => true ) ); // @todo The $origin needs to be validated. + self::send_header( 'AMP-Access-Control-Allow-Source-Origin', $origin, array( 'replace' => true ) ); } // Intercept POST requests which redirect. @@ -414,8 +414,10 @@ public static function filter_comment_post_redirect( $url, $comment ) { // Cause a page refresh if amp-live-list is not implemented for comments via add_theme_support( 'amp', array( 'comments_live_list' => true ) ). if ( empty( $theme_support[0]['comments_live_list'] ) ) { - - // Add the comment ID to the URL to force AMP to refresh the page. + /* + * Add the comment ID to the URL to force AMP to refresh the page. + * This is ideally a temporary workaround to deal with https://github.com/ampproject/amphtml/issues/14170 + */ $url = add_query_arg( 'comment', $comment->comment_ID, $url ); // Pass URL along to wp_redirect(). @@ -461,13 +463,14 @@ public static function filter_comment_post_redirect( $url, $comment ) { * } */ public static function handle_wp_die( $error, $title = '', $args = array() ) { - $status_code = 500; if ( is_int( $title ) ) { $status_code = $title; } elseif ( is_int( $args ) ) { $status_code = $args; } elseif ( is_array( $args ) && isset( $args['response'] ) ) { $status_code = $args['response']; + } else { + $status_code = 500; } status_header( $status_code );