Skip to content

Commit 7b41f65

Browse files
franfastlyfranfastly
committed
Respond 400 + Reset to malformed request (hyperium#747)
`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 parent 0077d3d commit 7b41f65

File tree

11 files changed

+110
-32
lines changed

11 files changed

+110
-32
lines changed

‎src/error.rs

+11-8
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ use crate::frame::StreamId;
33
use crate::proto::{self, Initiator};
44

55
use bytes::Bytes;
6+
use http::StatusCode;
67
use std::{error, fmt, io};
78

89
pub use crate::frame::Reason;
@@ -26,7 +27,7 @@ pub struct Error {
2627
enum Kind {
2728
/// A RST_STREAM frame was received or sent.
2829
#[allow(dead_code)]
29-
Reset(StreamId, Reason, Initiator),
30+
Reset(StreamId, Reason, Initiator, Option<StatusCode>),
3031

3132
/// A GO_AWAY frame was received or sent.
3233
GoAway(Bytes, Reason, Initiator),
@@ -51,7 +52,7 @@ impl Error {
5152
/// action taken by the peer (i.e. a protocol error).
5253
pub fn reason(&self) -> Option<Reason> {
5354
match self.kind {
54-
Kind::Reset(_, reason, _) | Kind::GoAway(_, reason, _) | Kind::Reason(reason) => {
55+
Kind::Reset(_, reason, _, _) | Kind::GoAway(_, reason, _) | Kind::Reason(reason) => {
5556
Some(reason)
5657
}
5758
_ => None,
@@ -101,7 +102,7 @@ impl Error {
101102
pub fn is_remote(&self) -> bool {
102103
matches!(
103104
self.kind,
104-
Kind::GoAway(_, _, Initiator::Remote) | Kind::Reset(_, _, Initiator::Remote)
105+
Kind::GoAway(_, _, Initiator::Remote) | Kind::Reset(_, _, Initiator::Remote, _)
105106
)
106107
}
107108

@@ -111,7 +112,7 @@ impl Error {
111112
pub fn is_library(&self) -> bool {
112113
matches!(
113114
self.kind,
114-
Kind::GoAway(_, _, Initiator::Library) | Kind::Reset(_, _, Initiator::Library)
115+
Kind::GoAway(_, _, Initiator::Library) | Kind::Reset(_, _, Initiator::Library, _)
115116
)
116117
}
117118
}
@@ -122,7 +123,9 @@ impl From<proto::Error> for Error {
122123

123124
Error {
124125
kind: match src {
125-
Reset(stream_id, reason, initiator) => Kind::Reset(stream_id, reason, initiator),
126+
Reset(stream_id, reason, initiator, status_code) => {
127+
Kind::Reset(stream_id, reason, initiator, status_code)
128+
}
126129
GoAway(debug_data, reason, initiator) => {
127130
Kind::GoAway(debug_data, reason, initiator)
128131
}
@@ -162,13 +165,13 @@ impl From<UserError> for Error {
162165
impl fmt::Display for Error {
163166
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
164167
let debug_data = match self.kind {
165-
Kind::Reset(_, reason, Initiator::User) => {
168+
Kind::Reset(_, reason, Initiator::User, _) => {
166169
return write!(fmt, "stream error sent by user: {}", reason)
167170
}
168-
Kind::Reset(_, reason, Initiator::Library) => {
171+
Kind::Reset(_, reason, Initiator::Library, _) => {
169172
return write!(fmt, "stream error detected: {}", reason)
170173
}
171-
Kind::Reset(_, reason, Initiator::Remote) => {
174+
Kind::Reset(_, reason, Initiator::Remote, _) => {
172175
return write!(fmt, "stream error received: {}", reason)
173176
}
174177
Kind::GoAway(ref debug_data, reason, Initiator::User) => {

‎src/proto/connection.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -447,7 +447,7 @@ where
447447
// Attempting to read a frame resulted in a stream level error.
448448
// This is handled by resetting the frame then trying to read
449449
// another frame.
450-
Err(Error::Reset(id, reason, initiator)) => {
450+
Err(Error::Reset(id, reason, initiator, _)) => {
451451
debug_assert_eq!(initiator, Initiator::Library);
452452
tracing::trace!(?id, ?reason, "stream error");
453453
self.streams.send_reset(id, reason);

‎src/proto/error.rs

+14-5
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@ use crate::codec::SendError;
22
use crate::frame::{Reason, StreamId};
33

44
use bytes::Bytes;
5+
use http::StatusCode;
56
use std::fmt;
67
use std::io;
78

89
/// Either an H2 reason or an I/O error
910
#[derive(Clone, Debug)]
1011
pub enum Error {
11-
Reset(StreamId, Reason, Initiator),
12+
Reset(StreamId, Reason, Initiator, Option<StatusCode>),
1213
GoAway(Bytes, Reason, Initiator),
1314
Io(io::ErrorKind, Option<String>),
1415
}
@@ -23,7 +24,7 @@ pub enum Initiator {
2324
impl Error {
2425
pub(crate) fn is_local(&self) -> bool {
2526
match *self {
26-
Self::Reset(_, _, initiator) | Self::GoAway(_, _, initiator) => initiator.is_local(),
27+
Self::Reset(_, _, initiator, _) | Self::GoAway(_, _, initiator) => initiator.is_local(),
2728
Self::Io(..) => true,
2829
}
2930
}
@@ -33,7 +34,15 @@ impl Error {
3334
}
3435

3536
pub(crate) fn library_reset(stream_id: StreamId, reason: Reason) -> Self {
36-
Self::Reset(stream_id, reason, Initiator::Library)
37+
Self::Reset(stream_id, reason, Initiator::Library, None)
38+
}
39+
40+
pub(crate) fn library_reset_with_status_code(
41+
stream_id: StreamId,
42+
reason: Reason,
43+
status_code: StatusCode,
44+
) -> Self {
45+
Self::Reset(stream_id, reason, Initiator::Library, Some(status_code))
3746
}
3847

3948
pub(crate) fn library_go_away(reason: Reason) -> Self {
@@ -45,7 +54,7 @@ impl Error {
4554
}
4655

4756
pub(crate) fn remote_reset(stream_id: StreamId, reason: Reason) -> Self {
48-
Self::Reset(stream_id, reason, Initiator::Remote)
57+
Self::Reset(stream_id, reason, Initiator::Remote, None)
4958
}
5059

5160
pub(crate) fn remote_go_away(debug_data: Bytes, reason: Reason) -> Self {
@@ -65,7 +74,7 @@ impl Initiator {
6574
impl fmt::Display for Error {
6675
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
6776
match *self {
68-
Self::Reset(_, reason, _) | Self::GoAway(_, reason, _) => reason.fmt(fmt),
77+
Self::Reset(_, reason, _, _) | Self::GoAway(_, reason, _) => reason.fmt(fmt),
6978
Self::Io(_, Some(ref inner)) => inner.fmt(fmt),
7079
Self::Io(kind, None) => io::Error::from(kind).fmt(fmt),
7180
}

‎src/proto/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ pub use self::error::{Error, Initiator};
1111
pub(crate) use self::peer::{Dyn as DynPeer, Peer};
1212
pub(crate) use self::ping_pong::UserPings;
1313
pub(crate) use self::streams::{DynStreams, OpaqueStreamRef, StreamRef, Streams};
14-
pub(crate) use self::streams::{Open, PollReset, Prioritized};
14+
pub(crate) use self::streams::{Open, PollReset, Prioritized, SendResetContext};
1515

1616
use crate::codec::Codec;
1717

‎src/proto/streams/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ mod streams;
1212

1313
pub(crate) use self::prioritize::Prioritized;
1414
pub(crate) use self::recv::Open;
15-
pub(crate) use self::send::PollReset;
15+
pub(crate) use self::send::{PollReset, SendResetContext};
1616
pub(crate) use self::streams::{DynStreams, OpaqueStreamRef, StreamRef, Streams};
1717

1818
use self::buffer::Buffer;

‎src/proto/streams/send.rs

+51-6
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,33 @@ pub(crate) enum PollReset {
4747
Streaming,
4848
}
4949

50+
/// Context for `send_reset`.
51+
pub(crate) struct SendResetContext {
52+
reason: Reason,
53+
initiator: Initiator,
54+
status_code: Option<http::StatusCode>,
55+
}
56+
57+
impl SendResetContext {
58+
/// Create a new `SendResetContext` with and optional `http::StatusCode`.
59+
pub(crate) fn with_status_code(
60+
reason: Reason,
61+
initiator: Initiator,
62+
status_code: Option<http::StatusCode>,
63+
) -> Self {
64+
Self {
65+
reason,
66+
initiator,
67+
status_code,
68+
}
69+
}
70+
71+
/// Create a new `SendResetContext`
72+
pub(crate) fn new(reason: Reason, initiator: Initiator) -> Self {
73+
Self::with_status_code(reason, initiator, None)
74+
}
75+
}
76+
5077
impl Send {
5178
/// Create a new `Send`
5279
pub fn new(config: &Config) -> Self {
@@ -170,8 +197,7 @@ impl Send {
170197
/// Send an explicit RST_STREAM frame
171198
pub fn send_reset<B>(
172199
&mut self,
173-
reason: Reason,
174-
initiator: Initiator,
200+
context: SendResetContext,
175201
buffer: &mut Buffer<Frame<B>>,
176202
stream: &mut store::Ptr,
177203
counts: &mut Counts,
@@ -182,18 +208,25 @@ impl Send {
182208
let is_empty = stream.pending_send.is_empty();
183209
let stream_id = stream.id;
184210

211+
let SendResetContext {
212+
reason,
213+
initiator,
214+
status_code,
215+
} = context;
216+
185217
tracing::trace!(
186218
"send_reset(..., reason={:?}, initiator={:?}, stream={:?}, ..., \
187219
is_reset={:?}; is_closed={:?}; pending_send.is_empty={:?}; \
188-
state={:?} \
220+
state={:?}; status_code={:?} \
189221
",
190222
reason,
191223
initiator,
192224
stream_id,
193225
is_reset,
194226
is_closed,
195227
is_empty,
196-
stream.state
228+
stream.state,
229+
status_code
197230
);
198231

199232
if is_reset {
@@ -225,6 +258,19 @@ impl Send {
225258
// `reclaim_all_capacity`.
226259
self.prioritize.clear_queue(buffer, stream);
227260

261+
// For malformed requests, a server may send an HTTP response prior to resetting the stream.
262+
if let Some(status_code) = status_code {
263+
tracing::trace!("send_reset -- sending response with status code: {status_code}");
264+
let pseudo = frame::Pseudo::response(status_code);
265+
let fields = http::HeaderMap::default();
266+
let mut frame = frame::Headers::new(stream.id, pseudo, fields);
267+
frame.set_end_stream();
268+
269+
tracing::trace!("send_reset -- queueing response; frame={:?}", frame);
270+
self.prioritize
271+
.queue_frame(frame.into(), buffer, stream, task);
272+
}
273+
228274
let frame = frame::Reset::new(stream.id, reason);
229275

230276
tracing::trace!("send_reset -- queueing; frame={:?}", frame);
@@ -378,8 +424,7 @@ impl Send {
378424
tracing::debug!("recv_stream_window_update !!; err={:?}", e);
379425

380426
self.send_reset(
381-
Reason::FLOW_CONTROL_ERROR,
382-
Initiator::Library,
427+
SendResetContext::new(Reason::FLOW_CONTROL_ERROR, Initiator::Library),
383428
buffer,
384429
stream,
385430
counts,

‎src/proto/streams/state.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,9 @@ impl State {
333333

334334
/// Set the stream state to reset locally.
335335
pub fn set_reset(&mut self, stream_id: StreamId, reason: Reason, initiator: Initiator) {
336-
self.inner = Closed(Cause::Error(Error::Reset(stream_id, reason, initiator)));
336+
self.inner = Closed(Cause::Error(Error::Reset(
337+
stream_id, reason, initiator, None,
338+
)));
337339
}
338340

339341
/// Set the stream state to a scheduled reset.
@@ -364,7 +366,7 @@ impl State {
364366
pub fn is_remote_reset(&self) -> bool {
365367
matches!(
366368
self.inner,
367-
Closed(Cause::Error(Error::Reset(_, _, Initiator::Remote)))
369+
Closed(Cause::Error(Error::Reset(_, _, Initiator::Remote, _)))
368370
)
369371
}
370372

@@ -446,7 +448,7 @@ impl State {
446448
/// Returns a reason if the stream has been reset.
447449
pub(super) fn ensure_reason(&self, mode: PollReset) -> Result<Option<Reason>, crate::Error> {
448450
match self.inner {
449-
Closed(Cause::Error(Error::Reset(_, reason, _)))
451+
Closed(Cause::Error(Error::Reset(_, reason, _, _)))
450452
| Closed(Cause::Error(Error::GoAway(_, reason, _)))
451453
| Closed(Cause::ScheduledLibraryReset(reason)) => Ok(Some(reason)),
452454
Closed(Cause::Error(ref e)) => Err(e.clone().into()),

‎src/proto/streams/streams.rs

+9-5
Original file line numberDiff line numberDiff line change
@@ -1537,8 +1537,7 @@ impl Actions {
15371537
) {
15381538
counts.transition(stream, |counts, stream| {
15391539
self.send.send_reset(
1540-
reason,
1541-
initiator,
1540+
proto::SendResetContext::new(reason, initiator),
15421541
send_buffer,
15431542
stream,
15441543
counts,
@@ -1557,15 +1556,20 @@ impl Actions {
15571556
counts: &mut Counts,
15581557
res: Result<(), Error>,
15591558
) -> Result<(), Error> {
1560-
if let Err(Error::Reset(stream_id, reason, initiator)) = res {
1559+
if let Err(Error::Reset(stream_id, reason, initiator, status_code)) = res {
15611560
debug_assert_eq!(stream_id, stream.id);
15621561

15631562
if counts.can_inc_num_local_error_resets() {
15641563
counts.inc_num_local_error_resets();
15651564

15661565
// Reset the stream.
1567-
self.send
1568-
.send_reset(reason, initiator, buffer, stream, counts, &mut self.task);
1566+
self.send.send_reset(
1567+
proto::SendResetContext::with_status_code(reason, initiator, status_code),
1568+
buffer,
1569+
stream,
1570+
counts,
1571+
&mut self.task,
1572+
);
15691573
Ok(())
15701574
} else {
15711575
tracing::warn!(

‎src/server.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ use crate::proto::{self, Config, Error, Prioritized};
121121
use crate::{FlowControl, PingPong, RecvStream, SendStream};
122122

123123
use bytes::{Buf, Bytes};
124-
use http::{HeaderMap, Method, Request, Response};
124+
use http::{HeaderMap, Method, Request, Response, StatusCode};
125125
use std::future::Future;
126126
use std::pin::Pin;
127127
use std::task::{Context, Poll};
@@ -1515,7 +1515,7 @@ impl proto::Peer for Peer {
15151515
macro_rules! malformed {
15161516
($($arg:tt)*) => {{
15171517
tracing::debug!($($arg)*);
1518-
return Err(Error::library_reset(stream_id, Reason::PROTOCOL_ERROR));
1518+
return Err(Error::library_reset_with_status_code(stream_id, Reason::PROTOCOL_ERROR, StatusCode::BAD_REQUEST));
15191519
}}
15201520
}
15211521

‎tests/h2-tests/tests/server.rs

+12
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,9 @@ async fn recv_invalid_authority() {
504504
let settings = client.assert_server_handshake().await;
505505
assert_default_settings!(settings);
506506
client.send_frame(bad_headers).await;
507+
client
508+
.recv_frame(frames::headers(1).status(StatusCode::BAD_REQUEST).eos())
509+
.await;
507510
client.recv_frame(frames::reset(1).protocol_error()).await;
508511
};
509512

@@ -1290,6 +1293,9 @@ async fn reject_pseudo_protocol_on_non_connect_request() {
12901293
)
12911294
.await;
12921295

1296+
client
1297+
.recv_frame(frames::headers(1).status(StatusCode::BAD_REQUEST).eos())
1298+
.await;
12931299
client.recv_frame(frames::reset(1).protocol_error()).await;
12941300
};
12951301

@@ -1329,6 +1335,9 @@ async fn reject_authority_target_on_extended_connect_request() {
13291335
)
13301336
.await;
13311337

1338+
client
1339+
.recv_frame(frames::headers(1).status(StatusCode::BAD_REQUEST).eos())
1340+
.await;
13321341
client.recv_frame(frames::reset(1).protocol_error()).await;
13331342
};
13341343

@@ -1364,6 +1373,9 @@ async fn reject_non_authority_target_on_connect_request() {
13641373
.send_frame(frames::headers(1).request("CONNECT", "https://bread/baguette"))
13651374
.await;
13661375

1376+
client
1377+
.recv_frame(frames::headers(1).status(StatusCode::BAD_REQUEST).eos())
1378+
.await;
13671379
client.recv_frame(frames::reset(1).protocol_error()).await;
13681380
};
13691381

‎tests/h2-tests/tests/stream_states.rs

+3
Original file line numberDiff line numberDiff line change
@@ -524,6 +524,9 @@ async fn recv_next_stream_id_updated_by_malformed_headers() {
524524
assert_default_settings!(settings);
525525
// bad headers -- should error.
526526
client.send_frame(bad_headers).await;
527+
client
528+
.recv_frame(frames::headers(1).status(StatusCode::BAD_REQUEST).eos())
529+
.await;
527530
client.recv_frame(frames::reset(1).protocol_error()).await;
528531
// this frame is good, but the stream id should already have been incr'd
529532
client

0 commit comments

Comments
 (0)