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

webtest.openURL sends an absolute URI when passed as "url" #1621

Closed
bertjwregeer opened this Issue Aug 15, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@bertjwregeer

bertjwregeer commented Aug 15, 2017

  • I'm submitting a ...
    [x] bug report
    [ ] feature request
    [ ] question about the decisions made in the repository

  • Do you want to request a feature or report a bug?

Currently openURL will pass the provided URL directly to the remote server, without parsing it. This no longer works due to changes made in cherrypy/cheroot#39 that disallows absolute URI's unless the server is configured as a proxy server.

  • What is the current behavior?

💥

  • If the current behavior is a bug, please provide the steps to reproduce and if possible a screenshots and logs of the problem.
[2017-08-15 21:11:17,794] (/home/vagrant/src/backend-http/tests/controllers/base.py:base:GET) - DEBUG - Requesting URL: http://localhost:35100/api/account/users/nothere/
Absolute URI not allowed if server is not a proxy.
  • What is the expected behavior?

That openURL does the appropriate thing and sends a valid HTTP request.

  • What is the motivation / use case for changing the behavior?

The RFC's state that absolute URI's are not allowed to be sent to a server unless it is a proxy server.

  • Other information

This is my work-around in a pytest fixture, this works for me. Most likely openURL should update it's host/port information if receiving an absolute URI to be able to connect to alternate host/port than what is configured for the WebCase.

    from cherrypy.test.webtest import openURL
    from urlparse import urlsplit
    _broken_openURL = openURL

    def openURL(url, *args, **kw):
        scheme, authority, path, qs, fragment = urlsplit(url)
        return _broken_openURL(path + '?' + qs, *args, **kw)

    webtest.openURL = openURL
@percious

This comment has been minimized.

percious commented Nov 28, 2017

+1

@webknjaz

This comment has been minimized.

Member

webknjaz commented Nov 28, 2017

Yes, sounds reasonable. It would be awesome to see this in the form of PR from the start, so that it's already actionable and would require less time for maintainers to decide and hit merge :)

@webknjaz

This comment has been minimized.

Member

webknjaz commented Nov 28, 2017

@bertjwregeer so now openURL moved to cheroot completely along with other stuff from cherrypy.test.webtest:
https://github.com/cherrypy/cherrypy/blob/311c215/cherrypy/test/webtest.py#L6

As you can see, cheroot.test.webtest.openURL accepts separate host and port arguments
https://github.com/cherrypy/cheroot/blob/6ff899d/cheroot/test/webtest.py#L533

On the other hand I can understand why you'd want to have a function supporting absolute-uri notation. Would you be satisfied with a separate function wrapping original one?

@webknjaz

This comment has been minimized.

Member

webknjaz commented Nov 28, 2017

I've added cheroot.test.webtest.openAbsoluteURI. Hopefully it will satisfy your needs.

@bertjwregeer

This comment has been minimized.

bertjwregeer commented Nov 28, 2017

No, this doesn't satisfy my needs. All of our testing uses webtest.openURL which does the wrong thing, it sends the full URL as part of the request:

GET http://fullurl.com:port/is/here/ HTTP/1.1

The above is an absoluteURI request. The new openAbsoluteURI you just created actually turns the request into an abs_path request, which is the only thing that is valid to send to a non-proxy server.

See https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html section 5.1.2:

The absoluteURI form is only allowed when the request is being made to a proxy.

Since in almost all cases of testing, when the user passes a URL to openURL they want to do a request against the locally running cherrypy you should NOT pass an absoluteURI instead you need to parse it, and send an abs_path with the query string parameters.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Nov 29, 2017

Sorry, I can't catch what is the difference, but I definitely don't want to change existing function behavior (because it would break someone else's flow).

You could use openURL('/is/here/', host='fullurl.com', port=port), what's wrong with it?

@webknjaz webknjaz reopened this Nov 29, 2017

@jaraco

This comment has been minimized.

Member

jaraco commented Nov 29, 2017

I was a little confused by this openURL thing until I realized that the url parameter is mis-named. It really should be request_uri... and it's the identifier as passed to the server.

In the systems where we use webtest, we pass only the uri portion of the request and not an absolute URL.

Since in almost all cases of testing, when the user passes a URL to openURL they want to do a request against the locally running cherrypy...

I think the user shouldn't pass a full URL as the URL parameter to openURL. openURL is not trying to mimic requests, but is instead soliciting the request uri as will be passed in the HTTP request line.

If I were you, I'd rewrite your tests not to have absolute URLs at all... or if having them is meaningful and useful, I'd continue to use your adapter wrapper. I'd not be opposed to supplying that adapter wrapper as a convenience if you can present a good reason why cheroot should support that interface.

@bertjwregeer

This comment has been minimized.

bertjwregeer commented Nov 29, 2017

@jaraco all of our API's currently return full URL's in JSON responses (using a standard known as Shoji) which would just be used directly in openURL, until the change was made in cheroot, this worked without issues because CherryPy (cheroot specifically) would happily accept the absoluteURI even though it is not a proxy server, and thus sending the full URL to the remote server was not an issue.

Now that the changes have been made in cheroot to no longer accept the absoluteURI when sending a request because it is not a proxy server (thereby upholding the standards better than before) existing tests/clients that are using openURL are broken.

I suggested the fix above because the goal was to send a request to the webtest server, and it is now expecting a valid request URI. I feel that stripping off the scheme/domain name makes sense to maintain backwards compatibility with existing test code, rather than forcing all users to rewrite their tests because cheroot has become more strict in what you may send it.

@jaraco

This comment has been minimized.

Member

jaraco commented Dec 4, 2017

I think now I understand why openAbsoluteURI isn't any help. The typical use of webtest is to invoke WebCase.getPage, which hard-codes the call to openURL. Thus, the addition of openAbsoluteURI provides little value for that use case. Moreover, it seems we've made a backward-incompatible change without providing guidance or clarity on how to adapt.

In cherrypy/cheroot (commit referenced above), I've taken a different tack to address this need. @bertjwregeer , can you take a look and see if the new approach will address your needs?

@jaraco

This comment has been minimized.

Member

jaraco commented Dec 4, 2017

CI tests caught an error in the commit above. Check out cherrypy/cheroot@31f97a3 instead.

@bertjwregeer

This comment has been minimized.

bertjwregeer commented Dec 4, 2017

This looks good. I can work with that.

@jaraco

This comment has been minimized.

Member

jaraco commented Dec 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment