cherrypy / cheroot

Cheroot is the high-performance, pure-Python HTTP server used by CherryPy. Docs -->
https://cheroot.cherrypy.dev
BSD 3-Clause "New" or "Revised" License
185 stars 90 forks source link

Processing OPTIONS request with query parameters in URL #142

Open IzmaylovAndrey opened 6 years ago

IzmaylovAndrey commented 6 years ago

That's behaviour appears after upgrading CherryPy from 11.0.0 to 12.0.0

andrey@asus-K55N:~$ curl -i -X OPTIONS http://localhost:5000/api/profile/tariffs?monthly=1
HTTP/1.1 404 NOT FOUND
Content-Type: text/html
Content-Length: 233
Access-Control-Allow-Origin: *
Date: Mon, 20 Nov 2017 13:15:34 GMT
Server: 0.0.0.0

andrey@asus-K55N:~$ curl -i -X OPTIONS http://localhost:5000/api/profile/tariffs
HTTP/1.1 200 OK
Content-Type: text/html; charset=utf-8
Allow: GET, OPTIONS, HEAD
Access-Control-Allow-Origin: *
Content-Length: 0
Date: Mon, 20 Nov 2017 13:15:37 GMT
Server: 0.0.0.0
webknjaz commented 6 years ago

Hi Andrey,

Thanks for reaching us. Have you tried downgrading just Cheroot?

We've refactored HTTP processing this summer in Cheroot, which might affect your case.

Could you please post some minimal reproducible code example for us to try it out?

IzmaylovAndrey commented 6 years ago

@webknjaz Hi, Sviatoslav, thanks for quick response I've already tried to downgrade cheroot to 5.8.3 version, that CherryPy 11.0.0 requires, with CherryPy 12.0.0, but it doesn't helps.

Unfortunately, now I have only proprietary project, that uses CherryPy and that I can't publish for open-source, but I'll try to make open-source project for testing that problem as soon as possible.

webknjaz commented 6 years ago

Sure, a tiny few-line script or a docker image would be enough.

webknjaz commented 6 years ago

Ref #1627

webknjaz commented 6 years ago

Prompt look into https://github.com/cherrypy/cherrypy/compare/v11.0.0...master#diff-1bdebf9f016e00f94b0a886fd3ed64acL409 didn't reveal possible source of issue to me.

Ideally, we need a clear test case and git bisect

IzmaylovAndrey commented 6 years ago

I make little project for this case: https://github.com/IzmaylovAndrey/hello-flask-cherrypy

If you'll need docker image: https://hub.docker.com/r/andreyizmaylov/hello-flask-cherrypy/

If you need some other help, I'll try to help you

webknjaz commented 6 years ago

@IzmaylovAndrey thanks. I may be unable to look at this till weekend because of slow and limited Internet connection. Maybe @jaraco will have some time earlier.

manthey commented 6 years ago

I've just ran into this as well.

webknjaz commented 6 years ago

@IzmaylovAndrey I'm starting to look into your STR and the first thing I noticed is that you use cherrypy only for WSGI, whereas WSGI part now completely lives in cheroot. You probably want to use cheroot directly and not require cherrypy dependency.

jaraco commented 6 years ago

I've distilled your repo to this script, which will run the server and test it with a few requests before exiting. It's configured to run with all the dependencies using rwt except cherrypy, so it can be run with different cherrypy versions. It requires Python 3.6.

Here are my results:

