Fail WebSocket channel when handshake fails.

net/websockets/websocket_stream_test.cc

Index: net/websockets/websocket_stream_test.cc
diff --git a/net/websockets/websocket_stream_test.cc b/net/websockets/websocket_stream_test.cc
index 3e11a95ac1c2d3c0b49ef1cd2e3136ee62cd3d87..9b26902034a6018b070398e7958d4fd0b9832039 100644
--- a/net/websockets/websocket_stream_test.cc
+++ b/net/websockets/websocket_stream_test.cc
@@ -45,7 +45,7 @@ class DeterministicKeyWebSocketHandshakeStreamCreateHelper
 
 class WebSocketStreamCreateTest : public ::testing::Test {
  protected:
-  WebSocketStreamCreateTest() : websocket_error_(0) {}
+  WebSocketStreamCreateTest(): has_failed_(false) {}
 
   void CreateAndConnectCustomResponse(
       const std::string& socket_url,
@@ -54,8 +54,19 @@ class WebSocketStreamCreateTest : public ::testing::Test {
       const std::string& origin,
       const std::string& extra_request_headers,
       const std::string& response_body) {
+    std::string sub_protocols_header;

[Adam Rice, 2013/12/06 07:02:31]

This looks too much like an implementation to appear in a unit test, in my
opinion. If someone changes a test and this logic and the logic in
websocket_basic_handshake_stream.cc both fail in the same way, we will never
know about it.

[yhirano, 2013/12/06 08:54:56]

Done.

+    if (sub_protocols.size() > 0) {
+      sub_protocols_header =
+          std::string("Sec-WebSocket-Protocol: ") + sub_protocols[0];
+      for (size_t i = 1; i < sub_protocols.size(); ++i) {
+        sub_protocols_header += ", " + sub_protocols[i];
+      }
+      sub_protocols_header += "\r\n";
+    }
     url_request_context_host_.SetExpectations(
-        WebSocketStandardRequest(socket_path, origin, extra_request_headers),
+        WebSocketStandardRequest(socket_path,
+                                 origin,
+                                 sub_protocols_header + extra_request_headers),
         response_body);
     CreateAndConnectStream(socket_url, sub_protocols, origin);
   }
@@ -110,18 +121,21 @@ class WebSocketStreamCreateTest : public ::testing::Test {
     return std::vector<std::string>();
   }
 
-  uint16 error() const { return websocket_error_; }
+  const std::string& failure_message() const { return failure_message_; }
+  bool has_failed() const { return has_failed_; }
 
   class TestConnectDelegate : public WebSocketStream::ConnectDelegate {
    public:
-    TestConnectDelegate(WebSocketStreamCreateTest* owner) : owner_(owner) {}
+    explicit TestConnectDelegate(WebSocketStreamCreateTest* owner)
+        : owner_(owner) {}
 
     virtual void OnSuccess(scoped_ptr<WebSocketStream> stream) OVERRIDE {
       stream.swap(owner_->stream_);
     }
 
-    virtual void OnFailure(uint16 websocket_error) OVERRIDE {
-      owner_->websocket_error_ = websocket_error;
+    virtual void OnFailure(const std::string& message) OVERRIDE {
+      owner_->has_failed_ = true;
+      owner_->failure_message_ = message;
     }
 
    private:
@@ -132,8 +146,9 @@ class WebSocketStreamCreateTest : public ::testing::Test {
   scoped_ptr<WebSocketStreamRequest> stream_request_;
   // Only set if the connection succeeded.
   scoped_ptr<WebSocketStream> stream_;
-  // Only set if the connection failed. 0 otherwise.
-  uint16 websocket_error_;
+  // Only set if the connection failed.
+  std::string failure_message_;
+  bool has_failed_;
 };
 
 // Confirm that the basic case works as expected.
@@ -141,6 +156,7 @@ TEST_F(WebSocketStreamCreateTest, SimpleSuccess) {
   CreateAndConnectStandard(
       "ws://localhost/", "/", NoSubProtocols(), "http://localhost/", "", "");
   RunUntilIdle();
+  EXPECT_FALSE(has_failed());
   EXPECT_TRUE(stream_);
 }
 
@@ -148,6 +164,7 @@ TEST_F(WebSocketStreamCreateTest, SimpleSuccess) {
 TEST_F(WebSocketStreamCreateTest, NeedsToRunLoop) {
   CreateAndConnectStandard(
       "ws://localhost/", "/", NoSubProtocols(), "http://localhost/", "", "");
+  EXPECT_FALSE(has_failed());
   EXPECT_FALSE(stream_);
 }
 
@@ -160,6 +177,7 @@ TEST_F(WebSocketStreamCreateTest, PathIsUsed) {
                            "",
                            "");
   RunUntilIdle();
+  EXPECT_FALSE(has_failed());
   EXPECT_TRUE(stream_);
 }
 
@@ -172,6 +190,7 @@ TEST_F(WebSocketStreamCreateTest, OriginIsUsed) {
                            "",
                            "");
   RunUntilIdle();
+  EXPECT_FALSE(has_failed());
   EXPECT_TRUE(stream_);
 }
 
@@ -184,11 +203,11 @@ TEST_F(WebSocketStreamCreateTest, SubProtocolIsUsed) {
                            "/testing_path",
                            sub_protocols,
                            "http://google.com/",
-                           "Sec-WebSocket-Protocol: chatv11.chromium.org, "
-                           "chatv20.chromium.org\r\n",
+                           "",
                            "Sec-WebSocket-Protocol: chatv20.chromium.org\r\n");
   RunUntilIdle();
   EXPECT_TRUE(stream_);
+  EXPECT_FALSE(has_failed());
   EXPECT_EQ("chatv20.chromium.org", stream_->GetSubProtocol());
 }
 
@@ -202,7 +221,11 @@ TEST_F(WebSocketStreamCreateTest, UnsolicitedSubProtocol) {
                            "Sec-WebSocket-Protocol: chatv20.chromium.org\r\n");
   RunUntilIdle();
   EXPECT_FALSE(stream_);
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "Response must not include 'Sec-WebSocket-Protocol' header "
+            "if not present in request: chatv20.chromium.org",
+            failure_message());
 }
 
 // Missing sub-protocol response is rejected.
