multiple alternative protocols not parsed correctly

[karlp]

From the WebSockets standard,

 The request MAY include a header field with the name
        |Sec-WebSocket-Protocol|.  If present, this value indicates one
        or more comma-separated subprotocol the client wishes to speak,
        ordered by preference.  The elements that comprise this value
        MUST be non-empty strings with characters in the range U+0021 to
        U+007E not including separator characters as defined in
        [RFC2616] and MUST all be unique strings.  The ABNF for the
        value of this header field is 1#token, where the definitions of
        constructs and rules are as given in [RFC2616].

An example of client code that does this: http://git.eclipse.org/c/paho/org.eclipse.paho.mqtt.javascript.git/tree/src/mqttws31.js#n973

this.socket = new WebSocket(wsurl, ["mqtt","mqttv3.1"]);

libwebsockets however, in the parser state machine for sec-websocket-protocol, does not split this on the comma, and try each protocol in turn. Instead, it looks for a struct libwebsocket_protocols with a name matching "mqtt, mqttv3.1".

You can't even work around this by registering a protocol with that name, as it then returns the combined name to the browser, which rejects it as being neither of the protocols requested.

Unfortunately, I'm finding the parsers.c::libwebsocket_parse() method to be... rather difficult to follow, so I've not managed to get a patch together for this at present.

[andrew-canaday]

This could be fixed without modifying parsers in the following way:

• server.c:_lws_handshakeserver: Use _strtokr on ',' to iterate over multiple protocols.
• server-handshake.c:_handshake0405: Return handshake response using wsi->protocol->name instead of WSI_TOKEN_PROTOCOL

I'm off for vacation for the next week, but I can take a crack at it afterwards if no one's patched it by then.

Ciao! -Andrew

On Fri, Jul 18, 2014 at 8:44 AM, Karl Palsson notifications@github.com wrote:

From the WebSockets standard,

The request MAY include a header field with the name |Sec-WebSocket-Protocol|. If present, this value indicates one or more comma-separated subprotocol the client wishes to speak, ordered by preference. The elements that comprise this value MUST be non-empty strings with characters in the range U+0021 to U+007E not including separator characters as defined in [RFC2616] and MUST all be unique strings. The ABNF for the value of this header field is 1#token, where the definitions of constructs and rules are as given in [RFC2616].

An example of client code that does this: http://git.eclipse.org/c/paho/org.eclipse.paho.mqtt.javascript.git/tree/src/mqttws31.js#n973

this.socket = new WebSocket(wsurl, ["mqtt","mqttv3.1"]);

libwebsockets however, in the parser state machine for sec-websocket-protocol, does not split this on the comma, and try each protocol in turn. Instead, it looks for a struct libwebsocket_protocols with a name matching "mqtt, mqttv3.1".

You can't even work around this by registering a protocol with that name, as it then returns the combined name to the browser, which rejects it as being neither of the protocols requested.

Unfortunately, I'm finding the parsers.c::libwebsocket_parse() method to be... rather difficult to follow, so I've not managed to get a patch together for this at present.

— Reply to this email directly or view it on GitHub https://github.com/warmcat/libwebsockets/issues/147.

[mhaberler]

I stumbled about this issue too. I propose a more general solution to deal with arbitrary protocol name lists, as outlined in https://github.com/mhaberler/libwebsockets/commit/de3e7c913dd4e5ad71f84d70846782b3d18ea9c5 :

• wsi gets a new 'selected_name' field, initialized to NULL. It can be set by the FILTER_PROTOCOL_CONNECTION callback as outlined in de3e7c9 in case there are several protocols offered by the client, and the server may choose one of them by setting the 'selected_name' field, and returning the protocol index+1 from the callback *)
• if the callback finds no agreeable protocol, it returns -1 to close the connection.
• if it doesnt care or callback not implemented, this means "choose the first matching protocol" which is either one with a specific name (nonzero length), or the 'default protocol' with "" as a name.
• the Sec-WebSocket-Protocol header in the server reply will contain wsi->selected_protocol if nonzero, and wsi->protocol->name otherwise.

This should fix the selection logic, and also enable more flexible protocol handling in the server.

I'll look into generating a PR.

[mhaberler]

ok, here is a tentative fix for the Sec-WebSocket-Protocol header issue, which at the same time enables the behavior outlined above (give FILTER_PROTOCOL_CONNECTION the option to choose and set a ws protocol name and - optionally - a non-default libwebsockets_protocol reference):

https://github.com/warmcat/libwebsockets/pull/158

Also fulfill's karlp's above requirement.

works great for me - usage example: http://git.mah.priv.at/gitweb?p=emc2-dev.git;a=blobdiff;f=src/machinetalk/webtalk/webtalk_wsproxy.cc;h=01ac1dc02d5145eaf549d45a57a209b02f943c5f;hp=a6f0572eab7e18752127bd2528bf70cdd8a5201d;hb=33e90e10554fd765229edc0adcb6bb2dd0d8f644;hpb=9c6e8063614dc6bfae243243bc177daaaca0f271

I am a bit unsure about the correctness of the lib/server-handshake.c diff - any field overflow detection required?

happy to update the docs if merged.

header example - FILTER_PROTOCOL_CONNECTION selected machinekit1.0:

Upgrade: websocket
Connection: Upgrade
Host: 193.228.47.216:4711
Origin: http://193.228.47.216:4711
Sec-WebSocket-Key: 7TU1v07PT2yXFihHhRKn4Q==
Sec-WebSocket-Version: 13
Sec-WebSocket-Protocol: http foo bar machinekit1.0 fasel

-----------------------
--- response header ---
HTTP/1.1 101 Switching Protocols
Upgrade: WebSocket
Connection: Upgrade
Sec-WebSocket-Accept: mIj9MYvF6koXih+/8Wt2bHorV04=
Sec-WebSocket-Protocol: machinekit1.0
-----------------------

[mhaberler]

just noticed a restriction with my approach: if there are several protocol structures, and their proto is switched from default, their per_session_data_size must be identical because by the time FILTER_PROTOCOL_CONNECTION is callled the userdata is already allocated from the default entry.

I see these options (in descending order of preference):

• accept this as a restriction for now, and see if it becomes an issue (dont think so). Throw a runtime error if sizes differ.
• reallocate the storage if sizes differ. Location might change.
• add a proto selection callback before wsi->user storage is allocated. Functional overlap with FILTER_PROTOCOL_CONNECTION.

What do you guys think?

[warmcat]

I'm not sure why my note didn't make it on this bug, but this was fixed three weeks ago with this

http://git.libwebsockets.org/cgi-bin/cgit/libwebsockets/commit/?id=7a8d86e0489cd6bd6f812cf55de447f5c695a359

[karlp]

I can confirm that the current code works properly with firefox 31, but chromium 27 gets protocol errors. Given the names of the last release, that sounds like it's now working as well as it can with those browsers :)