Add a separate filter for 'amp_validation_error_sanitized', add top-l…

…evel var The filter in accept_validation_errors() might not actually run, if get_acceptable_errors() doesn't return anything.
ampproject · Oct 4, 2019 · 19363fc · 19363fc
1 parent e738bcf
commit 19363fc
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 19 deletions.
diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php
@@ -95,6 +95,13 @@ class AMP_Theme_Support {
 	 */
 	const READER_MODE_TEMPLATE_DIRECTORY = 'amp';
 
+	/**
+	 * A top-level query var for AMP flags.
+	 *
+	 * @var string
+	 */
+	const AMP_FLAGS_QUERY_VAR = 'amp_flags';
+
 	/**
 	 * A query var to disable post processing.
 	 *
@@ -1893,7 +1900,7 @@ public static function start_output_buffering() {
 			newrelic_disable_autorum();
 		}
 
-		if ( isset( $_GET[ self::DISABLE_POST_PROCESSING_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap() ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
+		if ( isset( $_GET[ self::AMP_FLAGS_QUERY_VAR ][ self::DISABLE_POST_PROCESSING_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap() ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
 			return;
 		}
 
@@ -2026,7 +2033,7 @@ public static function prepare_response( $response, $args = [] ) {
 		 * @param bool $enable_response_caching Whether response caching is enabled.
 		 */
 		$enable_response_caching    = apply_filters( 'amp_response_caching_enabled', ! ( defined( 'WP_DEBUG' ) && WP_DEBUG ) || ! empty( $args['enable_response_caching'] ) );
-		$is_disabled_with_query_var = isset( $_GET[ self::DISABLE_RESPONSE_CACHE_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
+		$is_disabled_with_query_var = isset( $_GET[ self::AMP_FLAGS_QUERY_VAR ][ self::DISABLE_RESPONSE_CACHE_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
 
 		$enable_response_caching = (
 			$enable_response_caching
@@ -2316,13 +2323,7 @@ public static function prepare_response( $response, $args = [] ) {
 				}
 
 				// Remove debugging query args.
-				$non_amp_url = remove_query_arg(
-					[
-						self::DISABLE_RESPONSE_CACHE_QUERY_VAR,
-						AMP_Validation_Error_Taxonomy::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR,
-					],
-					$non_amp_url
-				);
+				$non_amp_url = remove_query_arg( self::AMP_FLAGS_QUERY_VAR, $non_amp_url );
 
 				/*
 				 * Temporary redirect because AMP page may return with blocking validation errors when auto-accepting sanitization
@@ -2369,7 +2370,7 @@ public static function prepare_response( $response, $args = [] ) {
 	 * @return bool Whether the prevent a redirect.
 	 */
 	public static function prevent_redirect_to_non_amp() {
-		return isset( $_GET[ self::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
+		return isset( $_GET[ self::AMP_FLAGS_QUERY_VAR ][ self::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
 	}
 
 	/**

diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php
@@ -281,6 +281,16 @@ public static function register() {
 		}
 
 		self::accept_validation_errors( AMP_Core_Theme_Sanitizer::get_acceptable_errors( get_template() ) );
+
+		if ( isset( $_GET[ AMP_Theme_Support::AMP_FLAGS_QUERY_VAR ][ self::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap() ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
+			add_filter(
+				'amp_validation_error_sanitized',
+				static function( $sanitized ) {
+					unset( $sanitized );
+					return false;
+				}
+			);
+		}
 	}
 
 	/**
@@ -519,10 +529,6 @@ static function( $sanitized, $error ) use ( $acceptable_errors ) {
 					return true;
 				}
 
-				if ( isset( $_GET[ self::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap() ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
-					return false;
-				}
-
 				if ( isset( $acceptable_errors[ $error['code'] ] ) ) {
 					if ( true === $acceptable_errors[ $error['code'] ] ) {
 						return true;

diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php
@@ -1447,7 +1447,7 @@ function newrelic_disable_autorum() {
 		$this->assertEquals( $initial_ob_level, ob_get_level() );
 
 		// When this query var is present, this method should exit early, and shouldn't buffer the output.
-		$_GET[ AMP_Theme_Support::DISABLE_POST_PROCESSING_QUERY_VAR ] = '';
+		$_GET[ AMP_Theme_Support::AMP_FLAGS_QUERY_VAR ][ AMP_Theme_Support::DISABLE_POST_PROCESSING_QUERY_VAR ] = '';
 		wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
 		$initial_ob_level = ob_get_level();
 		AMP_Theme_Support::start_output_buffering();
@@ -1741,7 +1741,7 @@ static function ( $url ) {
 		$this->reset_post_processor_cache_effectiveness();
 
 		// Test that the response is not cached if a certain query var is present.
-		$_GET[ AMP_Theme_Support::DISABLE_RESPONSE_CACHE_QUERY_VAR ] = '';
+		$_GET[ AMP_Theme_Support::AMP_FLAGS_QUERY_VAR ][ AMP_Theme_Support::DISABLE_RESPONSE_CACHE_QUERY_VAR ] = '';
 		wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
 		$call_prepare_response();
 		$server_timing_headers = $this->get_server_timing_headers();
@@ -2115,7 +2115,7 @@ public function test_prevent_redirect_to_non_amp() {
 		$this->assertFalse( AMP_Theme_Support::prevent_redirect_to_non_amp() );
 
 		// The query var is present, but the user doesn't have the right permission.
-		$_GET[ AMP_Theme_Support::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR ] = '';
+		$_GET[ AMP_Theme_Support::AMP_FLAGS_QUERY_VAR ][ AMP_Theme_Support::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR ] = '';
 		$this->assertFalse( AMP_Theme_Support::prevent_redirect_to_non_amp() );
 
 		// Now that the user has the right permission, this should be true.

diff --git a/tests/php/validation/test-class-amp-validation-error-taxonomy.php b/tests/php/validation/test-class-amp-validation-error-taxonomy.php
@@ -372,9 +372,9 @@ public function test_is_validation_error_sanitized_and_get_validation_error_sani
 		);
 
 		// New rejected => Ack rejected, as the query var should force this to be rejected.
-		$_GET[ AMP_Validation_Error_Taxonomy::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] = '';
+		$_GET[ AMP_Theme_Support::AMP_FLAGS_QUERY_VAR ][ AMP_Validation_Error_Taxonomy::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] = '';
 		wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
-		AMP_Validation_Error_Taxonomy::accept_validation_errors( [ self::MOCK_ACCEPTABLE_ERROR => true ] );
+		AMP_Validation_Error_Taxonomy::register();
 		$this->assertfalse( AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $error_foo ) );
 		$this->assertEquals(
 			[