diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 0da14ae6992a..96c2769ac798 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -138,6 +138,10 @@ void CodecClient::onData(Buffer::Instance& data) { host_->cluster().stats().upstream_cx_protocol_error_.inc(); } } + + // All data should be consumed at this point if the connection remains open. + ASSERT(data.length() == 0 || connection_->state() != Network::Connection::State::Open, + absl::StrCat("extraneous bytes after response complete: ", data.length())); } CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& connection, diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 6a803cca1e6c..f8c530cbcf48 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -552,6 +552,16 @@ Http::Status ConnectionImpl::dispatch(Buffer::Instance& data) { [&](Buffer::Instance& data) -> Http::Status { return innerDispatch(data); }, data); } +Http::Status ClientConnectionImpl::dispatch(Buffer::Instance& data) { + Http::Status status = ConnectionImpl::dispatch(data); + if (status.ok() && data.length() > 0) { + // The HTTP/1.1 codec pauses dispatch after a single response is complete. Extraneous data + // after a response is complete indicates an error. + return codecProtocolError("http/1.1 protocol error: extraneous data after response complete"); + } + return status; +} + Http::Status ConnectionImpl::innerDispatch(Buffer::Instance& data) { ENVOY_CONN_LOG(trace, "parsing {} bytes", connection_, data.length()); // Make sure that dispatching_ is set to false after dispatching, even when @@ -1285,6 +1295,9 @@ void ClientConnectionImpl::onMessageComplete() { pending_response_.reset(); headers_or_trailers_.emplace(nullptr); } + + // Pause the parser after a response is complete. Any remaining data indicates an error. + http_parser_pause(&parser_, 1); } void ClientConnectionImpl::onResetStream(StreamResetReason reason) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 867cd603f57a..f63b777c6a44 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -580,6 +580,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { bool cannotHaveBody(); // ConnectionImpl + Http::Status dispatch(Buffer::Instance& data) override; void onEncodeComplete() override {} Status onMessageBegin() override { return okStatus(); } Status onUrl(const char*, size_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index 474fd7ce1b0c..d82a6ae15b2f 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -526,6 +526,16 @@ Http::Status ConnectionImpl::dispatch(Buffer::Instance& data) { [&](Buffer::Instance& data) -> Http::Status { return innerDispatch(data); }, data); } +Http::Status ClientConnectionImpl::dispatch(Buffer::Instance& data) { + Http::Status status = ConnectionImpl::dispatch(data); + if (status.ok() && data.length() > 0) { + // The HTTP/1.1 codec pauses dispatch after a single response is complete. Extraneous data + // after a response is complete indicates an error. + return codecProtocolError("http/1.1 protocol error: extraneous data after response complete"); + } + return status; +} + Http::Status ConnectionImpl::innerDispatch(Buffer::Instance& data) { ENVOY_CONN_LOG(trace, "parsing {} bytes", connection_, data.length()); ASSERT(buffered_body_.length() == 0); diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 48382473f0bd..6510116d7a36 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -555,6 +555,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { bool cannotHaveBody(); // ConnectionImpl + Http::Status dispatch(Buffer::Instance& data) override; void onEncodeComplete() override {} void onMessageBegin() override {} void onUrl(const char*, size_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } diff --git a/test/common/http/codec_client_test.cc b/test/common/http/codec_client_test.cc index 48a1dad0da8e..25ba456a87fe 100644 --- a/test/common/http/codec_client_test.cc +++ b/test/common/http/codec_client_test.cc @@ -374,6 +374,7 @@ TEST_P(CodecNetworkTest, SendData) { upstream_connection_->write(data, false); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { EXPECT_EQ(full_data, data.toString()); + data.drain(data.length()); dispatcher_->exit(); return Http::okStatus(); })); @@ -397,10 +398,12 @@ TEST_P(CodecNetworkTest, SendHeadersAndClose) { .Times(2) .WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { EXPECT_EQ(full_data, data.toString()); + data.drain(data.length()); return Http::okStatus(); })) .WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { EXPECT_EQ("", data.toString()); + data.drain(data.length()); return Http::okStatus(); })); // Because the headers are not complete, the disconnect will reset the stream. @@ -435,6 +438,7 @@ TEST_P(CodecNetworkTest, SendHeadersAndCloseUnderReadDisable) { .Times(2) .WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { EXPECT_EQ(full_data, data.toString()); + data.drain(data.length()); return Http::okStatus(); })) .WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { diff --git a/test/integration/h1_corpus/alloc_headers b/test/integration/h1_corpus/alloc_headers new file mode 100644 index 000000000000..3e8a5e62f1a8 --- /dev/null +++ b/test/integration/h1_corpus/alloc_headers @@ -0,0 +1,12 @@ +events { + downstream_send_bytes: "POST /test/long/urlaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaattttttaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa448aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa HTTP/1.1\r\nhost: host\r\nx-lyft-user-id: 0\r\nx-forwaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaarded-for: -1113144117.0.0.1\r\ntransfer-encoding: chunked\r\n\r\n400\r\naaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaiaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa@aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234\234aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\344aaaaaaaaaaaaaaaaaaaaaa\r\n0\r\n\r\n" +} +events { + upstream_send_bytes: "HTTP/1.1 454 \002\002\002\002\002\002\002\002\002\002\002OK\r\ntransfer-encoding: chunked\r\n\r\n200\r\naaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa_aaaaaaaaaaaaaaaaFaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\r\n0\r\n\r\n" +} +events { + upstream_send_bytes: "HTTP/1.1 654 \002\002\002\002\002\002\002\002\002\002\002OK\r\ntransfer-encoding: chunked\r\n\r\n200\r\naaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa_aaaaaaaaaaaaaaaaFaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\r\n0\r\n\r\n" +} +events { + upstream_send_bytes: "HTTP/1.1 454 \002\002\002\002\002\002\002\002\002\002\002OK\r\ntransfer-encoding: chunked\r\n\r\n200\r\naaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa_aaaaaaaaaaaaaaaaFaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\r\n0\r\n\r\n" +} diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index f76ad6f889e2..944ccfe81343 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -483,6 +483,38 @@ TEST_P(IntegrationTest, TestSmuggling) { } } +TEST_P(IntegrationTest, TestPipelinedResponses) { + initialize(); + auto tcp_client = makeTcpConnection(lookupPort("http")); + + ASSERT_TRUE(tcp_client->write( + "POST /test/long/url HTTP/1.1\r\nHost: host\r\ntransfer-encoding: chunked\r\n\r\n")); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string data; + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); + ASSERT_THAT(data, HasSubstr("POST")); + + ASSERT_TRUE(fake_upstream_connection->write( + "HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\n0\r\n\r\n" + "HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\n0\r\n\r\n" + "HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\n0\r\n\r\n")); + + tcp_client->waitForData("0\r\n\r\n", false); + std::string response = tcp_client->data(); + + EXPECT_THAT(response, HasSubstr("HTTP/1.1 200 OK\r\n")); + EXPECT_THAT(response, HasSubstr("transfer-encoding: chunked\r\n")); + EXPECT_THAT(response, EndsWith("0\r\n\r\n")); + + ASSERT_TRUE(fake_upstream_connection->close()); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + tcp_client->close(); + EXPECT_EQ(test_server_->counter("cluster.cluster_0.upstream_cx_protocol_error")->value(), 1); +} + TEST_P(IntegrationTest, TestServerAllowChunkedLength) { config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&