Skip to content
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

Empty close reasons #1025

Closed
ghost opened this issue Mar 20, 2020 · 15 comments
Closed

Empty close reasons #1025

ghost opened this issue Mar 20, 2020 · 15 comments

Comments

@ghost
Copy link

ghost commented Mar 20, 2020

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
Copy link

ghost commented Mar 23, 2020

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
Copy link
Author

ghost commented Mar 24, 2020

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

@ghost
Copy link

ghost commented Mar 29, 2020

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
Copy link
Author

ghost commented Mar 29, 2020

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
Copy link
Author

ghost commented Mar 31, 2020

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

@ghost
Copy link

ghost commented Apr 3, 2020

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 ghost transferred this issue from uNetworking/uWebSockets.js Apr 4, 2020
@ghost ghost added the medium priority label Apr 21, 2020
@ghost
Copy link

ghost commented Jun 1, 2020

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
Copy link
Author

ghost commented Jun 1, 2020

Oh yeah, looks like that would suit the purpose.

@ghost
Copy link

ghost commented Jun 22, 2020

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
Copy link

ghost commented Jun 22, 2020

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

@ghost ghost added the uSockets label Jun 22, 2020
@ghost
Copy link

ghost commented Jun 24, 2020

uNetworking/uSockets#110

@ghost ghost added the v19 label Jun 26, 2020
@ghost
Copy link

ghost commented Jun 26, 2020

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
Copy link

ghost commented Jun 26, 2020

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

@ghost
Copy link

ghost commented Jun 26, 2020

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
Copy link

ghost commented Jun 27, 2020

Alright this is fixed now

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0 participants