WebSocket handshake doesn't check query component of URL

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ian 'Hixie' Hickson]

STEPS TO REPRODUCE
1. Try to connect to a server that returns the WebSocket-Location "ws://example.com/test", but use "ws://example.com/test?" as the URL to connect to.

ACTUAL RESULTS
It connects.

EXPECTED RESULTS
If the server's returned WebSocket-Location isn't the same as the actual location used, including the query component, then the client should refuse to connect.

[Ian 'Hixie' Hickson]

In fact it looks like if the server does return the query component, the connection fails because the client does do a full comparison, but ignores the returned query component when doing so.

[Fumitoshi Ukai]

Created attachment 43421 [details]
WebSocket handshake check query component of URL.

[Fumitoshi Ukai]

(In reply to comment #1)
> In fact it looks like if the server does return the query component, the
> connection fails because the client does do a full comparison, but ignores the
> returned query component when doing so.

One question.  if use "ws://example.com/test?", we must send "GET /test? HTTP/1.1"?

[Ian 'Hixie' Hickson]

Yes, same as with regular HTTP. See steps 9 to 11 of the "parse a Web Socket URL's components" algorithm in the spec, and then step 5 of the "establish a Web Socket connection" algorithm.

[Fumitoshi Ukai]

(In reply to comment #4)
> Yes, same as with regular HTTP. See steps 9 to 11 of the "parse a Web Socket
> URL's components" algorithm in the spec, and then step 5 of the "establish a
> Web Socket connection" algorithm.

Hmm, then we should fix WebSocketHandshake::clientHandshakeMessage().
For now, it sends "GET /test HTTP/1.1" for "ws://example.com/test?".

[Fumitoshi Ukai]

(In reply to comment #5)
> (In reply to comment #4)
> > Yes, same as with regular HTTP. See steps 9 to 11 of the "parse a Web Socket
> > URL's components" algorithm in the spec, and then step 5 of the "establish a
> > Web Socket connection" algorithm.
> 
> Hmm, then we should fix WebSocketHandshake::clientHandshakeMessage().
> For now, it sends "GET /test HTTP/1.1" for "ws://example.com/test?".

Should we add new method in KURL to construct request resource?
(or hasQuery()?)

I also found that pywebsocket couldn't handle /test? properly.

[Fumitoshi Ukai]

(In reply to comment #1)
> In fact it looks like if the server does return the query component, the
> connection fails because the client does do a full comparison, but ignores the
> returned query component when doing so.

Is it expected that the server doesn't return the query component, or it depends on server implementation?

If the server should not return the query component, is it the same that opening with query components should always fail?

[Ian 'Hixie' Hickson]

If the connection is to be established, the server should send back the same URL as was requested (constructing it from the request, taking the scheme from whether the request was encrypted, the domain from the Host: line, and the rest from the data after the GET).

Typically, I'd expect most servers to have the returned value hard-coded, so that they don't have to parse the handshake and they can rely on the client checking that the URL was correct.

[Alexey Proskuryakov]

Comment on attachment 43421 [details]
WebSocket handshake check query component of URL.

+    KURL location_url = m_url.copy();

As explained in a comment in KURL.h, copy() shouldn't be used here.

+    KURL location_url = m_url.copy();

In WebKit style, this would be named locationURL. I don't understand why the function and variable use the word "location" - where did it come from?

+    location_url.setHost(location_url.host().lower());

Hmm, I think that KURL should do this internally. Probably worth a FIXME for now.

We need more tests:
- what happens if the server echoes the string with query (my understanding is that handshake should succeed):
- non-empty query;
- what happens if someone (client or server) sends an URL with fragment;
- what happens if the URL includes credentials (e.g. ws://user:pass@server/path).

This patch changes behavior of clientLocation() in the latter case.

[Fumitoshi Ukai]

Created attachment 43909 [details]
WebSocket handshake check query component of URL.

[Fumitoshi Ukai]

(In reply to comment #10)
> Created an attachment (id=43909) [details]
> WebSocket handshake check query component of URL.

Note that some tests would fail on Mac by missing "WebSocket is closed".
I think it should be a bug in platform code. cf. https://bugs.webkit.org/show_bug.cgi?id=31659 ?

[Alexey Proskuryakov]

> Note that some tests would fail on Mac by missing "WebSocket is closed".
> I think it should be a bug in platform code.

If we're going to land this change before it's fixed, then failing tests should be added to platform/mac/Skipped.

[Alexey Proskuryakov]

Thanks for the many new tests!

+        (WebCore::KURL::resourceName):

I don't think it's appropriate to add a method with this name to KURL. The only "resource name" I know of is URN, which is is confusing, as it's a related notion to URL, but not to what this method does.

I think that to come up with a good name for this function, we should look at where it is used, and name it accordingly to what's needed there.

[Fumitoshi Ukai]

(In reply to comment #13)
> Thanks for the many new tests!
> 
> +        (WebCore::KURL::resourceName):
> 
> I don't think it's appropriate to add a method with this name to KURL. The only
> "resource name" I know of is URN, which is is confusing, as it's a related
> notion to URL, but not to what this method does.
> 
> I think that to come up with a good name for this function, we should look at
> where it is used, and name it accordingly to what's needed there.

I used "resource name" because it is used in the websocket protocol spec draft, and it is used for resource name in websocket code.
If "resource name" is not appropriate, how about pathForRequest, which is used in google url?

[Fumitoshi Ukai]

Created attachment 43933 [details]
WebSocket handshake check query component of URL

[Eric Seidel (no email)]

Comment on attachment 43933 [details]
WebSocket handshake check query component of URL

There are tabs in this patch.

[Fumitoshi Ukai]

Created attachment 44000 [details]
WebSocket handshake check query component of URL

[Fumitoshi Ukai]

(In reply to comment #16)
> (From update of attachment 43933 [details])
> There are tabs in this patch.

Sorry. Fixed.

[Adam Barth]

Attachment 44000 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Done processing WebCore/platform/KURLGooglePrivate.h
WebCore/platform/KURLGoogle.cpp:307:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Done processing WebCore/platform/KURLGoogle.cpp
Done processing WebCore/platform/KURL.cpp
Done processing WebCore/platform/KURL.h
Done processing WebCore/websockets/WebSocketHandshake.cpp
Total errors found: 1

[Fumitoshi Ukai]

Created attachment 44052 [details]
WebSocket handshake check query component of URL

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44052 [details] without any errors.

[Alexey Proskuryakov]

Comment on attachment 44052 [details]
WebSocket handshake check query component of URL

+# Missing "WebSocket is closed"

I don't think this is still needed - bug 31659	 is fixed now.

+var url = "ws://user:pass@localhost:8880/websocket/tests/echo-location";

Are you testing cross-origin WebSocket here? HTTP tests run from 127.0.0.1, not from localhost, so accessing localhost is cross origin. Are WebSocket tests different?

+String KURL::resourceName() const

Now that I see where "resource name" is defined, I'm positive that this accessor shouldn't be added to KURL. Resource name is solely a Web Sockets concept, so it should be confined to Web Sockets code.

[Fumitoshi Ukai]

Created attachment 44127 [details]
WebSocket handshake check query component of URL

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44127 [details] without any errors.

[Alexey Proskuryakov]

Comment on attachment 44127 [details]
WebSocket handshake check query component of URL

r=me, thanks for adding all these tests!

It worries me that accidentally using localhost instead of 127.0.0.1 didn't dramatically affect test behavior. It doesn't necessarily indicate a problem, but we need to add many new tests for cross-origin WebSocket usage to check that it functions correctly.

Per-function comments in WebCore/ChangeLog would be appreciated. The change to KURL::query() is particularly interesting, as someone working on bug 21015 will need to know the rationale of the change you're making here, and won't find it.

[Fumitoshi Ukai]

Committed r51589: <http://trac.webkit.org/changeset/51589>

[Fumitoshi Ukai]

(In reply to comment #25)
> (From update of attachment 44127 [details])
> r=me, thanks for adding all these tests!

Thanks for review!

> 
> It worries me that accidentally using localhost instead of 127.0.0.1 didn't
> dramatically affect test behavior. It doesn't necessarily indicate a problem,
> but we need to add many new tests for cross-origin WebSocket usage to check
> that it functions correctly.

I've file a bug https://bugs.webkit.org/show_bug.cgi?id=32055.
I'll work on this.
 
> Per-function comments in WebCore/ChangeLog would be appreciated. The change to
> KURL::query() is particularly interesting, as someone working on bug 21015 will
> need to know the rationale of the change you're making here, and won't find it.

Sure. Added comments.