search

Lawouach / WebSocket-for-Python

WebSocket client and server library for Python 2 and 3 as well as PyPy (ws4py 0.5.1)
https://ws4py.readthedocs.org/en/latest/
BSD 3-Clause "New" or "Revised" License
1.12k stars 289 forks source link

Fix Incorrect 'Sec-WebSocket-Accept' header value #252

Closed h3llrais3r closed 5 years ago

h3llrais3r commented 6 years ago

For cherrypyserver on Python 3 and above (tested with Python 3.6) Same logic applied as in wsgiutils.py

Screenshot of issue: image

As you can see, the value of the 'Sec-Websocket-Accept' header is in the wrong format. It must be decoded when running Py3.

h3llrais3r commented 6 years ago

@Lawouach Can you review it, merge and release it if you agree? This issue is blocking my websockets on py3 in combination with cherrypy. Thanks!

Lawouach commented 6 years ago

I think this as a bug in CherryPy that has been fixed and released in 18.0.1

https://github.com/cherrypy/cherrypy/issues/1738#issuecomment-419641132

Mind trying updating CherryPy just to see if that helps before we decide to merge this one?

Thanks.

h3llrais3r commented 6 years ago

@Lawouach I cannot upgrade to CherryPy 18.0.0 or above because it's Python3 only. I need support for both Python2 and Python3 and the latest version is 17.4.0.

I find it a bit strange that you did the same decoding for the wsgiutils.py, so I would assume that you do it also for cherrypyserver.py. :wink:

Please reconcider merging this one because versions below 18.0.0 of cherrypy should be able to work also with this fix.

Other solution is backporting the fix made in 18.0.1 to the 17.x maintenance branch. I tried it and this also works. image

@jaraco Any thoughts on that?

I leave it up to you 2 guys to decide where it should be fixed. :wink:

h3llrais3r commented 6 years ago

@Lawouach, @jaraco, in which project do you think it should be fixed? If needed, I can also make the PR for CherryPy with the changes as described in my previous comment.

Lawouach commented 6 years ago

I can merge it in this project but I'm away until next week. I'll try to cut a new release by Wednesday.

I don't know if the CherryPy project will backport though.

jaraco commented 6 years ago

Happy to have bugfixes applied to 17.x. @h3llrais3r , would you make the PR? Just target the pr to the maint/17.x branch in CP and reference the bug/PR/commit that fixed it for 18.x? Thanks.

h3llrais3r commented 5 years ago

@Lawouach, @jaraco I've provided the PR for both libraries... For which option will we go? Or perhaps for both?

Lawouach commented 5 years ago

Since the change was made in the master branch of CherryPy (https://github.com/cherrypy/cherrypy/issues/1738#issuecomment-419641132) I feel there would be symmetry in using your PR you submitted over there.

h3llrais3r commented 5 years ago

@Lawouach, I see that @jaraco has just merged it in the cherrypy:maint/17.x branch, so it should be fixed. 👍