Fix WebSocketServer extension parser.

net/server/web_socket_encoder.cc

Index: net/server/web_socket_encoder.cc
diff --git a/net/server/web_socket_encoder.cc b/net/server/web_socket_encoder.cc
index 1a5431affd8415a23e6ee6f3a553064d1447a803..e0e06319dbdb1cbb121b7c55f2b3e721b3fc5ca4 100644
--- a/net/server/web_socket_encoder.cc
+++ b/net/server/web_socket_encoder.cc
@@ -4,10 +4,14 @@
 
 #include "net/server/web_socket_encoder.h"
 
+#include <vector>
+
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/io_buffer.h"
+#include "net/websockets/websocket_deflate_parameters.h"
+#include "net/websockets/websocket_extension.h"
 #include "net/websockets/websocket_extension_parser.h"
 
 namespace net {
@@ -180,151 +184,108 @@ void EncodeFrameHybi17(const std::string& message,
 }  // anonymous namespace
 
 // static
-WebSocketEncoder* WebSocketEncoder::CreateServer(
-    const std::string& request_extensions,
-    std::string* response_extensions) {
-  bool deflate;
-  bool has_client_window_bits;
-  int client_window_bits;
-  int server_window_bits;
-  bool client_no_context_takeover;
-  bool server_no_context_takeover;
-  ParseExtensions(request_extensions, &deflate, &has_client_window_bits,
-                  &client_window_bits, &server_window_bits,
-                  &client_no_context_takeover, &server_no_context_takeover);
-
-  if (deflate) {
-    *response_extensions = base::StringPrintf(
-        "permessage-deflate; server_max_window_bits=%d%s", server_window_bits,
-        server_no_context_takeover ? "; server_no_context_takeover" : "");
-    if (has_client_window_bits) {
-      base::StringAppendF(response_extensions, "; client_max_window_bits=%d",
-                          client_window_bits);
-    } else {
-      DCHECK_EQ(client_window_bits, 15);
-    }
-    return new WebSocketEncoder(true /* is_server */, server_window_bits,
-                                client_window_bits, server_no_context_takeover);
-  } else {
-    *response_extensions = std::string();
-    return new WebSocketEncoder(true /* is_server */);
-  }
+scoped_ptr<WebSocketEncoder> WebSocketEncoder::CreateServer() {
+  return make_scoped_ptr(new WebSocketEncoder(FOR_SERVER, nullptr, nullptr));
 }
 
 // static
-WebSocketEncoder* WebSocketEncoder::CreateClient(
-    const std::string& response_extensions) {
-  bool deflate;
-  bool has_client_window_bits;
-  int client_window_bits;
-  int server_window_bits;
-  bool client_no_context_takeover;
-  bool server_no_context_takeover;
-  ParseExtensions(response_extensions, &deflate, &has_client_window_bits,
-                  &client_window_bits, &server_window_bits,
-                  &client_no_context_takeover, &server_no_context_takeover);
-
-  if (deflate) {
-    return new WebSocketEncoder(false /* is_server */, client_window_bits,
-                                server_window_bits, client_no_context_takeover);
-  } else {
-    return new WebSocketEncoder(false /* is_server */);
+scoped_ptr<WebSocketEncoder> WebSocketEncoder::CreateServer(
+    const std::string& request_extensions,
+    WebSocketDeflateParameters* deflate_parameters) {
+  WebSocketExtensionParser parser;
+  if (!parser.Parse(request_extensions)) {
+    // Failed to parse Sec-WebSocket-Extensions header. We MUST fail the
+    // connection.
+    return nullptr;
   }
-}
-
-// static
-void WebSocketEncoder::ParseExtensions(const std::string& header_value,
-                                       bool* deflate,
-                                       bool* has_client_window_bits,
-                                       int* client_window_bits,
-                                       int* server_window_bits,
-                                       bool* client_no_context_takeover,
-                                       bool* server_no_context_takeover) {
-  *deflate = false;
-  *has_client_window_bits = false;
-  *client_window_bits = 15;
-  *server_window_bits = 15;
-  *client_no_context_takeover = false;
-  *server_no_context_takeover = false;
-
-  if (header_value.empty())
-    return;
 
-  WebSocketExtensionParser parser;
-  if (!parser.Parse(header_value))
-    return;
-  const std::vector<WebSocketExtension>& extensions = parser.extensions();
-  // TODO(tyoshino): Fail if this method is used for parsing a response and
-  // there are multiple permessage-deflate extensions or there are any unknown
-  // extensions.
-  for (const auto& extension : extensions) {
-    if (extension.name() != "permessage-deflate") {
+  WebSocketDeflateParameters offered, response;

[tyoshino (SeeGerritForStatus), 2015/09/16 09:33:10]

response can be declared at L219

[tyoshino (SeeGerritForStatus), 2015/09/16 09:33:10]

offered -> accepted_offered_params or accepted_params or accepted_offer, etc.

[yhirano, 2015/09/16 10:47:46]

Done.

[yhirano, 2015/09/16 10:47:46]

I removed this variable and renamed |params| to |offer|.

+  std::string failure_message;
+  bool found = false;
+  for (const auto& extension : parser.extensions()) {
+    WebSocketDeflateParameters params;
+    if (!params.Initialize(extension, &failure_message) ||
+        !params.IsValidAsRequest(&failure_message)) {
+      // We decline unknown / malformed extensions.
       continue;
     }
-
-    const std::vector<WebSocketExtension::Parameter>& parameters =
-        extension.parameters();
-    for (const auto& param : parameters) {
-      const std::string& name = param.name();
-      // TODO(tyoshino): Fail the connection when an invalid value is given.
-      if (name == "client_max_window_bits") {
-        *has_client_window_bits = true;
-        if (param.HasValue()) {
-          int bits = 0;
-          if (base::StringToInt(param.value(), &bits) && bits >= 8 &&
-              bits <= 15) {
-            *client_window_bits = bits;
-          }
-        }
-      }
-      if (name == "server_max_window_bits" && param.HasValue()) {
-        int bits = 0;
-        if (base::StringToInt(param.value(), &bits) && bits >= 8 && bits <= 15)
-          *server_window_bits = bits;
-      }
-      if (name == "client_no_context_takeover")
-        *client_no_context_takeover = true;
-      if (name == "server_no_context_takeover")
-        *server_no_context_takeover = true;
-    }
-    *deflate = true;
-
+    found = true;
+    offered = params;
     break;
   }
-}
+  if (!found)
+    return make_scoped_ptr(new WebSocketEncoder(FOR_SERVER, nullptr, nullptr));
+
+  response = offered;
+  if (offered.is_client_max_window_bits_specified() &&
+      !offered.has_client_max_window_bits_value()) {
+    // We need to choose one value for the response.
+    response.SetClientMaxWindowBits(8);

[tyoshino (SeeGerritForStatus), 2015/09/16 09:33:10]

15? We had offline discussion.

[yhirano, 2015/09/16 10:47:46]

Done.

+  }
+  DCHECK(response.IsValidAsResponse());
+  DCHECK(offered.IsCompatibleWith(response));
 
-WebSocketEncoder::WebSocketEncoder(bool is_server) : is_server_(is_server) {
+  auto deflater = make_scoped_ptr(
+      new WebSocketDeflater(response.server_context_take_over_mode()));
+  auto inflater = make_scoped_ptr(
+      new WebSocketInflater(kInflaterChunkSize, kInflaterChunkSize));
+  if (!deflater->Initialize(response.PermissiveServerMaxWindowBits()) ||
+      !inflater->Initialize(response.PermissiveClientMaxWindowBits())) {
+    return make_scoped_ptr(new WebSocketEncoder(FOR_SERVER, nullptr, nullptr));
+  }
+  *deflate_parameters = response;
+  return make_scoped_ptr(
+      new WebSocketEncoder(FOR_SERVER, deflater.Pass(), inflater.Pass()));
 }
 
-WebSocketEncoder::WebSocketEncoder(bool is_server,
-                                   int deflate_bits,
-                                   int inflate_bits,
-                                   bool no_context_takeover)
-    : is_server_(is_server) {
-  deflater_.reset(new WebSocketDeflater(
-      no_context_takeover ? WebSocketDeflater::DO_NOT_TAKE_OVER_CONTEXT
-                          : WebSocketDeflater::TAKE_OVER_CONTEXT));
-  inflater_.reset(
-      new WebSocketInflater(kInflaterChunkSize, kInflaterChunkSize));
+// static
+WebSocketEncoder* WebSocketEncoder::CreateClient(
+    const std::string& response_extensions) {
+  // TODO(yhirano): Add a way to return an error.

[tyoshino (SeeGerritForStatus), 2015/09/16 09:33:10]

please also mark lines where we should error the encoder by adding TODOs

[yhirano, 2015/09/16 10:47:46]

Done.

 
-  if (!deflater_->Initialize(deflate_bits) ||
-      !inflater_->Initialize(inflate_bits)) {
-    // Disable deflate support.
-    deflater_.reset();
-    inflater_.reset();
+  WebSocketExtensionParser parser;
+  if (!parser.Parse(response_extensions)) {
+    // Parse error.
+    return new WebSocketEncoder(FOR_CLIENT, nullptr, nullptr);
+  }
+  if (parser.extensions().size() != 1) {
+    // Only permessage-deflate extension is supported.
+    return new WebSocketEncoder(FOR_CLIENT, nullptr, nullptr);
+  }

[tyoshino (SeeGerritForStatus), 2015/09/16 09:33:10]

handle no extension case?

[yhirano, 2015/09/16 10:47:46]

It is handled in L246-L249. I added comments.

[tyoshino (SeeGerritForStatus), 2015/09/17 02:24:37]

Ah, got it!

+  const auto& extension = parser.extensions()[0];
+  WebSocketDeflateParameters params;
+  std::string failure_message;
+  if (!params.Initialize(extension, &failure_message) ||
+      !params.IsValidAsResponse(&failure_message)) {
+    return new WebSocketEncoder(FOR_CLIENT, nullptr, nullptr);
+  }
+
+  auto deflater = make_scoped_ptr(
+      new WebSocketDeflater(params.client_context_take_over_mode()));
+  auto inflater = make_scoped_ptr(
+      new WebSocketInflater(kInflaterChunkSize, kInflaterChunkSize));
+  if (!deflater->Initialize(params.PermissiveClientMaxWindowBits()) ||
+      !inflater->Initialize(params.PermissiveServerMaxWindowBits())) {
+    return new WebSocketEncoder(FOR_CLIENT, nullptr, nullptr);
   }
-}
 
-WebSocketEncoder::~WebSocketEncoder() {
+  return new WebSocketEncoder(FOR_CLIENT, deflater.Pass(), inflater.Pass());
 }
 
+WebSocketEncoder::WebSocketEncoder(Type type,
+                                   scoped_ptr<WebSocketDeflater> deflater,
+                                   scoped_ptr<WebSocketInflater> inflater)
+    : type_(type), deflater_(deflater.Pass()), inflater_(inflater.Pass()) {}
+
+WebSocketEncoder::~WebSocketEncoder() {}
+
 WebSocket::ParseResult WebSocketEncoder::DecodeFrame(
     const base::StringPiece& frame,
     int* bytes_consumed,
     std::string* output) {
   bool compressed;
-  WebSocket::ParseResult result =
-      DecodeFrameHybi17(frame, is_server_, bytes_consumed, output, &compressed);
+  WebSocket::ParseResult result = DecodeFrameHybi17(
+      frame, type_ == FOR_SERVER, bytes_consumed, output, &compressed);
   if (result == WebSocket::FRAME_OK && compressed) {
     if (!Inflate(output))
       result = WebSocket::FRAME_ERROR;