From be6781844c0ed6acd66c9a7ad7367be458a11666 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 20 Nov 2019 21:01:48 -0800 Subject: [PATCH] Improve error messages when validation requests fail (#3793) * Remove broken removal of query args during URL normalization * Add missing encoding of validate URL query args in admin bar * Improve error messages shown after failing to perform validation requests * Fix PHP comments Co-Authored-By: Alain Schlesser --- .../options/class-amp-options-manager.php | 86 +++++--------- .../class-amp-validated-url-post-type.php | 52 +++++---- .../class-amp-validation-manager.php | 110 ++++++++++++++---- ...test-class-amp-validated-url-post-type.php | 42 +++---- 4 files changed, 169 insertions(+), 121 deletions(-) diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index bbfbbb76f47..8f3adc564b6 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -718,23 +718,9 @@ public static function handle_updated_theme_support_option() { $validation = AMP_Validation_Manager::validate_url( $url ); if ( is_wp_error( $validation ) ) { - $review_messages[] = esc_html( - sprintf( - /* translators: 1: error message. 2: error code. */ - __( 'However, there was an error when checking the AMP validity for your site.', 'amp' ), - $validation->get_error_message(), - $validation->get_error_code() - ) - ); - - $error_message = $validation->get_error_message(); - if ( $error_message ) { - $review_messages[] = $error_message; - } else { - /* translators: %s is the error code */ - $review_messages[] = esc_html( sprintf( __( 'Error code: %s.', 'amp' ), $validation->get_error_code() ) ); - } - $notice_type = 'error'; + $review_messages[] = esc_html__( 'However, there was an error when checking the AMP validity for your site.', 'amp' ); + $review_messages[] = AMP_Validation_Manager::get_validate_url_error_message( $validation->get_error_code(), $validation->get_error_message() ); + $notice_type = 'error'; } elseif ( is_array( $validation ) ) { $new_errors = 0; $rejected_errors = 0; @@ -757,7 +743,7 @@ public static function handle_updated_theme_support_option() { if ( $rejected_errors > 0 ) { $notice_type = 'error'; - $message = wp_kses_post( + $message = esc_html( sprintf( /* translators: %s is count of rejected errors */ _n( @@ -772,43 +758,37 @@ public static function handle_updated_theme_support_option() { ); if ( $invalid_url_screen_url ) { - $message .= ' ' . wp_kses_post( - sprintf( - /* translators: %s is URL to review issues */ - _n( - 'Review Issue.', - 'Review Issues.', - $rejected_errors, - 'amp' - ), - esc_url( $invalid_url_screen_url ) - ) + $message .= ' ' . sprintf( + /* translators: %s is URL to review issues */ + _n( + 'Review Issue.', + 'Review Issues.', + $rejected_errors, + 'amp' + ), + esc_url( $invalid_url_screen_url ) ); } $review_messages[] = $message; } else { - $message = wp_kses_post( - sprintf( - /* translators: %s is an AMP URL */ - __( 'View an AMP version of your site.', 'amp' ), - esc_url( $url ) - ) + $message = sprintf( + /* translators: %s is an AMP URL */ + __( 'View an AMP version of your site.', 'amp' ), + esc_url( $url ) ); if ( $new_errors > 0 && $invalid_url_screen_url ) { - $message .= ' ' . wp_kses_post( - sprintf( - /* translators: 1: URL to review issues. 2: count of new errors. */ - _n( - 'Please also review %2$s issue which may need to be fixed (for one URL at least).', - 'Please also review %2$s issues which may need to be fixed (for one URL at least).', - $new_errors, - 'amp' - ), - esc_url( $invalid_url_screen_url ), - number_format_i18n( $new_errors ) - ) + $message .= ' ' . sprintf( + /* translators: 1: URL to review issues. 2: count of new errors. */ + _n( + 'Please also review %2$s issue which may need to be fixed (for one URL at least).', + 'Please also review %2$s issues which may need to be fixed (for one URL at least).', + $new_errors, + 'amp' + ), + esc_url( $invalid_url_screen_url ), + number_format_i18n( $new_errors ) ); } @@ -831,18 +811,16 @@ public static function handle_updated_theme_support_option() { } break; case AMP_Theme_Support::READER_MODE_SLUG: - $message = wp_kses_post( - sprintf( - /* translators: %s is an AMP URL */ - __( 'Reader mode activated! View the AMP version of a recent post. It is recommended that you upgrade to Standard or Transitional mode.', 'amp' ), - esc_url( $url ) - ) + $message = sprintf( + /* translators: %s is an AMP URL */ + __( 'Reader mode activated! View the AMP version of a recent post. It is recommended that you upgrade to Standard or Transitional mode.', 'amp' ), + esc_url( $url ) ); break; } if ( isset( $message ) ) { - add_settings_error( self::OPTION_NAME, 'template_mode_updated', $message, $notice_type ); + add_settings_error( self::OPTION_NAME, 'template_mode_updated', wp_kses_post( $message ), $notice_type ); } } } diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index 5e0c925dfa6..c752e49444a 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -631,17 +631,13 @@ public static function get_url_from_post( $post ) { /** * Normalize a URL for storage. * - * This ensures that query vars like utm_* and the like will not cause duplicates. * The AMP query param is removed to facilitate switching between standard and transitional. * The URL scheme is also normalized to HTTPS to help with transition from HTTP to HTTPS. * * @param string $url URL. * @return string Normalized URL. - * @global WP $wp */ protected static function normalize_url_for_storage( $url ) { - global $wp; - // Only ever store the canonical version. $url = amp_remove_endpoint( $url ); @@ -651,12 +647,11 @@ protected static function normalize_url_for_storage( $url ) { // Normalize query args, removing all that are not recognized or which are removable. $url_parts = explode( '?', $url, 2 ); if ( 2 === count( $url_parts ) ) { - parse_str( $url_parts[1], $args ); + $args = wp_parse_args( $url_parts[1] ); foreach ( wp_removable_query_args() as $removable_query_arg ) { unset( $args[ $removable_query_arg ] ); } - $args = wp_array_slice_assoc( $args, $wp->public_query_vars ); - $url = $url_parts[0]; + $url = $url_parts[0]; if ( ! empty( $args ) ) { $url = $url_parts[0] . '?' . build_query( $args ); } @@ -1231,7 +1226,7 @@ public static function handle_bulk_action( $redirect, $action, $items ) { $validity = AMP_Validation_Manager::validate_url( $url ); if ( is_wp_error( $validity ) ) { - $errors[] = $validity->get_error_code(); + $errors[] = AMP_Validation_Manager::get_validate_url_error_message( $validity->get_error_code(), $validity->get_error_message() ); continue; } @@ -1259,13 +1254,13 @@ static function( $error ) { self::URLS_TESTED => count( $items ), ]; if ( ! empty( $errors ) ) { - $args['amp_validate_error'] = $errors; + $args['amp_validate_error'] = AMP_Validation_Manager::serialize_validation_error_messages( $errors ); } else { $args[ self::REMAINING_ERRORS ] = count( $remaining_invalid_urls ); } $redirect = remove_query_arg( wp_removable_query_args(), $redirect ); - return add_query_arg( $args, $redirect ); + return add_query_arg( rawurlencode_deep( $args ), $redirect ); } /** @@ -1278,14 +1273,23 @@ public static function print_admin_notice() { return; } - if ( isset( $_GET['amp_validate_error'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended - $error_codes = array_unique( array_map( 'sanitize_key', (array) $_GET['amp_validate_error'] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended - foreach ( $error_codes as $error_code ) { - printf( - '

%s

', - esc_html( AMP_Validation_Manager::get_validate_url_error_message( $error_code ) ), - esc_html__( 'Dismiss this notice.', 'amp' ) - ); + if ( isset( $_GET['amp_validate_error'] ) && is_string( $_GET['amp_validate_error'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended + // Note: The input var is validated by the unserialize_validation_error_messages method. + $errors = AMP_Validation_Manager::unserialize_validation_error_messages( wp_unslash( $_GET['amp_validate_error'] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended + if ( $errors ) { + foreach ( array_unique( $errors ) as $error_message ) { + printf( + '

%s

', + wp_kses( + $error_message, + [ + 'a' => array_fill_keys( [ 'href', 'target' ], true ), + 'code' => [], + ] + ), + esc_html__( 'Dismiss this notice.', 'amp' ) + ); + } } } @@ -1487,7 +1491,7 @@ public static function handle_validate_request() { $validity = AMP_Validation_Manager::validate_url( $url ); if ( is_wp_error( $validity ) ) { - throw new Exception( esc_html( $validity->get_error_code() ) ); + throw new Exception( AMP_Validation_Manager::get_validate_url_error_message( $validity->get_error_code(), $validity->get_error_message() ) ); } $errors = wp_list_pluck( $validity['results'], 'error' ); @@ -1502,7 +1506,7 @@ public static function handle_validate_request() { ) ); if ( is_wp_error( $stored ) ) { - throw new Exception( esc_html( $stored->get_error_code() ) ); + throw new Exception( AMP_Validation_Manager::get_validate_url_error_message( $stored->get_error_code(), $stored->get_error_message() ) ); } $redirect = get_edit_post_link( $stored, 'raw' ); @@ -1518,7 +1522,9 @@ static function ( $error ) { $args[ self::URLS_TESTED ] = '1'; $args[ self::REMAINING_ERRORS ] = $error_count; } catch ( Exception $e ) { - $args['amp_validate_error'] = $e->getMessage(); + $args['amp_validate_error'] = AMP_Validation_Manager::serialize_validation_error_messages( + [ $e->getMessage() ] + ); $args[ self::URLS_TESTED ] = '0'; if ( $post && self::POST_TYPE_SLUG === $post->post_type ) { @@ -1533,7 +1539,7 @@ static function ( $error ) { } } - wp_safe_redirect( add_query_arg( $args, $redirect ) ); + wp_safe_redirect( add_query_arg( rawurlencode_deep( $args ), $redirect ) ); exit(); } @@ -2073,7 +2079,7 @@ public static function get_recheck_url( $url_or_post ) { } return wp_nonce_url( - add_query_arg( $args, admin_url() ), + add_query_arg( rawurlencode_deep( $args ), admin_url() ), self::NONCE_ACTION ); } diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index a5d74659708..25f9aa93eb5 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1874,17 +1874,14 @@ public static function validate_url( $url ) { $response = wp_remote_retrieve_body( $r ); if ( trim( $response ) === '' ) { - $error_code = 'white_screen_of_death'; - return new WP_Error( $error_code, self::get_validate_url_error_message( $error_code ) ); + return new WP_Error( 'white_screen_of_death' ); } if ( ! preg_match( '#.*?#s', $response, $matches ) ) { - $error_code = 'response_comment_absent'; - return new WP_Error( $error_code, self::get_validate_url_error_message( $error_code ) ); + return new WP_Error( 'response_comment_absent' ); } $validation = json_decode( $matches[1], true ); if ( json_last_error() || ! isset( $validation['results'] ) || ! is_array( $validation['results'] ) ) { - $error_code = 'malformed_json_validation_errors'; - return new WP_Error( $error_code, self::get_validate_url_error_message( $error_code ) ); + return new WP_Error( 'malformed_json_validation_errors' ); } return array_merge( @@ -1893,38 +1890,105 @@ public static function validate_url( $url ) { ); } + /** + * Serialize validation error messages. + * + * In order to safely pass validation error messages through redirects with query parameters, they must be serialized + * with a HMAC for security. The messages contain markup so the HMAC prevents tampering. + * + * @since 1.4.2 + * @see AMP_Validation_Manager::unserialize_validation_error_messages() + * + * @param string[] $messages Messages. + * @return string Serialized. + */ + public static function serialize_validation_error_messages( $messages ) { + $encoded_messages = base64_encode( wp_json_encode( array_unique( $messages ) ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode + return wp_hash( $encoded_messages ) . ':' . $encoded_messages; + } + + /** + * Unserialize validation error messages. + * + * @since 1.4.2 + * @see AMP_Validation_Manager::serialize_validation_error_messages() + * + * @param string $serialized Serialized messages. + * @return string[]|null + */ + public static function unserialize_validation_error_messages( $serialized ) { + $parts = explode( ':', $serialized, 2 ); + if ( count( $parts ) !== 2 || wp_hash( $parts[1] ) !== $parts[0] ) { + return null; + } + return json_decode( base64_decode( $parts[1] ), true ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode + } + /** * Get error message for a validate URL failure. * - * @param string $error_code Error code. - * @return string Error message. + * @param string $error_code Error code. + * @param string $error_message Error message, typically technical such as from HTTP status text or cURL error message. + * @return string Error message with HTML markup. */ - public static function get_validate_url_error_message( $error_code ) { - $check_error_log = __( 'Please check your server\'s PHP error logs; to do this you may need to enable WP_DEBUG_LOG.', 'amp' ); + public static function get_validate_url_error_message( $error_code, $error_message = '' ) { + $check_error_log = sprintf( + /* translators: %s is link to Debugging in WordPress */ + ' ' . __( 'Please check your server PHP error logs; to do this you may need to enable WP_DEBUG_LOG.', 'amp' ), + esc_url( 'https://wordpress.org/support/article/debugging-in-wordpress/' ) + ); + + if ( $error_message ) { + $error_message = ' ' . rtrim( $error_message, '.' ) . '.'; + } + + $support_forum_message = ' ' . sprintf( + /* translators: %1$s: Link to support forum. %2$s: Link to new topic form in support forum. */ + __( 'If you are stuck, please search the support forum for possible related topics, or otherwise start a new support topic including the error message, the URL to your site, and your active theme/plugins.', 'amp' ), + esc_url( 'https://wordpress.org/support/plugin/amp/' ), + esc_url( 'https://wordpress.org/support/plugin/amp/#new-topic-0' ) + ); + + $site_health_message = ''; + if ( version_compare( get_bloginfo( 'version' ), '5.2', '>=' ) ) { + $site_health_message .= ' ' . sprintf( + /* translators: %s is link to Site Health */ + __( 'Please check your Site Health to verify it can perform loopback requests.', 'amp' ), + esc_url( admin_url( 'site-health.php' ) ) + ); + $support_forum_message .= ' ' . sprintf( + /* translators: %s is the URL to Site Health Info. */ + __( 'Please include your Site Health Info.', 'amp' ), + esc_url( admin_url( 'site-health.php?tab=debug' ) ) + ); + } + switch ( $error_code ) { case 'http_request_failed': - return __( 'Failed to fetch URL(s) to validate. This may be due to a request timeout.', 'amp' ) . ' ' . $check_error_log; + return __( 'Failed to fetch URL to validate.', 'amp' ) . $error_message . $site_health_message . $support_forum_message; case 'white_screen_of_death': - return __( 'Unable to validate URL. Encountered a white screen of death likely due to a fatal error.', 'amp' ) . ' ' . $check_error_log; + return __( 'Unable to validate URL. Encountered a white screen of death likely due to a PHP fatal error.', 'amp' ) . $error_message . $check_error_log . $support_forum_message; case '404': - return __( 'The fetched URL was not found. It may have been deleted. If so, you can trash this.', 'amp' ); + return __( 'The fetched URL was not found. It may have been deleted. If so, you can trash this.', 'amp' ) . $error_message . $support_forum_message; case '500': - return __( 'An internal server error occurred when fetching the URL for validation.', 'amp' ) . ' ' . $check_error_log; + return __( 'An internal server error occurred when fetching the URL for validation.', 'amp' ) . $error_message . $check_error_log . $support_forum_message; case 'response_comment_absent': return sprintf( - /* translators: %s: AMP_VALIDATION */ - __( 'URL validation failed to due to the absence of the expected JSON-containing %s comment after the body. This is often due to a PHP fatal error occurring.', 'amp' ), - 'AMP_VALIDATION' - ) . ' ' . $check_error_log; + /* translators: %1$s: AMP_VALIDATION, %2$s: */ + __( 'URL validation failed to due to the absence of the expected JSON-containing %1$s HTML comment after %2$s. This is often due to a PHP fatal error occurring.', 'amp' ), + 'AMP_VALIDATION', + '</body>' + ) . $error_message . $check_error_log . $support_forum_message; case 'malformed_json_validation_errors': return sprintf( - /* translators: %s: AMP_VALIDATION */ - __( 'URL validation failed to due to unexpected JSON in the %s comment after the body.', 'amp' ), - 'AMP_VALIDATION' - ); + /* translators: %1$s: AMP_VALIDATION, %2$s: */ + __( 'URL validation failed to due to unexpected JSON in the %1$s HTML comment after %2$s.', 'amp' ), + 'AMP_VALIDATION', + '</body>' + ) . $error_message . $support_forum_message; default: /* translators: %s is error code */ - return sprintf( __( 'URL validation failed. Error code: %s.', 'amp' ), $error_code ); // Note that $error_code has been sanitized with sanitize_key(); will be escaped below as well. + return sprintf( __( 'URL validation failed. Error code: %s.', 'amp' ), $error_code ) . $error_message . $support_forum_message; } } diff --git a/tests/php/validation/test-class-amp-validated-url-post-type.php b/tests/php/validation/test-class-amp-validated-url-post-type.php index 024bbe06c9b..8a7478918d0 100644 --- a/tests/php/validation/test-class-amp-validated-url-post-type.php +++ b/tests/php/validation/test-class-amp-validated-url-post-type.php @@ -265,7 +265,10 @@ public function test_get_invalid_url_post() { // Test normalized. $args = [ 'normalize' => false ]; - $url = add_query_arg( 'utm_foo', 'bar', get_permalink( $post ) . '#baz' ); + $url = add_query_arg( + array_fill_keys( wp_removable_query_args(), 'true' ), + get_permalink( $post ) . '#baz' + ); $url = set_url_scheme( $url, 'http' ); $this->assertNull( AMP_Validated_URL_Post_Type::get_invalid_url_post( $url, $args ) ); $args = [ 'normalize' => true ]; @@ -805,14 +808,8 @@ static function() { ]; } ); - $this->assertEquals( - add_query_arg( - [ - AMP_Validated_URL_Post_Type::URLS_TESTED => $urls_tested, - 'amp_validate_error' => [ 'response_comment_absent' ], - ], - $initial_redirect - ), + $this->assertContains( + 'amp_validate_error=', AMP_Validated_URL_Post_Type::handle_bulk_action( $initial_redirect, AMP_Validated_URL_Post_Type::BULK_VALIDATE_ACTION, $items ) ); } @@ -821,6 +818,8 @@ static function() { * Test for print_admin_notice() * * @covers \AMP_Validated_URL_Post_Type::print_admin_notice() + * @covers \AMP_Validation_Manager::serialize_validation_error_messages() + * @covers \AMP_Validation_Manager::unserialize_validation_error_messages() */ public function test_print_admin_notice() { add_theme_support( AMP_Theme_Support::SLUG ); @@ -853,9 +852,10 @@ public function test_print_admin_notice() { $output = get_echo( [ 'AMP_Validated_URL_Post_Type', 'print_admin_notice' ] ); $this->assertContains( 'The rechecked URL is free of non-removed invalid markup.', $output ); - $_GET['amp_validate_error'] = [ 'http_request_failed' ]; + $error_message = 'Something bad happened!'; + $_GET['amp_validate_error'] = AMP_Validation_Manager::serialize_validation_error_messages( [ $error_message ] ); $output = get_echo( [ 'AMP_Validated_URL_Post_Type', 'print_admin_notice' ] ); - $this->assertContains( 'Failed to fetch URL(s) to validate', $output ); + $this->assertContains( $error_message, $output ); unset( $GLOBALS['current_screen'] ); } @@ -926,8 +926,8 @@ static function( $error ) { $exception = $handle_validate_request(); $this->assertInstanceOf( 'Exception', $exception ); $this->assertEquals( 302, $exception->getCode() ); - $this->assertStringEndsWith( - '/edit.php?post_type=amp_validated_url&_validate_error=missing_url&_urls_tested=0', + $this->assertContains( + '/edit.php?post_type=amp_validated_url&_validate_error=', $exception->getMessage() ); unset( $_GET['post'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended @@ -937,8 +937,8 @@ static function( $error ) { $exception = $handle_validate_request(); $this->assertInstanceOf( 'Exception', $exception ); $this->assertEquals( 302, $exception->getCode() ); - $this->assertStringEndsWith( - '/edit.php?post_type=amp_validated_url&_validate_error=invalid_post&_urls_tested=0', + $this->assertContains( + '/edit.php?post_type=amp_validated_url&_validate_error=', $exception->getMessage() ); unset( $_GET['post'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended @@ -948,8 +948,8 @@ static function( $error ) { $exception = $handle_validate_request(); $this->assertInstanceOf( 'Exception', $exception ); $this->assertEquals( 302, $exception->getCode() ); - $this->assertStringEndsWith( - '/edit.php?post_type=amp_validated_url&_validate_error=invalid_post&_urls_tested=0', + $this->assertContains( + '/edit.php?post_type=amp_validated_url&_validate_error=', $exception->getMessage() ); unset( $_GET['post'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended @@ -959,8 +959,8 @@ static function( $error ) { $exception = $handle_validate_request(); $this->assertInstanceOf( 'Exception', $exception ); $this->assertEquals( 302, $exception->getCode() ); - $this->assertStringEndsWith( - sprintf( 'post.php?post=%s&action=edit&_urls_tested=1&_remaining_errors=2', $post_id ), + $this->assertContains( + sprintf( 'post.php?post=%s&action=edit&_urls_tested=', $post_id ), $exception->getMessage() ); unset( $_GET['post'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended @@ -990,8 +990,8 @@ static function( $error ) { $exception = $handle_validate_request(); $this->assertInstanceOf( 'Exception', $exception ); $this->assertEquals( 302, $exception->getCode() ); - $this->assertStringEndsWith( - 'wp-admin/edit.php?post_type=amp_validated_url&_validate_error=illegal_url&_urls_tested=0', + $this->assertContains( + 'wp-admin/edit.php?post_type=amp_validated_url&_validate_error=', $exception->getMessage() ); }