Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid URL protocol for local development #7508

Merged
merged 20 commits into from
Apr 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c1b40f9
Update `amp_is_dev_mode()` to return `true` for localhost without HTTPS.
thelovekesh Mar 31, 2023
dc571aa
Add new `allow_localhost_http_protocol` arg in `AMP_Tag_And_Attribute…
thelovekesh Mar 31, 2023
be2728b
Add `allow_localhost_http_protocol` arg to `AMP_Tag_And_Attribute_San…
thelovekesh Mar 31, 2023
e8c6261
Allow http in AMP markup if host is localhost
thelovekesh Mar 31, 2023
26be9cf
Fix conditionals code quality
thelovekesh Mar 31, 2023
de17161
Store multiple call for `amp_is_dev_mode()` in a variable
thelovekesh Mar 31, 2023
9cae8d9
Update test cases for `amp_is_dev_mode()`; add test case for enabling…
thelovekesh Mar 31, 2023
befc6ed
Define debug constants in PHPUnit tests
thelovekesh Mar 31, 2023
695ebb2
Update `home_url()` for `test_amp_is_dev_mode()`
thelovekesh Mar 31, 2023
bb95697
Add tests for `check_attr_spec_rule_allowed_protocol()` when `allow_l…
thelovekesh Mar 31, 2023
73a2b74
Add npm:script to generate PHPUnit html coverage report
thelovekesh Apr 1, 2023
c0539be
Improve test coverage for `::test_check_attr_spec_rule_allowed_protoc…
thelovekesh Apr 1, 2023
0aa7756
Update `amp_is_dev_mode()` to take local env in account
thelovekesh Apr 5, 2023
39ef021
Merge branch 'develop' into fix/invalid-url-protocol-in-local-env
thelovekesh Apr 5, 2023
6e9b8d0
Update test case for `amp_is_dev_mode()` when detecting local env
thelovekesh Apr 5, 2023
52a0418
Fix `::test_amp_is_dev_mode()` test in multisite testsuite
thelovekesh Apr 6, 2023
6ae2540
Update `$dev_mode_xpaths` to include `link[rel=manifest]` in local env
thelovekesh Apr 6, 2023
5ba46fa
Update test case to assert xpath for dev mode elements
thelovekesh Apr 6, 2023
e28f979
Mock `home_url` to return localhost in GitHub CI
thelovekesh Apr 6, 2023
15655b6
Merge branch 'develop' into fix/invalid-url-protocol-in-local-env
thelovekesh Apr 6, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/build-test-measure.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 19 additions & 2 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
)
)
);
}
Expand Down Expand Up @@ -1522,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 = [
Expand Down Expand Up @@ -1590,7 +1601,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' => $is_dev_mode,
],
];

Expand Down Expand Up @@ -1651,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.
*
Expand Down Expand Up @@ -1696,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 => [
Expand Down
16 changes: 14 additions & 2 deletions includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -184,6 +191,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 );
Expand Down Expand Up @@ -1939,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 ( $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;
}
}
Expand All @@ -1951,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 ( $this->args['allow_localhost_http_protocol'] && self::LOCALHOST === wp_parse_url( $url, PHP_URL_HOST ) ) {
return AMP_Rule_Spec::PASS;
thelovekesh marked this conversation as resolved.
Show resolved Hide resolved
} 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;
}
}
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add command for generating PHP tests coverage using wp-env.

"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'",
Expand All @@ -159,4 +160,4 @@
}
},
"title": "AMP for WordPress"
}
}
40 changes: 40 additions & 0 deletions tests/php/test-amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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() );
Expand All @@ -1916,6 +1920,26 @@ 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() );

if ( function_exists( 'wp_get_environment_type' ) ) {
// Just to ensure is_ssl() is false.
$_SERVER['HTTPS'] = false;

add_filter(
'home_url',
static function () {
return 'http://localhost';
}
);

$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' ) );
}
}

/**
Expand Down Expand Up @@ -1958,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();
Expand Down Expand Up @@ -2034,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() );
Expand Down Expand Up @@ -2091,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() );
Expand All @@ -2102,6 +2141,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']
);
Expand Down
Loading