search

moccu / django-omnibus

django-omnibus is a Django library which helps to create websocket-based connections between a browser and a server to deliver messages.
BSD 3-Clause "New" or "Revised" License
70 stars 14 forks source link

Fix 403 errors from tornado.access when handling new WebSocket connections #25

Closed synthead closed 9 years ago

synthead commented 9 years ago

I think an update to python-tornado (I'm on 4.0.2) breaks incoming WebSocket connections in Omnibus. In Googling around, the common answer is to simply add a check_origin method that returns True. This mimics the behavior tornado used to have and allows the WebSocket connections gracefully.

Before this commit, these messages would be displayed from omnibusd:

$ ./manage.py omnibusd
WARNING:tornado.access:403 GET /ec (127.0.0.1) 0.90ms
WARNING:tornado.access:403 GET /ec (127.0.0.1) 0.69ms
WARNING:tornado.access:403 GET /ec (127.0.0.1) 0.51ms
WARNING:tornado.access:403 GET /ec (127.0.0.1) 0.58ms
WARNING:tornado.access:403 GET /ec (127.0.0.1) 0.59ms
WARNING:tornado.access:403 GET /ec (127.0.0.1) 1.05ms
WARNING:tornado.access:403 GET /ec (127.0.0.1) 1.05ms
WARNING:tornado.access:403 GET /ec (127.0.0.1) 0.63ms

Some references: https://github.com/mopidy/mopidy/issues/788 http://stackoverflow.com/questions/24851207/tornado-403-get-warning-when-opening-websocket http://stackoverflow.com/questions/25177147/tornadio2-failed-error-during-websocket-handshake-unexpected-response-code

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.23%) when pulling 4ddc3b777525113cbc6531572d191f35323c07df on synthead:master into 3da735738ff8c688bcd530f1b7eb14401f6c7bbe on moccu:master.

synthead commented 9 years ago

More information from Tornado's documentation: http://www.tornadoweb.org/en/stable/websocket.html#tornado.websocket.WebSocketHandler.check_origin

EnTeQuAk commented 9 years ago

Hey @synthead thanks for the pull request, though, I'm wondering - do you explicitly need tornado 4.x? There is a branch in the works that fixes a few things - among them an upgrade to tornado 4.x (https://github.com/moccu/django-omnibus/commit/514c87a31267ab419958e4bc955d3e727f7e1f8f) - so I'm wondering - did you upgrade on purpose or did something upgrade the tornado package without acknowledging our <=4.x constraint?

EnTeQuAk commented 9 years ago

I'm merging this for now, I'm working on some other bugfixes and attempt to release a v0.2.1 sometime during the next few days.

synthead commented 9 years ago

Ah, I didn't realize that there is a <=4.x constraint! I should have payed more attention to the requirements. I upgraded Tornado on my machines in a routine full package upgrade and didn't think anything of it.

Will the 0.2.1 release provide support for the new Tornado?

EnTeQuAk commented 9 years ago

I can test it with 4.x - sure.