From 5e354772f8e17f5f01e65bb16b47de5f0a98f9a3 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Wed, 16 Sep 2026 22:47:36 +0200 Subject: [PATCH] THRIFT-6266: Read the THttpServer Content-Length against its grammar Client: py THttpServer converted Content-Length with int(), which accepts more than the field's grammar of one or more decimal digits (RFC 9110 8.6). int() takes a leading sign, underscores between digits, and whitespace such as a no-break space, a vertical tab or a form feed around the number. Each of these was read as a valid length, and the request was served. "-0" passed as zero, and the request then failed without a response. The value is now stripped of the spaces and tabs that surround a field value without being part of it (RFC 9110 5.5). What remains must consist of digits only. Anything else is answered with the 400 already sent for a malformed length. Co-Authored-By: Claude Opus 5 (1M context) --- lib/py/Makefile.am | 2 + lib/py/src/server/THttpServer.py | 11 +- .../test/test_http_server_content_length.py | 149 ++++++++++++++++++ 3 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 lib/py/test/test_http_server_content_length.py diff --git a/lib/py/Makefile.am b/lib/py/Makefile.am index c1a77b966c3..df67438bef8 100644 --- a/lib/py/Makefile.am +++ b/lib/py/Makefile.am @@ -34,6 +34,7 @@ py3-test: py3-build $(PYTHON3) test/thrift_TCompactProtocol.py $(PYTHON3) test/thrift_TNonblockingServer.py $(PYTHON3) test/test_http_server_body_size.py + $(PYTHON3) test/test_http_server_content_length.py $(PYTHON3) test/thrift_TSerializer.py $(PYTHON3) test/test_recursion_depth.py $(PYTHON3) test/test_container_sizing.py @@ -72,6 +73,7 @@ check-local: all py3-test $(PYTHON) test/thrift_TCompactProtocol.py $(PYTHON) test/thrift_TNonblockingServer.py $(PYTHON) test/test_http_server_body_size.py + $(PYTHON) test/test_http_server_content_length.py $(PYTHON) test/thrift_TSerializer.py THRIFT=${THRIFT} $(PYTHON) test/test_compiler/test_keyword_escape.py $(PYTHON) test/test_recursion_depth.py diff --git a/lib/py/src/server/THttpServer.py b/lib/py/src/server/THttpServer.py index 4449503ac64..73152c5fe0e 100644 --- a/lib/py/src/server/THttpServer.py +++ b/lib/py/src/server/THttpServer.py @@ -17,6 +17,7 @@ # under the License. # +import re import ssl import http.server as BaseHTTPServer @@ -25,6 +26,9 @@ from thrift.server import TServer from thrift.transport import TTransport +# Content-Length is 1*DIGIT (RFC 9110 8.6). +_CONTENT_LENGTH = re.compile('[0-9]+') + class ResponseException(Exception): """Allows handlers to override the HTTP response @@ -96,9 +100,14 @@ def do_POST(self): if length is None: self.send_error(411) return + # The whitespace around a field value is not part of it (RFC + # 9110 5.5). int() would also take a sign, underscores and + # whitespace of other kinds, so the value is checked first. + length = length.strip(' \t') try: - length = int(length) + length = int(length) if _CONTENT_LENGTH.fullmatch(length) else -1 except ValueError: + # More digits than int() converts. length = -1 if length < 0: self.send_error(400, "Invalid Content-Length") diff --git a/lib/py/test/test_http_server_content_length.py b/lib/py/test/test_http_server_content_length.py new file mode 100644 index 00000000000..8a2d57ff6a7 --- /dev/null +++ b/lib/py/test/test_http_server_content_length.py @@ -0,0 +1,149 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +# Content-Length is one or more decimal digits (RFC 9110 8.6), and the +# whitespace around a field value is not part of the value (RFC 9110 5.5). +# Python's int() accepts more than that: a sign, underscores between digits, +# and any whitespace it knows of, including a no-break space. These tests hold +# THttpServer to the grammar. +# + +import os +import socket +import struct +import sys +import threading +import unittest + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +import _import_local_thrift # noqa +from thrift.protocol import TBinaryProtocol # noqa +from thrift.server import THttpServer # noqa + +WAIT = 2.0 + +# A strict binary CALL header followed by an empty argument struct. +BODY = (struct.pack('!i', -2147418111) + struct.pack('!i', 4) + b'ping' + + struct.pack('!i', 0) + b'\x00') +LENGTH = str(len(BODY)) + + +class RecordingProcessor(object): + def __init__(self): + self.names = [] + + def on_message_begin(self, func): + pass + + def process(self, iprot, oprot): + name, _, _ = iprot.readMessageBegin() + self.names.append(name) + + +def post(port, length): + """Send BODY with the given Content-Length value and return the status. + + The header is sent as Latin-1, the encoding http.server decodes it with, + so that any character of the value reaches the server unchanged. None means + the server closed the connection without answering. + """ + sock = socket.create_connection(('127.0.0.1', port)) + try: + head = 'POST / HTTP/1.1\r\nHost: localhost\r\nContent-Length:%s\r\n\r\n' % length + sock.sendall(head.encode('latin-1') + BODY) + sock.settimeout(WAIT) + data = b'' + while b'\r\n' not in data: + chunk = sock.recv(1024) + if not chunk: + return None + data += chunk + return int(data.split(b' ', 2)[1]) + except ConnectionResetError: + return None + finally: + sock.close() + + +class HttpServerContentLengthTest(unittest.TestCase): + + def setUp(self): + self.processor = RecordingProcessor() + server = THttpServer.THttpServer( + self.processor, ('127.0.0.1', 0), + TBinaryProtocol.TBinaryProtocolFactory()) + # Keep the output readable: no access log, and no traceback for the + # requests these tests expect to fail. + server.httpd.RequestHandlerClass.log_message = lambda *args: None + server.httpd.handle_error = lambda request, client_address: None + thread = threading.Thread(target=server.serve) + thread.daemon = True + thread.start() + self.addCleanup(self.stop, server, thread) + self.port = server.httpd.server_address[1] + + @staticmethod + def stop(server, thread): + server.httpd.shutdown() + server.httpd.server_close() + thread.join(WAIT) + + def assertRefused(self, length, statuses=(400,)): + self.assertIn(post(self.port, length), statuses, repr(length)) + self.assertEqual(self.processor.names, [], repr(length)) + + def assertServed(self, length): + self.assertEqual(post(self.port, length), 200, repr(length)) + self.assertEqual(self.processor.names, ['ping'], repr(length)) + del self.processor.names[:] + + def test_digits_are_served(self): + self.assertServed(' ' + LENGTH) + self.assertServed(' 0' + LENGTH) + + def test_whitespace_around_the_value_is_not_part_of_it(self): + for length in (' %s ' % LENGTH, ' %s\t' % LENGTH, '\t%s' % LENGTH, LENGTH): + self.assertServed(length) + + def test_a_sign_is_refused(self): + self.assertRefused(' +' + LENGTH) + + def test_underscores_are_refused(self): + self.assertRefused(' %s_%s' % (LENGTH[0], LENGTH[1:])) + + def test_other_whitespace_is_refused(self): + for space in ('\xa0', '\x0b', '\x0c', '\x85'): + self.assertRefused(' ' + space + LENGTH) + self.assertRefused(' ' + LENGTH + space) + + def test_other_forms_of_a_number_are_refused(self): + for length in ('', ' ', ' 0x11', ' 1e1', ' 17.0', ' 1 7', ' -0'): + self.assertRefused(length) + + def test_a_list_of_lengths_is_refused(self): + self.assertRefused(' %s, %s' % (LENGTH, LENGTH)) + + def test_more_digits_than_int_converts_are_refused(self): + # From Python 3.11 on, int() refuses a string of more than 4300 digits + # by default; before, such a length is simply too large. + self.assertRefused(' ' + '1' * 5000, statuses=(400, 413)) + + +if __name__ == '__main__': + unittest.main()