Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wsgi: server MUST NOT send Content-Length/Transfer-Encoding header in response with a status code of 1xx, 204 or (2xx to CONNECT request) #747

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 16 additions & 4 deletions eventlet/wsgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,9 @@ def handle_one_response(self):
use_chunked = [False]
length = [0]
status_code = [200]
# Status code of 1xx or 204 or 2xx to CONNECT request MUST NOT send body and related headers
# https://httpwg.org/specs/rfc7230.html#rfc.section.3.3.1
bodyless = [False]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It bugs me a little that we're not actually enforcing the absence of a body -- if application code responds 204 No Content but then also returns an app iter that has some bytes, we're going to yield them out and mess up message framing. Bit of a slippery slope to go down, though, I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed, thank you. But also we have a funny hack to allow random header case for compatibility with some popular server.

IMHO, we should claim WSGI and HTTP are separated at eventlet.wsgi, it takes care of transfer headers, may drop body and proper application expects that anyway. But also allow things like 204 with body via feature flag.

But it seems a fair amount of change. Suggest to extract this as separate task and implement later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; it can certainly be a follow-up -- or we just tell application developers to be more careful. Definitely not a blocker to getting this fix merged.


def write(data):
towrite = []
Expand Down Expand Up @@ -536,10 +539,12 @@ def write(data):
self.close_connection = 1

if 'content-length' not in header_list:
if self.request_version == 'HTTP/1.1':
if bodyless[0]:
pass # client didn't expect a body anyway
elif self.request_version == 'HTTP/1.1':
use_chunked[0] = True
towrite.append(b'Transfer-Encoding: chunked\r\n')
elif 'content-length' not in header_list:
else:
# client is 1.0 and therefore must read to EOF
self.close_connection = 1

Expand All @@ -564,7 +569,7 @@ def write(data):
length[0] = length[0] + sum(map(len, towrite))

def start_response(status, response_headers, exc_info=None):
status_code[0] = status.split()[0]
status_code[0] = int(status.split(" ", 1)[0])
if exc_info:
try:
if headers_sent:
Expand All @@ -574,6 +579,13 @@ def start_response(status, response_headers, exc_info=None):
# Avoid dangling circular ref
exc_info = None

bodyless[0] = (
status_code[0] in (204, 304)
or self.command == "HEAD"
or (100 <= status_code[0] < 200)
or (self.command == "CONNECT" and 200 <= status_code[0] < 300)
)

# Response headers capitalization
# CONTent-TYpe: TExt/PlaiN -> Content-Type: TExt/PlaiN
# Per HTTP RFC standard, header name is case-insensitive.
Expand All @@ -597,7 +609,7 @@ def cap(x):
# Set content-length if possible
if headers_set and not headers_sent and hasattr(result, '__len__'):
# We've got a complete final response
if 'Content-Length' not in [h for h, _v in headers_set[1]]:
if not bodyless[0] and 'Content-Length' not in [h for h, _v in headers_set[1]]:
headers_set[1].append(('Content-Length', str(sum(map(len, result)))))
if request_input.should_send_hundred_continue:
# We've got a complete final response, and never sent a 100 Continue.
Expand Down
44 changes: 44 additions & 0 deletions tests/wsgi_test.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

import collections
import errno
import io
Expand Down Expand Up @@ -205,6 +206,8 @@ def read_http(sock):
if content_length_str:
num = int(content_length_str)
body = fd.read(num)
elif response_line.split()[1] in ('204', '304'):
body = ''
else:
# read until EOF
body = fd.read()
Expand Down Expand Up @@ -1929,6 +1932,47 @@ def test_close_idle_connections(self):
except Exception:
assert False, self.logfile.getvalue()

def test_no_transfer_encoding_in_empty_response(self):
def app(environ, start_response):
if environ["PATH_INFO"] == "/304":
status = "304 Not Modified"
else:
status = "204 OK"
write = start_response(status, [])
write(b"")
# "An application must return an iterable object, even if it uses
# write() to produce all or part of its response body."
return []

self.spawn_server(site=app)
sock = eventlet.connect(self.server_addr)

sock.sendall(b"DELETE /foo HTTP/1.1\r\nConnection: keep-alive\r\n\r\n")
response = read_http(sock)
assert response.status == "HTTP/1.1 204 OK"
assert "transfer-encoding" not in response.headers_lower
assert "content-length" not in response.headers_lower
assert response.headers_lower.get("connection") == "keep-alive"

# Since it's HTTP/1.1 and clients know there's no body,
# we can continue using the connection
sock.sendall(b"GET /304 HTTP/1.1\r\n\r\n")
response = read_http(sock)
assert response.status == "HTTP/1.1 304 Not Modified"
assert "transfer-encoding" not in response.headers_lower
assert "content-length" not in response.headers_lower
assert "connection" not in response.headers_lower

# HTTP/1.1 default to persistant connections, even without an explicit
# Connection header, so we can keep going
sock.sendall(b"DELETE /foo HTTP/1.1\r\n\r\n")
response = read_http(sock)
assert "transfer-encoding" not in response.headers_lower
assert "content-length" not in response.headers_lower
assert "connection" not in response.headers_lower

sock.close()

def test_rfc9112_reject_bad_request(self):
# (hberaud): Transfer-Encoding and Content-Length in the
# same header are not allowed by rfc9112.
Expand Down