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.

Sign up for GitHub

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

Jump to bottom

Return rejection if WebSocket request couldn't be upgraded #1135

Merged
merged 5 commits into from Jul 11, 2022
Merged

Return rejection if WebSocket request couldn't be upgraded #1135

merged 5 commits into from Jul 11, 2022

Conversation

davidpdrsn
Copy link
Member

Fixes #1132

Rather than panicking we'll return a rejection. Unfortunately hyper doesn't give any details about why the connection couldn't be upgraded.

@davidpdrsn davidpdrsn requested a review from jplatte July 1, 2022 08:18
@davidpdrsn davidpdrsn mentioned this pull request Jul 1, 2022
Closed
jplatte
jplatte reviewed Jul 1, 2022
View reviewed changes
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

I suggest somebody who actually knows stuff about WebSockets should review this.

davidpdrsn
davidpdrsn commented Jul 1, 2022
View reviewed changes
axum/src/extract/ws.rs Outdated Show resolved Hide resolved
@davidpdrsn
Copy link
Member Author

@jplatte I've changed the rejection to specifically call out not being able to upgrade the connection, perhaps because its http/1.0

@davidpdrsn davidpdrsn enabled auto-merge (squash) July 6, 2022 20:04
@davidpdrsn davidpdrsn requested a review from jplatte July 6, 2022 20:04
@davidpdrsn
Copy link
Member Author

@jplatte gentle ping (:

@jplatte
Copy link
Member

jplatte commented Jul 11, 2022

I still don't think I'm the right person to review this though 😅

@davidpdrsn
Copy link
Member Author

In that case I think I'll just yolo it. Its a pretty safe change and given what sean said it should be correct.

@davidpdrsn davidpdrsn disabled auto-merge July 11, 2022 12:33
@davidpdrsn davidpdrsn merged commit 92dbbd3 into main Jul 11, 2022
@davidpdrsn davidpdrsn deleted the reject-unupgradeable-ws-requests branch July 11, 2022 12:33
davidpdrsn added a commit that referenced this pull request Jul 15, 2022
@davidpdrsn
Return rejection if WebSocket request couldn't be upgraded (#1135)
e2ff65f 
* Return rejection if `WebSocket` request couldn't be upgraded

* make rejection about connection upgrades

* changelog links
davidpdrsn added a commit that referenced this pull request Jul 15, 2022
@davidpdrsn
axum: Version 0.5.13
527389a 
Backports #1135 to 0.5.x
@davidpdrsn davidpdrsn mentioned this pull request Jul 15, 2022
Merged
davidpdrsn added a commit that referenced this pull request Jul 15, 2022
@davidpdrsn
axum: Version 0.5.13 (#1164)
b7e70a2 
Backports #1135 to 0.5.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanmonstar seanmonstar seanmonstar left review comments

@jplatte jplatte Awaiting requested review from jplatte

Assignees
No one assigned
Labels
A-axum
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reverse proxied web socket extraction can panic
3 participants
@davidpdrsn @jplatte @seanmonstar