Bugfix/query string #39

digitalresistor · 2017-07-12T00:13:42Z

Fix for #38

webknjaz · 2017-07-12T14:06:04Z

cheroot/server.py

@@ -874,14 +870,14 @@ def parse_request_uri(self, uri):
            # If there's a scheme (and it must be http or https), then:
            # http_URL = "http:" "//" host [ ":" port ] [ abs_path [ "?" query
            # ]]
-            return parsed.scheme, parsed.netloc, parsed.path
+            return parsed.scheme, parsed.netloc, parsed.path, parsed.query


Could you please alter the docsrting of this method to match new behavior?

webknjaz

return at line 865 shall be updated as well, please also test this case as well as all other returns in if clauses

digitalresistor · 2017-07-12T15:20:12Z

I can update the docstring and the return line as requested.

I will need help writing test cases for the rest of it, I don't see any unit tests for this piece of code to verify that it works as intended (unless I missed them in my grepping). I am not sure that a full on HTTP request will hit all of the points it currently attempts to support.

digitalresistor · 2017-07-12T16:16:22Z

Docstring and return statement added.

webknjaz · 2017-07-12T23:42:48Z

@bertjwregeer possible test cases to cover are uri in ['*', '/some_abs_uri', 'some_rel_uri']. Feel free to add 'em next to the one you already wrote. Thanks in advance :)

digitalresistor · 2017-07-13T01:00:55Z

* is only valid in the case of an OPTIONS request, and can't be tested with a simple GET
relative_uri are never valid in the HTTP spec
absolute_path is already tested, since it is the default
absolute_uri is valid for a proxy and not for a general HTTP server and should be rejected unless the server is a proxy server.

See https://tools.ietf.org/html/rfc7230#section-5.3

I have added the tests, but I don't believe that getPage() will pass them through without modifying them by prepending a /.

Edit: Wow, it does...

digitalresistor · 2017-07-13T01:03:30Z

I also noticed that this is false: https://github.com/bertjwregeer/cheroot/blob/791f126947bd9406fe5648a089be778106672544/cheroot/server.py#L722

Since the returned path from urlparse will have it's params stripped (at least for the last component), since it is parsed. You'll want to modify parse_request_uri to return those too. Since it is part of "parsed.params". See https://docs.python.org/2/library/urlparse.html

digitalresistor · 2017-07-13T01:09:15Z

Actually Python recommends using urlsplit instead: https://docs.python.org/2/library/urlparse.html#urlparse.urlsplit since it will leave the path parameters alone for later processing.

digitalresistor · 2017-07-13T02:44:06Z

Alright. I'm done.

josephtate

LGTM

webknjaz · 2017-07-15T09:15:13Z

Sorry, I'm at EuroPython currently. I'll check the changes ASAP once have some free time. ETA: this week.
If I fail to give you a feedback or release changes, please feel free to ping me on Monday.

P.S. Thank you for your efforts! You've done much more than I expected, I really appreciate this.

josephtate · 2017-07-20T02:20:45Z

@webknjaz do you still have objections?

webknjaz · 2017-07-21T22:17:49Z

Sorry, I wrote a few comments and forgot to hit publish button. Yes, there are a couple of nitpicks.

webknjaz · 2017-07-15T22:23:56Z

cheroot/server.py


-        if scheme:
-            self.scheme = scheme
+        if method.upper() == 'OPTIONS' and not self.proxy:


According to RFC 2616 (section 5.1.1) and its successors RFC 7230 (section 3.1.1) and RFC 7231 (section 4.1) method names are case-sensitive and uppercase. Otherwise the request line is malicious and HTTP server must return 400 Bad Request.

Another nitpick: .upper() is being called 3 times for the most of requests, which should be avoided.

webknjaz · 2017-07-15T22:39:16Z