@@ -211,11 +234,15 @@ TEST_F(WebSocketStreamCreateTest, UnacceptedSubProtocol) {
                            "/testing_path",
                            std::vector<std::string>(1, "chat.example.com"),
                            "http://localhost/",
-                           "Sec-WebSocket-Protocol: chat.example.com\r\n",
+                           "",
                            "");
   RunUntilIdle();
   EXPECT_FALSE(stream_);
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "Sent non-empty 'Sec-WebSocket-Protocol' header "
+            "but no response is received",
+            failure_message());
 }
 
 // Only one sub-protocol can be accepted.
@@ -227,13 +254,36 @@ TEST_F(WebSocketStreamCreateTest, MultipleSubProtocolsInResponse) {
                            "/testing_path",
                            sub_protocols,
                            "http://google.com/",
-                           "Sec-WebSocket-Protocol: chatv11.chromium.org, "
-                           "chatv20.chromium.org\r\n",
+                           "",
                            "Sec-WebSocket-Protocol: chatv11.chromium.org, "
                            "chatv20.chromium.org\r\n");
   RunUntilIdle();
   EXPECT_FALSE(stream_);
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "'Sec-WebSocket-Protocol' header must not appear "
+            "more than once in a response",
+            failure_message());
+}
+
+// Unmatched sub-protocol should be jrected.
+TEST_F(WebSocketStreamCreateTest, UnmatchedSubProtocolInResponse) {
+  std::vector<std::string> sub_protocols;
+  sub_protocols.push_back("chatv11.chromium.org");
+  sub_protocols.push_back("chatv20.chromium.org");
+  CreateAndConnectStandard("ws://localhost/testing_path",
+                           "/testing_path",
+                           sub_protocols,
+                           "http://google.com/",
+                           "",
+                           "Sec-WebSocket-Protocol: chatv21.chromium.org\r\n");
+  RunUntilIdle();
+  EXPECT_FALSE(stream_);
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "'Sec-WebSocket-Protocol' header value 'chatv21.chromium.org' "
+            "in response does not match any of sent values",
+            failure_message());
 }
 
 // Unknown extension in the response is rejected
@@ -246,7 +296,11 @@ TEST_F(WebSocketStreamCreateTest, UnknownExtension) {
                            "Sec-WebSocket-Extensions: x-unknown-extension\r\n");
   RunUntilIdle();
   EXPECT_FALSE(stream_);
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "Found an unsupported extension 'x-unknown-extension' "
+            "in 'Sec-WebSocket-Extensions' header",
+            failure_message());
 }
 
 // Additional Sec-WebSocket-Accept headers should be rejected.
@@ -260,7 +314,11 @@ TEST_F(WebSocketStreamCreateTest, DoubleAccept) {
       "Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n");
   RunUntilIdle();
   EXPECT_FALSE(stream_);
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "'Sec-WebSocket-Accept' header must not appear "
+            "more than once in a response",
+            failure_message());
 }
 
 // Response code 200 must be rejected.
@@ -278,7 +336,8 @@ TEST_F(WebSocketStreamCreateTest, InvalidStatusCode) {
                                  "",
                                  kInvalidStatusCodeResponse);
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("", failure_message());
 }
 
 // Redirects are not followed (according to the WHATWG WebSocket API, which
@@ -299,7 +358,8 @@ TEST_F(WebSocketStreamCreateTest, RedirectsRejected) {
                                  "",
                                  kRedirectResponse);
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("", failure_message());
 }
 
 // Malformed responses should be rejected. HttpStreamParser will accept just
@@ -321,7 +381,8 @@ TEST_F(WebSocketStreamCreateTest, MalformedResponse) {
                                  "",
                                  kMalformedResponse);
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("", failure_message());

[Adam Rice, 2013/12/06 07:02:31]

It will be hard to produce a good failure message for this case, since it is
treated as a valid response. We may have to use the same "Invalid status code"
message as for the test above.

[yhirano, 2013/12/06 08:54:56]

Done.

 }
 
 // Upgrade header must be present.
@@ -338,7 +399,9 @@ TEST_F(WebSocketStreamCreateTest, MissingUpgradeHeader) {
                                  "",
                                  kMissingUpgradeResponse);
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: 'Upgrade' header is missing",
+            failure_message());
 }
 
 // There must only be one upgrade header.
@@ -350,7 +413,31 @@ TEST_F(WebSocketStreamCreateTest, DoubleUpgradeHeader) {
       "http://localhost/",
       "", "Upgrade: HTTP/2.0\r\n");
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "'Upgrade' header must not appear more than once in a response",
+            failure_message());
+}
+
+// There must only be one correct upgrade header.
+TEST_F(WebSocketStreamCreateTest, IncorrectUpgradeHeader) {
+  static const char kMissingUpgradeResponse[] =
+      "HTTP/1.1 101 Switching Protocols\r\n"
+      "Connection: Upgrade\r\n"
+      "Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n"
+      "Upgrade: hogefuga\r\n"
+      "\r\n";
+  CreateAndConnectCustomResponse("ws://localhost/",
+                                 "/",
+                                 NoSubProtocols(),
+                                 "http://localhost/",
+                                 "",
+                                 kMissingUpgradeResponse);
+  RunUntilIdle();
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "'Upgrade' header value is not 'WebSocket': hogefuga",
+            failure_message());
 }
 
 // Connection header must be present.
