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

Sanic support: buggy request parsing #42

Closed
MarSoft opened this issue Mar 23, 2017 · 1 comment · Fixed by #43
Closed

Sanic support: buggy request parsing #42

MarSoft opened this issue Mar 23, 2017 · 1 comment · Fixed by #43

Comments

@MarSoft
Copy link
Contributor

MarSoft commented Mar 23, 2017

When trying to use websocket support with git version of Sanic, I had a problem: server kept responding with 400 Bad request when the browser tried to upgrade connection to websocket.
In the logs I saw the following:

WARNING:engineio:Invalid session 060c95b4c0d54ba5b95665acf1ed751d?EIO=3

Turned out that engineio.AsyncServer.handle_request got two values for sid, first of which is the wrong and second is correct.
Next, QUERY_STRING was taken from the output of self._async['translate_request']. And Sanic-specific implementation turned out to be buggy.

In engineio.async_sanic.translate_request function (line 46) it takes request.url and conditionally appends request.query_string to it. But according to Sanic docs, request.url holds "The full URL of the request, ie: http://localhost:8000/posts/1/?foo=bar"
This bug resulted in query-string being duplicated and URL being treated like this:
'http://localhost:8000/socket.io/?EIO=3&transport=polling&t=LhzFKOB&sid=c0a70e70d8bc465e9611f5bae89ce083?EIO=3&transport=polling&t=LhzFKOB&sid=c0a70e70d8bc465e9611f5bae89ce083 - notice the duplication.

I will now propose the trivial fix as a pull request.

MarSoft added a commit to MarSoft/python-engineio that referenced this issue Mar 23, 2017
According to sanic docs, `request.url` already contains query string, so adding it results in data corruption.
This fix worked for me.
Fixes miguelgrinberg#42
@miguelgrinberg
Copy link
Owner

Thanks for the detailed description.

Turns out they have changed the way request.url works just a few days ago, in this commit: sanic-org/sanic@1fbde87. That used to be just the path portion of the URL, now they have changed it to the full URL, and if you just want the path, that's request.path. Oh well...

Thanks again. I'll go ahead and merge your change, but I'll hold off on a release, since the current release works well with the 0.4.x releases of sanic and this change will break those. What a mess.

miguelgrinberg pushed a commit that referenced this issue Mar 23, 2017
According to sanic docs, `request.url` already contains query string, so adding it results in data corruption.
This fix worked for me.
Fixes #42
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 a pull request may close this issue.

2 participants