diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 862cb47cd223f..4e24332d7b712 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -4,6 +4,9 @@ Version history 1.15.0 (Pending) ================ +* http: fixed a bug where the upgrade header was not cleared on responses to non-upgrade requests. + Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false. + 1.14.1 (April 8, 2020) ====================== * request_id_extension: fixed static initialization for noop request id extension. diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 615bcf06baa5f..2526ac56b1149 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -378,7 +378,11 @@ void ConnectionManagerUtility::mutateResponseHeaders( } } else { response_headers.removeConnection(); + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.fix_upgrade_response")) { + response_headers.removeUpgrade(); + } } + response_headers.removeTransferEncoding(); if (request_headers != nullptr && request_headers->EnvoyForceTrace()) { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 05b3b0657a564..de1258a57ad31 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -357,6 +357,9 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end if (method->value() == Headers::get().MethodValues.Head) { head_request_ = true; } + if (Utility::isUpgrade(headers)) { + upgrade_request_ = true; + } connection_.copyToBuffer(method->value().getStringView().data(), method->value().size()); connection_.addCharToBuffer(' '); connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); @@ -579,7 +582,7 @@ int ConnectionImpl::onHeadersCompleteBase() { protocol_ = Protocol::Http10; } RequestOrResponseHeaderMap& request_or_response_headers = requestOrResponseHeaders(); - if (Utility::isUpgrade(request_or_response_headers)) { + if (Utility::isUpgrade(request_or_response_headers) && upgradeAllowed()) { // Ignore h2c upgrade requests until we support them. // See https://github.com/envoyproxy/envoy/issues/7161 for details. if (request_or_response_headers.Upgrade() && @@ -1008,6 +1011,13 @@ int ClientConnectionImpl::onHeadersComplete() { return cannotHaveBody() ? 1 : 0; } +bool ClientConnectionImpl::upgradeAllowed() const { + if (pending_response_.has_value()) { + return pending_response_->encoder_.upgrade_request_; + } + return false; +} + void ClientConnectionImpl::onBody(Buffer::Instance& data) { ASSERT(!deferred_end_stream_headers_); if (pending_response_.has_value()) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index cdc651c197664..ddb7a746015c5 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -150,6 +150,8 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder { void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override; void encodeTrailers(const RequestTrailerMap& trailers) override { encodeTrailersBase(trailers); } + bool upgrade_request_{}; + private: bool head_request_{}; }; @@ -314,6 +316,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::LoggablemergeValues( + {{"envoy.reloadable_features.fix_upgrade_response", "false"}}); + // Test with the request headers not valid upgrade headers { TestRequestHeaderMapImpl request_headers{{"upgrade", "foo"}}; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 0c26f621f9e8c..dd19b620354bb 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1815,6 +1815,15 @@ TEST_F(Http1ClientConnectionImplTest, GiantPath) { codec_->dispatch(response); } +TEST_F(Http1ClientConnectionImplTest, PrematureUpgradeResponse) { + initialize(); + + // make sure upgradeAllowed doesn't cause crashes if run with no pending response. + Buffer::OwnedImpl response( + "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: upgrade\r\nUpgrade: websocket\r\n\r\n"); + EXPECT_THROW(codec_->dispatch(response), PrematureResponseException); +} + TEST_F(Http1ClientConnectionImplTest, UpgradeResponse) { initialize(); @@ -1822,7 +1831,11 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponse) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); - TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + TestRequestHeaderMapImpl headers{{":method", "GET"}, + {":path", "/"}, + {":authority", "host"}, + {"connection", "upgrade"}, + {"upgrade", "websocket"}}; request_encoder.encodeHeaders(headers, true); // Send upgrade headers @@ -1853,7 +1866,11 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponseWithEarlyData) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); - TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + TestRequestHeaderMapImpl headers{{":method", "GET"}, + {":path", "/"}, + {":authority", "host"}, + {"connection", "upgrade"}, + {"upgrade", "websocket"}}; request_encoder.encodeHeaders(headers, true); // Send upgrade headers diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index c18599476f5f5..ec5125a1cd95e 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1254,4 +1254,30 @@ TEST_P(IntegrationTest, TestManyBadRequests) { EXPECT_EQ(0, test_server_->counter("http1.response_flood")->value()); } +// Regression test for https://github.com/envoyproxy/envoy/issues/10566 +TEST_P(IntegrationTest, TestUpgradeHeaderInResponse) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT(fake_upstream_connection != nullptr); + ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 200 OK\r\n" + "connection: upgrade\r\n" + "upgrade: h2\r\n" + "Transfer-encoding: chunked\r\n\r\n" + "b\r\nHello World\r\n0\r\n\r\n", + false)); + + response->waitForHeaders(); + EXPECT_EQ(nullptr, response->headers().Upgrade()); + EXPECT_EQ(nullptr, response->headers().Connection()); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("Hello World", response->body()); + cleanupUpstreamAndDownstream(); +} + } // namespace Envoy