search

socketry / async-websocket

Asynchronous WebSocket client and server, supporting HTTP/1 and HTTP/2 for Ruby.
MIT License
167 stars 18 forks source link

Doesn't seem to respect or configure `Sec-WebSocket-Protocol` header. #1

Closed shift closed 5 years ago

shift commented 5 years ago

Seems to silently drop the Sec-WebSocket-Protocol header instead of returning it, also not been able to find a place to specify accepted protocols which should get passed onto websocket-driver.

ioquatix commented 5 years ago

Can you provide a simple example code which shows the issue?

shift commented 5 years ago

curl 'wss://localhost:port' -H 'Pragma: no-cache' -H 'Accept-Encoding: gzip, deflate, br' -H 'Accept-Language: en-GB,en-US;q=0.9,en;q=0.8' -H 'Sec-WebSocket-Key: FyaPwix5MFpclAvzFywTDg==' -H 'Upgrade: websocket' -H 'Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits' -H 'Cache-Control: no-cache' -H 'Connection: Upgrade' -H 'Sec-WebSocket-Version: 13' -H 'Sec-WebSocket-Protocol: NOT_IN_REPLY' --compressed which should return a header with the matching Sec-WebSocket-Protocol.

destructobeam commented 5 years ago

I am getting this same issue when using the apollo-link-ws javascript library for GraphQL subscriptions, which looks like it expects at least the Sec-WebSocket-Protocol header in the response.

It's connection request is:

Accept-Encoding: gzip, deflate, br
Accept-Language: en-GB,en-US;q=0.9,en;q=0.8
Cache-Control: no-cache
Connection: Upgrade
Host: localhost:9292
Origin: http://localhost:3000
Pragma: no-cache
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits
Sec-WebSocket-Key: znkyBVgitSBg6WCgvRoRAA==
Sec-WebSocket-Protocol: graphql-ws
Sec-WebSocket-Version: 13
Upgrade: websocket
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.119 Safari/537.36

Applicable RFC stuff: https://tools.ietf.org/html/rfc6455#page-57

ioquatix commented 5 years ago

Is this a bug in async-websocket code or a bug in websocket-driver code?

destructobeam commented 5 years ago

Been looking through the codebases a little, it seems like the Sec-WebSocket-Key handshake is handled in the websocket-driver code okay, but it looks like the Sec-WebSocket-Protocol is more of an implementation detail of the specific ws server instance, just an acceptance of that protocol sent back in the header.

Seems you can define the acceptable protocols here, is that settable through the Async::WebSocket::Server class? https://github.com/faye/websocket-driver-ruby/blob/ee39af83d03ae3059c775583e4c4b291641257b8/lib/websocket/driver/hybi.rb#L57

ioquatix commented 5 years ago

If you can make a PR I will review. Would make sense to make a spec too.

destructobeam commented 5 years ago

Have made a commit here, but not too familar with the ruby client side of things to finish the spec as I'm not using it, have tested with the JS client I am using and works, and not failing on any current specs, any help would be appreciated with verifying the response header. Cheers!

https://github.com/destructobeam/async-websocket/commit/a2d16c404b57d47097df1c6bbe9c1ab8ece0148b

ioquatix commented 5 years ago

That seems logical. Can you submit a PR?

shift commented 5 years ago

As it seems a MR for this has been merged could we cut a new tag to rubygems?

ioquatix commented 5 years ago

Okay done.

ioquatix commented 5 years ago

Would be great to get a PR for documentation on on the updated options/protocols stuff.

shift commented 5 years ago

In my testing from git master nothing was required and I got the header back.