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 10 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
16 changes: 14 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
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;
} 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
30 changes: 30 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 );

// `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() );
Expand All @@ -1916,6 +1920,24 @@ 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;

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 ) );

if ( function_exists( 'wp_get_environment_type' ) ) {
$this->assertEquals( 'local', wp_get_environment_type() );
}
}

/**
Expand Down Expand Up @@ -1958,6 +1980,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();
Expand Down Expand Up @@ -2034,6 +2060,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() );
Expand Down
48 changes: 48 additions & 0 deletions tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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(
'<amp-autocomplete filter="substring"
src="http://localhost/static/samples/json/amp-autocomplete-cities.json">
<input>
</amp-autocomplete>'
);
$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' => [
Expand Down