Bug 133296 – Need to check invalid scheme in navigator content utils

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

According to spec, scheme value should not contain a colon. So, this cl checks if scheme contains a colon.

"The scheme value, if it contains a colon (as in "mailto:"), will never match anything, since schemes don't contain colons."

Spec: http://www.whatwg.org/specs/web-apps/current-work/#custom-handlers

Blink back merge : https://src.chromium.org/viewvc/blink?view=rev&revision=174748

[Gyuyoung Kim]

Created attachment 232108 [details]
Patch

[Build Bot]

Comment on attachment 232108 [details]
Patch

Attachment 232108 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5637229720371200

New failing tests:
fast/dom/NavigatorContentUtils/unregister-protocol-handler.html
fast/dom/NavigatorContentUtils/register-protocol-handler.html

[Build Bot]

Created attachment 232110 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

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

[Build Bot]

Comment on attachment 232108 [details]
Patch

Attachment 232108 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4755105177927680

New failing tests:
fast/dom/NavigatorContentUtils/unregister-protocol-handler.html
fast/dom/NavigatorContentUtils/register-protocol-handler.html

[Build Bot]

Created attachment 232112 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Gyuyoung Kim]

Created attachment 232113 [details]
WIP

[Gyuyoung Kim]

Created attachment 232190 [details]
Patch for ews

[Build Bot]

Comment on attachment 232190 [details]
Patch for ews

Attachment 232190 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5469519300526080

New failing tests:
fast/dom/NavigatorContentUtils/unregister-protocol-handler.html
fast/dom/NavigatorContentUtils/register-protocol-handler.html

[Build Bot]

Created attachment 232192 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 232190 [details]
Patch for ews

Attachment 232190 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5124010387963904

New failing tests:
fast/dom/NavigatorContentUtils/unregister-protocol-handler.html
fast/dom/NavigatorContentUtils/register-protocol-handler.html

[Build Bot]

Created attachment 232193 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

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

[Build Bot]

Comment on attachment 232190 [details]
Patch for ews

Attachment 232190 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6382869865824256

New failing tests:
fast/dom/NavigatorContentUtils/unregister-protocol-handler.html
fast/dom/NavigatorContentUtils/register-protocol-handler.html

[Build Bot]

Created attachment 232194 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Gyuyoung Kim]

Created attachment 232230 [details]
Patch

[Gyuyoung Kim]

Created attachment 232234 [details]
Patch

[Darin Adler]

Comment on attachment 232234 [details]
Patch

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

> Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.cpp:115
> +    size_t index = scheme.find(':');

This should use "contains", not "find".

[Gyuyoung Kim]

Created attachment 232249 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #16)
> (From update of attachment 232234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232234&action=review
> 
> > Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.cpp:115
> > +    size_t index = scheme.find(':');
> 
> This should use "contains", not "find".

Fixed. Thanks.

[Darin Adler]

Comment on attachment 232249 [details]
Patch

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

This code has lots of small mistakes; not the new patch, but the existing code.

It’s strange that the protocol whitelist is case sensitive. The isProtocolWhitelisted function only allows all-lowercase protocols. Is that intentional? Most other code treats protocols as ASCII case-insensitive.

The isProtocolWhitelisted function doesn’t handle null schemes, so there may be a crash if the scheme passed is null instead of a string. We should be testing that.

The verifyProtocolHandlerScheme function uses isValidProtocol for schemes that start with web+, but in this new code we are only checking for the colon character specifically. Instead, why wouldn’t we use isValidProtocol in both cases? That will check for colons, but also for other aspects of what it takes to be a valid protocol.

What this patch ends up accomplishing is returning SYNTAX_ERR instead of SECURITY_ERR when colon is involved. The function already would give SECURITY_ERR for anything not on the white list. It's a little silly to do this work before calling isProtocolWhitelisted; performance isn’t critical, but it’s redundant to do this check when the white list is going to fail anyway. I would put the check after the return statement.

review- because I think this should be using isValidProtocol, but please consider these other questions

> Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.cpp:114
> +    // The specification requires that schemes don't contain colons.

I think this check should go before the "web+" check above.

I also noticed a typo above this: "characteres".

> LayoutTests/fast/dom/NavigatorContentUtils/unregister-protocol-handler.html:63
> +        debug('Fail: Invalid scheme "' + scheme + '" allowed.');

This is incorrect. It will claim that the scheme was "allowed" even when an exception is raised, if the exception name is not SyntaxError.

[Gyuyoung Kim]

(In reply to comment #19)

> (From update of attachment 232249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232249&action=review
> 
> This code has lots of small mistakes; not the new patch, but the existing code.

First of all, thank you for your review. I replied your comments as inline.


> It’s strange that the protocol whitelist is case sensitive. The isProtocolWhitelisted function only allows all-lowercase protocols. Is that intentional? Most other code treats protocols as ASCII case-insensitive.

According to spec, scheme should be compared as ASCII case-insensitive. We need to handle it as case-insensitive. I will file a bug soon.

"A scheme, such as mailto or web+auth. The scheme must be compared in an ASCII case-insensitive manner by user agents for the purposes of comparing with the scheme part of URLs that they consider against the list of registered handlers."


> The isProtocolWhitelisted function doesn’t handle null schemes, so there may be a crash if the scheme passed is null instead of a string. We should be testing that.

Yes, this should be checked. I'd like to file a bug for this as well.

 
> The verifyProtocolHandlerScheme function uses isValidProtocol for schemes that start with web+, but in this new code we are only checking for the colon character specifically. Instead, why wouldn’t we use isValidProtocol in both cases? That will check for colons, but also for other aspects of what it takes to be a valid protocol.
> 
> What this patch ends up accomplishing is returning SYNTAX_ERR instead of SECURITY_ERR when colon is involved. The function already would give SECURITY_ERR for anything not on the white list. It's a little silly to do this work before calling isProtocolWhitelisted; performance isn’t critical, but it’s redundant to do this check when the white list is going to fail anyway. I would put the check after the return statement.

I thought that it would be good to check if scheme has colon explicitly. However, spec doesn't mention what exception user agent generates when scheme contains colon. So, I used SYNTAX_ERR because it looks colon is close to SYNTAX_ERR rather than SECURITY_ERR. As you said, if there is no need to generate SYNTAX_ERR exception when scheme contains colon, we don't need to check it. I will remove the explicit check code in new patch.

Finally, I hope to land this patch with fixing those you pointed out. Then, I would like to fix those.

[Gyuyoung Kim]

Created attachment 232370 [details]
Patch

[Build Bot]

Comment on attachment 232370 [details]
Patch

Attachment 232370 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6155218211307520

New failing tests:
fast/dom/NavigatorContentUtils/unregister-protocol-handler.html
fast/dom/NavigatorContentUtils/register-protocol-handler.html

[Build Bot]

Created attachment 232373 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

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

[Build Bot]

Comment on attachment 232370 [details]
Patch

Attachment 232370 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4817361668407296

New failing tests:
fast/dom/NavigatorContentUtils/unregister-protocol-handler.html
fast/dom/NavigatorContentUtils/register-protocol-handler.html

[Build Bot]

Created attachment 232374 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

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

[Build Bot]

Comment on attachment 232370 [details]
Patch

Attachment 232370 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5320791965040640

New failing tests:
fast/dom/NavigatorContentUtils/unregister-protocol-handler.html
fast/dom/NavigatorContentUtils/register-protocol-handler.html

[Build Bot]

Created attachment 232376 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

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

[Gyuyoung Kim]

Created attachment 232415 [details]
Patch

[Darin Adler]

Comment on attachment 232415 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        According to spec, scheme value should not contain a colon. So, this cl checks if scheme contains a colon.
> +        
> +        "The scheme value, if it contains a colon (as in "mailto:"), will never match anything, since schemes don't contain colons."
> +        
> +        Spec: http://www.whatwg.org/specs/web-apps/current-work/#custom-handlers
> +
> +        Blink back merge : https://src.chromium.org/viewvc/blink?view=rev&revision=174748

This change log is peculiar, since this change has no effect on scheme validity and the comments are not at all relevant to this change.

> Source/WebCore/ChangeLog:16
> +        No new tests, covered by existing tests.

This is wrong. Please just omit this line.

> Source/WebCore/ChangeLog:19
> +        * Modules/navigatorcontentutils/NavigatorContentUtils.cpp:
> +        (WebCore::verifyProtocolHandlerScheme):

The changes to this file are OK, but they have nothing to do with this bug, really. They are just formatting and comments changes.

> LayoutTests/ChangeLog:14
> +        According to spec, scheme value should not contain a colon. So, this cl checks if scheme contains a colon.
> +        
> +        "The scheme value, if it contains a colon (as in "mailto:"), will never match anything, since schemes don't contain colons."
> +        
> +        Spec: http://www.whatwg.org/specs/web-apps/current-work/#custom-handlers
> +
> +        Blink back merge : https://src.chromium.org/viewvc/blink?view=rev&revision=174748

This change log entry is really strange. It should just say:

    Add tests that check that schemes with colons in their names are rejected.

[Gyuyoung Kim]

Created attachment 232459 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 232459 [details]
Patch for landing

Clearing flags on attachment: 232459

Committed r169579: <http://trac.webkit.org/changeset/169579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.