Fix invalid URL protocol for local development

[thelovekesh]

Summary

Fixes #5402

Checklist

• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[thelovekesh]

@westonruter Should we include href attribute as well?

Moreover, please note that the included elements are those that have attributes with the same origin URLs.

[westonruter]

The href attribute would have to be included, yes, as it needs to handle the case of the Web App Manifest link.

However, I think it should only include the starts-with() predicates and not contains().

Nevertheless, there won't address the case where there is a protocol-relative link. For example:

<link rel="web-app-manifest" href="//localhost/sw.js">

Also, it wouldn't handle a relative path:

<link rel="web-app-manifest" href="/sw.js">

Therefore, I'm wondering if marking with dev mode is going to work reliably.

[westonruter]

In particular, the problematic logic in the sanitizer is the following:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Lines 1275 to 1277 in 2f37721

	} elseif ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOWED_PROTOCOL ] ) &&
	AMP_Rule_Spec::FAIL === $this->check_attr_spec_rule_allowed_protocol( $node, $attr_name, $attr_spec_rule ) ) {
	$attrs_to_remove[] = [ $attr_node, self::INVALID_URL_PROTOCOL, null ]; // @todo A javascript: protocol could be treated differently. It should have a JS error type.

I wonder, should the check_attr_spec_rule_allowed_protocol method be modified with a special case, to always return PASS if the normalized URL points to localhost? This should also involve introducing a new arg to that sanitizer, similar to prefer_bento, which is the switch for whether to enable this special case.

The special case arg can then be passed in here:

amp-wp/includes/amp-helper-functions.php

Lines 1602 to 1604 in 7c66bd0

	AMP_Tag_And_Attribute_Sanitizer::class => [
	'prefer_bento' => amp_is_bento_enabled(),
	],

[thelovekesh]

However, I think it should only include the starts-with() predicates and not contains().

Then we will need some other workaround to filter out elements that have same-origin URLs in their attars. We still want to show an invalid URL protocol error if the URL is cross-origin... right?

---

Makes more sense to update the sanitizer to handle the case. But there are two more cases:

1. Usage of localhost IP - 127.0.0.1
2. Change in hostname using /etc/hosts mapping. // Should we even consider this?

[westonruter]

Then we will need some other workaround to filter out elements that have same-origin URLs in their attars. We still want to show an invalid URL protocol error if the URL is cross-origin... right?

What's an example of this attribute? Now that I think of it, srcset is one case in which there are multiple URLs in one attribute.

Makes more sense to update the sanitizer to handle the case. But there are two more cases:

What we should follow here is what browsers allow for bypassing the HTTPS security requirement for certain web platform features. I know this includes localhost and 127.0.0.1, but it is also true for subdomain.localhost. It would not be true for custom hostnames defined in /etc/hosts. See more at https://web.dev/when-to-use-local-https/

[thelovekesh]

I mean a case something like this:

// Should consider development mode
<amp-autocomplete filter="substring"
    src="http://localhost/static/samples/json/amp-autocomplete-cities.json">
    <input>
 </amp-autocomplete>

// Maybe not consider it as development mode?
<amp-autocomplete filter="substring"
    src="http://example.com/static/samples/json/amp-autocomplete-cities.json">
    <input>
 </amp-autocomplete>

[westonruter]

Right, the exception should only be granted if:

1. The page is being served from localhost, and
2. The attribute is linking to localhost.

[thelovekesh]

It would not be true for custom hostnames defined in /etc/hosts. See more at https://web.dev/when-to-use-local-https/

Perfect. Thanks for sharing. Also, I see parse_url() returns the host as localhost while using 127.0.0.1 or [::1].

[thelovekesh]

@westonruter What about amp_is_dev_mode as new arg in AMP_Tag_And_Attribute_Sanitizer?

[westonruter]

How about something more specific, like allow_localhost_http_protocol

[westonruter]

Minor point, but since we check amp_is_dev_mode() below as well, might as well save the value to a variable and use it in both places.

[thelovekesh]

Make sense, since there is no caching in amp_is_dev_mode().

[github-actions]

Plugin builds for 15655b6 are ready 🛎️!

• Download development build
• Download production build

[thelovekesh]

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

[westonruter]

I tried this out with the bultin dev environment and I got as expected with the PWA plugin activated:

The source code included:

<link rel="manifest" href="http://localhost:8888/index.php?rest_route=/wp/v2/web-app-manifest">

Nevertheless, there's one thing which seems to be missing here, and that is the data-ampdevmode attribute on this element which would bypass the error from being reported by the AMP Validator extension in the first place. I realize there is not really anything that can be done about this using the implementation approach we worked out. Otherwise, we'd need to go back to the original approach which I suppose would involve extending AMP_Dev_Mode_Sanitizer to look for all potential elements that have href and src attributes and if they would possibly violate the protocol constraint. But this would end up being more expensive or else not be as robust.

But one thing that could be done is to at least include //link[ @rel = "web-app-manifest" ] among the elements that are included among the dev-mode exemptions. This could be done in AMP_PWA_Script_Sanitizer. But perhaps better in amp_get_content_sanitizers() to push the XPath onto $dev_mode_xpaths if ! is_ssl() && 'localhost' === $host.