http: changing mutate[request|response] to special case all upgrade headers

include/envoy/http/header_map.h

@@ -106,31 +106,6 @@ class HeaderString {
    */
   bool find(const char* str) const { return strstr(c_str(), str); }
 
-  /**
-   * HeaderString is in token list form, each token separated by commas or whitespace,
-   * see https://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.1 for more information,
-   * header field value's case sensitivity depends on each header.
-   * @return whether contains token in case insensitive manner.
-   */
-  bool caseInsensitiveContains(const char* token) const {
-    // Avoid dead loop if token argument is empty.
-    const int n = strlen(token);
-    if (n == 0) {
-      return false;
-    }
-
-    // Find token substring, skip if it's partial of other token.
-    const char* tokens = c_str();
-    for (const char* p = tokens; (p = strcasestr(p, token)); p += n) {
-      if ((p == tokens || *(p - 1) == ' ' || *(p - 1) == ',') &&
-          (*(p + n) == '\0' || *(p + n) == ' ' || *(p + n) == ',')) {
-        return true;
-      }
-    }
-
-    return false;
-  }
-
   /**
    * Set the value of the string by copying data into it. This overwrites any existing string.
    */

source/common/http/conn_manager_utility.cc

@@ -23,9 +23,9 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
     ConnectionManagerConfig& config, const Router::Config& route_config,
     Runtime::RandomGenerator& random, Runtime::Loader& runtime,
     const LocalInfo::LocalInfo& local_info) {
-  // If this is a WebSocket Upgrade request, do not remove the Connection and Upgrade headers,
+  // If this is a Upgrade request, do not remove the Connection and Upgrade headers,
   // as we forward them verbatim to the upstream hosts.
-  if (protocol == Protocol::Http11 && Utility::isWebSocketUpgradeRequest(request_headers)) {
+  if (protocol == Protocol::Http11 && Utility::isUpgrade(request_headers)) {
     // The current WebSocket implementation re-uses the HTTP1 codec to send upgrade headers to
     // the upstream host. This adds the "transfer-encoding: chunked" request header if the stream
     // has not ended and content-length does not exist. In HTTP1.1, if transfer-encoding and
@@ -45,6 +45,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
   request_headers.removeEnvoyInternalRequest();
   request_headers.removeKeepAlive();
   request_headers.removeProxyConnection();
+  // TODO(alyssawilk) handle this with current and new websocket here and below.
   request_headers.removeTransferEncoding();
 
   // If we are "using remote address" this means that we create/append to XFF with our immediate
@@ -303,7 +304,19 @@ void ConnectionManagerUtility::mutateXfccRequestHeader(Http::HeaderMap& request_
 void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_headers,
                                                      const Http::HeaderMap& request_headers,
                                                      const std::string& via) {
-  response_headers.removeConnection();
+  if (Utility::isUpgrade(request_headers) && Utility::isUpgrade(response_headers)) {
+    // As in mutateRequestHeaders, Upgrade responses have special handling.
+    //
+    // Unlike mutateRequestHeaders there is no explicit protocol check. If Envoy is proxying an
+    // upgrade response it has already passed the protocol checks.
+    const bool no_body =
+        (!response_headers.TransferEncoding() && !response_headers.ContentLength());
+    if (no_body) {
+      response_headers.insertContentLength().value(uint64_t(0));
+    }
+  } else {
+    response_headers.removeConnection();
+  }
   response_headers.removeTransferEncoding();
 
   if (request_headers.EnvoyForceTrace() && request_headers.RequestId()) {

source/common/http/utility.cc

@@ -201,15 +201,18 @@ uint64_t Utility::getResponseStatus(const HeaderMap& headers) {
   return response_code;
 }
 
-bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) {
+bool Utility::isUpgrade(const HeaderMap& headers) {
   // In firefox the "Connection" request header value is "keep-alive, Upgrade",
   // we should check if it contains the "Upgrade" token.
   return (headers.Connection() && headers.Upgrade() &&
-          headers.Connection()->value().caseInsensitiveContains(
-              Http::Headers::get().ConnectionValues.Upgrade.c_str()) &&
-          (0 == StringUtil::caseInsensitiveCompare(
-                    headers.Upgrade()->value().c_str(),
-                    Http::Headers::get().UpgradeValues.WebSocket.c_str())));
+          Envoy::StringUtil::caseFindToken(headers.Connection()->value().getStringView(), ",",
+                                           Http::Headers::get().ConnectionValues.Upgrade.c_str()));
+}
+
+bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) {
+  return (isUpgrade(headers) && (0 == StringUtil::caseInsensitiveCompare(
+                                          headers.Upgrade()->value().c_str(),
+                                          Http::Headers::get().UpgradeValues.WebSocket.c_str())));
 }
 
 Http2Settings

source/common/http/utility.h

@@ -93,6 +93,14 @@ std::string makeSetCookieValue(const std::string& key, const std::string& value,
  */
 uint64_t getResponseStatus(const HeaderMap& headers);
 
+/**
+ * Determine whether these headers are a valid Upgrade request or response.
+ * This function returns true if the following HTTP headers and values are present:
+ * - Connection: Upgrade
+ * - Upgrade: [any value]
+ */
+bool isUpgrade(const HeaderMap& headers);
+
 /**
  * Determine whether this is a WebSocket Upgrade request.
  * This function returns true if the following HTTP headers and values are present:

test/common/http/conn_manager_utility_test.cc

@@ -548,17 +548,74 @@ TEST_F(ConnectionManagerUtilityTest, RemoveConnectionUpgradeForHttp2Requests) {
 // Test cleaning response headers.
 TEST_F(ConnectionManagerUtilityTest, MutateResponseHeaders) {
   TestHeaderMapImpl response_headers{
-      {"connection", "foo"}, {"transfer-encoding", "foo"}, {"custom_header", "foo"}};
+      {"connection", "foo"}, {"transfer-encoding", "foo"}, {"custom_header", "custom_value"}};
   TestHeaderMapImpl request_headers{{"x-request-id", "request-id"}};
 
   ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, "");
 
   EXPECT_EQ(1UL, response_headers.size());
-  EXPECT_EQ("foo", response_headers.get_("custom_header"));
+  EXPECT_EQ("custom_value", response_headers.get_("custom_header"));
   EXPECT_FALSE(response_headers.has("x-request-id"));
   EXPECT_FALSE(response_headers.has(Headers::get().Via));
 }
 
+// Make sure we don't remove connection headers on all Upgrade responses.
+TEST_F(ConnectionManagerUtilityTest, DoNotRemoveConnectionUpgradeForWebSocketResponses) {
+  TestHeaderMapImpl request_headers{{"connection", "UpGrAdE"}, {"upgrade", "foo"}};
+  TestHeaderMapImpl response_headers{
+      {"connection", "upgrade"}, {"transfer-encoding", "foo"}, {"upgrade", "bar"}};
+  EXPECT_TRUE(Utility::isUpgrade(request_headers));
+  EXPECT_TRUE(Utility::isUpgrade(response_headers));
+  ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, "");
+
+  EXPECT_EQ(2UL, response_headers.size()) << response_headers;
+  EXPECT_EQ("upgrade", response_headers.get_("connection"));
+  EXPECT_EQ("bar", response_headers.get_("upgrade"));
+}
+
+TEST_F(ConnectionManagerUtilityTest, ClearUpgradeHeadersForNonUpgradeRequests) {
+  // Test clearing non-upgrade request and response headers
+  {
+    TestHeaderMapImpl request_headers{{"x-request-id", "request-id"}};
+    TestHeaderMapImpl response_headers{
+        {"connection", "foo"}, {"transfer-encoding", "bar"}, {"custom_header", "custom_value"}};
+    EXPECT_FALSE(Utility::isUpgrade(request_headers));
+    EXPECT_FALSE(Utility::isUpgrade(response_headers));
+    ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, "");
+
+    EXPECT_EQ(1UL, response_headers.size()) << response_headers;
+    EXPECT_EQ("custom_value", response_headers.get_("custom_header"));
+  }
+
+  // Test with the request headers not valid upgrade headers
+  {
+    TestHeaderMapImpl request_headers{{"upgrade", "foo"}};
+    TestHeaderMapImpl response_headers{{"connection", "upgrade"},
+                                       {"transfer-encoding", "eep"},
+                                       {"upgrade", "foo"},
+                                       {"custom_header", "custom_value"}};
+    EXPECT_FALSE(Utility::isUpgrade(request_headers));
+    EXPECT_TRUE(Utility::isUpgrade(response_headers));
+    ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, "");
+
+    EXPECT_EQ(2UL, response_headers.size()) << response_headers;
+    EXPECT_EQ("custom_value", response_headers.get_("custom_header"));
+    EXPECT_EQ("foo", response_headers.get_("upgrade"));
+  }
+
+  // Test with the response headers not valid upgrade headers
+  {
+    TestHeaderMapImpl request_headers{{"connection", "UpGrAdE"}, {"upgrade", "foo"}};
+    TestHeaderMapImpl response_headers{{"transfer-encoding", "foo"}, {"upgrade", "bar"}};
+    EXPECT_TRUE(Utility::isUpgrade(request_headers));
+    EXPECT_FALSE(Utility::isUpgrade(response_headers));
+    ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, "");
+
+    EXPECT_EQ(1UL, response_headers.size()) << response_headers;
+    EXPECT_EQ("bar", response_headers.get_("upgrade"));
+  }
+}
+
 // Test that we correctly return x-request-id if we were requested to force a trace.
 TEST_F(ConnectionManagerUtilityTest, MutateResponseHeadersReturnXRequestId) {
   TestHeaderMapImpl response_headers;

test/common/http/header_map_impl_test.cc

@@ -261,31 +261,6 @@ TEST(HeaderStringTest, All) {
     EXPECT_EQ(HeaderString::Type::Reference, string.type());
   }
 
-  // caseInsensitiveContains
-  {
-    const std::string static_string("keep-alive, Upgrade, close");
-    HeaderString string(static_string);
-    EXPECT_TRUE(string.caseInsensitiveContains("keep-alive"));
-    EXPECT_TRUE(string.caseInsensitiveContains("Keep-alive"));
-    EXPECT_TRUE(string.caseInsensitiveContains("Upgrade"));
-    EXPECT_TRUE(string.caseInsensitiveContains("upgrade"));
-    EXPECT_TRUE(string.caseInsensitiveContains("close"));
-    EXPECT_TRUE(string.caseInsensitiveContains("Close"));
-    EXPECT_FALSE(string.caseInsensitiveContains(""));
-    EXPECT_FALSE(string.caseInsensitiveContains("keep"));
-    EXPECT_FALSE(string.caseInsensitiveContains("alive"));
-    EXPECT_FALSE(string.caseInsensitiveContains("grade"));
-
-    const std::string small("close");
-    string.setCopy(small.c_str(), small.size());
-    EXPECT_FALSE(string.caseInsensitiveContains("keep-alive"));
-
-    const std::string empty("");
-    string.setCopy(empty.c_str(), empty.size());
-    EXPECT_FALSE(string.caseInsensitiveContains("keep-alive"));
-    EXPECT_FALSE(string.caseInsensitiveContains(""));
-  }
-
   // getString
   {
     std::string static_string("HELLO");

test/common/http/utility_test.cc

@@ -46,6 +46,8 @@ TEST(HttpUtility, isWebSocketUpgradeRequest) {
   EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(TestHeaderMapImpl{{"upgrade", "websocket"}}));
   EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(
       TestHeaderMapImpl{{"Connection", "close"}, {"Upgrade", "websocket"}}));
+  EXPECT_FALSE(Utility::isUpgrade(
+      TestHeaderMapImpl{{"Connection", "IsNotAnUpgrade"}, {"Upgrade", "websocket"}}));
 
   EXPECT_TRUE(Utility::isWebSocketUpgradeRequest(
       TestHeaderMapImpl{{"Connection", "upgrade"}, {"Upgrade", "websocket"}}));
@@ -55,6 +57,23 @@ TEST(HttpUtility, isWebSocketUpgradeRequest) {
       TestHeaderMapImpl{{"connection", "Upgrade"}, {"upgrade", "WebSocket"}}));
 }
 
+TEST(HttpUtility, isUpgrade) {
+  EXPECT_FALSE(Utility::isUpgrade(TestHeaderMapImpl{}));
+  EXPECT_FALSE(Utility::isUpgrade(TestHeaderMapImpl{{"connection", "upgrade"}}));
+  EXPECT_FALSE(Utility::isUpgrade(TestHeaderMapImpl{{"upgrade", "foo"}}));
+  EXPECT_FALSE(Utility::isUpgrade(TestHeaderMapImpl{{"Connection", "close"}, {"Upgrade", "foo"}}));
+  EXPECT_FALSE(
+      Utility::isUpgrade(TestHeaderMapImpl{{"Connection", "IsNotAnUpgrade"}, {"Upgrade", "foo"}}));
+  EXPECT_FALSE(Utility::isUpgrade(
+      TestHeaderMapImpl{{"Connection", "Is Not An Upgrade"}, {"Upgrade", "foo"}}));
+
+  EXPECT_TRUE(Utility::isUpgrade(TestHeaderMapImpl{{"Connection", "upgrade"}, {"Upgrade", "foo"}}));
+  EXPECT_TRUE(Utility::isUpgrade(TestHeaderMapImpl{{"connection", "upgrade"}, {"upgrade", "foo"}}));
+  EXPECT_TRUE(Utility::isUpgrade(TestHeaderMapImpl{{"connection", "Upgrade"}, {"upgrade", "FoO"}}));
+  EXPECT_TRUE(Utility::isUpgrade(
+      TestHeaderMapImpl{{"connection", "keep-alive, Upgrade"}, {"upgrade", "FOO"}}));
+}
+
 TEST(HttpUtility, appendXff) {
   {
     TestHeaderMapImpl headers;

test/integration/websocket_integration_test.cc

@@ -71,6 +71,13 @@ void WebsocketIntegrationTest::validateFinalDownstreamData(const std::string& re
   EXPECT_EQ(received_data, expected_data);
 }
 
+void WebsocketIntegrationTest::validateFinalUpstreamData(const std::string& received_data,
+                                                         const std::string& expected_data) {
+  std::regex extra_response_headers("x-request-id:.*\r\n");
+  std::string stripped_data = std::regex_replace(received_data, extra_response_headers, "");
+  EXPECT_EQ(expected_data, stripped_data);
+}
+
 TEST_P(WebsocketIntegrationTest, WebSocketConnectionDownstreamDisconnect) {
   config_helper_.addConfigModifier(setRouteUsingWebsocket(nullptr));
   initialize();
@@ -296,4 +303,64 @@ TEST_P(WebsocketIntegrationTest, WebSocketLogging) {
                                                    ip_port_regex, ip_port_regex, ip_port_regex)));
 }
 
+TEST_P(WebsocketIntegrationTest, BidirectionalChunkedData) {
+  config_helper_.addConfigModifier(setRouteUsingWebsocket(nullptr));
+  initialize();
+  const std::string upgrade_req_str = "GET /websocket/test HTTP/1.1\r\nHost: host\r\nconnection: "
+                                      "keep-alive, Upgrade\r\nupgrade: Websocket\r\n"
+                                      "transfer-encoding: chunked\r\n\r\n"
+                                      "3\r\n123\r\n0\r\n\r\n"
+                                      "SomeWebSocketPayload";
+
+  // Upgrade, send initial data and wait for it to be received.
+  IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http"));
+  tcp_client->write(upgrade_req_str);
+  FakeRawConnectionPtr fake_upstream_connection = fake_upstreams_[0]->waitForRawConnection();
+  // TODO(alyssawilk) We should be able to wait for SomeWebSocketPayload but it
+  // is not flushed immediately.
+  const std::string data =
+      fake_upstream_connection->waitForData(FakeRawConnection::waitForInexactMatch("\r\n\r\n"));
+
+  // Finish the upgrade.
+  const std::string upgrade_resp_str =
+      "HTTP/1.1 101 Switching Protocols\r\nconnection: Upgrade\r\nupgrade: Websocket\r\n"
+      "transfer-encoding: chunked\r\n\r\n"
+      "4\r\nabcd\r\n0\r\n"
+      "SomeWebsocketResponsePayload";
+  fake_upstream_connection->write(upgrade_resp_str);
+  tcp_client->waitForData("Payload", false);
+
+  // Verify bidirectional data still works.
+  tcp_client->write("FinalClientPayload");
+  std::string final_data = fake_upstream_connection->waitForData(
+      FakeRawConnection::waitForInexactMatch("FinalClientPayload"));
+  fake_upstream_connection->write("FinalServerPayload");
+  tcp_client->waitForData("FinalServerPayload", false);
+
+  // Clean up.
+  tcp_client->close();
+  fake_upstream_connection->waitForDisconnect();
+
+  // TODO(alyssawilk) the current stack is stripping chunked encoding, then
+  // adding back the chunked encoding header without actually chunk encoding.
+  // Data about HTTP vs websocket data boundaries is therefore lost. Fix by
+  // actually chunk encoding.
+  const std::string old_style_modified_payload = "GET /websocket HTTP/1.1\r\n"
+                                                 "host: host\r\n"
+                                                 "connection: keep-alive, Upgrade\r\n"
+                                                 "upgrade: Websocket\r\n"
+                                                 "x-forwarded-proto: http\r\n"
+                                                 "x-envoy-original-path: /websocket/test\r\n"
+                                                 "transfer-encoding: chunked\r\n\r\n"
+                                                 "123SomeWebSocketPayloadFinalClientPayload";
+  validateFinalUpstreamData(final_data, old_style_modified_payload);
+
+  const std::string modified_downstream_payload =
+      "HTTP/1.1 101 Switching Protocols\r\nconnection: Upgrade\r\nupgrade: Websocket\r\n"
+      "transfer-encoding: chunked\r\n\r\n"
+      "4\r\nabcd\r\n0\r\n"
+      "SomeWebsocketResponsePayloadFinalServerPayload";
+  validateFinalDownstreamData(tcp_client->data(), modified_downstream_payload);
+}
+
 } // namespace Envoy

test/integration/websocket_integration_test.h

@@ -17,6 +17,8 @@ class WebsocketIntegrationTest : public HttpIntegrationTest,
   void validateInitialDownstreamData(const std::string& received_data);
   void validateFinalDownstreamData(const std::string& received_data,
                                    const std::string& expected_data);
+  void validateFinalUpstreamData(const std::string& received_data,
+                                 const std::string& expected_data);
 
   const std::string upgrade_req_str_ = "GET /websocket/test HTTP/1.1\r\nHost: host\r\nConnection: "
                                        "keep-alive, Upgrade\r\nUpgrade: websocket\r\n\r\n";