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

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

Merged
merged 13 commits into from Oct 19, 2022

Conversation

Argannor
Copy link
Contributor

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 Kludex mentioned this pull request Sep 21, 2022
5 tasks
@Kludex Kludex requested a review from euri10 September 21, 2022 21:03
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

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

uvicorn/main.py Outdated Show resolved Hide resolved
if name == b"connection":
connection = [token.lower().strip() for token in value.split(b",")]
if name == b"upgrade":
upgrade = value.lower().strip()
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Is there a reason for stripping?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Argannor
Copy link
Contributor Author

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

uvicorn/protocols/http/h11_impl.py Outdated Show resolved Hide resolved
uvicorn/protocols/http/h11_impl.py Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

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. 👍

uvicorn/main.py Outdated Show resolved Hide resolved
uvicorn/main.py Outdated Show resolved Hide resolved
Kludex
Kludex previously approved these changes Sep 22, 2022
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

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

Thanks @Argannor! 🙌

@euri10 Would you like to check this as well?

docs/settings.md Outdated Show resolved Hide resolved
@Kludex Kludex dismissed their stale review September 22, 2022 07:47

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

Kludex
Kludex previously approved these changes Sep 22, 2022
@Argannor
Copy link
Contributor Author

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

tests/protocols/test_http.py Outdated Show resolved Hide resolved
@euri10
Copy link
Member

euri10 commented Sep 26, 2022

@euri10 Would you like to check this as well?

wont have the time sorry

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 26, 2022

@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
Copy link
Sponsor Member

Kludex commented Sep 26, 2022

@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
Copy link
Contributor Author

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

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 26, 2022

@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? 🤔

return (
b"upgrade" in connection
and upgrade == b"websocket"
and not self.config.ws_ignore_upgrade
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think the warning should stay tho 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Argannor Argannor Sep 27, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

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 #1501.
  • Support websockets being configured as "off" without raising warnings on ws upgrades. As discussed in #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
Copy link
Contributor Author

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
Copy link
Member

This seems sensible to me now.

tests/protocols/test_http.py Outdated Show resolved Hide resolved
uvicorn/protocols/http/httptools_impl.py Show resolved Hide resolved
return upgrade
return None

def _should_upgrade_to_ws(self) -> bool:
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Sponsor Member

Kludex commented Oct 1, 2022

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

@Kludex Kludex dismissed their stale review October 1, 2022 10:55

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

@Argannor
Copy link
Contributor Author

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

Kludex
Kludex approved these changes Oct 19, 2022
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Thanks @Argannor 🙏

@Kludex Kludex merged commit 255dcde into encode:master Oct 19, 2022
15 checks passed
Kludex pushed a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants