python-hyper / wsproto

Sans-IO WebSocket protocol implementation
https://wsproto.readthedocs.io/
MIT License
261 stars 38 forks source link

Fail on invalid local-only close code #183

Open samsamoa opened 1 year ago

samsamoa commented 1 year ago

Proposed fix for https://github.com/python-hyper/wsproto/issues/182

Fail instead of silently reporting success when a local-only close code is specified.

Kriechi commented 1 year ago

In #182 you wrote:

if you close a websocket with code 1006 (which is not allowed), the result is a 1000-code closure

The question is: why would anyone set an invalid code in the first place? Did you find this as part of a valid / reasonable use case in hypercorn?

I think the intention of the current behaviour is to "be liberal in what to accept" as in swallowing a potential data issue and simply replace it with a valid status code. Throwing an exception feels like a significant change from the current behaviour and wsproto API. It might be easier / less impactful to change the 1000 to a 1002 code, though the implication on wsproto users is unclear at this point to me as well.

samsamoa commented 1 year ago

I believe that hypercorn mistakenly tried to close with 1006 for an internal server error when it should have closed with 1011.

Using 1002 by default seems reasonable to me, though I admit I am not particularly familiar with the users of wsproto and how they might be affected.

samsamoa commented 1 year ago

If 1002 would be acceptable, please let me know and I'll update the PR. (Or if there's another code that could more clearly indicate a server error, that'd be even better.)

njsmith commented 1 year ago

I think this fix is good. I definitely don't like sending 1002 here - that indicates that we're closing the connection due to a protocol error, which in context I think means that we're telling the other side that they sent us some kind of invalid garbage. (Eg the RFC suggests that 1002 be used if we detect that the peer messed up their masking.)

Converting it to 1011 would be more defensible - that's the generic "ugh something on my side is messed up" error, like HTTP 500. But even so, I'm not a big fan of Postel's law. It was never intended to apply to APIs in the first place; and fwiw websockets explicitly reject it as a design philosophy too.

But, I do think we should coordinate with hypercorn to fix their end of things before landing this fix in wsproto, so we don't cause unnecessary pain/crashes for hypercorn users.

samsamoa commented 1 year ago

The hypercorn fix went in a couple months ago: https://github.com/pgjones/hypercorn/pull/112 Given that, should this one be merged as well? I'm not sure what the expectation should be for folks who pin hypercorn but not wsproto.