Merge pull request #423 from kenballus/main

Validate HTTP versions and methods

src/waitress/adjustments.py

@@ -290,7 +290,6 @@ class Adjustments:
     server_name = "waitress.invalid"
 
     def __init__(self, **kw):
-
         if "listen" in kw and ("host" in kw or "port" in kw):
             raise ValueError("host or port may not be set if listen is set.")
 

src/waitress/buffers.py

@@ -23,7 +23,6 @@
 
 
 class FileBasedBuffer:
-
     remain = 0
 
     def __init__(self, file, from_buffer=None):

src/waitress/parser.py

@@ -247,6 +247,9 @@ def parse_header(self, header_plus):
 
         # command, uri, version will be bytes
         command, uri, version = crack_first_line(first_line)
+        if command == uri == version == b"":
+            raise ParsingError("Start line is invalid")
+
         # self.request_uri is like nginx's request_uri:
         # "full original request URI (with arguments)"
         self.request_uri = uri.decode("latin-1")
@@ -407,36 +410,33 @@ def get_header_lines(header):
 
 
 first_line_re = re.compile(
-    b"([^ ]+) "
-    b"((?:[^ :?#]+://[^ ?#/]*(?:[0-9]{1,5})?)?[^ ]+)"
-    b"(( HTTP/([0-9.]+))$|$)"
+    rb"(?P<method>[!#$%&'*+\-.^_`|~0-9A-Za-z]+) "
+    rb"(?P<uri>(?:[^ :?#]+://[^ ?#/]*(?:[0-9]{1,5})?)?[^ ]+)"
+    rb"(?: HTTP/(?P<version>[0-9]\.[0-9]))?"
 )
 
 
 def crack_first_line(line):
-    m = first_line_re.match(line)
+    m = first_line_re.fullmatch(line)
 
-    if m is not None and m.end() == len(line):
-        if m.group(3):
-            version = m.group(5)
-        else:
-            version = b""
-        method = m.group(1)
+    if m is None:
+        return b"", b"", b""
 
-        # the request methods that are currently defined are all uppercase:
-        # https://www.iana.org/assignments/http-methods/http-methods.xhtml and
-        # the request method is case sensitive according to
-        # https://tools.ietf.org/html/rfc7231#section-4.1
+    version = m["version"] or b""
+    method = m["method"]
+    uri = m["uri"]
 
-        # By disallowing anything but uppercase methods we save poor
-        # unsuspecting souls from sending lowercase HTTP methods to waitress
-        # and having the request complete, while servers like nginx drop the
-        # request onto the floor.
+    # the request methods that are currently defined are all uppercase:
+    # https://www.iana.org/assignments/http-methods/http-methods.xhtml and
+    # the request method is case sensitive according to
+    # https://tools.ietf.org/html/rfc7231#section-4.1
 
-        if method != method.upper():
-            raise ParsingError('Malformed HTTP method "%s"' % str(method, "latin-1"))
-        uri = m.group(2)
+    # By disallowing anything but uppercase methods we save poor
+    # unsuspecting souls from sending lowercase HTTP methods to waitress
+    # and having the request complete, while servers like nginx drop the
+    # request onto the floor.
 
-        return method, uri, version
-    else:
-        return b"", b"", b""
+    if method != method.upper():
+        raise ParsingError('Malformed HTTP method "%s"' % str(method, "latin-1"))
+
+    return method, uri, version

src/waitress/receiver.py

@@ -19,7 +19,6 @@
 
 
 class FixedStreamReceiver:
-
     # See IStreamConsumer
     completed = False
     error = None
@@ -61,7 +60,6 @@ def getbuf(self):
 
 
 class ChunkedReceiver:
-
     chunk_remainder = 0
     validate_chunk_end = False
     control_line = b""

src/waitress/server.py

@@ -177,7 +177,6 @@ def close(self):
 
 
 class BaseWSGIServer(wasyncore.dispatcher):
-
     channel_class = HTTPChannel
     next_channel_cleanup = 0
     socketmod = socket  # test shim
@@ -372,7 +371,7 @@ def getsockname(self):
         )
 
     def set_socket_options(self, conn):
-        for (level, optname, value) in self.adj.socket_options:
+        for level, optname, value in self.adj.socket_options:
             conn.setsockopt(level, optname, value)
 
 

src/waitress/task.py

@@ -190,7 +190,7 @@ def build_response_header(self):
         server_header = None
         connection_close_header = None
 
-        for (headername, headerval) in self.response_headers:
+        for headername, headerval in self.response_headers:
             headername = "-".join([x.capitalize() for x in headername.split("-")])
 
             if headername == "Content-Length":

src/waitress/utilities.py

@@ -143,6 +143,8 @@ def unpack_rfc822(m):
 )
 
 rfc850_reg = re.compile(rfc850_date)
+
+
 # they actually unpack the same way
 def unpack_rfc850(m):
     g = m.group

src/waitress/wasyncore.py

@@ -274,7 +274,6 @@ def compact_traceback():
 
 
 class dispatcher:
-
     debug = False
     connected = False
     accepting = False

tests/test_functional.py

@@ -63,7 +63,6 @@ def __init__(self, application, queue, **kw):  # pragma: no cover
 
 
 class SubprocessTests:
-
     exe = sys.executable
 
     server = None
@@ -133,7 +132,6 @@ def send_check_error(self, to_send):
 
 
 class TcpTests(SubprocessTests):
-
     server = FixtureTcpWSGIServer
 
     def make_http_connection(self):
@@ -924,7 +922,6 @@ def test_no_content_length(self):
 
 
 class TooLargeTests:
-
     toobig = 1050
 
     def setUp(self):
@@ -1606,7 +1603,6 @@ def __init__(self, application, queue, **kw):  # pragma: no cover
             queue.put(self.socket.getsockname())
 
     class UnixTests(SubprocessTests):
-
         server = FixtureUnixWSGIServer
 
         def make_http_connection(self):

tests/test_parser.py

@@ -35,7 +35,6 @@
 
 class TestHTTPRequestParser(unittest.TestCase):
     def setUp(self):
-
         my_adj = Adjustments()
         self.parser = HTTPRequestParser(my_adj)
 
@@ -98,7 +97,6 @@ def test_received_already_completed(self):
         self.assertEqual(result, 0)
 
     def test_received_cl_too_large(self):
-
         self.parser.adj.max_request_body_size = 2
         data = b"GET /foobar HTTP/8.4\r\nContent-Length: 10\r\n\r\n"
         result = self.parser.received(data)
@@ -107,7 +105,6 @@ def test_received_cl_too_large(self):
         self.assertTrue(isinstance(self.parser.error, RequestEntityTooLarge))
 
     def test_received_headers_not_too_large_multiple_chunks(self):
-
         data = b"GET /foobar HTTP/8.4\r\nX-Foo: 1\r\n"
         data2 = b"X-Foo-Other: 3\r\n\r\n"
         self.parser.adj.max_request_header_size = len(data) + len(data2) + 1
@@ -119,7 +116,6 @@ def test_received_headers_not_too_large_multiple_chunks(self):
         self.assertFalse(self.parser.error)
 
     def test_received_headers_too_large(self):
-
         self.parser.adj.max_request_header_size = 2
         data = b"GET /foobar HTTP/8.4\r\nX-Foo: 1\r\n\r\n"
         result = self.parser.received(data)
@@ -252,7 +248,6 @@ def test_parse_header_transfer_encoding_invalid(self):
             self.assertTrue(False)
 
     def test_parse_header_transfer_encoding_invalid_multiple(self):
-
         data = b"GET /foobar HTTP/1.1\r\ntransfer-encoding: gzip\r\ntransfer-encoding: chunked\r\n"
 
         try:
@@ -571,6 +566,14 @@ def test_crack_first_line_missing_version(self):
         result = self._callFUT(b"GET /")
         self.assertEqual(result, (b"GET", b"/", b""))
 
+    def test_crack_first_line_bad_method(self):
+        result = self._callFUT(b"GE\x00 /foobar HTTP/8.4")
+        self.assertEqual(result, (b"", b"", b""))
+
+    def test_crack_first_line_bad_version(self):
+        result = self._callFUT(b"GET /foobar HTTP/.1.")
+        self.assertEqual(result, (b"", b"", b""))
+
 
 class TestHTTPRequestParserIntegration(unittest.TestCase):
     def setUp(self):
@@ -713,7 +716,6 @@ def testSpoofedHeadersDropped(self):
 
 class Test_unquote_bytes_to_wsgi(unittest.TestCase):
     def _callFUT(self, v):
-
         return unquote_bytes_to_wsgi(v)
 
     def test_highorder(self):