Tighten four RFC 9112 rules that Waitress did not enforce

CHANGES.txt

@@ -1,5 +1,39 @@
-Unreleased
-----------
+4.0.0 (unreleased)
+------------------
+
+Backward Incompatibilities
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+- A request whose HTTP version has a major version other than 1 is now
+  rejected with a 505 (HTTP Version Not Supported), and one with a higher
+  minor version than Waitress implements, such as ``HTTP/1.9``, is processed
+  as ``HTTP/1.1``. RFC 9112 section 2.3 asks for both. Previously any
+  ``[0-9].[0-9]`` was accepted and kept verbatim, and because everything
+  version dependent is keyed on the version being exactly ``1.0`` or ``1.1``,
+  an unrecognised one fell through all of it: a request claiming ``HTTP/2.0``
+  had its ``Connection`` and ``Expect`` header fields silently ignored.
+
+- Obsolete line folding in a request header field is now rejected with a 400
+  (Bad Request). RFC 9112 section 5.2 requires a server to either reject an
+  obs-fold or replace it with SP before interpreting the field value, and
+  folding has been deprecated since RFC 7230 in 2014. Waitress used to join
+  the continuation onto the preceding line, which left it reading a field one
+  way while something in front of it that rejects folding, or unfolds it
+  differently, read it another.
+
+- Header fields named in the ``Connection`` header field of a request are no
+  longer passed to the WSGI application. RFC 9110 section 7.6.1 makes them
+  connection specific, applying to a single hop and not to be forwarded.
+  Waitress is the end of the connection, so handing them to the application
+  amounts to forwarding them: a client could name a header there that a proxy
+  in front of Waitress believes only it controls, and the application has no
+  way to tell the two apart.
+
+- Only empty lines are now skipped before the request-line. RFC 9112 section
+  2.2 sanctions ignoring a CRLF received before a request-line, which lets a
+  client send a spare one after a request when pipelining. Waitress stripped
+  any leading whitespace, which also covered spaces, tabs, vertical tabs, form
+  feeds and lone CR or LF octets, none of which may appear there.
 
 Bugfix
 ~~~~~~

src/waitress/parser.py

