[webkit-reviews] review denied: [Bug 133296] Need to check invalid scheme in navigator content utils : [Attachment 232249] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 133296: Need to check invalid scheme in navigator content utils
https://bugs.webkit.org/show_bug.cgi?id=133296

Attachment 232249: Patch
https://bugs.webkit.org/attachment.cgi?id=232249&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.