@@ -367,7 +454,31 @@ TEST_F(WebSocketStreamCreateTest, MissingConnectionHeader) {
                                  "",
                                  kMissingConnectionResponse);
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "'Connection' header is missing",
+            failure_message());
+}
+
+// Connection header must contain "Upgrade".
+TEST_F(WebSocketStreamCreateTest, IncorrectConnectionHeader) {
+  static const char kMissingConnectionResponse[] =
+      "HTTP/1.1 101 Switching Protocols\r\n"
+      "Upgrade: websocket\r\n"
+      "Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n"
+      "Connection: hogefuga\r\n"
+      "\r\n";
+  CreateAndConnectCustomResponse("ws://localhost/",
+                                 "/",
+                                 NoSubProtocols(),
+                                 "http://localhost/",
+                                 "",
+                                 kMissingConnectionResponse);
+  RunUntilIdle();
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "'Connection' header value must contain 'Upgrade'",
+            failure_message());
 }
 
 // Connection header is permitted to contain other tokens.
@@ -385,6 +496,7 @@ TEST_F(WebSocketStreamCreateTest, AdditionalTokenInConnectionHeader) {
                                  "",
                                  kAdditionalConnectionTokenResponse);
   RunUntilIdle();
+  EXPECT_FALSE(has_failed());
   EXPECT_TRUE(stream_);
 }
 
@@ -402,7 +514,10 @@ TEST_F(WebSocketStreamCreateTest, MissingSecWebSocketAccept) {
                                  "",
                                  kMissingAcceptResponse);
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "'Sec-WebSocket-Accept' header is missing",
+            failure_message());
 }
 
 // Sec-WebSocket-Accept header must match the key that was sent.
@@ -420,7 +535,10 @@ TEST_F(WebSocketStreamCreateTest, WrongSecWebSocketAccept) {
                                  "",
                                  kIncorrectAcceptResponse);
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("Error during WebSocket handshake: "
+            "Incorrect 'Sec-WebSocket-Accept' header value",
+            failure_message());
 }
 
 // Cancellation works.
@@ -429,6 +547,7 @@ TEST_F(WebSocketStreamCreateTest, Cancellation) {
       "ws://localhost/", "/", NoSubProtocols(), "http://localhost/", "", "");
   stream_request_.reset();
   RunUntilIdle();
+  EXPECT_FALSE(has_failed());
   EXPECT_FALSE(stream_);
 }
 
@@ -441,7 +560,8 @@ TEST_F(WebSocketStreamCreateTest, ConnectionFailure) {
   CreateAndConnectRawExpectations("ws://localhost/", NoSubProtocols(),
                                   "http://localhost/", socket_data.Pass());
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("", failure_message());
 }
 
 // Connect timeout must look just like any other failure.
@@ -453,7 +573,8 @@ TEST_F(WebSocketStreamCreateTest, ConnectionTimeout) {
   CreateAndConnectRawExpectations("ws://localhost/", NoSubProtocols(),
                                   "http://localhost/", socket_data.Pass());
   RunUntilIdle();
-  EXPECT_EQ(1006, error());
+  EXPECT_TRUE(has_failed());
+  EXPECT_EQ("", failure_message());
 }
 
 // Cancellation during connect works.
@@ -467,6 +588,7 @@ TEST_F(WebSocketStreamCreateTest, CancellationDuringConnect) {
                                   socket_data.Pass());
   stream_request_.reset();
   RunUntilIdle();
+  EXPECT_FALSE(has_failed());
   EXPECT_FALSE(stream_);
 }
 
@@ -487,6 +609,7 @@ TEST_F(WebSocketStreamCreateTest, CancellationDuringWrite) {
   socket_data->Run();
   stream_request_.reset();
   RunUntilIdle();
+  EXPECT_FALSE(has_failed());
   EXPECT_FALSE(stream_);
 }
 
@@ -508,6 +631,7 @@ TEST_F(WebSocketStreamCreateTest, CancellationDuringRead) {
   socket_data->Run();
   stream_request_.reset();
   RunUntilIdle();
+  EXPECT_FALSE(has_failed());
   EXPECT_FALSE(stream_);
 }