websocket: http request header Connection may contains multiple values

include/envoy/http/header_map.h

@@ -102,6 +102,31 @@ 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

@@ -21,19 +21,30 @@ void ConnectionManagerUtility::mutateRequestHeaders(
     ConnectionManagerConfig& config, const Router::Config& route_config,
     Runtime::RandomGenerator& random, Runtime::Loader& runtime,
     const LocalInfo::LocalInfo& local_info) {
-  // Clean proxy headers.
-  request_headers.removeEnvoyInternalRequest();
-  request_headers.removeKeepAlive();
-  request_headers.removeProxyConnection();
-  request_headers.removeTransferEncoding();
-
   // If this is a WebSocket 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::isWebSocketUpgradeRequest(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
+    // content-length both do not exist this means there is no request body. After transfer-encoding
+    // is stripped here, the upstream request becomes invalid. We can fix it by explicitly adding a
+    // "content-length: 0" request header here.
+    const bool no_body = (!request_headers.TransferEncoding() && !request_headers.ContentLength());
+    if (no_body) {
+      request_headers.insertContentLength().value(uint64_t(0));
+    }
+  } else {
     request_headers.removeConnection();
     request_headers.removeUpgrade();
   }
 
+  // Clean proxy headers.
+  request_headers.removeEnvoyInternalRequest();
+  request_headers.removeKeepAlive();
+  request_headers.removeProxyConnection();
+  request_headers.removeTransferEncoding();
+
   // If we are "using remote address" this means that we create/append to XFF with our immediate
   // peer. Cases where we don't "use remote address" include trusted double proxy where we expect
   // our peer to have already properly set XFF, etc.

source/common/http/utility.cc

@@ -184,10 +184,11 @@ bool Utility::isInternalRequest(const HeaderMap& headers) {
 }
 
 bool Utility::isWebSocketUpgradeRequest(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() &&
-          (0 == StringUtil::caseInsensitiveCompare(
-                    headers.Connection()->value().c_str(),
-                    Http::Headers::get().ConnectionValues.Upgrade.c_str())) &&
+          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())));

test/common/http/header_map_impl_test.cc

@@ -258,6 +258,31 @@ TEST(HeaderStringTest, All) {
     EXPECT_EQ(11U, string.size());
     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(""));
