From c61891b2dad0d1f3c3a9ce1df8bc26431247e62d Mon Sep 17 00:00:00 2001 From: Delta Regeer Date: Sun, 2 Aug 2026 16:49:25 -0600 Subject: [PATCH 1/3] Do not close the connection after a bodiless response RFC 9112 section 6.3: a response with a 1xx, 204 or 304 status carries no message body and is terminated by the first empty line after the header fields. It is therefore framed unambiguously without a Content-Length or a Transfer-Encoding, and there is nothing for closing the connection to delimit. Waitress closed it anyway. Every 304 Not Modified was answered with Connection: close, so a cache revalidating against Waitress had to open a fresh connection for each one. The close now happens only where it is doing work: a response that should have carried a body but has no length to declare is delimited by the chunked coding, and the connection is still closed after it. That is unchanged. See https://github.com/Pylons/waitress/issues/197 for the other half of this code path. --- CHANGES.txt | 9 +++++ src/waitress/task.py | 16 ++++++-- tests/fixtureapps/notmodified.py | 7 ++++ tests/test_functional.py | 43 +++++++++++++++++++++ tests/test_task.py | 66 +++++++++++++++++++++----------- 5 files changed, 114 insertions(+), 27 deletions(-) create mode 100644 tests/fixtureapps/notmodified.py diff --git a/CHANGES.txt b/CHANGES.txt index c7f32ea4..67ff7407 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -4,6 +4,15 @@ Unreleased Bugfix ~~~~~~ +- Waitress no longer closes the connection after a response with a status code + of 1xx, 204 or 304. RFC 9112 section 6.3 says such a response carries no + message body and is terminated by the first empty line after the header + fields, so it is framed unambiguously without a ``Content-Length`` or a + ``Transfer-Encoding`` and the connection may be reused. Previously every one + of them was answered with ``Connection: close``, which meant a cache + revalidating against Waitress had to reconnect for each ``304 Not Modified`` + it received. See https://github.com/Pylons/waitress/issues/197 + - Renamed the HTTP header "Trailers" to "Trailer" to fix a typo and comply with the correct header name as specified in RFC 7230. diff --git a/src/waitress/task.py b/src/waitress/task.py index bda3ae50..406a3396 100644 --- a/src/waitress/task.py +++ b/src/waitress/task.py @@ -243,15 +243,23 @@ def build_response_header(self): self.set_close_on_finish() if not content_length_header: - # RFC 7230: MUST NOT send Transfer-Encoding or Content-Length - # for any response with a status code of 1xx, 204 or 304. + # RFC 9112 section 6.3: MUST NOT send Transfer-Encoding or + # Content-Length for any response with a status code of 1xx, + # 204 or 304. + # + # Such a response carries no message body and is terminated by + # the first empty line after the header fields, so it is + # already framed unambiguously and the connection may be + # reused. Only a response that should have carried a body but + # has no length to declare needs the connection closed in + # order to delimit it. if self.has_body: self.response_headers.append(("Transfer-Encoding", "chunked")) self.chunked_response = True - if not self.close_on_finish: - self.set_close_on_finish() + if not self.close_on_finish: + self.set_close_on_finish() # under HTTP 1.1 keep-alive is default, no need to set the header else: diff --git a/tests/fixtureapps/notmodified.py b/tests/fixtureapps/notmodified.py new file mode 100644 index 00000000..1c53fd5e --- /dev/null +++ b/tests/fixtureapps/notmodified.py @@ -0,0 +1,7 @@ +def app(environ, start_response): # pragma: no cover + if environ["PATH_INFO"] == "/nocontent": + start_response("204 No Content", []) + else: + start_response("304 Not Modified", [("ETag", '"abc"')]) + + return [b""] diff --git a/tests/test_functional.py b/tests/test_functional.py index fae412c5..07e6a5d5 100644 --- a/tests/test_functional.py +++ b/tests/test_functional.py @@ -593,6 +593,42 @@ def test_pipelining(self): self.assertEqual(response_body, expect_body) +class NotModifiedTests: + def setUp(self): + from tests.fixtureapps import notmodified + + self.start_subprocess(notmodified.app) + + def tearDown(self): + self.stop_subprocess() + + def _check_bodiless_keepalive(self, path, status, reason): + # RFC 9112 section 6.3: a response with a 1xx, 204 or 304 status + # carries no message body and is terminated by the first empty line + # after the header fields. It is therefore framed unambiguously + # without a Content-Length or a Transfer-Encoding, and the connection + # may be reused; a cache revalidating against us should not have to + # reconnect for every 304 it gets back. + to_send = b"GET %s HTTP/1.1\r\nHost: localhost\r\n\r\n" % path + self.connect() + self.sock.send(to_send * 2) + + with self.sock.makefile("rb", 0) as fp: + for _ in range(2): + line = fp.readline() # status line + self.assertline(line, status, reason, "HTTP/1.1") + headers = parse_headers(fp) + self.assertNotIn("connection", headers) + self.assertNotIn("content-length", headers) + self.assertNotIn("transfer-encoding", headers) + + def test_304_does_not_close_connection(self): + self._check_bodiless_keepalive(b"/", "304", "Not Modified") + + def test_204_does_not_close_connection(self): + self._check_bodiless_keepalive(b"/nocontent", "204", "No Content") + + class ExpectContinueTests: def setUp(self): from tests.fixtureapps import echo @@ -1566,6 +1602,10 @@ class TcpPipeliningTests(PipeliningTests, TcpTests, unittest.TestCase): pass +class TcpNotModifiedTests(NotModifiedTests, TcpTests, unittest.TestCase): + pass + + class TcpExpectContinueTests(ExpectContinueTests, TcpTests, unittest.TestCase): pass @@ -1641,6 +1681,9 @@ class UnixEchoTests(EchoTests, UnixTests, unittest.TestCase): class UnixPipeliningTests(PipeliningTests, UnixTests, unittest.TestCase): pass + class UnixNotModifiedTests(NotModifiedTests, UnixTests, unittest.TestCase): + pass + class UnixExpectContinueTests(ExpectContinueTests, UnixTests, unittest.TestCase): pass diff --git a/tests/test_task.py b/tests/test_task.py index 278ab538..e921d5a9 100644 --- a/tests/test_task.py +++ b/tests/test_task.py @@ -203,55 +203,75 @@ def test_build_response_header_v11_200_no_content_length(self): self.assertIn(("Connection", "close"), inst.response_headers) def test_build_response_header_v11_204_no_content_length_or_transfer_encoding(self): - # RFC 7230: MUST NOT send Transfer-Encoding or Content-Length - # for any response with a status code of 1xx or 204. + # RFC 9112 section 6.3: MUST NOT send Transfer-Encoding or + # Content-Length for any response with a status code of 1xx, 204 or + # 304. Such a response has no message body to delimit, so the + # connection may be reused. inst = self._makeOne() inst.request = DummyParser() inst.version = "1.1" inst.status = "204 No Content" result = inst.build_response_header() lines = filter_lines(result) - self.assertEqual(len(lines), 4) + self.assertEqual(len(lines), 3) self.assertEqual(lines[0], b"HTTP/1.1 204 No Content") - self.assertEqual(lines[1], b"Connection: close") - self.assertTrue(lines[2].startswith(b"Date:")) - self.assertEqual(lines[3], b"Server: waitress") - self.assertTrue(inst.close_on_finish) - self.assertIn(("Connection", "close"), inst.response_headers) + self.assertTrue(lines[1].startswith(b"Date:")) + self.assertEqual(lines[2], b"Server: waitress") + self.assertFalse(inst.close_on_finish) + self.assertNotIn(("Connection", "close"), inst.response_headers) def test_build_response_header_v11_1xx_no_content_length_or_transfer_encoding(self): - # RFC 7230: MUST NOT send Transfer-Encoding or Content-Length - # for any response with a status code of 1xx or 204. + # RFC 9112 section 6.3: MUST NOT send Transfer-Encoding or + # Content-Length for any response with a status code of 1xx, 204 or + # 304. Such a response has no message body to delimit, so the + # connection may be reused. inst = self._makeOne() inst.request = DummyParser() inst.version = "1.1" inst.status = "100 Continue" result = inst.build_response_header() lines = filter_lines(result) - self.assertEqual(len(lines), 4) + self.assertEqual(len(lines), 3) self.assertEqual(lines[0], b"HTTP/1.1 100 Continue") - self.assertEqual(lines[1], b"Connection: close") - self.assertTrue(lines[2].startswith(b"Date:")) - self.assertEqual(lines[3], b"Server: waitress") - self.assertTrue(inst.close_on_finish) - self.assertIn(("Connection", "close"), inst.response_headers) + self.assertTrue(lines[1].startswith(b"Date:")) + self.assertEqual(lines[2], b"Server: waitress") + self.assertFalse(inst.close_on_finish) + self.assertNotIn(("Connection", "close"), inst.response_headers) def test_build_response_header_v11_304_no_content_length_or_transfer_encoding(self): - # RFC 7230: MUST NOT send Transfer-Encoding or Content-Length - # for any response with a status code of 1xx, 204 or 304. + # RFC 9112 section 6.3: MUST NOT send Transfer-Encoding or + # Content-Length for any response with a status code of 1xx, 204 or + # 304. Such a response has no message body to delimit, so the + # connection may be reused; a 304 is the answer to a cache + # revalidation and closing on every one of them is expensive. inst = self._makeOne() inst.request = DummyParser() inst.version = "1.1" inst.status = "304 Not Modified" result = inst.build_response_header() lines = filter_lines(result) - self.assertEqual(len(lines), 4) + self.assertEqual(len(lines), 3) self.assertEqual(lines[0], b"HTTP/1.1 304 Not Modified") - self.assertEqual(lines[1], b"Connection: close") - self.assertTrue(lines[2].startswith(b"Date:")) - self.assertEqual(lines[3], b"Server: waitress") + self.assertTrue(lines[1].startswith(b"Date:")) + self.assertEqual(lines[2], b"Server: waitress") + self.assertFalse(inst.close_on_finish) + self.assertNotIn(("Connection", "close"), inst.response_headers) + + def test_build_response_header_v11_no_content_length_chunked_still_closes(self): + # A response that should have carried a body but has no length to + # declare is delimited by the chunked coding, and Waitress closes the + # connection afterwards; that behaviour is unchanged. + inst = self._makeOne() + inst.request = DummyParser() + inst.version = "1.1" + inst.status = "200 OK" + result = inst.build_response_header() + lines = filter_lines(result) + self.assertEqual(lines[0], b"HTTP/1.1 200 OK") + self.assertIn(b"Connection: close", lines) + self.assertIn(b"Transfer-Encoding: chunked", lines) self.assertTrue(inst.close_on_finish) - self.assertIn(("Connection", "close"), inst.response_headers) + self.assertTrue(inst.chunked_response) def test_build_response_header_via_added(self): inst = self._makeOne() From a2ac8e182404c9ff6c7acea6f945af8f34992442 Mon Sep 17 00:00:00 2001 From: Delta Regeer Date: Sun, 2 Aug 2026 16:49:51 -0600 Subject: [PATCH 2/3] Include the received-protocol in the Via response header RFC 9110 section 7.6.3 defines Via as Via = #( received-protocol RWS received-by [ RWS comment ] ) The received-protocol is not optional, so the pseudonym on its own is not a well formed field value. Waitress wrote "Via: waitress"; it now writes "Via: 1.1 waitress", eliding the protocol-name as the grammar allows for HTTP and using the version of the request received. This header is only added when the WSGI application supplies its own Server header. Whether that is the right trigger, and whether an origin server should be adding a Via at all, are left alone here. --- CHANGES.txt | 6 ++++++ src/waitress/task.py | 7 ++++++- tests/test_task.py | 15 ++++++++++++++- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 67ff7407..5d77a737 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -13,6 +13,12 @@ Bugfix revalidating against Waitress had to reconnect for each ``304 Not Modified`` it received. See https://github.com/Pylons/waitress/issues/197 +- The ``Via`` response header that Waitress adds when the WSGI application + supplies its own ``Server`` header now includes the received-protocol that + RFC 9110 section 7.6.3 requires, for example ``Via: 1.1 waitress`` rather + than the ``Via: waitress`` written before, which is not a well formed field + value. + - Renamed the HTTP header "Trailers" to "Trailer" to fix a typo and comply with the correct header name as specified in RFC 7230. diff --git a/src/waitress/task.py b/src/waitress/task.py index 406a3396..beecd694 100644 --- a/src/waitress/task.py +++ b/src/waitress/task.py @@ -273,7 +273,12 @@ def build_response_header(self): if ident: self.response_headers.append(("Server", ident)) else: - self.response_headers.append(("Via", ident or "waitress")) + # RFC 9110 section 7.6.3: Via = #( received-protocol RWS + # received-by [ RWS comment ] ). The received-protocol is not + # optional, so the pseudonym on its own is not a well formed field + # value; the protocol-name may only be elided when it is HTTP, + # which leaves the version of the request we received. + self.response_headers.append(("Via", f"{version} {ident or 'waitress'}")) if not date_header: self.response_headers.append(("Date", build_http_date(self.start_time))) diff --git a/tests/test_task.py b/tests/test_task.py index e921d5a9..aac56cb7 100644 --- a/tests/test_task.py +++ b/tests/test_task.py @@ -274,6 +274,8 @@ def test_build_response_header_v11_no_content_length_chunked_still_closes(self): self.assertTrue(inst.chunked_response) def test_build_response_header_via_added(self): + # RFC 9110 section 7.6.3: the received-protocol is a required part of + # a Via field value, the pseudonym alone is not well formed inst = self._makeOne() inst.request = DummyParser() inst.version = "1.0" @@ -285,7 +287,18 @@ def test_build_response_header_via_added(self): self.assertEqual(lines[1], b"Connection: close") self.assertTrue(lines[2].startswith(b"Date:")) self.assertEqual(lines[3], b"Server: abc") - self.assertEqual(lines[4], b"Via: waitress") + self.assertEqual(lines[4], b"Via: 1.0 waitress") + + def test_build_response_header_via_uses_request_version(self): + inst = self._makeOne() + inst.request = DummyParser() + inst.version = "1.1" + inst.status = "200 OK" + inst.content_length = 0 + inst.response_headers = [("Server", "abc")] + result = inst.build_response_header() + lines = filter_lines(result) + self.assertIn(b"Via: 1.1 waitress", lines) def test_build_response_header_date_exists(self): inst = self._makeOne() From 899ad180c4946b6e309b9c9549273bca7e302a3b Mon Sep 17 00:00:00 2001 From: Delta Regeer Date: Sun, 2 Aug 2026 16:50:00 -0600 Subject: [PATCH 3/3] Document why header names containing an underscore are dropped Replaces the TODO asking whether such a request should be rejected instead. It should not, and the reasoning is worth recording where the next person looks rather than rediscovering it. An underscore is a valid tchar, so the field name is legal, but the CGI style mapping the WSGI environ uses is ambiguous for it: both X-Forwarded-For and X_Forwarded_For arrive as HTTP_X_FORWARDED_FOR, which lets a client forge a header the proxy in front of us believes only it can set. Dropping the field removes that ambiguity, and having dropped it there is nothing further a 400 would protect against -- it would only risk legitimate traffic, and rejecting outright is itself hazardous in front of pipelining or proxies. This is also what everyone else settled on: nginx via underscores_in_headers plus ignore_invalid_headers, Apache httpd since 2.4, mod_wsgi since 4.3.0, Werkzeug's development server, and gunicorn, whose header_map setting documents "drop" as its safe default and offers refusal only as an opt-in. Django applied the same rule at the framework level in CVE-2015-0219, where this class of bug was first described. No behaviour change. --- docs/reverse-proxy.rst | 33 +++++++++++++++++++++++++++++++++ src/waitress/parser.py | 14 +++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/docs/reverse-proxy.rst b/docs/reverse-proxy.rst index 6490e3d7..865aea23 100644 --- a/docs/reverse-proxy.rst +++ b/docs/reverse-proxy.rst @@ -112,6 +112,39 @@ To configure waitress to use the ``Forwarded`` header, set:: contain the IP address of the proxy. +Header field names containing underscores +----------------------------------------- + +Waitress silently drops any request header field whose name contains an +underscore, and does not pass it to the WSGI application. + +An underscore is a valid character in a field name, so ``X_Forwarded_For`` is a +legal HTTP header. The problem is the CGI style mapping that the WSGI environ +uses: field names are upper-cased and dashes are replaced with underscores, so +both ``X-Forwarded-For`` and ``X_Forwarded_For`` arrive at the application as +``HTTP_X_FORWARDED_FOR``. Without this rule a client could forge a header that +the proxy in front of Waitress believes only it is able to set, which would +defeat the ``trusted_proxy`` handling described above. + +This is the same conclusion every other implementation of the CGI mapping has +reached. nginx marks such names invalid via ``underscores_in_headers off`` and +then discards them under ``ignore_invalid_headers``; Apache httpd has dropped +them when building CGI variables since 2.4, as has mod_wsgi since 4.3.0; +Werkzeug's development server skips them unconditionally; and gunicorn's +``header_map`` setting documents ``drop`` as its safe default. Django applied +the same rule at the framework level in CVE-2015-0219, which is where this +class of bug was first described. + +Dropping the field rather than rejecting the whole request is deliberate. +Discarding it already removes the ambiguity that makes the header spoofable, so +refusing the request buys no further protection while risking legitimate +traffic — an underscore is a valid character in a field name, and rejecting +outright is itself hazardous in front of pipelining or proxies. + +If you need such a header to reach your application, rename it at the proxy to +use dashes instead. + + Using ``url_prefix`` to influence ``SCRIPT_NAME`` and ``PATH_INFO`` ------------------------------------------------------------------- diff --git a/src/waitress/parser.py b/src/waitress/parser.py index 1af4594d..d0ab077f 100644 --- a/src/waitress/parser.py +++ b/src/waitress/parser.py @@ -232,7 +232,19 @@ def parse_header(self, header_plus): key, value = header.group("name", "value") if b"_" in key: - # TODO(xistence): Should we drop this request instead? + # An underscore is a valid tchar, so this is a legal field + # name, but the CGI style mapping the WSGI environ uses is + # ambiguous for it: both "X-Forwarded-For" and + # "X_Forwarded_For" become HTTP_X_FORWARDED_FOR, so a client + # could otherwise forge a header that a proxy in front of us + # believes only it can set. + # + # Dropping the field rather than the request is what nginx + # (underscores_in_headers off, then ignore_invalid_headers), + # Apache httpd, mod_wsgi, Werkzeug and gunicorn (header_map, + # which documents "drop" as the safe default) all do. Dropping + # is enough to remove the ambiguity, so rejecting the request + # outright would buy nothing and only risk legitimate traffic. continue