cheroot/server.py

        """Initialize HTTP request container instance.

        Args:
            server (HTTPServer): web server object receiving this request
            conn (HTTPConnection): HTTP connection object for this request
+
+        Kwargs:
+            proxy (Boolean): Whether this HTTPServer should behave as a PROXY


Please use bool for type, because its actual builtin you refer to

Please start the description with lowercase letter, because of the style used everywhere around

I'd vote for proxy_mode var name (just a nitpick)

webknjaz · 2017-07-15T22:39:34Z

cheroot/server.py

+        Kwargs:
+            proxy (Boolean): Whether this HTTPServer should behave as a PROXY
+            server for certain requests.
+            strict (Boolean): Whether we should return a 400 Bad Request when


Nit: strict_mode

webknjaz · 2017-07-15T22:40:12Z

cheroot/server.py

@@ -628,6 +635,8 @@ def __init__(self, server, conn):
        self.close_connection = self.__class__.close_connection
        self.chunked_read = False
        self.chunked_write = self.__class__.chunked_write
+        self.proxy = proxy


Naming this attribute is_proxy would be beneficial

(or is_proxy_mode etc.)

digitalresistor · 2017-07-21T23:30:07Z

You have push permissions to the branch on my repo as a maintainer. Feel free to make those modifications and push there; or pull these changes into a local branch and fixup before pushing back to a new PR.

I already maintain Waitress, I really have no want or desire to maintain or spend even more time fixing yet another Python HTTP server.

webknjaz · 2017-07-22T08:53:02Z

Deal, thanks for your contribution anyway :)

Since we go through the trouble of parsing it, might as well keep em. Especially if we want to be a "proxy" these might be important.

webknjaz · 2017-07-31T12:59:04Z

cheroot/server.py

+                return False
+            path = b'%2F'.join(atoms)
+
+        if path[0] != b'/':


Wow! It turned out that path[0] returns int, not bytes, which is 47 for /, thus prepending / unconditionally.

(just a note)

Ref: PEP 358

.__getitem__(int | slice) -> int | bytes

jaraco · 2017-07-31T16:47:13Z

cheroot/server.py

@@ -598,12 +599,17 @@ class HTTPRequest(object):
    A HeaderReader instance or compatible reader.
    """

-    def __init__(self, server, conn):
+    def __init__(self, server, conn, proxy_mode=False, strict_mode=True):


I'm not a fan of boolean parameters, especially when they represent a switch on behavior, because they often are limited in what they can describe and lead to branchy and unmaintainable code. Often it's better to use subclasses or handler functions or enumerated values to signal behavior. I'm not sure what's the best approach here without digging deeper, but I wanted to signal this potential issue.

I see your point, but currently I cannot suggest cleaner solution. So for now I'm going to merge this as is.

webknjaz · 2017-08-01T16:45:30Z

@bertjwregeer I've cut the tag v5.8.0, it will be available @ PYPI once this build succeeds.

webknjaz · 2017-08-05T18:14:46Z

JFI: merging this broke some things and integration with CherryPy, so this is going to be fixed.

Fix URI encoding issue introduced in #39

Fixes #39 regression

webknjaz reviewed Jul 12, 2017

View reviewed changes

webknjaz requested changes Jul 12, 2017

View reviewed changes

josephtate approved these changes Jul 15, 2017

View reviewed changes

webknjaz reviewed Jul 21, 2017

View reviewed changes

Bert JW Regeer and others added 13 commits July 31, 2017 13:27

Add failing test

5a8cecc

Add fix for failing test

b5db875

Make sure OPTIONS supports paths

1179e03

Save scheme/authority

e396b23

Since we go through the trouble of parsing it, might as well keep em. Especially if we want to be a "proxy" these might be important.

Add a strict mode

aeabf6b

Update tests for the new strict mode

1f58cc0

Add fragment test

bdf8c6b

Make sure path always starts with /

97f141a

Adjust docstring style

7b53a4b

Change proxy arg to proxy_mode

85a9e84