+  }
 }
 
 TEST(HeaderMapImplTest, InlineInsert) {

test/integration/fake_upstream.cc

@@ -364,12 +364,13 @@ FakeRawConnectionPtr FakeUpstream::waitForRawConnection() {
   return connection;
 }
 
-void FakeRawConnection::waitForData(uint64_t num_bytes) {
+std::string FakeRawConnection::waitForData(uint64_t num_bytes) {
   std::unique_lock<std::mutex> lock(lock_);
   while (data_.size() != num_bytes) {
     ENVOY_LOG(debug, "waiting for {} bytes of data", num_bytes);
     connection_event_.wait(lock);
   }
+  return data_;
 }
 
 void FakeRawConnection::write(const std::string& data) {

test/integration/fake_upstream.h

@@ -252,7 +252,7 @@ class FakeRawConnection : Logger::Loggable<Logger::Id::testing>, public FakeConn
     connection_.addReadFilter(Network::ReadFilterSharedPtr{new ReadFilter(*this)});
   }
 
-  void waitForData(uint64_t num_bytes);
+  std::string waitForData(uint64_t num_bytes);
   void write(const std::string& data);
 
 private:

test/integration/integration_test.cc

@@ -193,31 +193,36 @@ TEST_P(IntegrationTest, WebSocketConnectionDownstreamDisconnect) {
   IntegrationTcpClientPtr tcp_client;
   FakeRawConnectionPtr fake_upstream_connection;
   const std::string upgrade_req_str = "GET /websocket/test HTTP/1.1\r\nHost: host\r\nConnection: "
-                                      "Upgrade\r\nUpgrade: websocket\r\n\r\n";
+                                      "keep-alive, Upgrade\r\nUpgrade: websocket\r\n\r\n";
   const std::string upgrade_resp_str =
       "HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: websocket\r\n\r\n";
 
   tcp_client = makeTcpConnection(lookupPort("http"));
   // Send websocket upgrade request
   // The request path gets rewritten from /websocket/test to /websocket.
-  // The size of headers received by the destination is 225 bytes.
+  // The size of headers received by the destination is 228 bytes.
   tcp_client->write(upgrade_req_str);
   fake_upstream_connection = fake_upstreams_[0]->waitForRawConnection();
-  fake_upstream_connection->waitForData(225);
+  const std::string data = fake_upstream_connection->waitForData(228);
+  // In HTTP1, the transfer-length is defined by use of the "chunked" transfer-coding, even if
+  // content-length header is present. No body websocket upgrade request send to upstream has
+  // content-length header and has no transfer-encoding header.
+  EXPECT_NE(data.find("content-length:"), std::string::npos);
+  EXPECT_EQ(data.find("transfer-encoding:"), std::string::npos);
   // Accept websocket upgrade request
   fake_upstream_connection->write(upgrade_resp_str);
   tcp_client->waitForData(upgrade_resp_str);
   // Standard TCP proxy semantics post upgrade
   tcp_client->write("hello");
-  // datalen = 225 + strlen(hello)
-  fake_upstream_connection->waitForData(230);
+  // datalen = 228 + strlen(hello)
+  fake_upstream_connection->waitForData(233);
   fake_upstream_connection->write("world");
   tcp_client->waitForData(upgrade_resp_str + "world");
   tcp_client->write("bye!");
   // downstream disconnect
   tcp_client->close();
-  // datalen = 225 + strlen(hello) + strlen(bye!)
-  fake_upstream_connection->waitForData(234);
+  // datalen = 228 + strlen(hello) + strlen(bye!)
+  fake_upstream_connection->waitForData(237);
   fake_upstream_connection->waitForDisconnect();
 }
 
@@ -230,23 +235,28 @@ TEST_P(IntegrationTest, WebSocketConnectionUpstreamDisconnect) {
   IntegrationTcpClientPtr tcp_client;
   FakeRawConnectionPtr fake_upstream_connection;
   const std::string upgrade_req_str = "GET /websocket/test HTTP/1.1\r\nHost: host\r\nConnection: "
-                                      "Upgrade\r\nUpgrade: websocket\r\n\r\n";
+                                      "keep-alive, Upgrade\r\nUpgrade: websocket\r\n\r\n";
   const std::string upgrade_resp_str =
       "HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: websocket\r\n\r\n";
   tcp_client = makeTcpConnection(lookupPort("http"));
   // Send websocket upgrade request
   tcp_client->write(upgrade_req_str);
   fake_upstream_connection = fake_upstreams_[0]->waitForRawConnection();
   // The request path gets rewritten from /websocket/test to /websocket.
-  // The size of headers received by the destination is 225 bytes.
-  fake_upstream_connection->waitForData(225);
+  // The size of headers received by the destination is 228 bytes.
+  const std::string data = fake_upstream_connection->waitForData(228);
+  // In HTTP1, the transfer-length is defined by use of the "chunked" transfer-coding, even if
+  // content-length header is present. No body websocket upgrade request send to upstream has
+  // content-length header and has no transfer-encoding header.
+  EXPECT_NE(data.find("content-length:"), std::string::npos);
+  EXPECT_EQ(data.find("transfer-encoding:"), std::string::npos);
   // Accept websocket upgrade request
   fake_upstream_connection->write(upgrade_resp_str);
   tcp_client->waitForData(upgrade_resp_str);
   // Standard TCP proxy semantics post upgrade
   tcp_client->write("hello");
-  // datalen = 225 + strlen(hello)
-  fake_upstream_connection->waitForData(230);
+  // datalen = 228 + strlen(hello)
+  fake_upstream_connection->waitForData(233);
   fake_upstream_connection->write("world");
   // upstream disconnect
   fake_upstream_connection->close();