[webkit-reviews] review granted: [Bug 32085] WebSocket should block the same ports that are blocked for resource loading : [Attachment 44255] updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 32085: WebSocket should block the same ports that are blocked for resource
loading
https://bugs.webkit.org/show_bug.cgi?id=32085

Attachment 44255: updated patch
https://bugs.webkit.org/attachment.cgi?id=44255&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>      // JavaScript URLs are "valid" and should be executed even if KURL
decides they are invalid.
>      // The free function protocolIsJavaScript() should be used instead. 
> -    ASSERT(strcmp(protocol, "javascript") != 0);
> +    ASSERT(strcasecmp(protocol, "javascript") != 0);

I think that it's not good for us to use strcasecmp since, like the <ctype.h>
functions, it depends on the POSIX locale.

While I don't think it's urgent to do this, I think we should eventually outlaw
strcasecmp in WebKit code with a technique similar to the functions like
islower.

On way to rewrite our existing uses of strcasecmp would be to overload the
equalIgnoringCase function in PlatformString.h so it can work on two C-style
strings. However, this slightly conflicts with a different idea. Dan Bernstein
and I discussed making the C-style string arguments to functions like
equalIgnoringCase be designed for string literals. So instead of case folding
such strings, it would assert they have no non-ASCII characters or uppercase
ASCII characters in them, along the lines of what is done in the protocolIs
function in KURL.cpp.

Sorry for the long aside -- not really relevant to this bug.

> +    DEFINE_STATIC_LOCAL(DefaultPortsMap, defaultPorts, ());
> +    if (defaultPorts.isEmpty()) {

Sometimes I wish we had a better way to initialize maps than an explicit
isEmpty check.

> +bool isDefaultPortForProtocol(unsigned short port, const String& protocol);
> +bool portAllowed(const KURL&); // Blacklist ports that should never be used
for Web resources.

Two thoughts about these:

   1) Longer term we might want to move these URL policies *slightly* higher
level than then URL class itself. Another source file in the platform directory
rather than URL.h itself. Perhaps even in the networking subdirectory of
platform.

   2) The second function probably is easier to understand if its sense is
reversed, since the concept is a blacklist. The name could be something like
hasForbiddenPort or portShouldBeBlocked; maybe even just portBlocked. One other
subtle point is that it's not ports that are blocked, it's specific
port/protocol combinations, so ideally the very short name would reflect this.
Maybe hasForbiddenPortProtocolPair -- well, I'm sure we could do better than
that. We might think of some even better name. Since the function is still used
in only a small number of places I think it's practical to rename it later, so
I'm not to worried about landing it with any of these names.

> -	   LOG_ERROR("Error: wrong url for WebSocket %s",
url.string().utf8().data());
> +	   LOG(Network, "Wrong url scheme for WebSocket %s",
url.string().utf8().data());

This change means that you think most people are not interested in the error.
LOG_ERROR is used for situations so unusual that anyone using a debug build
would want to see a message on the console. Whereas the LOG(Network) variant is
for things that someone explicitly wants to turn on. It's normally used for
logging that occurs even in non-failure cases. I think that neither LOG_ERROR
nor LOG is really what we're after. The people most interested in these types
of errors are probably using the console in the web inspector, so the big win
is hooking this up to that, something the people working on WebSocket have
discussed. Lets make sure we do it.

I think I slightly prefer LOG_ERROR here, though.

r=me