Respond with an HTTP 4xx than only a RST_STREAM to malformed requests
#747
Comments
|
I remember that conversation. I think that's probably generally fine. cc @acfoltzer @nox @Noah-Kennedy, thoughts? |
|
@seanmonstar I've been working with @franfastly on this investigation and an internal patch we've been cooking up to address the issue. I think overall I would prefer that |
franfastly
added a commit
to franfastly/h2
that referenced
this issue
Feb 20, 2024
`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`. Make `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 be sent to the client before the `RST_STREAM` frame is sent. 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 a `HEADERS`+`400`+`END_STREAM` frame is now received before the `RST_STREAM + PROTOCOL_ERROR` frame. In addition to validating this change against the `h2` tests and the `h2spec` test suite, I have also tested it with other clients like `curl` a custom ad-hoc client written with `python3+httpx` and `varnishtest`.
franfastly
added a commit
to franfastly/h2
that referenced
this issue
Feb 20, 2024
`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`. Make `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`.
franfastly
added a commit
to franfastly/h2
that referenced
this issue
Feb 22, 2024
`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`. Make `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`.
franfastly
added a commit
to franfastly/h2
that referenced
this issue
Feb 22, 2024
`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`. Make `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`.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Issue
h2returnsRST_STREAMframes with thePROTOCOL_ERRORbit 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 inhyper, would result in an HTTP 400 Bad Request response returned to the client rather than a TCP reset (the HTTP/1 equivalent of aRST_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 withh2.Proposal
There was a conversation in the mailing list of the IETF-HTTP-WG about whether a server can send a response to malformed requests instead of treating them as a stream error. From Steffan Eising's last response to that thread:
Although there was no consensus about specifying a single correct behaviour, I think it was clear that a 400 response was acceptable.
I could create a PR to make
h2reply aHEADERS+400+END_STREAMframe followed by aRST_STREAM+PROTOCOL_ERRORframe to all themalformed!()macro invocations inh2/src/server.rs
Line 1506 in 0077d3d
The text was updated successfully, but these errors were encountered: