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

fix(client): Fix HTTP/2 websocket request #165

Merged
merged 6 commits into from
Jan 28, 2025

Conversation

0x676e67
Copy link
Contributor

@0x676e67 0x676e67 commented Jan 26, 2025

tracking: hyperium/hyper#3840

Through debugging, it is found that HTTP/2 CONNECT is treated as HTTP/1.1 CONNECT and the request path is processed. Comment this part of the code and the handshake is normal

According to RFC 8841, the example must carry the :path pseudo-header

This is the simplest fix, I don't know if there is something else I'm missing

@0x676e67 0x676e67 changed the title fix(client): Fix HTTP/2 upgrade WebSocket fix(client): Fix HTTP/2 WebSocket request Jan 27, 2025
@0x676e67 0x676e67 changed the title fix(client): Fix HTTP/2 WebSocket request fix(client): Fix HTTP/2 websocket request Jan 27, 2025
@seanmonstar
Copy link
Member

Wow, nice detective work! Would you be up to adding a legacy test after fn client_upgrade() that checks the same thing but over HTTP/2? I wouldn't want to break this in a refactor.

If you don't have time, I can get to it eventually.

@0x676e67
Copy link
Contributor Author

OK, I'll do this.

@0x676e67
Copy link
Contributor Author

I’ve added a new HTTP/2 upgrade test. If you need anything else, feel free to let me know.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@seanmonstar seanmonstar merged commit 0c402d8 into hyperium:master Jan 28, 2025
16 checks passed
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.

2 participants