uNetworking / uWebSockets

Simple, secure & standards compliant web server for the most demanding of applications
Apache License 2.0
17.42k stars 1.76k forks source link

Empty close reasons #1025

Closed ghost closed 4 years ago

ghost commented 4 years ago

When websocket gets closed because of maxPayloadLength exceeded, it closes with 1006 and empty reason data.

Would it be possible to have option to turn on details for close reasons?

I would like to monitor socket closes also log if someone gets disconnected for exceeded payload length to know that there is something wrong client side and its sending unexpected sized messages or in case of attempted DoS attack etc.

I could also do this on message handler but would that be good idea?

Thanks

ghost commented 4 years ago

There's no logging or way to know why a WebSocket was closed other than if the client sends a reason. I agree that's problematic but would require an API breaking change to fix and would be more complex to use.

Maybe do logging in a proxy?

ghost commented 4 years ago

I will do it in JS side for now and manually call ws.close if packet size exceeds the threshold. Thanks anyway.

ghost commented 4 years ago

That's not a good production solution as it really doesn't hinder anyone from sending too large messages. You get the messages after they have been received so denying them at that point has no use.

Maybe the best solution is to just reserve some non-standard close codes for internal use.

ghost commented 4 years ago

I thought if someone sends really big packet and gets disconnected immediately, it would still mitigate it a lot because for sending more that big packets he would need to reconnect. Yeah, using the non-reserved close codes could work.

ghost commented 4 years ago

Also what does the maxPayloadLength check? Is it payload size of single received packet which might be part of larger message?

ghost commented 4 years ago

maxPayload is a limit to how big WebSocket messages will be received no matter how they are fragmented. This check runs in the very tip of the header so the server will force close any connection trying to send a bigger message than what's allowed. This is different from what you talk about, receiving it in full then ignoring it.

ghost commented 4 years ago

There are better close codes we can use (standard):

1009 Message too big The endpoint is terminating the connection because a data frame was received that is too large.
ghost commented 4 years ago

Oh yeah, looks like that would suit the purpose.

ghost commented 4 years ago

Too bad the execution path does not allow passing custom data like that, we only trigger a close, then close handler has a hard-coded 1006 for all force closes. This would have to be fixed in uSockets.

ghost commented 4 years ago

us_socket_close would have to pass void *user, probably. Then we could start putting more meaningful close reasons

ghost commented 4 years ago

https://github.com/uNetworking/uSockets/issues/110

ghost commented 4 years ago

https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent

I'm interested in this. There should be at least five different reasons for internal close

ghost commented 4 years ago

You can use close code 3000 as a library reserved together with custom text, because that code cannot be sent

ghost commented 4 years ago

Oh, I see now. You can't really use any code other than 1006 like we already do, because that's the only code that the other end MUST NOT send as code (well, 1005 also) - 1006 is a pseudo code that can only be reported from the library itself. That's why we only report that code. Reporting 1009 as an internal code is wrong, since that overlaps with the possibility of the other end actually sending 1009 themselves.

So what we can do is we can report 1006 with a varying string message like "Message too big", etc. So it becomes a debug feature in English text. We can do that

ghost commented 4 years ago

Alright this is fixed now