fix sometimes returns response body to HEAD requests

source/common/http/async_client_impl.cc

@@ -112,6 +112,10 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) {
 }
 
 void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) {
+  if (Http::Headers::get().MethodValues.Head == headers.Method()->value().c_str()) {
+    is_head_request_ = true;
+  }
+
   is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers);
   headers.insertEnvoyInternalRequest().value().setReference(
       Headers::get().EnvoyInternalRequestValues.True);

source/common/http/async_client_impl.h

@@ -282,7 +282,7 @@ class AsyncStreamImpl : public AsyncClient::Stream,
           encodeHeaders(std::move(headers), end_stream);
         },
         [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); },
-        remote_closed_, code, body);
+        remote_closed_, code, body, is_head_request_);
   }
   // The async client won't pause if sending an Expect: 100-Continue so simply
   // swallows any incoming encode100Continue.
@@ -308,6 +308,7 @@ class AsyncStreamImpl : public AsyncClient::Stream,
   bool remote_closed_{};
   Buffer::InstancePtr buffered_body_;
   bool is_grpc_request_{};
+  bool is_head_request_{false};
   friend class AsyncClientImpl;
 };
 

source/common/http/conn_manager_impl.cc

@@ -425,7 +425,7 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() {
   } else {
     sendLocalReply(request_headers_ != nullptr &&
                        Grpc::Common::hasGrpcContentType(*request_headers_),
-                   Http::Code::RequestTimeout, "stream timeout", nullptr);
+                   Http::Code::RequestTimeout, "stream timeout", nullptr, is_head_request_);
   }
 }
 
@@ -482,6 +482,10 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() {
 
 void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) {
   request_headers_ = std::move(headers);
+  if (Http::Headers::get().MethodValues.Head == request_headers_->Method()->value().c_str()) {
+    is_head_request_ = true;
+  }
+
   const bool upgrade_rejected = createFilterChain() == false;
 
   maybeEndDecode(end_stream);
@@ -513,7 +517,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
     request_info_.protocol(protocol);
     if (!connection_manager_.config_.http1Settings().accept_http_10_) {
       // Send "Upgrade Required" if HTTP/1.0 support is not explictly configured on.
-      sendLocalReply(false, Code::UpgradeRequired, "", nullptr);
+      sendLocalReply(false, Code::UpgradeRequired, "", nullptr, is_head_request_);
       return;
     } else {
       // HTTP/1.0 defaults to single-use connections. Make sure the connection
@@ -537,7 +541,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
     } else {
       // Require host header. For HTTP/1.1 Host has already been translated to :authority.
       sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "",
-                     nullptr);
+                     nullptr, is_head_request_);
       return;
     }
   }
@@ -551,7 +555,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
   // envoy users who do not wish to proxy large headers.
   if (request_headers_->byteSize() > (60 * 1024)) {
     sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_),
-                   Code::RequestHeaderFieldsTooLarge, "", nullptr);
+                   Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_);
     return;
   }
 
@@ -562,8 +566,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
   // don't support that currently.
   if (!request_headers_->Path() || request_headers_->Path()->value().c_str()[0] != '/') {
     connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc();
-    sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "",
-                   nullptr);
+    sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr,
+                   is_head_request_);
     return;
   }
 
@@ -608,7 +612,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
       // Do not allow WebSocket upgrades if the route does not support it.
       connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc();
       sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "",
-                     nullptr);
+                     nullptr, is_head_request_);
       return;
     }
     // Allow non websocket requests to go through websocket enabled routes.