Change strict arg to strict_mode

ffca032

Adjust coding style

54c3e39

Add strict check for uppercase HTTP method name

17415cf

webknjaz reviewed Jul 31, 2017

View reviewed changes

webknjaz added 3 commits July 31, 2017 16:13

Fix path starting with slash check

648c564

Use forward slash constant in cheroot.server

4000052

Use constants for quoted slash code in server mod

6297e2d

jaraco reviewed Jul 31, 2017

View reviewed changes

webknjaz added 7 commits July 31, 2017 20:20

Fix decoding of unicode URIs and add smoke test

c8c5d5b

Drop unused NUMBER_SIGN and QUESTION_MARK consts

a0089d0

Add test for non-uppercase HTTP method

e86cc3e

Fix str.format markers to support 2.6

91d8f70

Fix HTTP method comparison

5d17992

Improve test checks for absolute and asterisk URI

aa2f792

Improve handling of unencoded URI and add test

1da3f9e

webknjaz approved these changes Jul 31, 2017

View reviewed changes

Improve unicode in URI check under Python 2

51c004a

webknjaz merged commit 51c004a into cherrypy:master Aug 1, 2017

webknjaz added a commit that referenced this pull request Aug 5, 2017

Recode URI using UTF-8 in test.helper

8dd412a

Fix URI encoding issue introduced in #39

webknjaz added a commit that referenced this pull request Aug 6, 2017

Check that absolute-path@request-target starts w /

eee2204

Fixes #39 regression

webknjaz mentioned this pull request Aug 11, 2017

cherrypy doesn't handle case where URL path contains multiple slashes cherrypy/cherrypy#815

Closed

digitalresistor deleted the bugfix/query_string branch August 15, 2017 19:48

digitalresistor mentioned this pull request Aug 15, 2017

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

Closed

jaraco mentioned this pull request Nov 15, 2017

Non-ascii no longer allowed in URIs on Python 2 only #55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/query string #39

Bugfix/query string #39

digitalresistor commented Jul 12, 2017

webknjaz Jul 12, 2017

webknjaz left a comment

digitalresistor commented Jul 12, 2017

digitalresistor commented Jul 12, 2017

webknjaz commented Jul 12, 2017

digitalresistor commented Jul 13, 2017 •

edited

Loading

digitalresistor commented Jul 13, 2017

digitalresistor commented Jul 13, 2017

digitalresistor commented Jul 13, 2017

josephtate left a comment

webknjaz commented Jul 15, 2017

josephtate commented Jul 20, 2017

webknjaz commented Jul 21, 2017

webknjaz Jul 15, 2017

webknjaz Jul 15, 2017

webknjaz Jul 15, 2017

webknjaz Jul 15, 2017

webknjaz Jul 15, 2017

digitalresistor commented Jul 21, 2017

webknjaz commented Jul 22, 2017

webknjaz Jul 31, 2017

webknjaz Jul 31, 2017

jaraco Jul 31, 2017

webknjaz Jul 31, 2017

webknjaz commented Aug 1, 2017

webknjaz commented Aug 5, 2017

Bugfix/query string #39

Bugfix/query string #39

Conversation

digitalresistor commented Jul 12, 2017

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

digitalresistor commented Jul 12, 2017

digitalresistor commented Jul 12, 2017

webknjaz commented Jul 12, 2017

digitalresistor commented Jul 13, 2017 • edited Loading

digitalresistor commented Jul 13, 2017

digitalresistor commented Jul 13, 2017

digitalresistor commented Jul 13, 2017

josephtate left a comment

Choose a reason for hiding this comment

webknjaz commented Jul 15, 2017

josephtate commented Jul 20, 2017

webknjaz commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalresistor commented Jul 21, 2017

webknjaz commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented Aug 1, 2017

webknjaz commented Aug 5, 2017

digitalresistor commented Jul 13, 2017 •

edited

Loading