Bug 133296

Summary: Need to check invalid scheme in navigator content utils
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: New BugsAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, bunhere, cdumez, commit-queue, darin, gyuyoung.kim, rniwa, sergio
Priority: P2 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
WIP
none
Patch for ews
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch
none
Patch for landing none

Description Gyuyoung Kim 2014-05-26 21:25:56 PDT
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
Comment 1 Gyuyoung Kim 2014-05-26 21:26:59 PDT
Created attachment 232108 [details]
Patch
Comment 2 Build Bot 2014-05-26 22:57:32 PDT
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
Comment 3 Build Bot 2014-05-26 22:57:38 PDT
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
Comment 4 Build Bot 2014-05-26 23:51:35 PDT
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
Comment 5 Build Bot 2014-05-26 23:51:41 PDT
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
Comment 6 Gyuyoung Kim 2014-05-26 23:52:46 PDT
Created attachment 232113 [details]
WIP
Comment 7 Gyuyoung Kim 2014-05-28 04:31:55 PDT
Created attachment 232190 [details]
Patch for ews
Comment 8 Build Bot 2014-05-28 05:26:00 PDT
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
Comment 9 Build Bot 2014-05-28 05:26:08 PDT
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
Comment 10 Build Bot 2014-05-28 05:52:58 PDT
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
Comment 11 Build Bot 2014-05-28 05:53:05 PDT
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
Comment 12 Build Bot 2014-05-28 06:54:13 PDT
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
Comment 13 Build Bot 2014-05-28 06:54:19 PDT
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
Comment 14 Gyuyoung Kim 2014-05-28 20:17:36 PDT
Created attachment 232230 [details]
Patch
Comment 15 Gyuyoung Kim 2014-05-28 22:07:16 PDT
Created attachment 232234 [details]
Patch
Comment 16 Darin Adler 2014-05-29 07:54:04 PDT
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".
Comment 17 Gyuyoung Kim 2014-05-29 08:06:21 PDT
Created attachment 232249 [details]
Patch
Comment 18 Gyuyoung Kim 2014-05-29 08:06:50 PDT
(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.
Comment 19 Darin Adler 2014-05-29 10:03:49 PDT
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.
Comment 20 Gyuyoung Kim 2014-06-02 01:07:14 PDT
(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.
Comment 21 Gyuyoung Kim 2014-06-02 01:08:01 PDT
Created attachment 232370 [details]
Patch
Comment 22 Build Bot 2014-06-02 02:36:04 PDT
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
Comment 23 Build Bot 2014-06-02 02:36:10 PDT
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
Comment 24 Build Bot 2014-06-02 03:36:26 PDT
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
Comment 25 Build Bot 2014-06-02 03:36:32 PDT
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
Comment 26 Build Bot 2014-06-02 04:36:39 PDT
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
Comment 27 Build Bot 2014-06-02 04:36:46 PDT
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
Comment 28 Gyuyoung Kim 2014-06-02 23:07:40 PDT
Created attachment 232415 [details]
Patch
Comment 29 Darin Adler 2014-06-03 17:36:14 PDT
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.
Comment 30 Gyuyoung Kim 2014-06-03 19:55:07 PDT
Created attachment 232459 [details]
Patch for landing
Comment 31 WebKit Commit Bot 2014-06-03 20:34:00 PDT
Comment on attachment 232459 [details]
Patch for landing

Clearing flags on attachment: 232459

Committed r169579: <http://trac.webkit.org/changeset/169579>
Comment 32 WebKit Commit Bot 2014-06-03 20:34:10 PDT
All reviewed patches have been landed.  Closing bug.