HttpServer WebSocket: don't crash on data before server handshake

net/server/web_socket.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/server/web_socket.h"
 
 #include <vector>
 
 #include "base/base64.h"
 #include "base/logging.h"
@@ skipping 80 matching lines @@
     }
   }
   server_->SendRaw(connection_->id(),
                    ValidResponseString(encoded_hash, response_extensions));
 }
 
 WebSocket::ParseResult WebSocket::Read(std::string* message) {
   if (closed_)
     return FRAME_CLOSE;
 
+  if (!encoder_) {
+    // RFC6455, section 4.1 says "Once the client's opening handshake has been
+    // sent, the client MUST wait for a response from the server before sending
+    // any further data". If encoder_ is null here, we haven't even sent a

[mmenke, 2017/01/23 16:17:21]

nit (x2):  |encoder_| is the more common pattern in net/, for class and stack
variable names.

[Maks Orlovich, 2017/01/23 17:28:29]

Done. I incorrectly assumed this was just for API docs...

+    // server handshake in the first place, so the client clearly didn't do
+    // that, so we can reject them guilt-free, especially since we can't

[mmenke, 2017/01/23 16:17:21]

nit (x2):  Avoid we in comments, due to ambiguity.

[Maks Orlovich, 2017/01/23 17:28:29]

Rephrased.

+    // proceed without an encoder_ anyway.
+    Fail();

[mmenke, 2017/01/23 16:17:21]

Hrm...The caller already closes the socket when we return FRAME_CLOSE or
FRAME_ERROR.  There's no documented API contract here, but digging into the
encoder_->DecodeFrame call below, it looks like on errors it does not close the
socket, but instead just returns FRAME_ERROR, so I think it's more consistent
not to call Fail() here.

[Maks Orlovich, 2017/01/23 17:28:29]

Good point, done. This does mean we don't set closed_, but since the connection
is closed no one should be able to call into something like WebSocket::Send()
anyway.

+    return FRAME_CLOSE;

[mmenke, 2017/01/23 16:17:21]

FRAME_ERROR seems more accurate.

[Maks Orlovich, 2017/01/23 17:28:29]

Done.

+  }
+
   HttpConnection::ReadIOBuffer* read_buf = connection_->read_buf();
   base::StringPiece frame(read_buf->StartOfBuffer(), read_buf->GetSize());
   int bytes_consumed = 0;
   ParseResult result = encoder_->DecodeFrame(frame, &bytes_consumed, message);
   if (result == FRAME_OK)
     read_buf->DidConsume(bytes_consumed);
   if (result == FRAME_CLOSE)
     closed_ = true;
   return result;
 }
@@ skipping 13 matching lines @@
 }
 
 void WebSocket::SendErrorResponse(const std::string& message) {
   if (closed_)
     return;
   closed_ = true;
   server_->Send500(connection_->id(), message);
 }
 
 }  // namespace net