@@ -924,7 +928,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() {
 
 void ConnectionManagerImpl::ActiveStream::sendLocalReply(
     bool is_grpc_request, Code code, const std::string& body,
-    std::function<void(HeaderMap& headers)> modify_headers) {
+    std::function<void(HeaderMap& headers)> modify_headers, bool is_head_request) {
   Utility::sendLocalReply(is_grpc_request,
                           [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
                             if (modify_headers != nullptr) {
@@ -940,7 +944,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply(
                             // request instead.
                             encodeData(nullptr, data, end_stream);
                           },
-                          state_.destroyed_, code, body);
+                          state_.destroyed_, code, body, is_head_request);
 }
 
 void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders(
@@ -1586,19 +1590,19 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() {
       parent_.state_.encoder_filters_streaming_ = true;
       stopped_ = false;
 
-      Http::Utility::sendLocalReply(Grpc::Common::hasGrpcContentType(*parent_.request_headers_),
-                                    [&](HeaderMapPtr&& response_headers, bool end_stream) -> void {
-                                      parent_.response_headers_ = std::move(response_headers);
-                                      parent_.response_encoder_->encodeHeaders(
-                                          *parent_.response_headers_, end_stream);
-                                      parent_.state_.local_complete_ = end_stream;
-                                    },
-                                    [&](Buffer::Instance& data, bool end_stream) -> void {
-                                      parent_.response_encoder_->encodeData(data, end_stream);
-                                      parent_.state_.local_complete_ = end_stream;
-                                    },
-                                    parent_.state_.destroyed_, Http::Code::InternalServerError,
-                                    CodeUtility::toString(Http::Code::InternalServerError));
+      Http::Utility::sendLocalReply(
+          Grpc::Common::hasGrpcContentType(*parent_.request_headers_),
+          [&](HeaderMapPtr&& response_headers, bool end_stream) -> void {
+            parent_.response_headers_ = std::move(response_headers);
+            parent_.response_encoder_->encodeHeaders(*parent_.response_headers_, end_stream);
+            parent_.state_.local_complete_ = end_stream;
+          },
+          [&](Buffer::Instance& data, bool end_stream) -> void {
+            parent_.response_encoder_->encodeData(data, end_stream);
+            parent_.state_.local_complete_ = end_stream;
+          },
+          parent_.state_.destroyed_, Http::Code::InternalServerError,
+          CodeUtility::toString(Http::Code::InternalServerError), parent_.is_head_request_);
       parent_.maybeEndEncode(parent_.state_.local_complete_);
     } else {
       resetStream();

source/common/http/conn_manager_impl.h

@@ -170,7 +170,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     }
     void sendLocalReply(Code code, const std::string& body,
                         std::function<void(HeaderMap& headers)> modify_headers) override {
-      parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers);
+      parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers,
+                             parent_.is_head_request_);
     }
     void encode100ContinueHeaders(HeaderMapPtr&& headers) override;
     void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override;
@@ -278,7 +279,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming);
     HeaderMap& addEncodedTrailers();
     void sendLocalReply(bool is_grpc_request, Code code, const std::string& body,
-                        std::function<void(HeaderMap& headers)> modify_headers);
+                        std::function<void(HeaderMap& headers)> modify_headers,
+                        bool is_head_request);
     void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers);
     void encodeHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers, bool end_stream);
     void encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instance& data, bool end_stream);
@@ -404,6 +406,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     // By default, we will assume there are no 100-Continue headers. If encode100ContinueHeaders
     // is ever called, this is set to true so commonContinue resumes processing the 100-Continue.
     bool has_continue_headers_{};
+    bool is_head_request_{false};
   };
 
   typedef std::unique_ptr<ActiveStream> ActiveStreamPtr;

source/common/http/http1/codec_impl.cc

@@ -244,11 +244,10 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_
   if (!method || !path) {
     throw CodecClientException(":method and :path must be specified");
   }
-
   if (method->value() == Headers::get().MethodValues.Head.c_str()) {
     head_request_ = true;
   }
-
+  connection_.onEncodeHeaders(headers);
   connection_.reserveBuffer(std::max(4096U, path->value().size() + 4096));
   connection_.copyToBuffer(method->value().c_str(), method->value().size());
   connection_.addCharToBuffer(' ');
@@ -685,9 +684,10 @@ StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& response_decoder)
   return *request_encoder_;
 }
 
