added host validation for urls #983

rubengonzalezmrf · 2018-03-01T09:28:59Z

Fixes #977.

westonruter

Thanks for the PR. A few changes needed.

Also, doesn't look like the unit test assertions are working: https://travis-ci.org/Automattic/amp-wp/jobs/347682592#L279

westonruter · 2018-03-01T18:53:39Z

tests/test-tag-and-attribute-sanitizer.php

+				'<a class="foo" href="http://foo bar">value</a>',
+				'<a class="foo" href="">value</a>',
+			),
+


Please add test cases for examples found in #977 (comment)

westonruter · 2018-03-01T18:55:52Z

includes/sanitizers/class-amp-allowed-tags-generated.php

@@ -28,6 +28,7 @@ class AMP_Allowed_Tags_Generated {
 						'value_url' => array(
 							'allow_empty' => true,
 							'allow_relative' => true,
+							'valid_host' => true,


This isn't going to work since this file is generated from the spec via amphtml-update.py, so your change here will be lost.

westonruter · 2018-03-01T18:56:47Z

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

+			 * If given attribute's value is a URL with a host, the host must
+			 * be valid
+			 */
+			if ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::VALID_HOST ] ) ) {


Instead of introducing AMP_Rule_Spec::VALID_HOST here, which isn't actually part of the spec, I suggest you validate the host whenever isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ] ). Should it not be validated for every attribute that is a URL value?

westonruter · 2018-03-01T18:57:06Z

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

@@ -1122,6 +1138,40 @@ private function check_attr_spec_rule_value_regex_casei( $node, $attr_name, $att
 		return AMP_Rule_Spec::NOT_APPLICABLE;
 	}

+	/**
+	 * Check if attribute has a valid host value
+	 *


Please add a @since 0.7 here

westonruter · 2018-03-01T18:57:45Z

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

+				$urls_to_test = explode( ',', $attr_value );
+
+				foreach ( $urls_to_test as $url ) {
+					$url_host = AMP_WP_Utils::parse_url( urldecode( $url ), PHP_URL_HOST );


As noted in #876, AMP_WP_Utils is obsolete and you can use wp_parse_url() directly.

westonruter · 2018-03-01T18:58:20Z

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

+
+				foreach ( $urls_to_test as $url ) {
+					$url_host = AMP_WP_Utils::parse_url( urldecode( $url ), PHP_URL_HOST );
+					if ( $url_host && preg_match( '/[!"#$%&\'()*+,\/:;<=>?@[\]^`{|}~\s]/i', $url_host ) ) {


I should think that a whitelist should be used rather than a blacklist here.

I tend to think like you, but the amp validator is doing it this way, looking for blacklisted chars, so i thought it would be better to follow the same approach.

So this is supposed to match everything matched by hostCharIsValid() in https://github.com/ampproject/amphtml/blob/af1e3a550feeafd732226202b8d1f26dcefefa18/validator/engine/parse-url.js#L69-L103 ?

westonruter · 2018-03-06T17:54:48Z

I'm getting the same failures locally.

westonruter · 2018-03-06T17:59:11Z

If I add post_content that includes:

<a class="foo" href="http://foo bar">value</a>

<a class="foo" href="mail to:[email protected]">value</a>

I get out:

<p><a class="foo" href="">value</a></p>

<p><a class="foo" href="mail%20to:[email protected]">value</a></p>

So it seems the mail to check is failing.

westonruter · 2018-03-06T18:00:06Z

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

+					}
+
+					// Check if the protocol contains invalid chars.
+					$url_scheme = AMP_WP_Utils::parse_url( $url, PHP_URL_SCHEME );


Use wp_parse_url() instead of AMP_WP_Utils::parse_url() here.

westonruter · 2018-03-06T18:01:43Z

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

+			if ( $node->hasAttribute( $attr_name ) ) {
+				$attr_value   = $node->getAttribute( $attr_name );
+				$attr_value   = preg_replace( '/\s*,\s*/', ',', $attr_value );
+				$urls_to_test = explode( ',', $attr_value );


I'm curious about the comma-separated list of URLs. Naturally this shouldn't be allowed in an href or src. What elements do this?

You can also combine these two lines into:

$urls_to_test = preg_split( '/\s*,\s*/', $attr_value );

an image with a srcset attribute would have a list of comma separated urls, so i thought it could be a good idea to check all of them

rubengonzalezmrf · 2018-03-07T09:08:50Z

I have post content now with the same 2 examples you added, and i get out the expected results. But unit tests fails anyway

westonruter · 2018-03-07T22:53:46Z

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

+		if ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ] ) ) {
+			if ( $node->hasAttribute( $attr_name ) ) {
+				$attr_value   = $node->getAttribute( $attr_name );
+				$urls_to_test = preg_split( '/\s*,\s*/', $attr_value );


Since only srcset has comma-separated list of URLs, maybe this should check of 'srcset' === $attr_name for whether to split. Maybe that's overkill. My only concern would be if a non-srcset URL happening to have a comma in it and that this would result in the attribute being invalid.

The alternative could be to do this:

$urls_to_test = preg_split( '#\s*,\s*(?=https?://)#', $attr_value );

That will ensure that it will only split where a comma is followed by another URL.

Ah, I see that check_attr_spec_rule_allowed_protocol and check_attr_spec_rule_disallowed_relative do simply comma-separated splits. So probably not something to worry about here then.

westonruter · 2018-03-08T00:08:03Z

But unit tests fails anyway

The tests may not be failing. The jobs are failing due to a PHPCS error:

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php:1171:36: error - Use Yoda Condition checks, you must. (WordPress.PHP.YodaConditions.NotYoda)

https://travis-ci.org/Automattic/amp-wp/jobs/350210562#L216

* Fix phpcs yoda error. * Fix phpunit failures in AMP_Tag_And_Attribute_Sanitizer_Attr_Spec_Rules_Test.

westonruter · 2018-03-08T00:27:58Z

tests/test-tag-and-attribute-sanitizer.php

+			'a_with_wrong_host'                                         => array(
+				'<a class="foo" href="http://foo bar">value</a>',
+				'<a class="foo" href="">value</a>',
+			),


Probably should add a test here for scheme-less URLs, like: <a class="foo" href="//bad domain with a space.com/foo">value</a>

westonruter · 2018-03-08T00:29:07Z

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

+					}
+
+					// Check if the protocol contains invalid chars.
+					$dots_pos = strpos( $url, ':' );


Should this not be using $scheme = wp_parse_url( $url, PHP_URL_SCHEME ) to obtain the schema instead of ':'? What if a URL contains a port, then this could match in the case of a scheme-less URL like //example.com:80/foo/ or (more likely) the URL contains a colon in the path.

In this case, you could omit PHP_URL_HOST param above and then just check for $parsed_url['scheme'].

The point is that mail to:[email protected] won't be correctly parsed by wp_parse_url, the scheme is null.
If the url contains a port number the code will check that no invalid chars are being used before the port number which will pass if the url is correct.

westonruter · 2018-03-08T00:29:34Z

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

+				$urls_to_test = preg_split( '/\s*,\s*/', $node->getAttribute( $attr_name ) );
+				foreach ( $urls_to_test as $url ) {
+					// Check if the host contains invalid chars.
+					$url_host = wp_parse_url( urldecode( $url ), PHP_URL_HOST );


Good use of urldecode() here. If I have a URL like https://%65%78%61%6d%70%6c%65%2e%63%6f%6d/ then this should be valid in AMP, as it is just https://example.com/ 👍

added host validation for urls

a39e073

rubengonzalezmrf mentioned this pull request Mar 1, 2018

Malformed url in links creates an invalid AMP page #977

Closed

Rubén added 2 commits March 1, 2018 10:34

added host validation for urls

843760a

added host validation for urls

f78ac49

westonruter requested changes Mar 1, 2018

View reviewed changes

Rubén added 5 commits March 2, 2018 08:50

added tests and follow the spec

4f6dcc2

added test cases and checks

b8fed2e

solved linting issues

5a6ae74

removed too generic check

2067992

deleted generated file changes

331d152

westonruter reviewed Mar 6, 2018

View reviewed changes

improved protocol checks

454b285

westonruter reviewed Mar 7, 2018

View reviewed changes

Simplify splitting a string by commas

7a3e497

* Fix phpcs yoda error. * Fix phpunit failures in AMP_Tag_And_Attribute_Sanitizer_Attr_Spec_Rules_Test.

westonruter reviewed Mar 8, 2018

View reviewed changes

Rubén and others added 5 commits March 8, 2018 08:40

added urldecode

ea52b1e

added urldecode

aca760d

added urldecode

3ced093

Revert composer changes to be considered in separate PR

790dec8

Add test for link with scheme-less URL having bad URL

8e6698b

westonruter approved these changes Mar 8, 2018

View reviewed changes

westonruter added 3 commits March 8, 2018 12:26

Skip composer install on PHP 5.3 due to phpcs not being available

a04782a

Update readme with changelog entry for URL host name validation

3604c6e

Add test URL host name validity test for encoded host

0d9d50a

westonruter added this to the v0.7 milestone Mar 8, 2018

westonruter merged commit d0f4f01 into ampproject:develop Mar 8, 2018

westonruter mentioned this pull request Sep 7, 2018

Fix URL protocol validation and parsing attribute values with multiple URLs #1411

Merged

westonruter mentioned this pull request Jan 23, 2020

Prevent validation errors for URLs with leading or trailing spaces #4166

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added host validation for urls #983

added host validation for urls #983

rubengonzalezmrf commented Mar 1, 2018 •

edited by westonruter

Loading

westonruter left a comment

westonruter Mar 1, 2018

westonruter Mar 1, 2018

westonruter Mar 1, 2018

westonruter Mar 1, 2018

westonruter Mar 1, 2018

westonruter Mar 1, 2018

rubengonzalezmrf Mar 2, 2018

westonruter Mar 2, 2018

westonruter commented Mar 6, 2018

westonruter commented Mar 6, 2018

westonruter Mar 6, 2018

westonruter Mar 6, 2018

westonruter Mar 6, 2018

rubengonzalezmrf Mar 7, 2018

rubengonzalezmrf commented Mar 7, 2018

westonruter Mar 7, 2018

westonruter Mar 8, 2018

westonruter commented Mar 8, 2018

westonruter Mar 8, 2018

westonruter Mar 8, 2018

westonruter Mar 8, 2018

rubengonzalezmrf Mar 8, 2018 •

edited

Loading

westonruter Mar 8, 2018

added host validation for urls #983

added host validation for urls #983

Conversation

rubengonzalezmrf commented Mar 1, 2018 • edited by westonruter Loading

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Mar 6, 2018

westonruter commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubengonzalezmrf commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubengonzalezmrf Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubengonzalezmrf commented Mar 1, 2018 •

edited by westonruter

Loading

rubengonzalezmrf Mar 8, 2018 •

edited

Loading