Closed mstyura closed 1 month ago
Thanks! For writing a unit test, you can put it in client_request.rs
, similar to this one: https://github.com/hyperium/h2/blob/0d66e3cba2da9925dc3f277b9c71b96356789a76/tests/h2-tests/tests/client_request.rs#L527
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.
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:
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.
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 CONNECT
s it was impossible to use protocol
helper method to properly construct extended CONNECT
.
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;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 } }
Hello there! Is there anything I can do to facilitate this PR get reviewed? Thanks a lot in advance.
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 😆.
I was actually mid review when you asked
That's complete coincidence or some universe tricks :)
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 forCONNECT
method. Empty:path
field forOPTIONS
requests now translates to'*'
value sent in:path
field.CONNECT
request changes were tested against server based on hyper 1.2.