hand shake will fail if client has "Sec-WebSocket-Protocol" in the header

ninjasource / Ninja.WebSockets

A c# implementation of System.Net.WebSockets.WebSocket for .Net Standard 2.0

MIT License

172 stars 39 forks source link

hand shake will fail if client has "Sec-WebSocket-Protocol" in the header #4

Closed yingfangdu closed 6 years ago

yingfangdu commented 6 years ago

I am using this library at server side, and the client is chromium browser which always contains "Sec-WebSocket-Protocol" in the header with "undefined" value.

[https://github.com/yingfangdu/Ninja.WebSockets/commit/0133e2d6f241341a08765c37eeb2868d49c549cc]

ninjasource commented 6 years ago

Thanks, I’ll take a look. I’m not too sure about the security implications of echoing back header information like that. However, I have taken a look at the spec again and I see that the library does need to respond to the sub protocol header in some way. I’ll fix it!

ninjasource commented 6 years ago

I have reread the spec and it looks like it should be ok to return an empty Sec-WebSocket-Protocol response (the the header still needs to be there but it can have no value). Please get the latest version and confirm if chrome is still closing the connection because of this. I cannot reproduce the problem either way.

ninjasource commented 6 years ago

I have published a new nuget package with this fix. Please let me know if this is still a problem, thanks!

yingfangdu commented 6 years ago

Thanks, let me verify the fix and I will reply back here.

yingfangdu commented 6 years ago

I tried that it still fails as below:

[0620/170422.571:INFO:CONSOLE(1)] "WebSocket connection to 'ws://localhost:8080/' failed: Error during WebSocket handshake: 'Sec-WebSocket-Protocol' header value '' in response does not match any of sent values", source: file:///E:/Repos/BAE/src/electron/forge/htmlui/static/js/main.b74e780b.js (1) Web Socket handshake response sent. Stream ready.

ninjasource commented 6 years ago

I've now (hopefully) implemented this properly. This is exposed via new properties on the WebSocketServerOptions, WebSocketClientOptions and WebSocketHttpContext classes.

See DemoServer for how this can be used on the server. If you run the ComplexTest on the DemoClient, it will request a sub protocol. See the StressTest.Run function. You can do the same thing to the SimpleTest if you don't want to have to deal with multithreaded debugging

yingfangdu commented 6 years ago

thanks. Let me try.

OfekShilon commented 6 years ago

I solved this in the code eventually. In PerformHandshakeAsync, I replaced

`string response = ("HTTP/1.1 101 Switching Protocols\r\n"

"Connection: Upgrade\r\n"
"Upgrade: websocket\r\n"
$"Sec-WebSocket-Protocol: {subProtocol}\r\n"
$"Sec-WebSocket-Accept: {setWebSocketAccept}"); with string response = ("HTTP/1.1 101 Switching Protocols\r\n"
- "Connection: Upgrade\r\n"
- "Upgrade: websocket\r\n"
- (subProtocol!=null ? $"Sec-WebSocket-Protocol: {subProtocol}\r\n" : "")
- $"Sec-WebSocket-Accept: {setWebSocketAccept}"); ` I didn't dive into the spec, just took chrome's complaint literally. Should I open a pull request for it?

ninjasource commented 6 years ago

Hi Ofek,

Thank you very much for spotting this! I have made the changes to the code that you recommended so no need for a pull request. Updated the nuget package too. Were you the one who posed on Code Project about the connection issue? If so, I've been busy hosting this on nginx as a reverse proxy to replicate your setup. Didn't get to the end though.

David

OfekShilon commented 6 years ago

@ninjasource I didn't post anything in CodeProject, thanks for fixing it.