Handle malformed HTTP request with multiple Connection headers

[zioproto]

This PR fixes a problem with Websockets when running Uvicorn on GitHub Codespaces

Detailed discussion:
https://github.com/orgs/community/discussions/57596

Summary

Please do not merge quickly, I am not sure if this is a Uvicorn bug or a problem with the GitHub codespaces reverse proxy sending a malformed HTTP requests.

According to RFC7230 there should not be duplicated Connection headers.

A sender MUST NOT generate multiple header fields with the same field
name in a message unless either the entire field value for that
header field is defined as a comma-separated list [i.e., #(values)]
or the header field is a well-known exception (as noted below).

I am running Uvicorn in GitHub Codespaces. I am forwarding a port with visibility public and port protocol HTTP.
My first Websocket connection works, but after a few seconds the Websocket endpoint starts returning HTTP 404.

I captured traffic at the Uvicorn server with tcpdump and I figured out that when I receive a HTTP 404 the HTTP request has a duplicated Connection header.

This is a problem and looking at the Uvicorn implementation it will make the Websockets connection fail to upgrade randomly depending on which one of the duplicated Connection header is read first.

uvicorn/uvicorn/protocols/http/h11_impl.py

Lines 173 to 183 in 1cb58c7

	def _get_upgrade(self) -> Optional[bytes]:
	connection = []
	upgrade = None
	for name, value in self.headers:
	if name == b"connection":
	connection = [token.lower().strip() for token in value.split(b",")]
	if name == b"upgrade":
	upgrade = value.lower()
	if b"upgrade" in connection:
	return upgrade
	return None

I confirm that with the patch proposed in this PR fixes the problem completely because connection will be a list with the values ['upgrade', 'keep-alive']. However it could be that the request is just malformed and Uvicorn should not implement anything to cope with malformed HTTP requests.

Please review.

Checklist

• I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• I've updated the documentation accordingly.

[Kludex]

[zioproto]

@Kludex do you think I need to update the current tests in place at tests/protocols/test_websocket.py ?

Like implementing a test:
test_websocket_upgrade_with_multiple_connection_headers ?

Do you think that is a valid HTTP Request ?

[Kludex]

According to RFC7230 there should not be duplicated Connection headers.

Then we shouldn't change uvicorn. 😅

[Kludex]

Does the same happens with hypercorn?

[zioproto]

Does the same happens with hypercorn?

I dont know, I never used it.

This is confirmed being GitHub Codespaces sending duplicated Connection: header when port forwarding.

I will change the PR to return a proper HTTP return code in case of multiple Connection headers.

Should I just return a HTTP 400 error?
I read the 4XX client errors but I can't find a more specific one.

[Kludex]

I dont know, I never used it.

It would be interesting to check the behavior of another server implementation.

I will change the PR to return a proper HTTP return code in case of multiple Connection headers.

No need. I think the current behavior is fine.

[Kludex]

Since GitHub is in fault, and you got a solution for the problem, I'll be closing the PR.

Thanks for the investigation. 🙏