[webkit-reviews] review denied: [Bug 54438] Add check for invalid characters in the protocol field in registerProtocolHandler. : [Attachment 82418] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied James Kozianski <koz at chromium.org>'s
request for review:
Bug 54438: Add check for invalid characters in the protocol field in
registerProtocolHandler.
https://bugs.webkit.org/show_bug.cgi?id=54438

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=82418&action=review

Looks fine. Should use the code already present in KURL.cpp to check if the
protocol is valid.

> Source/WebCore/page/Navigator.cpp:204
> +bool isInvalidSchemeChar(UChar c)

Since this is local to the file it should have static so it gets internal
linkage.

The word “character” should be spelled out in the function name. And the
argument name should be a word, not a character.

> Source/WebCore/page/Navigator.cpp:206
> +    c = tolower(c);

This will have unpredictable effects depending on the locale setting, so we
avoid it and use the functions from <wtf/ASCIICType.h> instead. In this case it
would be toASCIILower, but you actually don’t need that at all.

> Source/WebCore/page/Navigator.cpp:207
> +    bool valid = (c >= 'a' && c <= 'z') || c == '-' || c == '.';

This can be written with isASCIIAlpha instead of handwriting the equivalent.
For one thing, isASCIIAlpha is more efficient. For another it’s arguably easier
to read.

> Source/WebCore/page/Navigator.cpp:218
> +    if (scheme.find(isInvalidSchemeChar, 0) != notFound) {

We should not write a new function for this.

KURL.cpp already has a function named isValidProtocol that does the same thing.
This function should be made public in KURL.h and used here.

The function in KURL.cpp implements the RFC rules correctly covering cases not
handled correctly here including disallowing the empty string, forbidding
non-alphanumerics for the first character, and allowing "+" as well as "-" and
".".

It’s possible that the use of a separate URL library for Google will cause
difficulty for Chromium, but that should be resolved. Our use of an alternate
URL library for one port does not justify putting duplicate URL code here.