Upgrade request handling: ignore http/2 and optionally ignore websocket

[Argannor]

This PR addresses the discussion #1659 (raised by me) and issue #1501.

Rationale: As outlined in the discussion and issue above the HTTP/1.x spec allows servers to ignore upgrade requests by responding with a normal response.

Changes made:

• Since uvicorn currently only supports upgrades to websocket protocol, all other upgrade requests are ignored by default.
• Websocket requests can be optionally ignored by setting the new config property ws_ignore_upgrades to True
• Introduced a new method _should_upgrade in both http protocol implementations to be overwritten by users that need a more fine grained logic to decide on upgrades
• Test cases added for both cases
• Documentation updated

After making the change for #1659 I also stumbled across the PR #1642 (adressing #1501). Since both changes affect the same area I took the liberty to refactor my change to include graceful handling of #1501 as well.

Looking forward to your feedback!

[Kludex]

If we reach handle_upgrade, we know that upgrade_value == b"websocket", so we can remove the header verification over there.

[Kludex]

Is there a reason for stripping?

Suggested change

	upgrade = value.lower().strip()
	upgrade = value.lower()

[Argannor]

No specific reason, kept it for symmetry reasons and because I generally like to sanitize inputs. Removed it

[Argannor]

If we reach handle_upgrade, we know that upgrade_value == b"websocket", so we can remove the header verification over there.

You're right, I kept it in if we want to support additional protocols, but that's a bad reason

[Kludex]

I'm not sure if we should rename the handle_upgrade to handle_websocket_upgrade, but that's not a blocker.

Those two last comments were my last here. 👍

[Kludex]

I'm fine with this implementation (just one last thing on the docs).

Thanks @Argannor! 🙌

@euri10 Would you like to check this as well?

Hold on... I want the event parameter out. 👀

[Argannor]

Sry the previous commit only changed the signature not the call, have been doing multiple things at once..
@Kludex Thank you for your review!

[euri10]

@euri10 Would you like to check this as well?

wont have the time sorry

[Kludex]

@euri10 Would you like to check this as well?

wont have the time sorry

Ok. I'll deal with this. Thanks for letting me know. 🙏

[Kludex]

@Argannor I actually have a question that I didn't think before... But if I have one of the WebSockets package installed, why should we ignore the upgrade request to websocket?

[Argannor]

@Kludex The application might run in a shared environment (globally installed packages) but not implement websocket endpoints.

[Kludex]

@Kludex The application might run in a shared environment (globally installed packages) but not implement websocket endpoints.

But you can set the ws=None even if the packages are installed. I'm trying to understand something here: isn't the real issue the fact that the current implementation returns a 400 instead of ignoring the upgrade when the ws parameter is None? 🤔

[Argannor]

Hm I think we could eliminate the new flag by changing it to this

Suggested change

	and not self.config.ws_ignore_upgrade
	and self.ws_protocol_class is not None

and removing the corresponding block from handle_websocket_upgrade. I think that would achieve the same thing.

[Kludex]

I think the warning should stay tho 🤔

[Argannor]

So trying to understand what that block tries to achieve is

• logging a warning
• logging another warning if no protocol was found
• responding with 400

But should these warnings really be logged on every upgrade request again? Especially the second one.
In my use case this will lead to a lot of unnecessary log messages.

[Argannor]

What do you think about moving the second log statement to the run method in main.py? I feel like this indicates a possible configuration / setup issue which would be easier to catch if printed up front on startup. We should rephrase to account for the fact that there are applications which will use the default setting auto without the need for an implementation to be present.

I am still undecided on what to do with the first warning. Since I already have a custom log handler, I could filter this message out to prevent the broken window for me.

Alternatively we could document the behaviour and necessary remediation inside the docs to help confused devs figure out why their request gets ignored and or turn down the log level.

Side note: I am not available today, but will come back to this tomorrow

[tomchristie]

Hi folks.

Thanks for looping me into the review @Kludex. There's two different things being addressed here, and I think we need to treat them independently...

• Ignore Upgrade requests that we don't handle. As raised by Gracefully handle HTTP/2 upgrade request #1501.
• Support websockets being configured as "off" without raising warnings on ws upgrades. As discussed in Ignore Websocket Upgrade Requests #1659.

I would suggest that we start off by dealing with the first of these. That looks clearly sensible to me, and we can improve our behaviour here without adding any additional configuration.

It looks to me like the second of these cases should be handled by ensuring that --ws none has the desired behaviour, rather than adding an additional configuration option? (As it happens that would probably be a much better default than auto, but that's a different consideration.)

[Argannor]

Hi @tomchristie thanks for having a look at this too!

I pushed a new iteration of this PR and integrated the feedback of both of you. The check is now based on the ws_protocol_class but if it isn't set to None warnings will be logged.

I also tried to split the concerns of #1501 and #1659, however I think that it makes the code more complicated in the case of httptools. While still trying to use the natively implemented parser.should_upgrade in the hope of it being more performant.

[tomchristie]

This seems sensible to me now.

[Kludex]

Why the one in the other protocol is implemented in one way, and this one in another? 🤔

[Argannor]

Both protocols work a bit different, and in the case of httptools the "check and return" is repeated in several places, but the actual upgrade handling is only done once in the except block. While the h11 implementation only does it in one single place and both the returning and handling is in the same place.

[Kludex]

We are checking all the headers 3 times on the httptools implementation now. Is it something we should be concerned?

I'm checking if it's a problem to iterate over the headers multiple times for httptools

[Argannor]

Hi @Kludex, thank you for looking into this. Are there any updates, or changes that I should make?

[Kludex]

Thanks @Argannor 🙏