$ rwt -q cherrypy==11.0.0 -- issue-1662.py
first a get
then options no params
then options with params
$ rwt -q cherrypy==11.1.0 -- issue-1662.py
first a get
then options no params
then options with params
Traceback (most recent call last):
  File "issue-1662.py", line 57, in <module>
    @ServerContext()
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-dcxfy1c_/autocommand/autocommand.py", line 66, in autocommand_decorator
    func = automain(module)(func)
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-dcxfy1c_/autocommand/automain.py", line 55, in automain_decorator
    sys.exit(main(*args, **kwargs))
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-dcxfy1c_/autocommand/autoparse.py", line 301, in autoparse_wrapper
    return func(*parsed_args.args, **parsed_args.kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/contextlib.py", line 52, in inner
    return func(*args, **kwds)
  File "issue-1662.py", line 74, in run
    resp.raise_for_status()
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-dcxfy1c_/requests/models.py", line 935, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 404 Client Error: NOT FOUND for url: http://localhost:54171/hello?name=test

So somewhere between 11.0.0 and 11.1.0 is where the issue arises.

jaraco commented 6 years ago

git bisect is an absolute dream. Using that script, I bisected the codebase. It implicated c87bc9f73740dfe35e4d2ca9082dccb6278bdc53.

webknjaz commented 6 years ago

@IzmaylovAndrey It's not reproducible for me using the docker image you provided

$ curl -i -X OPTIONS http://localhost:5000/hello
HTTP/1.1 200 OK
Content-Type: text/html; charset=utf-8
Allow: GET, OPTIONS, HEAD
Content-Length: 0
Date: Wed, 29 Nov 2017 22:47:08 GMT
Server: 0.0.0.0
webknjaz commented 6 years ago

@jaraco I cannot think of anything in that commit introducing this issue.

webknjaz commented 6 years ago

@IzmaylovAndrey It also works the following way:

root@87ac7d0f52da:/# cat wsgi_srv.py
import cheroot.wsgi
from app import app
srv = cheroot.wsgi.Server(('0.0.0.0', 5000), app)
srv.safe_start()
IzmaylovAndrey commented 6 years ago

@webknjaz @jaraco Thank you for solving my problem! Svyatoslav's solution with using only cheroot works fine.

webknjaz commented 6 years ago

@manthey does the solution proposed above satisfies your needs as well?

jaraco commented 6 years ago

@webknjaz In your replication attempt, you’re not passing the query params. The 404 only occurs if there are query params present.

webknjaz commented 6 years ago

@jaraco In fact, I tried passing params as well. I just didn't post the log.

manthey commented 6 years ago

I'm not sure what to try. We start our application via

cherrypy.engine.start()
cherrypy.engine.block()
webknjaz commented 6 years ago

@manthey do you use anything non-WSGI from CherryPy? If so, can you share a minimal reproducible example?

manthey commented 6 years ago

Here is a simple example showing my issue:

import cherrypy

class First(object):
    exposed = True

    def GET(self, *args, **kwargs):
        return 'Did first get\n'

class Second(object):
    exposed = True

    def GET(self, *args, **kwargs):
        return 'Did second get\n'

    def OPTIONS(self, *args, **kwargs):
        return 'Did second options\n'

config = {
    '/': {
        'request.dispatch': cherrypy.dispatch.MethodDispatcher(),
    },
    '/test': {
        'request.dispatch': cherrypy.dispatch.MethodDispatcher(),
    },
}

Root = First()
Root.test = Second()
cherrypy.tree.mount(Root, '/', config)
cherrypy.engine.start()
cherrypy.engine.block()

If you curl "http://127.0.0.1:8080/test" -X GET, you see "Did second get". If you curl "http://127.0.0.1:8080/test?key=value" -X GET, you see "Did second get". If you curl "http://127.0.0.1:8080/test" -X OPTIONS, you see "Did second options" If you curl "http://127.0.0.1:8080/test?key=value" -X OPTIONS, you get a 405 error (this is different than in versions before 11.1 where it prints "Did second options".

mark-anders commented 6 years ago

I'm running into this too. Like @IzmaylovAndrey, I'm hosting a Flask app in CherryPy. Downgrading to 11.0.0 fixed my problem.

webknjaz commented 6 years ago

@mark-anders also look into using only Cheroot https://github.com/cherrypy/cherrypy/issues/1662#issuecomment-348028013

webknjaz commented 6 years ago

So I noticed when doing

$ http OPTIONS :8080/test?key=value

I see following in the log:

127.0.0.1 - - [26/Jan/2018:17:33:30] "OPTIONS /test?key=value?key=value HTTP/1.1" 405 1366 "" "HTTPie/0.9.2"

which is weird: why the params are double appended there?

webknjaz commented 6 years ago

Also, extra logging reveals that First object is selected for /test?key=value query, while Second object is selected for /test.

kieran-whittenburg commented 6 years ago

I ran into the same problem as in the snippet @webknjaz just posted. GET requests with parameters worked perfectly, but I was getting the same issue with duplicate params in OPTIONS preflight requests and it was breaking my entire workflow. I was getting 404 errors, not 405 errors, but otherwise exactly the same issue.

Downgrading to CherryPy 11.0.0 fixed the problem for me.

webknjaz commented 6 years ago

Well, downgrading is not a fix, just a workaround. I'm trying to find the root of issue and fix it finally. I guess it's related with with summer refactoring.

webknjaz commented 6 years ago

So for v11.0.0 wsgi env has:

'PATH_INFO': '/test'

And for master it's:

'PATH_INFO': '/test?key=value'
webknjaz commented 6 years ago

So I narrowed it down to this line: https://github.com/cherrypy/cherrypy/commit/c87bc9f73740dfe35e4d2ca9082dccb6278bdc53#diff-374eff2e4f53210da8f97f72e832e5d5R27 Basically, it's broken in Cheroot in case when proxy mode is enabled (proxy_mode=True), switching it to False fixes the behavior. But we need to fix underlying issue in Cheroot

webknjaz commented 6 years ago

I think, the bug it that here https://github.com/cherrypy/cheroot/blob/211e75e/cheroot/server.py#L741-L746 uri is not validated and in proxy mode it should either raise an exception if uri is not absolute or use path.

manthey commented 6 years ago

Is there anything I can do to help resolve this issue?

webknjaz commented 6 years ago

We need to improve cherrypy/cheroot#74 by making it RFC-compliant

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Rhistina commented 5 years ago

Hello. I'm trying to upgrade to cherrypy 18.1.0... Is the only viable workaround to use version 11?

aliwo commented 5 years ago

using cherrypy 18.1.0 flask 1.0.2

encountered same problem. I wasted the whole night yesterday not knowing this is a bug :(

grove commented 5 years ago

I can confirm that we have also run into this exact issue. 404 is returned in OPTIONS requests when there are query parameters in the URL.

grove commented 5 years ago

cheroot 8.2.0 has a fix for this and it seems to work for us.

webknjaz commented 5 years ago

I guess we still need tests for this. Expecially, with proxy_mode reverted to False here: https://github.com/cherrypy/cherrypy/commit/c87bc9f73740dfe35e4d2ca9082dccb6278bdc53#diff-374eff2e4f53210da8f97f72e832e5d5R27