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

http3: only use :protocol pseudo-header for Extended CONNECT #4261

Merged
merged 4 commits into from
Jan 26, 2024

Conversation

taoso
Copy link
Contributor

@taoso taoso commented Jan 23, 2024

The default value should be "HTTP/3.0".

Copy link

google-cla bot commented Jan 23, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

The default value should be "HTTP/3.0".
@marten-seemann
Copy link
Member

Can you explain the motivation behind this PR?

@taoso
Copy link
Contributor Author

taoso commented Jan 23, 2024

Can you explain the motivation behind this PR?

hi @marten-seemann If the client fire a CONNECT request without :protocol, this req.Proto will be empty without this patch.

@marten-seemann marten-seemann changed the title Fix protocol http3: set the http.Request.Proto to "HTTP/3.0" Jan 24, 2024
Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

I think this PR is correct, but I'd to tighten the validation logic here a bit. The :protocol pseudo header is only defined for Extended Connect requests (RFC 9220). We should reject any non-Extended Connected request that sets this header field.

@taoso Do you want to update the PR? We'd also need a test case that tests that we correctly reject requests with :protocol header.

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a2cf43d) 84.06% compared to head (c437c54) 84.07%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4261      +/-   ##
==========================================
+ Coverage   84.06%   84.07%   +0.01%     
==========================================
  Files         150      150              
  Lines       15425    15424       -1     
==========================================
+ Hits        12966    12967       +1     
+ Misses       1955     1954       -1     
+ Partials      504      503       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@taoso
Copy link
Contributor Author

taoso commented Jan 24, 2024

Hi @marten-seemann , let me try.

The :protocol pseudo header is only defined for
Extended Connect requests (RFC 9220).
http3/headers.go Outdated Show resolved Hide resolved
@marten-seemann marten-seemann added this to the v0.42 milestone Jan 24, 2024
Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for pushing this through @taoso!

@marten-seemann marten-seemann changed the title http3: set the http.Request.Proto to "HTTP/3.0" http3: only use :protocol pseudo-header for Extended Connect, set Request.Proto for Connect Jan 24, 2024
@marten-seemann marten-seemann changed the title http3: only use :protocol pseudo-header for Extended Connect, set Request.Proto for Connect http3: fix usage of :protocol pseudo-header for CONNECT requests Jan 24, 2024
@marten-seemann marten-seemann changed the title http3: fix usage of :protocol pseudo-header for CONNECT requests http3: only use :protocol pseudo-header for Extended CONNECT requests Jan 24, 2024
@marten-seemann marten-seemann changed the title http3: only use :protocol pseudo-header for Extended CONNECT requests http3: only use :protocol pseudo-header for Extended CONNECT Jan 24, 2024
@taoso
Copy link
Contributor Author

taoso commented Jan 25, 2024

Hi @marten-seemann Could this PR be merged?

@marten-seemann marten-seemann merged commit 808f849 into quic-go:master Jan 26, 2024
19 checks passed
@taoso taoso deleted the proto branch January 26, 2024 05:44
mgjeong pushed a commit to mgjeong/quic-go that referenced this pull request Feb 13, 2024
…#4261)

* Fix protocol

The default value should be "HTTP/3.0".

* Reject normal request with :protocol header

The :protocol pseudo header is only defined for
Extended Connect requests (RFC 9220).

* save one branch check

* Fix review issue
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

2 participants