Description
Gyuyoung Kim
2014-05-26 21:25:56 PDT
Created attachment 232108 [details]
Patch
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 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 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 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
Created attachment 232113 [details]
WIP
Created attachment 232190 [details]
Patch for ews
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 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 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 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 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 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
Created attachment 232230 [details]
Patch
Created attachment 232234 [details]
Patch
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". Created attachment 232249 [details]
Patch
(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 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. (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. Created attachment 232370 [details]
Patch
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 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 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 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 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 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
Created attachment 232415 [details]
Patch
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. Created attachment 232459 [details]
Patch for landing
Comment on attachment 232459 [details] Patch for landing Clearing flags on attachment: 232459 Committed r169579: <http://trac.webkit.org/changeset/169579> All reviewed patches have been landed. Closing bug. |