-
Notifications
You must be signed in to change notification settings - Fork 604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chrome 16 kills the example server #32
Comments
This is actually intended behavior. Your problem is with this line where you are incorrectly specifying the protocol as a component of the path in the URL: websocket = new WebSocket('ws://****.l:8081/echo-protocol'); It should be: websocket = new WebSocket('ws://****.l:8081/', 'echo-protocol'); The reason that the server is throwing an error is that, according to the WebSockets spec, the server cannot specify a subprotocol ( To avoid the possibility of the server "crashing" due to specifying a protocol that a client didn't request, you need to be sure to check the list of subprotocols requested by the client and make sure that the one you want to speak has been requested before accepting the connection. If it has not requested your desired subprotocol and you cannot fall back to one of the other protocols that the client would like to speak, you must explicitly reject the connection instead of accepting it. |
I should mention that the list of subprotocols requested by the client can be accessed at Example: if (request.requestedProtocols.indexOf('my_great_protocol') === -1) {
request.reject();
}
else {
var connection = request.accept('my_great_protocol', request.origin);
} Note that any protocols will have been converted to all lowercase during parsing. |
thanks a lot! works great! |
No problem! |
I've this problem too and I tested the script provided by theturtle32 above. Instead of the server rejects requested connection coming from illegal protocol (not 'echo-protocol' but 'some-others-protocol') it just stopped working. How do I keep the server running? Thanks |
Hi ketting00,
Cheers, |
@alinz Yes, you can do that, but it's not recommended. The sec-websocket-protocol header is allowed to contain multiple values, and so the simple string matching method you show in your example will not always work as expected. A full HTTP-compliant parser should be used to determine the actual value(s) of the header. As for requestedProtocol, it is indeed being set by the framework. It's used internally for verification and if it were not being set, the entire framework would not work and would throw errors when you tried to accept incoming connections. I recommend you double-check your code. |
So how would we get the websocket protocol if the framework doesn't set for |
No, it is being set by the framework. Double-check your code, or if you are certain there is a bug, please post some example code that shows the problem you're running into. |
I'm doing the following and it works fine for me: // process protocols
for(var i=0, l=request.requestedProtocols.length; i < l; i++) {
if(request.requestedProtocols[i] === 'videre_1.1') {
connection = request.accept(request.requestedProtocols[i], request.origin);
break;
}
}
// test if no connection was created, due to no protocol match
if(!connection) {
console.log((new Date()) + ' Connection rejected, invalid protocol(s): ' + request.requestedProtocols);
connection = request.reject();
return;
} The above is where request is the request passed by the 'request' event by the server. |
@alinz I tried your method. It doesn't work. |
Otherwise, use upstart and monit to automatically restart your app if it stopped. |
Thanks for the great answers guys |
Hello I'm having the same problem and obviously there is a solution.
In the browser's extension develop tools I'm using (MQTT2Chrome) I have this error: I think it's related but haven't figured out how to solve this.
|
This is not the proper place to ask questions regarding how the WebSocket protocol (and its "subprotocol negotiation") works. Please, read the doc: |
Its not working I'm also getting below error, WebSocket connection to 'ws://localhost:8091/join/pan' failed: Error during WebSocket handshake: Sent non-empty 'Sec-WebSocket-Protocol' header but no response was received |
Are you serious here? A client requests an unknown protocol, and instead of just rejecting it, you crash the server with a throw Error? And someone else suggests using a monitoring tool to restart the server if it fails? Tell me, what happens to all the other poor connections that are working correctly? A rogue connection taking down the server does not seem like sound design. |
@daveime - Your description is inaccurate. The server doesn't crash when a client requests an unknown protocol. Because the protocol is flexible enough to allow the client to specify multiple potentially acceptable protocols, the server can choose one from the list of protocols proposed by the client. It's your responsibility to choose the correct protocol, and attempting to accept the connection with any protocol other than one proposed by the client is your own programming error. This is not a condition that can arise from a mis-behaving client. If you're seeing this error, then your server-side code is trying to do something it shouldn't be doing. It is an explicit design decision to raise and crash on mis-behaving server code rather than potentially silently swallow those errors, because they're important things that you need to be aware of and fix. |
What if the clients are browsers? Someone can deliberately crash a websocket server by just sending another protocol |
That's just not true. There server has the responsibility to inspect the list of protocols specified by the client and reject the connection if none of them are supported. Not bothering to write this required logic is what will crash your server process.
…Sent from my iPhone
On May 26, 2017, at 2:00 PM, jimmaay ***@***.***> wrote:
What if the clients are browsers? Someone can deliberately crash a websocket server by just sending another protocol
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Maybe put it somewhere on the documentation and the example. I don't see any way to find out that it'll crash the server unless you find this issue. I'm sure lots of people using this don't know about having to manually reject the connection in order to prevent crashing
On May 26, 2017 3:01 PM, "Brian McKelvey" <notifications@github.com<mailto:notifications@github.com>> wrote:
That's just not true. There server has the responsibility to inspect the list of protocols specified by the client and reject the connection if none of them are supported. Not bothering to write this required logic is what will crash your server process.
Sent from my iPhone
On May 26, 2017, at 2:00 PM, jimmaay ***@***.******@***.***>> wrote:
What if the clients are browsers? Someone can deliberately crash a websocket server by just sending another protocol
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#32 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFMB_Z4bmfcVyN3KQ6kwmzG73xlpxEf0ks5r90ujgaJpZM4AE_Py>.
|
https://tools.ietf.org/html/rfc6455 Users should read protocol specifications before using libraries that implement such a protocol. |
The protocol doesn't specify that it should crash the server.
On May 26, 2017 3:22 PM, "Iñaki Baz Castillo" <notifications@github.com<mailto:notifications@github.com>> wrote:
Maybe put it somewhere on the documentation and the example.
https://tools.ietf.org/html/rfc6455
Users should read protocol specifications before using libraries that implement such a protocol.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#32 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFMB_f7hD8OlCy3VXbFil0HOa514yTwzks5r91CvgaJpZM4AE_Py>.
|
Sigh. I cannot believe this conversation is still continuing. Failing to catch errors is lazy, expecting an API to hold your hand is lazy. If you write code that fails because you didn't read the protocol and didn't catch any potential exceptions then you only have yourself to blame. The owners of this repository are not going to change the logic just because you guys are lazy and/or don't understand the concept of capturing thrown exceptions. I even offered a link to my github project that I wrote over four years ago that demonstrates clearly how to catch the errors and handle them. You have this lib for free, it's FOSS, from the generosity of the author. Stop being lazy, invest some of your own time and accept responsibility for your inability to code appropriately. |
The protocol states that replyig with an non matching WC subprotocol is not valid and hence the Node-WebSocket library must react according. It's up to the developer to properly implement the logic. |
I'm just giving a recommendation because I can see this becoming an issue with people. It's up to the developer to implement the logic and that can be to not crash the server or at least put it in the documentation if you are going to go the crash route. I'd rather go the prevention route. If prepared SQL statements were more prevalent in API's in the beginning there wouldn't be so many SQL injection vulnerabilities. |
Either way it is up to the developers of this library. They can choose to do something or not. This issue is bound to continue coming up. I'm not going to argue any longer, I've stated my reasons already. |
I think it is clear that the code will not change. However clearer examples or documentation may make make it easier for some people. So I would suggest that if you find this an issue @jimmaay (and others), then you could write some examples and doco and do a pull request. Unfortunately people are often eager to ask for improvements. But the best way to get improvements in, or to address a problem, is to do it yourself and do a pull request. The alternative is to do a fork and make changes as you see fit. |
Yes, please send a PR with the proper documentation. |
@jimmaay & @daveime - Just checked, it turns out that the documentation is already pretty clear.
|
if i run in the chrome console:
the server just crashes with this exception:
using this example code:
any ideas?
The text was updated successfully, but these errors were encountered: