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

Fail on invalid local-only close code #183

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samsamoa
Copy link

@samsamoa samsamoa commented Mar 19, 2023

Proposed fix for #182

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

@samsamoa samsamoa changed the title Fail on invalid local-only close Fail on invalid local-only close code Mar 19, 2023
@Kriechi
Copy link
Member

Kriechi commented Mar 21, 2023

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

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

samsamoa commented Mar 24, 2023

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

njsmith commented Apr 11, 2023

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

samsamoa commented Sep 2, 2023

The hypercorn fix went in a couple months ago: pgjones/hypercorn#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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants