Bug 143381 – URLs containing tabs or newlines are parsed incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Maciej Stachowiak]

WebKit appears to treat raw newline or tab characters in a URL hostname as spaces. Other browsers appear to strip them (and the URL spec calls for this). Illustrated example here:

https://url.spec.whatwg.org/interop/test-results/d7d52bebd0?select=current&baseline=safari

Live test case here:

http://intertwingly.net/tmp/url0.html

[Simon Pieters (:zcorpan)]

Also in the scheme. (Gecko changed that recently.)

See https://github.com/whatwg/url/issues/101

and https://github.com/whatwg/html/issues/823#issuecomment-193236577

[Simon Pieters (:zcorpan)]

Test in web-platform-tests: http://w3c-test.org/url/a-element.html

Parsing: <h	t t p://h	o s t:9	0 0 0/p	a t h?q	u e ry#f	r a g> against <about:blank>

[Jiewen Tan]

<rdar://problem/26567214>

[Jiewen Tan]

Created attachment 280267 [details]
test cases

Please put it under LayoutTests/fast/url, and run it.

[Radar WebKit Bug Importer]

<rdar://problem/26586115>

[Jiewen Tan]

(In reply to comment #5)
> <rdar://problem/26586115>

This radar is duplicated.

[Jiewen Tan]

Created attachment 280395 [details]
Patch

[WebKit Commit Bot]

Attachment 280395 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/URL.cpp:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 280395 [details]
Patch

Attachment 280395 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1426035

New failing tests:
fast/url/tab-and-newline-stripping.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNS.html
imported/w3c/web-platform-tests/html/dom/reflection-text.html
fast/dom/HTMLAnchorElement/set-href-attribute-host.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttribute.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttribute.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNodeNS.html
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNodeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-htmldom.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-htmldom.html

[Build Bot]

Created attachment 280404 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280395 [details]
Patch

Attachment 280395 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1426042

New failing tests:
fast/url/tab-and-newline-stripping.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNS.html
imported/w3c/web-platform-tests/html/dom/reflection-text.html
fast/dom/HTMLAnchorElement/set-href-attribute-host.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttribute.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttribute.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNodeNS.html
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNodeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-htmldom.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-htmldom.html

[Build Bot]

Created attachment 280406 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280395 [details]
Patch

Attachment 280395 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1426036

New failing tests:
fast/url/tab-and-newline-stripping.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttribute.html
fast/dom/HTMLAnchorElement/set-href-attribute-host.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttribute.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom.html
imported/w3c/web-platform-tests/html/dom/reflection-text.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNodeNS.html
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNodeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-htmldom.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-htmldom.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNode.html

[Build Bot]

Created attachment 280409 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 280395 [details]
Patch

Attachment 280395 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1426044

New failing tests:
fast/url/tab-and-newline-stripping.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttribute.html
fast/dom/HTMLAnchorElement/set-href-attribute-host.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttribute.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNodeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNodeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-htmldom.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-htmldom.html

[Build Bot]

Created attachment 280412 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jiewen Tan]

Created attachment 280484 [details]
Patch

[WebKit Commit Bot]

Attachment 280484 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/URL.cpp:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 280484 [details]
Patch

Attachment 280484 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1431113

New failing tests:
http/tests/security/xssAuditor/javascript-link-control-char2.html

[Build Bot]

Created attachment 280488 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280484 [details]
Patch

Attachment 280484 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1431143

New failing tests:
http/tests/security/xssAuditor/javascript-link-control-char2.html

[Build Bot]

Created attachment 280490 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280484 [details]
Patch

Attachment 280484 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1431140

New failing tests:
http/tests/security/xssAuditor/javascript-link-control-char2.html

[Build Bot]

Created attachment 280493 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 280484 [details]
Patch

Attachment 280484 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1431151

New failing tests:
http/tests/security/xssAuditor/javascript-link-control-char2.html

[Build Bot]

Created attachment 280494 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jiewen Tan]

Created attachment 280620 [details]
Patch

[WebKit Commit Bot]

Attachment 280620 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/URL.cpp:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:1940:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 280620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280620&action=review

> Source/WebCore/platform/URL.cpp:485
> +    // Get rid of leading and trailing whitespace and control characters.
> +    String rel = relative.stripWhiteSpace(shouldTrimFromURL);
> +
> +    // Get rid of any tabs and newlines.
> +    rel = rel.removeCharacters(isTabNewline);

Could we do this in one step?

[Jiewen Tan]

Comment on attachment 280620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280620&action=review

>> Source/WebCore/platform/URL.cpp:485
>> +    rel = rel.removeCharacters(isTabNewline);
> 
> Could we do this in one step?

We could build a customized function to do the trick, but I don't think there is any WTFString builtin support for doing it.

Are you aware of any tricks that could help to achieve this without replicating many of the WTFString codes?

[Alex Christensen]

(In reply to comment #30)
> Are you aware of any tricks that could help to achieve this without
> replicating many of the WTFString codes?
I am not aware of such tricks.  They may exist, or they may need to be made.
I think we need to rewrite the URL parser.

[Jiewen Tan]

(In reply to comment #31)
> (In reply to comment #30)
> > Are you aware of any tricks that could help to achieve this without
> > replicating many of the WTFString codes?
> I am not aware of such tricks.  They may exist, or they may need to be made.
> I think we need to rewrite the URL parser.

I have no objections of rewriting the URL parser, but we should try our best to harden the current one before that actually happens.

[Brent Fulgham]

Comment on attachment 280620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280620&action=review

> Source/WebCore/platform/URL.cpp:1942
> +        else

This else isn't necessary. Once we stop 'continuing' in the loop, we are no longer leading. Just set 'leading = false' once we stop the continue branch.

> Source/WebCore/platform/URL.cpp:1951
> +        if (isLetterMatchIgnoringCase(url[i], protocol[j]))

Couldn't this have just been written:

if (!isLetterMatchIgnoringCase(url[i], protocol[j]))
    return false;

++j;

> LayoutTests/fast/url/segments-expected.txt:18
> +PASS segments('http://f:\n/c') is '["http:","f","","/c","",""]'

Are you sure this change is correct? Why do we expect your whitespace handling changes to result in a different segment output?

> LayoutTests/fast/url/segments-from-data-url-expected.txt:18
> +PASS segments('http://f:\n/c') is '["http:","f","","/c","",""]'

Ditto.

> LayoutTests/fast/url/segments-from-data-url.html:25
> +  ["http://f:\\n/c",                         ["http:","f","","/c","",""]],

Ditto.

> LayoutTests/fast/url/segments.html:26
> +  ["http://f:\\n/c",                         ["http:","f","","/c","",""]],

Ditto.

[Jiewen Tan]

Comment on attachment 280620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280620&action=review

Thanks for reviewing my patch!

>> LayoutTests/fast/url/segments-expected.txt:18
>> +PASS segments('http://f:\n/c') is '["http:","f","","/c","",""]'
> 
> Are you sure this change is correct? Why do we expect your whitespace handling changes to result in a different segment output?

Yes, I think so. Before my patch, the URL parser will think any URL containing whitespace as invalid URLs. Therefore, there is no segments information this test can extract from the test URL.

Now, with my patch. The test URL becomes http://f:/c. So, the protocol = 'http:', the hostname = 'f', the port = '', the path = '/c', the query = '', and the segment = ''.

[Brent Fulgham]

Comment on attachment 280620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280620&action=review

>>> LayoutTests/fast/url/segments-expected.txt:18
>>> +PASS segments('http://f:\n/c') is '["http:","f","","/c","",""]'
>> 
>> Are you sure this change is correct? Why do we expect your whitespace handling changes to result in a different segment output?
> 
> Yes, I think so. Before my patch, the URL parser will think any URL containing whitespace as invalid URLs. Therefore, there is no segments information this test can extract from the test URL.
> 
> Now, with my patch. The test URL becomes http://f:/c. So, the protocol = 'http:', the hostname = 'f', the port = '', the path = '/c', the query = '', and the segment = ''.

I see! That makes sense.

[Brent Fulgham]

Comment on attachment 280620 [details]
Patch

r=me, but please fix the return logic in protocolIs as I suggested (and the style-bot told you to fix).

[Jiewen Tan]

Comment on attachment 280620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280620&action=review

>> Source/WebCore/platform/URL.cpp:1942
>> +        else
> 
> This else isn't necessary. Once we stop 'continuing' in the loop, we are no longer leading. Just set 'leading = false' once we stop the continue branch.

Fixed.

>> Source/WebCore/platform/URL.cpp:1951
>> +        if (isLetterMatchIgnoringCase(url[i], protocol[j]))
> 
> Couldn't this have just been written:
> 
> if (!isLetterMatchIgnoringCase(url[i], protocol[j]))
>     return false;
> 
> ++j;

Fixed.

[Jiewen Tan]

Created attachment 280658 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 280658 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/URL.cpp:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 280658 [details]
Patch for landing

Clearing flags on attachment: 280658

Committed r201740: <http://trac.webkit.org/changeset/201740>