Bug 54438 – Add check for invalid characters in the protocol field in registerProtocolHandler.

• Component: New Bugs
• Status: RESOLVED LATER

[James Kozianski]

Add check for invalid characters in the protocol field in registerProtocolHandler.

[James Kozianski]

Created attachment 82418 [details]
Patch

[Darin Adler]

Comment on attachment 82418 [details]
Patch

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.

[Darin Adler]

Comment on attachment 82418 [details]
Patch

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

> LayoutTests/fast/dom/register-protocol-handler.html:20
> +var invalid_protocols = ['http', 'https', 'file', '%%%', 'search:'];

We need to cover more kinds of invalid protocols in this test case. See the RFC for the BNF that should inspire more test cases.

[James Kozianski]

Darin, thanks for the review and for taking the time to point out those useful functions :-)

I've created a new bug for exposing isValidProtocol() in KURL.h (bug 54594) which I'd be glad for your input on as well.

I'll update the test case to have more examples of invalid schemes, too, and reupload once the other bug is resolved.

(In reply to comment #2)
> (From update of attachment 82418 [details])
> 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.