hyperium / h2

HTTP 2.0 client & server implementation for Rust.
MIT License
1.34k stars 265 forks source link

Respond 400 + Reset to malformed request #748

Open franfastly opened 4 months ago

franfastly commented 4 months ago

h2 returns RST_STREAM frames with the PROTOCOL_ERROR bit set as a response to many types of errors in client requests. Many of those cases, when handled by an HTTP/1 server such as the one used in hyper, would result in an HTTP 400 Bad Request response returned to the client rather than a TCP reset (the HTTP/1 equivalent of a RST_STREAM). As a consequence, a client will observe differences in behaviour between HTTP/1 and HTTP/2: a malformed request will result in a 400 response when HTTP/1 is used whereas the same request will result in a reset stream with h2.

This PR makes h2 reply a HEADERS+400+END_STREAM frame followed by a RST_STREAM+PROTOCOL_ERROR frame to all the malformed!() macro invocations in Peer::convert_poll_message() in src/server.rs.

The Reset variant in the h2::proto::error::Error Enum now contains an Option<http::StatusCode> value that is set by the malformed!() macro with a Some(400). That value is propagated all the way until h2::proto::streams::send::Send::send_reset() where, if not None, a h2::frame::headers::Headers frame with the status code and the END_STREAM flag set will now be sent to the client before the RST_STREAM frame.

Some of the parameters passed to send_reset() have been grouped into a new SendResetContext Struct in order to avoid a clippy::too_many_arguments lint.

Tests where malformed requests were sent have been updated to check that an additional HEADERS+400+END_STREAM frame is now received before the RST_STREAM + PROTOCOL_ERROR frame.

This change has been validated with other clients like curl, a custom ad-hoc client written with python3+httpx and varnishtest.

Fixes #747

franfastly commented 4 months ago

I'm not sure why clippy started complaining about this. I could amend this PR to appease clippy or push a fix in separate PR.

seanmonstar commented 4 months ago

Could be a newer clippy being a jerk.

acfoltzer commented 4 months ago

@seanmonstar I'm taking over this one from Fran while he's on vacation for the next few weeks. I would offer to do the review myself, but I'm the coauthor of the internal predecessor to this patch; it probably would be good to have a third party perspective before landing this change.