@@ -27,6 +27,7 @@
 from waitress.rfc7230 import HEADER_FIELD_RE, ONLY_DIGIT_RE
 from waitress.utilities import (
     BadRequest,
+    HTTPVersionNotSupported,
     RequestEntityTooLarge,
     RequestHeaderFieldsTooLarge,
     ServerNotImplemented,
@@ -49,6 +50,10 @@ class TransferEncodingNotImplemented(Exception):
     pass
 
 
+class HTTPVersionNotSupportedError(Exception):
+    pass
+
+
 class HTTPRequestParser:
     """A structure that collects the HTTP request.
 
@@ -130,11 +135,20 @@ def received(self, data):
                 # Header finished.
                 header_plus = s[:index]
 
-                # Remove preceding blank lines. This is suggested by
-                # https://tools.ietf.org/html/rfc7230#section-3.5 to support
-                # clients sending an extra CR LF after another request when
-                # using HTTP pipelining
-                header_plus = header_plus.lstrip()
+                # Remove preceding blank lines. RFC 9112 section 2.2 says a
+                # server that is expecting to read a start-line SHOULD ignore
+                # at least one empty line received before it, which supports
+                # clients sending an extra CRLF after a request when they are
+                # pipelining.
+                #
+                # NB: only whole CRLF pairs are skipped. A bare lstrip() also
+                # eats spaces, tabs, vertical tabs, form feeds and lone CR or
+                # LF octets, none of which RFC 9112 allows before a
+                # request-line, and swallowing them here would hide them from
+                # the checks further down.
+
+                while header_plus.startswith(b"\r\n"):
+                    header_plus = header_plus[2:]
 
                 if not header_plus:
                     self.empty = True
@@ -148,6 +162,9 @@ def received(self, data):
                     except TransferEncodingNotImplemented as e:
                         self.error = ServerNotImplemented(e.args[0])
                         self.completed = True
+                    except HTTPVersionNotSupportedError as e:
+                        self.error = HTTPVersionNotSupported(e.args[0])
+                        self.completed = True
                     else:
                         if self.body_rcv is None:
                             # no content-length header and not a t-e: chunked
@@ -265,6 +282,30 @@ def parse_header(self, header_plus):
         version = version.decode("latin-1")
         command = command.decode("latin-1")
         self.command = command
+
+        if version:
+            # RFC 9112 section 2.3: a recipient that receives a message with a
+            # major version it implements but a higher minor version than it
+            # implements SHOULD process the message as if it were in the
+            # highest minor version it is conformant with, and a server SHOULD
+            # respond with a 505 (HTTP Version Not Supported) when the major
+            # version is one it does not support.
+            #
+            # This matters beyond tidiness: everything version dependent below
+            # is keyed on the version being exactly "1.0" or "1.1", so an
+            # unrecognised one used to fall through all of it. A request
+            # claiming HTTP/2.0 had its Connection and Expect header fields
+            # ignored, and nothing else in Waitress noticed either.
+            major, _, minor = version.partition(".")
+
+            if major != "1":
+                raise HTTPVersionNotSupportedError(
+                    "HTTP version %s is not supported" % version
+                )
+
+            if minor > "1":
+                version = "1.1"
+
         self.version = version
         (
             self.proxy_scheme,
@@ -408,7 +449,7 @@ def split_uri(uri):
 
 def get_header_lines(header):
     """
-    Splits the header into lines, putting multi-line headers together.
+    Splits the header into lines.
     """
     r = []
     lines = header.split(b"\r\n")
@@ -423,12 +464,25 @@ def get_header_lines(header):
             )
 
         if line.startswith((b" ", b"\t")):
-            if not r:
-                # https://corte.si/posts/code/pathod/pythonservers/index.html
-                raise ParsingError('Malformed header line "%s"' % str(line, "latin-1"))
-            r[-1] += line
-        else:
-            r.append(line)
+            # RFC 9112 section 5.2: a server that receives an obs-fold in a
+            # request message that is not within a message/http container MUST
+            # either reject the message with a 400 (Bad Request), preferably
+            # with a representation explaining that obsolete line folding is
+            # unacceptable, or replace each obs-fold with one or more SP
+            # octets before interpreting the field value.
+            #
+            # We reject. Folding has been deprecated since RFC 7230 in 2014,
+            # and unfolding leaves us interpreting a field one way while
+            # something in front of us that rejects folding, or unfolds it
+            # differently, interprets it another.
+            #
+            # NB: a folded line arriving first has nothing to fold onto, which
+            # used to be the only case rejected here. See
+            # https://corte.si/posts/code/pathod/pythonservers/index.html
+            raise ParsingError(
+                'Obsolete line folding is not allowed "%s"' % str(line, "latin-1")
+            )
+        r.append(line)
 
     return r
 

src/waitress/rfc7230.py

@@ -73,3 +73,23 @@
 QUOTED_PAIR_RE = re.compile(QUOTED_PAIR)
 QUOTED_STRING_RE = re.compile(QUOTED_STRING)
 CHUNK_EXT_RE = re.compile(("^" + CHUNK_EXT + "$").encode("latin-1"))
+
+
+def connection_options(value):
+    """
+    Return the set of connection options in a Connection header field value.
+
+    RFC 9112 Section 9.1 defines the field as a comma separated list of
+    options, so an option has to be looked for as a member of that list rather
+    than by comparing the whole field value.
+
+    Options are case insensitive, and a field that appeared more than once has
+    already been joined with commas by the parser, which is exactly the same
+    grammar, so this handles that as well.
+    """
+
+    return {
+        option.strip(" \t").lower()
+        for option in value.split(",")
+        if option.strip(" \t")
+    }

src/waitress/task.py

@@ -18,6 +18,7 @@
 import time
 
 from .buffers import ReadOnlyFileBasedBuffer
+from .rfc7230 import connection_options
 from .utilities import build_http_date, logger, queue_logger
 
 rename_headers = {  # or keep them without the HTTP_ prefix added
@@ -558,7 +559,25 @@ def get_environment(self):
             "wsgi.input_terminated": True,  # wsgi.input is EOF terminated
         }
 
+        # RFC 9110 section 7.6.1: the field names listed in the Connection
+        # header field are connection specific, they apply to this hop only
+        # and a recipient must not forward them. Waitress is the end of the
+        # line for the connection, so handing them to the application is the
+        # equivalent of forwarding: a client can name a header there that the
+        # proxy in front of us believes only it controls, and the application
+        # has no way to tell the difference.
+        #
+        # The field names Connection itself may carry are dropped along with
+        # the ones it names, since they describe the connection rather than
+        # the request.
+        hop_by_hop_fields = {
+            option.upper().replace("-", "_")
+            for option in connection_options(request.headers.get("CONNECTION", ""))
+        }
+
         for key, value in dict(request.headers).items():
+            if key in hop_by_hop_fields:
+                continue
             mykey = rename_headers.get(key, None)
             if mykey is None:
                 mykey = "HTTP_" + key

src/waitress/utilities.py

@@ -298,3 +298,8 @@ class InternalServerError(Error):
 class ServerNotImplemented(Error):
     code = 501
     reason = "Not Implemented"
+
+
+class HTTPVersionNotSupported(Error):
+    code = 505
+    reason = "HTTP Version Not Supported"