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

kienstra · 2020-01-23T06:53:17Z

Summary

Prevents a validation error if a URL has a leading or trailing space.

Background

As Weston found, a leading space in a URL can cause a validation error:

<a href=" https://wp.org">Click Here</a>

Unknown error (INVALID_URL_PROTOCOL)

Preventing that error with uncovered an INVALID_URL error, which needed trim() here:

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

Line 1629 in bede7e3

$url = urldecode( $url );

Checklist

My pull request is addressing an open issue (please create one otherwise).
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).

As Weston found, something like: <a href=" https://wp.org"> would have a validation error.

Those trim( $url ) calls were passed to parse_protocol(). So do something similar inside parse_protocol(), don't match leading space.

There is a matching group of ([^/]), so there probably isn't a need for the lookahead.

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

Instead of changing this, simply trim() the URL before passing it to parse_protocol()

westonruter · 2020-01-23T17:24:57Z

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

@@ -1626,7 +1626,7 @@ private function check_attr_spec_rule_value_regex_casei( DOMElement $node, $attr
 	private function check_attr_spec_rule_valid_url( DOMElement $node, $attr_name, $attr_spec_rule ) {
 		if ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ] ) && $node->hasAttribute( $attr_name ) ) {
 			foreach ( $this->extract_attribute_urls( $node->getAttributeNode( $attr_name ) ) as $url ) {
-				$url = urldecode( $url );
+				$url = urldecode( trim( $url ) );


Note that the AMP validator does this same trimming up front: https://github.com/ampproject/amphtml/blob/af1e3a550feeafd732226202b8d1f26dcefefa18/validator/engine/parse-url.js#L214-L215

However, it is also doing one more thing:

// Browsers remove Tab/CR/LF from the entire URL, so we do too. unparsed = unparsed.replace(/[\t\r\n]/g, '');

I think that should be incorporated here as well.

This being the case, instead of calling trim() directly, let's create a new private method like normalize_url_from_attribute_value() which does both the trimming and the tab/newline removal and then use that method everywhere that you are currently using trim() instead.

Sounds good, it'll probably be several hours before I could do that, if that's alright.

911602d adds a method to normalize the URLs, sorry for the delay.

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

As Weston mentioned, there might not be a need for it to run earlier.

parse_protocol() no longer trims space on its own.

Use this instead of trim() for the URLs in attributes.

westonruter · 2020-01-24T03:39:30Z

@kienstra as opposed to adding empty commits, why not just restart the failed jobs in Travis?

In any case, please rebase the branch to remove the throwaway commits when done.

kienstra · 2020-01-24T03:42:54Z

@kienstra as opposed to adding empty commits, why not just restart the failed jobs in Travis?

Yeah, that's a good idea. I had restarted the jobs several times, but pushed an empty commit just to see.

In any case, please rebase the branch to remove the throwaway commits when done.

Sure.

kienstra · 2020-01-24T04:08:59Z

This build passed, but I'll trigger the external-http jobs once more to make sure.

kienstra · 2020-01-24T04:14:03Z

Another external-https build passed. It's still a little troubling that they failed a few times with this PR. I'll trigger it again.

westonruter · 2020-01-24T04:17:43Z

I've noticed the external-http requests being flaky, so I don't think it's anything to do with your code.

kienstra · 2020-01-24T04:19:41Z

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

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


I think the space in:

mail to:[email protected]

...is what caused this be invalid.

Passing <a class="foo" href="mailto:[email protected]"> to the AMP validator shows that it's valid.

This should be resolved by implementing my change here: #4166 (comment)

The mail to protocol should still be invalid, so this chunk should be reverted.

This code: <a class="foo" href="mail to:[email protected]"> should be invalid.

Sure, good point. That's applied in f193019

westonruter · 2020-01-24T04:21:47Z

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

+	 * @return string The normalized URL.
+	 */
+	private function normalize_url_from_attribute_value( $url ) {
+		return preg_replace( '/\s/', '', $url );


This isn't quite right.

Only tabs and newlines should be stripped. See https://github.com/ampproject/amphtml/blob/af1e3a550feeafd732226202b8d1f26dcefefa18/validator/engine/parse-url.js#L217-L218

So this, along with the trimming, should be:

Suggested change

return preg_replace( '/\s/', '', $url );

return preg_replace( '/[\t\r\n]/', '', trim( $url ) );

Sure, I'll make that change.

It's applied in f193019

Use this from the AMP validator, instead of a simple regex, also use trim().

The behavior of the new parse_url_ method is changed, so these can change back.

kienstra · 2020-01-24T06:56:00Z

Thanks! 🎉

westonruter · 2020-01-26T04:46:03Z

Interestingly, the change here suppresses a validation error from occurring. In particular, consider a theme that is outputting invalid HTML like this:

		<h4 class="name">
			<a href="<a href='https://wpthemetestdata.wordpress.com/' rel='external nofollow ugc' class='url'>themedemos</a>">
				themedemos
			</a>
		</h4>

Before the change here, this results in an AMP validation error:

After the change here, no validation error is raised and the HTML markup output by the sanitizer is:

		<h4 class="name">
			<a href="&lt;a_href=%27https%3A%2F%2Fwpthemetestdata.wordpress.com%2F%27+rel%3D%27external+nofollow+ugc%27+class%3D%27url%27%3Ethemedemos%3C%2Fa%3E&amp;amp" rel="amphtml">
				themedemos
			</a>
		</h4>

It turns out this is valid AMP, so this is not a bug. But it shows some difference in handling, however.

kienstra added 2 commits January 22, 2020 23:14

Allow a URL to have a leading space without a validation error

fe3acd0

As Weston found, something like: <a href=" https://wp.org"> would have a validation error.

Replace 2 calls of trim() with longer regex in parse_protocol()

9bd24e6

Those trim( $url ) calls were passed to parse_protocol(). So do something similar inside parse_protocol(), don't match leading space.

googlebot added the cla: yes Signed the Google CLA label Jan 23, 2020

As there's now a capturing group, remove lookahead

a01b51f

There is a matching group of ([^/]), so there probably isn't a need for the lookahead.

westonruter reviewed Jan 23, 2020

View reviewed changes

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved

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

Revert change to regex, in favor of trimming before it's passed

761981e

Instead of changing this, simply trim() the URL before passing it to parse_protocol()

westonruter requested changes Jan 23, 2020

View reviewed changes

westonruter added this to the v1.5 milestone Jan 23, 2020

kienstra added 3 commits January 23, 2020 11:34

Move urldecode() lower in the function

bd4f45f

As Weston mentioned, there might not be a need for it to run earlier.

Remove test case that is no longer handled

c703052

parse_protocol() no longer trims space on its own.

Add a new method to normalize URLs

911602d

Use this instead of trim() for the URLs in attributes.

kienstra force-pushed the fix/href-validation branch from 0a02e69 to 911602d Compare January 24, 2020 04:03

kienstra commented Jan 24, 2020

View reviewed changes

westonruter requested changes Jan 24, 2020

View reviewed changes

kienstra added 2 commits January 23, 2020 22:37

Use suggested normalization, update tests

f193019

Use this from the AMP validator, instead of a simple regex, also use trim().

Revert change in some tests, as they're not needed anymore

92bb4e6

The behavior of the new parse_url_ method is changed, so these can change back.

westonruter approved these changes Jan 24, 2020

View reviewed changes

westonruter merged commit 277ba23 into develop Jan 24, 2020

westonruter deleted the fix/href-validation branch January 24, 2020 06:50

westonruter added the Changelogged label Mar 23, 2020

westonruter mentioned this pull request Jun 27, 2020

Leading and trailing whitespace on URLs can cause unexpected behavior (including leaked validation errors) #4950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kienstra commented Jan 23, 2020 •

edited

Loading

westonruter Jan 23, 2020

westonruter Jan 23, 2020

kienstra Jan 23, 2020

westonruter Jan 24, 2020

kienstra Jan 24, 2020

westonruter commented Jan 24, 2020

kienstra commented Jan 24, 2020 •

edited

Loading

kienstra commented Jan 24, 2020

kienstra commented Jan 24, 2020

westonruter commented Jan 24, 2020

kienstra Jan 24, 2020

westonruter Jan 24, 2020

kienstra Jan 24, 2020

westonruter Jan 24, 2020

kienstra Jan 24, 2020

kienstra Jan 24, 2020

kienstra commented Jan 24, 2020

westonruter commented Jan 26, 2020

	return preg_replace( '/\s/', '', $url );
	return preg_replace( '/[\t\r\n]/', '', trim( $url ) );

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

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

Conversation

kienstra commented Jan 23, 2020 • edited Loading

Summary

Background

Checklist

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 Jan 24, 2020

kienstra commented Jan 24, 2020 • edited Loading

kienstra commented Jan 24, 2020

kienstra commented Jan 24, 2020

westonruter commented Jan 24, 2020

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

kienstra commented Jan 24, 2020

westonruter commented Jan 26, 2020

kienstra commented Jan 23, 2020 •

edited

Loading

kienstra commented Jan 24, 2020 •

edited

Loading