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

Missing close status codes for websockets #5192

Closed
achimnol opened this issue Nov 2, 2020 · 6 comments · Fixed by #5198
Closed

Missing close status codes for websockets #5192

achimnol opened this issue Nov 2, 2020 · 6 comments · Fixed by #5198
Labels

Comments

@achimnol
Copy link
Member

achimnol commented Nov 2, 2020

🐞 Describe the bug

ref: https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent
current: https://github.com/aio-libs/aiohttp/blob/707736b/aiohttp/http_websocket.py#L31-L42

The close code 1014, 1015 is missing, and one of my use case (websocket proxy) involves 1014,
which results in an unexpected error "Invalid close code: 1014".

I think we need to update the list.

@achimnol achimnol added the bug label Nov 2, 2020
@derlih
Copy link
Contributor

derlih commented Nov 2, 2020

@achimnol do you mean that the values are missing in the WSCloseCode enum?

@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2020

There is an interesting question: how to prevent such problem in future, when a new code will be added again?

@derlih
Copy link
Contributor

derlih commented Nov 3, 2020

It could be a GitHub action that periodically fetchs the https://github.com/mdn/content/blob/main/files/en-us/web/api/closeevent/index.html , parses it and compares to the enum in master branch. In case of mismatch creates an issue with a tag.

@derlih
Copy link
Contributor

derlih commented Nov 3, 2020

@achimnol I think 1015 shouldn't be set in aiohttp. The connection error should be raised in this case IMHO.
From MDN:

Indicates that the connection was closed due to a failure to perform a TLS handshake (e.g., the server certificate can't be verified).

@achimnol
Copy link
Member Author

achimnol commented Nov 4, 2020

Even the original RFC and its follow-up docs does not specify all of the close codes listed in the MDN document.
Since the status codes are largely left as "extensible", I think we should allow arbitrary unknown close codes in the range of 1000 to 4999 as valid with an optional conversion to pre-defined constant enums.

I agree with that 1015 might not be necessary for the application layer, i.e., aiohttp., but if we decide to accept any status code in the range of 1000 to 4999, this wouldn't be important anyway.

@derlih
Copy link
Contributor

derlih commented Nov 4, 2020

I think we are talking not only the enum values, but also about the handing the "unexpected" close codes.

def _feed_data(self, data: bytes) -> Tuple[bool, bytes]:
for fin, opcode, payload, compressed in self.parse_frame(data):
if compressed and not self._decompressobj:
self._decompressobj = zlib.decompressobj(wbits=-zlib.MAX_WBITS)
if opcode == WSMsgType.CLOSE:
if len(payload) >= 2:
close_code = UNPACK_CLOSE_CODE(payload[:2])[0]
if close_code < 3000 and close_code not in ALLOWED_CLOSE_CODES:
raise WebSocketError(
WSCloseCode.PROTOCOL_ERROR,
f"Invalid close code: {close_code}",
)

IMO the question is: should we change this behavior and how?
For example we could just use the value from close_code = UNPACK_CLOSE_CODE(payload[:2])[0] without additional checks and keep the enum to improve clearness of the code like http.HTTPStatus does.
But on the other hand it changes the behavior of the code.

May be it would be better to raise an excpetion if the code is not OK (1000).

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

Successfully merging a pull request may close this issue.

3 participants