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

#769: Fix request pseudo-header construction for CONNECT & OPTION methods #770

Merged
merged 1 commit into from
May 17, 2024

Conversation

mstyura
Copy link
Contributor

@mstyura mstyura commented Apr 23, 2024

CONNECT & OPTIONS request has special requirements regarding :path & :scheme pseudo-header which were not met.

Construction of pseudo-header was fixed to not include :path & :scheme fields for CONNECT method. Empty :path field for OPTIONS requests now translates to '*' value sent in :path field.

CONNECT request changes were tested against server based on hyper 1.2.

@seanmonstar
Copy link
Member

Thanks! For writing a unit test, you can put it in client_request.rs, similar to this one:

async fn http_11_request_without_scheme_or_authority() {

Set the correct fields for the http::Request::builder(), and then on the server mock, assert that the frame::headers() was received without setting .scheme() or path.

CONNECT & OPTIONS request has special requirements regarding :path & :scheme pseudo-header which were not met.

Construction of pseudo-header was fixed to not include :path & :scheme fields for CONNECT method.
Empty :path field for OPTIONS requests now translates to '*' value sent in :path field.

In addition to tests included in MR the CONNECT request changes were tested against server based on hyper 1.2.
@mstyura
Copy link
Contributor Author

mstyura commented Apr 28, 2024

Thanks @seanmonstar! It is occurred that when tested locally I didn't run whole set of tests and didn't notice I've broken extended connect request. So to fix this a bit more changes were required.
Here I'll try to explain each change I've introduced:

  1. In src/frame/headers.rs I've fixed Pseudo::request to properly handle regular CONNECT request as well as special case of OPTIONS request. In addition to tests which checks CONNECT request on higher level I've also decided to add additional simple unit tests for Pseudo::request to validate Pseudo properly constructed from provided arguments for regular requests (GET, POST, etc) as well as CONNECT, extended CONNECT and special case of OPTIONS request.

  2. In tests/h2-support/src/frames.rs I've decided to replace helper method to set :protocol with method to define whole Pseudo struct. The problem of protocol method I've noticed is that the presence of protocol change the rules of generation pseudo fields - :scheme & :path become mandatory in case of extended CONNECT and with fixed implementation of regular CONNECTs it was impossible to use protocol helper method to properly construct extended CONNECT.

  3. In tests/h2-tests/tests/client_request.rs:

  • extended_connect_request test started to fail exactly due to regular CONNECT implementation has been fixed and setting protocol after pseudo already constructed didn't lead to :scheme & :path included;
  • http_2_connect_request_omit_scheme_and_path_fields test you've suggested to add - it check that regular CONNECT request does not send :path and :scheme to server;
  1. In tests/h2-tests/tests/server.rs
  • extended_connect_protocol_disabled_by_default relied on request + protocol helper methods which started to produce incorrect extended CONNECT request - replaced with newly added pseudo helper method;
  • extended_connect_protocol_enabled_during_handshake same reason to change as above;. reject_pseudo_protocol_on_non_connect_request relied on request + protocol and due to nature of test protocol method worked here, but it was single "valid" usage of it, so I've replaced it with pseudo and removed protocol.
  • reject_authority_target_on_extended_connect_request in my opinion test name was misleading, the actual behavior it tested was rejection of connection due to missing :scheme in extended connect request. Renamed it to reject_extended_connect_request_without_scheme. For the reference here are logs of test run on current master branch:
│ │ │ ├─  0ms DEBUG h2::server malformed headers: missing scheme
│ │ │ ├─  0ms TRACE h2::proto::streams::send send_reset(..., reason=PROTOCOL_ERROR, initiator=Library, stream=StreamId(1), ..., is_reset=false; is_closed=false; pending_send.is_empty=true; state=State { inner: Open { local: AwaitingHeaders, remote: Streaming } } 
  • reject_non_authority_target_on_connect_request IMO was also misleadingly named and it actually tested incorrect behaviour of regular CONNECT which I've fixed. So that test becoming to fail because there is no reason to constructed request being rejected. I've renamed test to reject_extended_connect_request_without_path and adjusted it to test valid reason to reject of extended connect request i.e. absence of :path. For the reference here are logs of test run on current master branch:
│ │ │ ├─  0ms DEBUG h2::server malformed headers: :scheme in CONNECT
│ │ │ ├─  0ms TRACE h2::proto::streams::send send_reset(..., reason=PROTOCOL_ERROR, initiator=Library, stream=StreamId(1), ..., is_reset=false; is_closed=false; pending_send.is_empty=true; state=State { inner: Open { local: AwaitingHeaders, remote: Streaming } } 

@mstyura mstyura marked this pull request as ready for review April 28, 2024 17:11
@mstyura
Copy link
Contributor Author

mstyura commented May 15, 2024

Hello there! Is there anything I can do to facilitate this PR get reviewed? Thanks a lot in advance.

Copy link
Contributor

@Noah-Kennedy Noah-Kennedy left a comment

Choose a reason for hiding this comment

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

I reviewed the aspects of this to do with extended connect, and that looked alright to me.

Going to leave the final review to @seanmonstar.

}
_ => {}
}
let (scheme, path) = if method == Method::CONNECT && protocol.is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The handling of extended connect looks correct to me here

@seanmonstar should an attempt to make a malformed request be silently corrected or should we panic?

@Noah-Kennedy
Copy link
Contributor

Hello there! Is there anything I can do to facilitate this PR get reviewed? Thanks a lot in advance.

It was blocked on me having time to review for the past two weeks.

Not sure if you asked in discord earlier and that's why Sean pinged me, but I was actually mid review when you asked 😆.

@mstyura
Copy link
Contributor Author

mstyura commented May 16, 2024

I was actually mid review when you asked

That's complete coincidence or some universe tricks :)

@seanmonstar seanmonstar merged commit c83b2d5 into hyperium:master May 17, 2024
6 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.

3 participants