[bug] gunicorn re-chunks responses which were already chunked

[twitchyliquid64]

Heya! I think I found a bug in gunicorn's handling of responses which were already chunked.

The issue

If a handler upstream to gunicorn:

1. Does not set Content-Length
2. Sets Transfer-Encoding: chunked
3. Streams back data which is already chunked (i.e. response data is already framed as <length>\r\n<data>\r\n)

gunicorn wraps the already-chunked content in chunks, corrupting the data stream with erroneous framing.

Suspected code

is_chunked is used to turn on gunicorn's own chunking, but does not consider if the response is already chunked.

gunicorn/gunicorn/http/wsgi.py

Lines 286 to 301 in bacbf8a

	def is_chunked(self):
	# Only use chunked responses when the client is
	# speaking HTTP/1.1 or newer and there was
	# no Content-Length header set.
	if self.response_length is not None:
	return False
	elif self.req.version <= (1, 0):
	return False
	elif self.req.method == 'HEAD':
	# Responses to a HEAD request MUST NOT contain a response body.
	return False
	elif self.status_code in (204, 304):
	# Do not use chunked responses when the response is guaranteed to
	# not have a response body.
	return False
	return True

Repro steps

A flask app which streams its own response like this. Works fine with other WSGI wrappers:

@some_blueprint.route('/stream', methods=['POST'])
def stream():
  r = Response(chunk_generator(), content_type='application/chunked-json')
  r.automatically_set_content_length = False
  r.direct_passthrough = True
  r.headers['Transfer-Encoding'] = 'chunked'
  return r

def chunk_generator() -> Generator[bytes, None, None]:
  for x in range(50):
    c = wire_chunk('some bytes'.encode('utf-8'))
    yield c
  yield '0\r\n\r\n'.encode('utf-8') # Symbolizes end of stream

def wire_chunk(data: bytes) -> bytes:
    data_len = len(data)
    return f'{data_len:x}'.encode('utf-8') + '\r\n'.encode('utf-8') + data + '\r\n'.encode('utf-8')

[benoitc]

well it is not xpected that a wsgi appication shunk itself the body itself. This is where it should be fixed imo.

[pajod]

@benoitc If we follow the (imho, reasonable) rules laid out in PEP 3333 section "Other HTTP Features", then this applies in both directions:

1. Making the app rely on hop-by-hop headers should be deleted on the request side (I'll comment on that at #3260)

2. Obviously incorrect usage when received from app like above (violating the "must not") should be detected+raised.

   Servers should check for errors in the headers at the time start_response is called, so that an error can be raised while the application is still running.

   Applications and middleware are forbidden from using HTTP/1.1 “hop-by-hop” features or headers

   In general, the server or gateway is responsible for ensuring that correct headers are sent to the client

(.. and if we do not follow those rules.. then it is only reasonable to expect Gunicorn to gracefully deal with the consequences: to check for the header and disable chunking, when present)

[twitchyliquid64]

Agree that perhaps the application should not chunk itself - this was particularly heinous for me because the client HTTP library I was using made one set of assumptions about chunks and how to drive their API, and python-WSGI makes different ones. I ultimately wasn't able to come up with a streaming approach that worked without reading things byte-by-byte, and I'm frustrated by how hard this is.

Agree this isn't the neatest thing in the world, but is it really so bad to turn off chunking if chunking has already been signaled from the application?