-void ClientConnectionImpl::onEncodeComplete() {
-  // Transfer head request state into the pending response before we reuse the encoder.
-  pending_responses_.back().head_request_ = request_encoder_->headRequest();
+void ClientConnectionImpl::onEncodeHeaders(const HeaderMap& headers) {
+  if (headers.Method()->value() == Headers::get().MethodValues.Head.c_str()) {
+    pending_responses_.back().head_request_ = true;
+  }
 }
 
 int ClientConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) {

source/common/http/http1/codec_impl.h

@@ -98,7 +98,6 @@ class ResponseStreamEncoderImpl : public StreamEncoderImpl {
 class RequestStreamEncoderImpl : public StreamEncoderImpl {
 public:
   RequestStreamEncoderImpl(ConnectionImpl& connection) : StreamEncoderImpl(connection) {}
-
   bool headRequest() { return head_request_; }
 
   // Http::StreamEncoder
@@ -123,6 +122,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
    */
   virtual void onEncodeComplete() PURE;
 
+  /**
+   * Called when headers are encoded.
+   */
+  virtual void onEncodeHeaders(const HeaderMap& headers) PURE;
+
   /**
    * Called when resetStream() has been called on an active stream. In HTTP/1.1 the only
    * valid operation after this point is for the connection to get blown away, but we will not
@@ -303,6 +307,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
 
   // ConnectionImpl
   void onEncodeComplete() override;
+  void onEncodeHeaders(const HeaderMap&) override {}
   void onMessageBegin() override;
   void onUrl(const char* data, size_t length) override;
   int onHeadersComplete(HeaderMapImplPtr&& headers) override;
@@ -339,7 +344,8 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
   bool cannotHaveBody();
 
   // ConnectionImpl
-  void onEncodeComplete() override;
+  void onEncodeComplete() override {}
+  void onEncodeHeaders(const HeaderMap& headers) override;
   void onMessageBegin() override {}
   void onUrl(const char*, size_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
   int onHeadersComplete(HeaderMapImplPtr&& headers) override;

source/common/http/utility.cc

@@ -240,22 +240,22 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co
 }
 
 void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks,
-                             const bool& is_reset, Code response_code,
-                             const std::string& body_text) {
+                             const bool& is_reset, Code response_code, const std::string& body_text,
+                             bool is_head_request) {
   sendLocalReply(is_grpc,
                  [&](HeaderMapPtr&& headers, bool end_stream) -> void {
                    callbacks.encodeHeaders(std::move(headers), end_stream);
                  },
                  [&](Buffer::Instance& data, bool end_stream) -> void {
                    callbacks.encodeData(data, end_stream);
                  },
-                 is_reset, response_code, body_text);
+                 is_reset, response_code, body_text, is_head_request);
 }
 
 void Utility::sendLocalReply(
     bool is_grpc, std::function<void(HeaderMapPtr&& headers, bool end_stream)> encode_headers,
     std::function<void(Buffer::Instance& data, bool end_stream)> encode_data, const bool& is_reset,
-    Code response_code, const std::string& body_text) {
+    Code response_code, const std::string& body_text, bool is_head_request) {
   // encode_headers() may reset the stream, so the stream must not be reset before calling it.
   ASSERT(!is_reset);
   // Respond with a gRPC trailers-only response if the request is gRPC
@@ -265,7 +265,7 @@ void Utility::sendLocalReply(
         {Headers::get().ContentType, Headers::get().ContentTypeValues.Grpc},
         {Headers::get().GrpcStatus,
          std::to_string(enumToInt(Grpc::Utility::httpToGrpcStatus(enumToInt(response_code))))}}};
-    if (!body_text.empty()) {
+    if (!body_text.empty() && !is_head_request) {
       // TODO: GrpcMessage should be percent-encoded
       response_headers->insertGrpcMessage().value(body_text);
     }
@@ -279,6 +279,12 @@ void Utility::sendLocalReply(
     response_headers->insertContentLength().value(body_text.size());
     response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text);
   }
+
+  if (is_head_request) {
+    encode_headers(std::move(response_headers), true);
+    return;
+  }
+
   encode_headers(std::move(response_headers), body_text.empty());
   // encode_headers()) may have changed the referenced is_reset so we need to test it
   if (!body_text.empty() && !is_reset) {

source/common/http/utility.h

@@ -131,9 +131,10 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption
  * @param response_code supplies the HTTP response code.
  * @param body_text supplies the optional body text which is sent using the text/plain content
  *                  type.
+ * @param is_head_request tells if this is a response to a HEAD request
  */
 void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset,
-                    Code response_code, const std::string& body_text);
+                    Code response_code, const std::string& body_text, bool is_head_request);
 
 /**
  * Create a locally generated response using the provided lambdas.
@@ -150,7 +151,8 @@ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const
 void sendLocalReply(bool is_grpc,
                     std::function<void(HeaderMapPtr&& headers, bool end_stream)> encode_headers,
                     std::function<void(Buffer::Instance& data, bool end_stream)> encode_data,
-                    const bool& is_reset, Code response_code, const std::string& body_text);
+                    const bool& is_reset, Code response_code, const std::string& body_text,
+                    bool is_head_request = false);
 
 struct GetLastAddressFromXffInfo {
   // Last valid address pulled from the XFF header.

source/extensions/filters/http/jwt_authn/filter.cc

@@ -21,7 +21,6 @@ void Filter::onDestroy() {
 
 Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool) {
   ENVOY_LOG(debug, "Called Filter : {}", __func__);
-
   // Remove headers configured to pass payload
   auth_->sanitizePayloadHeaders(headers);
 
@@ -51,8 +50,8 @@ void Filter::onComplete(const Status& status) {
     // verification failed
     Http::Code code = Http::Code::Unauthorized;
     // return failure reason as message body
-    Http::Utility::sendLocalReply(false, *decoder_callbacks_, false, code,
-                                  ::google::jwt_verify::getStatusString(status));
+    decoder_callbacks_->sendLocalReply(code, ::google::jwt_verify::getStatusString(status),
+                                       nullptr);
     return;
   }
 

test/common/http/async_client_impl_test.cc

@@ -741,6 +741,31 @@ TEST_F(AsyncClientImplTest, StreamTimeout) {
       cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("upstream_rq_504").value());
 }
 
+TEST_F(AsyncClientImplTest, StreamTimeoutHeadReply) {
+  EXPECT_CALL(cm_.conn_pool_, newStream(_, _))
+      .WillOnce(Invoke([&](StreamDecoder&,
+                           ConnectionPool::Callbacks& callbacks) -> ConnectionPool::Cancellable* {
+        callbacks.onPoolReady(stream_encoder_, cm_.conn_pool_.host_);
+        return nullptr;
+      }));
+
+  MessagePtr message{new RequestMessageImpl()};
+  HttpTestUtility::addDefaultHeaders(message->headers(), "HEAD");
+  EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(&message->headers()), true));
+  timer_ = new NiceMock<Event::MockTimer>(&dispatcher_);
+  EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(40)));
+  EXPECT_CALL(stream_encoder_.stream_, resetStream(_));
+
+  TestHeaderMapImpl expected_timeout{
+      {":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}};
+  EXPECT_CALL(stream_callbacks_, onHeaders_(HeaderMapEqualRef(&expected_timeout), true));
+
+  AsyncClient::Stream* stream =
+      client_.start(stream_callbacks_, std::chrono::milliseconds(40), false);
+  stream->sendHeaders(message->headers(), true);
+  timer_->callback_();
+}
+
 TEST_F(AsyncClientImplTest, RequestTimeout) {
   EXPECT_CALL(cm_.conn_pool_, newStream(_, _))
       .WillOnce(Invoke([&](StreamDecoder&,

test/common/http/common.cc

@@ -5,9 +5,10 @@
 #include "envoy/http/header_map.h"
 
 namespace Envoy {
-void HttpTestUtility::addDefaultHeaders(Http::HeaderMap& headers) {
+void HttpTestUtility::addDefaultHeaders(Http::HeaderMap& headers,
+                                        const std::string default_method) {
   headers.insertScheme().value(std::string("http"));
-  headers.insertMethod().value(std::string("GET"));
+  headers.insertMethod().value(default_method);
   headers.insertHost().value(std::string("host"));
   headers.insertPath().value(std::string("/"));
 }

test/common/http/common.h

@@ -63,6 +63,6 @@ struct ConnPoolCallbacks : public Http::ConnectionPool::Callbacks {
  */
 class HttpTestUtility {
 public:
-  static void addDefaultHeaders(Http::HeaderMap& headers);
+  static void addDefaultHeaders(Http::HeaderMap& headers, const std::string default_method = "GET");
 };
 } // namespace Envoy

test/common/http/conn_manager_impl_corpus/codec_exception

@@ -1,6 +1,10 @@
 actions {
   new_stream {
     request_headers {
+      headers {
+        key: ":method"
+        value: "GET"
+      }
       headers {
         key: "foo"
         value: "bar"