From c1b40f9faaf53ccc3f4f664de0a6ee1c5fc9ea18 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Fri, 31 Mar 2023 13:49:50 +0530 Subject: [PATCH 01/18] Update `amp_is_dev_mode()` to return `true` for localhost without HTTPS. --- includes/amp-helper-functions.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index cdd40a28278..56dcb983791 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1450,6 +1450,16 @@ function amp_is_dev_mode() { ( is_admin_bar_showing() && is_user_logged_in() ) || is_customize_preview() + || + ( + ! is_ssl() + && + ( + ( function_exists( 'wp_get_environment_type' ) && 'local' === wp_get_environment_type() ) + || + 'localhost' === wp_parse_url( home_url(), PHP_URL_HOST ) + ) + ) ) ); } From dc571aad90f9177e1a738d1cdc49c85c0b2a5548 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 00:12:09 +0530 Subject: [PATCH 02/18] Add new `allow_localhost_http_protocol` arg in `AMP_Tag_And_Attribute_Sanitizer::class` --- includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php | 1 + 1 file changed, 1 insertion(+) diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php index d8ff0a9d0ce..a8a51232efd 100644 --- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php +++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php @@ -184,6 +184,7 @@ public function __construct( $dom, $args = [] ) { 'amp_globally_allowed_attributes' => AMP_Allowed_Tags_Generated::get_allowed_attributes(), 'amp_layout_allowed_attributes' => AMP_Allowed_Tags_Generated::get_layout_attributes(), 'prefer_bento' => false, + 'allow_localhost_http_protocol' => false, ]; parent::__construct( $dom, $args ); From be2728b13644eb5246c2383e2afc053d09dd176e Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 00:14:03 +0530 Subject: [PATCH 03/18] Add `allow_localhost_http_protocol` arg to `AMP_Tag_And_Attribute_Sanitizer` class init --- includes/amp-helper-functions.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 56dcb983791..cee951df7eb 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1600,7 +1600,8 @@ function amp_get_content_sanitizers( $post = null ) { AMP_Accessibility_Sanitizer::class => [], // Note: This validating sanitizer must come at the end to clean up any remaining issues the other sanitizers didn't catch. AMP_Tag_And_Attribute_Sanitizer::class => [ - 'prefer_bento' => amp_is_bento_enabled(), + 'prefer_bento' => amp_is_bento_enabled(), + 'allow_localhost_http_protocol' => amp_is_dev_mode(), ], ]; From e8c6261d0546112f23d1f4a4cca36c1267102630 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 01:03:09 +0530 Subject: [PATCH 04/18] Allow http in AMP markup if host is localhost --- .../class-amp-tag-and-attribute-sanitizer.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php index a8a51232efd..1b83ad4c485 100644 --- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php +++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php @@ -92,6 +92,13 @@ class AMP_Tag_And_Attribute_Sanitizer extends AMP_Base_Sanitizer { const SPECIFIED_LAYOUT_INVALID = 'SPECIFIED_LAYOUT_INVALID'; const WRONG_PARENT_TAG = 'WRONG_PARENT_TAG'; + /** + * Key for localhost. + * + * @var string + */ + const LOCALHOST = 'localhost'; + /** * Allowed tags. * @@ -1940,7 +1947,9 @@ private function check_attr_spec_rule_allowed_protocol( DOMElement $node, $attr_ if ( $node->hasAttribute( $attr_name ) ) { foreach ( $this->extract_attribute_urls( $node->getAttributeNode( $attr_name ) ) as $url ) { $url_scheme = $this->parse_protocol( $this->normalize_url_from_attribute_value( $url ) ); - if ( isset( $url_scheme ) && ! in_array( strtolower( $url_scheme ), $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOWED_PROTOCOL ], true ) ) { + if ( self::LOCALHOST === wp_parse_url( $url, PHP_URL_HOST ) && $this->args['allow_localhost_http_protocol'] ) { + return AMP_Rule_Spec::PASS; + } elseif ( isset( $url_scheme ) && ! in_array( strtolower( $url_scheme ), $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOWED_PROTOCOL ], true ) ) { return AMP_Rule_Spec::FAIL; } } @@ -1952,7 +1961,9 @@ private function check_attr_spec_rule_allowed_protocol( DOMElement $node, $attr_ if ( $node->hasAttribute( $alternative_name ) ) { foreach ( $this->extract_attribute_urls( $node->getAttributeNode( $alternative_name ), $attr_name ) as $url ) { $url_scheme = $this->parse_protocol( $this->normalize_url_from_attribute_value( $url ) ); - if ( isset( $url_scheme ) && ! in_array( strtolower( $url_scheme ), $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOWED_PROTOCOL ], true ) ) { + if ( self::LOCALHOST === wp_parse_url( $url, PHP_URL_HOST ) && $this->args['allow_localhost_http_protocol'] ) { + return AMP_Rule_Spec::PASS; + } elseif ( isset( $url_scheme ) && ! in_array( strtolower( $url_scheme ), $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOWED_PROTOCOL ], true ) ) { return AMP_Rule_Spec::FAIL; } } From 26be9cf81d788e7fad27c6350a32b17699ad0a92 Mon Sep 17 00:00:00 2001 From: Lovekesh Kumar Date: Sat, 1 Apr 2023 01:16:39 +0530 Subject: [PATCH 05/18] Fix conditionals code quality Co-authored-by: Weston Ruter --- includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php index 1b83ad4c485..47069101e68 100644 --- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php +++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php @@ -1947,7 +1947,7 @@ private function check_attr_spec_rule_allowed_protocol( DOMElement $node, $attr_ if ( $node->hasAttribute( $attr_name ) ) { foreach ( $this->extract_attribute_urls( $node->getAttributeNode( $attr_name ) ) as $url ) { $url_scheme = $this->parse_protocol( $this->normalize_url_from_attribute_value( $url ) ); - if ( self::LOCALHOST === wp_parse_url( $url, PHP_URL_HOST ) && $this->args['allow_localhost_http_protocol'] ) { + if ( $this->args['allow_localhost_http_protocol'] && self::LOCALHOST === wp_parse_url( $url, PHP_URL_HOST ) ) { return AMP_Rule_Spec::PASS; } elseif ( isset( $url_scheme ) && ! in_array( strtolower( $url_scheme ), $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOWED_PROTOCOL ], true ) ) { return AMP_Rule_Spec::FAIL; @@ -1961,7 +1961,7 @@ private function check_attr_spec_rule_allowed_protocol( DOMElement $node, $attr_ if ( $node->hasAttribute( $alternative_name ) ) { foreach ( $this->extract_attribute_urls( $node->getAttributeNode( $alternative_name ), $attr_name ) as $url ) { $url_scheme = $this->parse_protocol( $this->normalize_url_from_attribute_value( $url ) ); - if ( self::LOCALHOST === wp_parse_url( $url, PHP_URL_HOST ) && $this->args['allow_localhost_http_protocol'] ) { + if ( $this->args['allow_localhost_http_protocol'] && self::LOCALHOST === wp_parse_url( $url, PHP_URL_HOST ) ) { return AMP_Rule_Spec::PASS; } elseif ( isset( $url_scheme ) && ! in_array( strtolower( $url_scheme ), $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOWED_PROTOCOL ], true ) ) { return AMP_Rule_Spec::FAIL; From de17161bde6fdab959771238ddc18639e4a5f85d Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 01:19:48 +0530 Subject: [PATCH 06/18] Store multiple call for `amp_is_dev_mode()` in a variable --- includes/amp-helper-functions.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index cee951df7eb..4d4f327a528 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1532,6 +1532,7 @@ function amp_get_content_sanitizers( $post = null ) { AMP_Theme_Support::TRANSITIONAL_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) ); + $is_dev_mode = amp_is_dev_mode(); $native_img_used = amp_is_native_img_used(); $sanitizers = [ @@ -1601,7 +1602,7 @@ function amp_get_content_sanitizers( $post = null ) { // Note: This validating sanitizer must come at the end to clean up any remaining issues the other sanitizers didn't catch. AMP_Tag_And_Attribute_Sanitizer::class => [ 'prefer_bento' => amp_is_bento_enabled(), - 'allow_localhost_http_protocol' => amp_is_dev_mode(), + 'allow_localhost_http_protocol' => $is_dev_mode, ], ]; @@ -1662,7 +1663,7 @@ function amp_get_content_sanitizers( $post = null ) { */ $sanitizers = apply_filters( 'amp_content_sanitizers', $sanitizers, $post ); - if ( amp_is_dev_mode() ) { + if ( $is_dev_mode ) { /** * Filters the XPath queries for elements that should be enabled for dev mode. * From 9cae8d92f1c2f14d987550935376e1eaa955ac72 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 01:44:27 +0530 Subject: [PATCH 07/18] Update test cases for `amp_is_dev_mode()`; add test case for enabling dev mode when using localhost --- tests/php/test-amp-helper-functions.php | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index 4e21a562bde..8b9f2915143 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -1897,6 +1897,10 @@ public function test_amp_get_content_embed_handlers() { public function test_amp_is_dev_mode() { AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + // `amp_is_dev_mode()` uses `is_ssl()` so enable HTTPS, so it doesn't interfere with the usage of + // dev mode while showing admin bar or customizer screen. + $_SERVER['HTTPS'] = 'on'; + $this->assertFalse( amp_is_dev_mode() ); add_filter( 'amp_dev_mode_enabled', '__return_true' ); $this->assertTrue( amp_is_dev_mode() ); @@ -1916,6 +1920,17 @@ public function test_amp_is_dev_mode() { $this->assertFalse( is_user_logged_in() ); $this->assertTrue( is_admin_bar_showing() ); $this->assertFalse( amp_is_dev_mode() ); + + // Test on localhost with HTTP. + $_SERVER['HTTPS'] = false; + + $this->assertFalse( is_ssl() ); + $this->assertTrue( amp_is_dev_mode() ); + $this->assertTrue( 'localhost' === wp_parse_url( home_url(), PHP_URL_HOST ) ); + + if ( function_exists( 'wp_get_environment_type' ) ) { + $this->assertEquals( 'local', wp_get_environment_type() ); + } } /** @@ -1958,6 +1973,10 @@ public function test_amp_get_content_sanitizers() { $post = self::factory()->post->create_and_get(); add_filter( 'amp_content_sanitizers', [ $this, 'capture_filter_call' ], 10, 2 ); + // `amp_is_dev_mode()` uses `is_ssl()` so enable HTTPS, so it doesn't interfere with the usage of + // dev mode while showing admin bar or customizer screen. + $_SERVER['HTTPS'] = 'on'; + $this->last_filter_call = null; AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); $handlers = amp_get_content_sanitizers(); @@ -2034,6 +2053,10 @@ function ( $xpaths ) use ( $element_xpaths ) { } ); + // `amp_is_dev_mode()` uses `is_ssl()` so enable HTTPS, so it doesn't interfere with the usage of + // dev mode while showing admin bar or customizer screen. + $_SERVER['HTTPS'] = 'on'; + // Check that AMP_Dev_Mode_Sanitizer is not registered if not in dev mode. $sanitizers = amp_get_content_sanitizers(); $this->assertFalse( amp_is_dev_mode() ); From befc6eda8923377312cb803ff1b0f74cc224c70b Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 03:35:32 +0530 Subject: [PATCH 08/18] Define debug constants in PHPUnit tests --- .github/workflows/build-test-measure.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/build-test-measure.yml b/.github/workflows/build-test-measure.yml index 5e09eace199..9464f0d898b 100644 --- a/.github/workflows/build-test-measure.yml +++ b/.github/workflows/build-test-measure.yml @@ -542,6 +542,10 @@ jobs: if: needs.pre-run.outputs.changed-php-count > 0 run: cp -r "$PWD" "$WP_CORE_DIR/src/wp-content/plugins/amp" + - name: Setup required WP constants + run: | + echo "WP_ENVIRONMENT_TYPE=local" >> $GITHUB_ENV + - name: Run tests if: ${{ matrix.coverage == false && needs.pre-run.outputs.changed-php-count > 0 }} run: vendor/bin/phpunit --verbose From 695ebb28d3b0b9c2bdbeb173734415605d9a9d65 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 03:39:28 +0530 Subject: [PATCH 09/18] Update `home_url()` for `test_amp_is_dev_mode()` --- tests/php/test-amp-helper-functions.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index 8b9f2915143..def36f97732 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -1924,6 +1924,13 @@ public function test_amp_is_dev_mode() { // Test on localhost with HTTP. $_SERVER['HTTPS'] = false; + add_filter( + 'home_url', + static function () { + return 'http://localhost'; + } + ); + $this->assertFalse( is_ssl() ); $this->assertTrue( amp_is_dev_mode() ); $this->assertTrue( 'localhost' === wp_parse_url( home_url(), PHP_URL_HOST ) ); From bb9569773ad0418d7257fed69337010b7b081d5e Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 04:12:02 +0530 Subject: [PATCH 10/18] Add tests for `check_attr_spec_rule_allowed_protocol()` when `allow_localhost_http_protocol` is passed --- ...nd-attribute-sanitizer-private-methods.php | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php b/tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php index a67a78f4995..5948e7d61c8 100644 --- a/tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php +++ b/tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php @@ -5,6 +5,8 @@ use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; use AmpProject\AmpWP\Tests\TestCase; +use AmpProject\Html\Attribute; +use AmpProject\Validator\Spec\Tag\Html; class AMP_Tag_And_Attribute_Sanitizer_Attr_Spec_Rules_Test extends TestCase { @@ -1788,6 +1790,52 @@ public function test_check_attr_spec_rule_allowed_protocol( $data, $expected ) { $this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) ); } + /** + * Test `check_attr_spec_rule_allowed_protocol()` with `allow_localhost_http_protocol` arg. + * + * @covers AMP_Tag_And_Attribute_Sanitizer::check_attr_spec_rule_allowed_protocol() + * @group allowed-tags-private-methods + */ + public function test_check_attr_spec_rule_allowed_protocol_with_allow_localhost_http_protocol() { + $dom = AMP_DOM_Utils::get_dom_from_content( + ' + + ' + ); + $node = $dom->getElementsByTagName( 'amp-autocomplete' )->item( 0 ); + $attr = Attribute::SRC; + $attr_spec_rule = [ + 'value_url' => [ + 'allow_relative' => true, + 'protocol' => [ + 'https', + ], + ], + ]; + $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( + $dom, + [ + 'allow_localhost_http_protocol' => true, + ] + ); + + $got = $this->call_private_method( $sanitizer, 'check_attr_spec_rule_allowed_protocol', [ $node, $attr, $attr_spec_rule ] ); + + $this->assertEquals( AMP_Rule_Spec::PASS, $got ); + + $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( + $dom, + [ + 'allow_localhost_http_protocol' => false, + ] + ); + + $got = $this->call_private_method( $sanitizer, 'check_attr_spec_rule_allowed_protocol', [ $node, $attr, $attr_spec_rule ] ); + + $this->assertEquals( AMP_Rule_Spec::FAIL, $got ); + } + public function get_check_attr_spec_rule_disallowed_relative() { return [ 'no_attributes' => [ From 73a2b746cdcc1f286d22f6004a06fd91536532b1 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 13:45:16 +0530 Subject: [PATCH 11/18] Add npm:script to generate PHPUnit html coverage report --- package.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 50ad1bdc365..a19c855f0d3 100644 --- a/package.json +++ b/package.json @@ -144,6 +144,7 @@ "test:js:update-snapshots": "npm run test:js -- --updateSnapshot", "test:js:watch": "npm run test:js -- --watch", "test:php": "wp-env run phpunit 'env WP_PHPUNIT__TESTS_CONFIG=/wordpress-phpunit/wp-tests-config.php WORDPRESS_TABLE_PREFIX=wptests_ WP_TESTS_DIR=/var/www/wordpress-develop/tests/phpunit /var/www/html/wp-content/plugins/amp/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/amp/phpunit.xml.dist $npm_config_args'", + "test:php:coverage": "wp-env run tests-wordpress 'env WP_TESTS_DIR=/var/www/wordpress-develop/tests/phpunit WP_PHPUNIT__TESTS_CONFIG=/wordpress-phpunit/wp-tests-config.php /var/www/html/wp-content/plugins/amp/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/amp/phpunit.xml.dist --coverage-html /var/www/html/wp-content/plugins/amp/coverage'", "test:php:multisite": "wp-env run phpunit 'env WP_MULTISITE=1 WP_PHPUNIT__TESTS_CONFIG=/wordpress-phpunit/wp-tests-config.php WORDPRESS_TABLE_PREFIX=wptests_ WP_TESTS_DIR=/var/www/wordpress-develop/tests/phpunit /var/www/html/wp-content/plugins/amp/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/amp/phpunit.xml.dist $npm_config_args'", "test:php:xdebug": "wp-env run tests-wordpress 'env PHP_IDE_CONFIG=serverName=localhost WORDPRESS_TABLE_PREFIX=wptests_ WP_TESTS_DIR=/var/www/wordpress-develop/tests/phpunit WP_PHPUNIT__TESTS_CONFIG=/wordpress-phpunit/wp-tests-config.php /var/www/html/wp-content/plugins/amp/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/amp/phpunit.xml.dist $npm_config_args'", "test:php:help": "npm run test:php --args='--help'", @@ -159,4 +160,4 @@ } }, "title": "AMP for WordPress" -} +} \ No newline at end of file From c0539bea39c245c1a8420e671075965b5928721f Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Sat, 1 Apr 2023 13:46:22 +0530 Subject: [PATCH 12/18] Improve test coverage for `::test_check_attr_spec_rule_allowed_protocol_with_allow_localhost_http_protocol()` --- ...nd-attribute-sanitizer-private-methods.php | 193 +++++++++++++++--- 1 file changed, 162 insertions(+), 31 deletions(-) diff --git a/tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php b/tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php index 5948e7d61c8..3b3d853ddd5 100644 --- a/tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php +++ b/tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php @@ -1790,50 +1790,181 @@ public function test_check_attr_spec_rule_allowed_protocol( $data, $expected ) { $this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) ); } + /** @return array */ + public function get_check_attr_spec_rule_allowed_protocol_with_allow_localhost_http_protocol() { + return [ + 'protocol_pass_and_allow_localhost_http_protocol' => [ + [ + 'source' => '
', + 'node_tag_name' => 'div', + 'attr_name' => 'attribute1', + 'attr_spec_rule' => [ + 'value_url' => [ + 'protocol' => [ + 'https', + ], + ], + ], + ], + AMP_Rule_Spec::PASS, + true, + ], + 'protocol_alternative_pass_and_allow_localhost_http_protocol' => [ + [ + 'source' => '
', + 'node_tag_name' => 'div', + 'attr_name' => 'attribute1', + 'attr_spec_rule' => [ + 'alternative_names' => [ + 'attribute1_alternative1', + ], + 'value_url' => [ + 'protocol' => [ + 'https', + ], + ], + ], + ], + AMP_Rule_Spec::PASS, + true, + ], + 'protocol_pass_and_allow_localhost_http_protocol_with_cross_origin_url' => [ + [ + 'source' => '
', + 'node_tag_name' => 'div', + 'attr_name' => 'attribute1', + 'attr_spec_rule' => [ + 'value_url' => [ + 'protocol' => [ + 'https', + ], + ], + ], + ], + AMP_Rule_Spec::FAIL, + true, + ], + 'protocol_alternative_pass_and_allow_localhost_http_protocol_with_cross_origin_url' => [ + [ + 'source' => '
', + 'node_tag_name' => 'div', + 'attr_name' => 'attribute1', + 'attr_spec_rule' => [ + 'alternative_names' => [ + 'attribute1_alternative1', + ], + 'value_url' => [ + 'protocol' => [ + 'https', + ], + ], + ], + ], + AMP_Rule_Spec::FAIL, + true, + ], + 'protocol_pass_and_disallow_localhost_http_protocol' => [ + [ + 'source' => '
', + 'node_tag_name' => 'div', + 'attr_name' => 'attribute1', + 'attr_spec_rule' => [ + 'value_url' => [ + 'protocol' => [ + 'https', + ], + ], + ], + ], + AMP_Rule_Spec::FAIL, + false, + ], + 'protocol_alternative_pass_and_disallow_localhost_http_protocol' => [ + [ + 'source' => '
', + 'node_tag_name' => 'div', + 'attr_name' => 'attribute1', + 'attr_spec_rule' => [ + 'alternative_names' => [ + 'attribute1_alternative1', + ], + 'value_url' => [ + 'protocol' => [ + 'https', + ], + ], + ], + ], + AMP_Rule_Spec::FAIL, + false, + ], + 'protocol_pass_and_disallow_localhost_http_protocol_with_cross_origin_url' => [ + [ + 'source' => '
', + 'node_tag_name' => 'div', + 'attr_name' => 'attribute1', + 'attr_spec_rule' => [ + 'value_url' => [ + 'protocol' => [ + 'https', + ], + ], + ], + ], + AMP_Rule_Spec::FAIL, + false, + ], + 'protocol_alternative_pass_and_disallow_localhost_http_protocol_with_cross_origin_url' => [ + [ + 'source' => '
', + 'node_tag_name' => 'div', + 'attr_name' => 'attribute1', + 'attr_spec_rule' => [ + 'alternative_names' => [ + 'attribute1_alternative1', + ], + 'value_url' => [ + 'protocol' => [ + 'https', + ], + ], + ], + ], + AMP_Rule_Spec::FAIL, + false, + ], + ]; + } + /** * Test `check_attr_spec_rule_allowed_protocol()` with `allow_localhost_http_protocol` arg. * + * @dataProvider get_check_attr_spec_rule_allowed_protocol_with_allow_localhost_http_protocol + * * @covers AMP_Tag_And_Attribute_Sanitizer::check_attr_spec_rule_allowed_protocol() * @group allowed-tags-private-methods + * + * @param array $data The data. + * @param int $expected The expected result. + * @param bool $allow_localhost_http_protocol Whether to allow localhost http protocol. */ - public function test_check_attr_spec_rule_allowed_protocol_with_allow_localhost_http_protocol() { - $dom = AMP_DOM_Utils::get_dom_from_content( - ' - - ' - ); - $node = $dom->getElementsByTagName( 'amp-autocomplete' )->item( 0 ); - $attr = Attribute::SRC; - $attr_spec_rule = [ - 'value_url' => [ - 'allow_relative' => true, - 'protocol' => [ - 'https', - ], - ], - ]; - $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( - $dom, - [ - 'allow_localhost_http_protocol' => true, - ] - ); - - $got = $this->call_private_method( $sanitizer, 'check_attr_spec_rule_allowed_protocol', [ $node, $attr, $attr_spec_rule ] ); - - $this->assertEquals( AMP_Rule_Spec::PASS, $got ); - + public function test_check_attr_spec_rule_allowed_protocol_with_allow_localhost_http_protocol( + $data, + $expected, + $allow_localhost_http_protocol + ) { + $dom = AMP_DOM_Utils::get_dom_from_content( $data['source'] ); + $node = $dom->getElementsByTagName( $data['node_tag_name'] )->item( 0 ); $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, [ - 'allow_localhost_http_protocol' => false, + 'allow_localhost_http_protocol' => $allow_localhost_http_protocol, ] ); - $got = $this->call_private_method( $sanitizer, 'check_attr_spec_rule_allowed_protocol', [ $node, $attr, $attr_spec_rule ] ); + $got = $this->call_private_method( $sanitizer, 'check_attr_spec_rule_allowed_protocol', [ $node, $data['attr_name'], $data['attr_spec_rule'] ] ); - $this->assertEquals( AMP_Rule_Spec::FAIL, $got ); + $this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) ); } public function get_check_attr_spec_rule_disallowed_relative() { From 0aa7756a0692b4bf4ecb7d13f16f3557e016576b Mon Sep 17 00:00:00 2001 From: Lovekesh Kumar Date: Thu, 6 Apr 2023 00:00:51 +0530 Subject: [PATCH 13/18] Update `amp_is_dev_mode()` to take local env in account Co-authored-by: Weston Ruter --- includes/amp-helper-functions.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 4d4f327a528..104beba80e9 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1454,11 +1454,11 @@ function amp_is_dev_mode() { ( ! is_ssl() && - ( - ( function_exists( 'wp_get_environment_type' ) && 'local' === wp_get_environment_type() ) - || - 'localhost' === wp_parse_url( home_url(), PHP_URL_HOST ) - ) + function_exists( 'wp_get_environment_type' ) + && + 'local' === wp_get_environment_type() + && + 'localhost' === wp_parse_url( home_url(), PHP_URL_HOST ) ) ) ); From 6e9b8d0d69cc106393261aadc34419759298548c Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Thu, 6 Apr 2023 00:32:07 +0530 Subject: [PATCH 14/18] Update test case for `amp_is_dev_mode()` when detecting local env --- tests/php/test-amp-helper-functions.php | 40 ++++++++++--------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index def36f97732..89803ec6962 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -1897,10 +1897,6 @@ public function test_amp_get_content_embed_handlers() { public function test_amp_is_dev_mode() { AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); - // `amp_is_dev_mode()` uses `is_ssl()` so enable HTTPS, so it doesn't interfere with the usage of - // dev mode while showing admin bar or customizer screen. - $_SERVER['HTTPS'] = 'on'; - $this->assertFalse( amp_is_dev_mode() ); add_filter( 'amp_dev_mode_enabled', '__return_true' ); $this->assertTrue( amp_is_dev_mode() ); @@ -1921,22 +1917,24 @@ public function test_amp_is_dev_mode() { $this->assertTrue( is_admin_bar_showing() ); $this->assertFalse( amp_is_dev_mode() ); - // Test on localhost with HTTP. - $_SERVER['HTTPS'] = false; - - add_filter( - 'home_url', - static function () { - return 'http://localhost'; - } - ); + if ( function_exists( 'wp_get_environment_type' ) ) { + // Just to ensure is_ssl() is false. + $_SERVER['HTTPS'] = false; - $this->assertFalse( is_ssl() ); - $this->assertTrue( amp_is_dev_mode() ); - $this->assertTrue( 'localhost' === wp_parse_url( home_url(), PHP_URL_HOST ) ); + add_filter( + 'home_url', + static function () { + return 'http://localhost'; + } + ); - if ( function_exists( 'wp_get_environment_type' ) ) { + $this->assertFalse( is_ssl() ); + $this->assertTrue( amp_is_dev_mode() ); $this->assertEquals( 'local', wp_get_environment_type() ); + $this->assertTrue( 'localhost' === wp_parse_url( home_url(), PHP_URL_HOST ) ); + } else { + $this->assertFalse( amp_is_dev_mode() ); + $this->assertFalse( function_exists( 'wp_get_environment_type' ) ); } } @@ -1980,10 +1978,6 @@ public function test_amp_get_content_sanitizers() { $post = self::factory()->post->create_and_get(); add_filter( 'amp_content_sanitizers', [ $this, 'capture_filter_call' ], 10, 2 ); - // `amp_is_dev_mode()` uses `is_ssl()` so enable HTTPS, so it doesn't interfere with the usage of - // dev mode while showing admin bar or customizer screen. - $_SERVER['HTTPS'] = 'on'; - $this->last_filter_call = null; AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); $handlers = amp_get_content_sanitizers(); @@ -2060,10 +2054,6 @@ function ( $xpaths ) use ( $element_xpaths ) { } ); - // `amp_is_dev_mode()` uses `is_ssl()` so enable HTTPS, so it doesn't interfere with the usage of - // dev mode while showing admin bar or customizer screen. - $_SERVER['HTTPS'] = 'on'; - // Check that AMP_Dev_Mode_Sanitizer is not registered if not in dev mode. $sanitizers = amp_get_content_sanitizers(); $this->assertFalse( amp_is_dev_mode() ); From 52a041887360a4f11cb343d6db963127f745d43f Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Thu, 6 Apr 2023 14:01:21 +0530 Subject: [PATCH 15/18] Fix `::test_amp_is_dev_mode()` test in multisite testsuite --- tests/php/test-amp-helper-functions.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index 89803ec6962..ac0dd5e5689 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -1897,6 +1897,10 @@ public function test_amp_get_content_embed_handlers() { public function test_amp_is_dev_mode() { AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + // Enabale HTTPS to short circuit `amp_is_dev_mode()` in multiste tests which depends on wp-env. + // We require it because wp-env already meets the prerequisites for local development. + $_SERVER['HTTPS'] = 'on'; + $this->assertFalse( amp_is_dev_mode() ); add_filter( 'amp_dev_mode_enabled', '__return_true' ); $this->assertTrue( amp_is_dev_mode() ); @@ -1978,6 +1982,10 @@ public function test_amp_get_content_sanitizers() { $post = self::factory()->post->create_and_get(); add_filter( 'amp_content_sanitizers', [ $this, 'capture_filter_call' ], 10, 2 ); + // Enabale HTTPS to short circuit `amp_is_dev_mode()` in multiste tests which depends on wp-env. + // We require it because wp-env already meets the prerequisites for local development. + $_SERVER['HTTPS'] = 'on'; + $this->last_filter_call = null; AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); $handlers = amp_get_content_sanitizers(); @@ -2054,6 +2062,10 @@ function ( $xpaths ) use ( $element_xpaths ) { } ); + // Enabale HTTPS to short circuit `amp_is_dev_mode()` in multiste tests which depends on wp-env. + // We require it because wp-env already meets the prerequisites for local development. + $_SERVER['HTTPS'] = 'on'; + // Check that AMP_Dev_Mode_Sanitizer is not registered if not in dev mode. $sanitizers = amp_get_content_sanitizers(); $this->assertFalse( amp_is_dev_mode() ); From 6ae25402ea9ea29af7721bf5df8bba57bf2b1a05 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Thu, 6 Apr 2023 14:18:38 +0530 Subject: [PATCH 16/18] Update `$dev_mode_xpaths` to include `link[rel=manifest]` in local env --- includes/amp-helper-functions.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 104beba80e9..1844bb0bf06 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1708,6 +1708,11 @@ function amp_get_content_sanitizers( $post = null ) { ); } + // Mark the manifest output by PWA plugin as being in dev mode. + if ( ! is_ssl() && 'localhost' === $parsed_home_url['host'] ) { + $dev_mode_xpaths[] = '//link[@rel="manifest" and contains(@href, "web-app-manifest")]'; + } + $sanitizers = array_merge( [ AMP_Dev_Mode_Sanitizer::class => [ From 5ba46fa3891f7086149c8aefafc51452efab4225 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Thu, 6 Apr 2023 14:21:07 +0530 Subject: [PATCH 17/18] Update test case to assert xpath for dev mode elements --- tests/php/test-amp-helper-functions.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index ac0dd5e5689..fb527e4a0be 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -2134,6 +2134,7 @@ public function test_amp_get_content_sanitizers_with_post_preview() { '//*[ @id = "wpadminbar" ]//*', '//style[ @id = "admin-bar-inline-css" ]', '//script[ not( @src ) and contains( text(), "document.location.search" ) and contains( text(), "preview=true" ) and contains( text(), "unload" ) and contains( text(), "window.name" ) and contains( text(), "wp-preview-' . $post . '" ) ]', + '//link[@rel="manifest" and contains(@href, "web-app-manifest")]', ], $sanitizers[ AMP_Dev_Mode_Sanitizer::class ]['element_xpaths'] ); From e28f9792ef46e478fc33bb3eaabc4529fb4afed5 Mon Sep 17 00:00:00 2001 From: thelovekesh Date: Thu, 6 Apr 2023 14:50:49 +0530 Subject: [PATCH 18/18] Mock `home_url` to return localhost in GitHub CI --- tests/php/test-amp-helper-functions.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index fb527e4a0be..5e8e2a56f53 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -2123,6 +2123,13 @@ public function test_amp_get_content_sanitizers_with_post_preview() { ); $this->go_to( get_preview_post_link( $post ) ); + add_filter( + 'home_url', + static function () { + return 'http://localhost'; + } + ); + $sanitizers = amp_get_content_sanitizers(); $this->assertTrue( is_admin_bar_showing() ); $this->assertTrue( amp_is_dev_mode() );