[WebSocket] Do not send response headers for failed handshake.

This patch changes network service to restrict sending the headers to
the renderer not to leak info if handshake was failed.

This also changes WebRequest API and devtools event listening as failed
when handshake was failed.

Change-Id: I03160b06546711365273180a8020239e09528a47
Bug: 944619
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1847592
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705363}

chrome/test/data/extensions/api_test/webrequest/test_websocket_auth.js

@@ -68,20 +68,6 @@ chrome.test.getConfig(function(config) {
                       initiator: getDomain(initiators.WEB_INITIATED)
                     }
                   },
-                  {
-                    label: 'onResponseStarted',
-                    event: 'onResponseStarted',
-                    details: {
-                      url: url,
-                      type: 'websocket',
-                      ip: '127.0.0.1',
-                      fromCache: false,
-                      statusCode: 401,
-                      statusLine: 'HTTP/1.0 401 Unauthorized',
-                      responseHeadersExist: true,
-                      initiator: getDomain(initiators.WEB_INITIATED)
-                    }
-                  },
                   {
                     label: 'onErrorOccurred',
                     event: 'onErrorOccurred',
@@ -98,8 +84,7 @@ chrome.test.getConfig(function(config) {
                 [  // event order
                   [
                     'onBeforeRequest', 'onBeforeSendHeaders', 'onSendHeaders',
-                    'onHeadersReceived', 'onAuthRequired', 'onResponseStarted',
-                    'onErrorOccurred'
+                    'onHeadersReceived', 'onAuthRequired', 'onErrorOccurred'
                   ]
                 ],
                 {urls: ['<all_urls>']},  // filter
@@ -168,19 +153,6 @@ chrome.test.getConfig(function(config) {
                       initiator: getDomain(initiators.WEB_INITIATED)
                     }
                   },
-                  {
-                    label: 'onResponseStarted',
-                    event: 'onResponseStarted',
-                    details: {
-                      url: url,
-                      type: 'websocket',
-                      fromCache: false,
-                      ip: '127.0.0.1',
-                      statusCode: 401,
-                      statusLine: 'HTTP/1.0 401 Unauthorized',
-                      initiator: getDomain(initiators.WEB_INITIATED)
-                    }
-                  },
                   {
                     label: 'onErrorOccurred',
                     event: 'onErrorOccurred',
@@ -197,8 +169,7 @@ chrome.test.getConfig(function(config) {
                 [  // event order
                   [
                     'onBeforeRequest', 'onBeforeSendHeaders', 'onSendHeaders',
-                    'onHeadersReceived', 'onAuthRequired', 'onResponseStarted',
-                    'onErrorOccurred'
+                    'onHeadersReceived', 'onAuthRequired', 'onErrorOccurred'
                   ]
                 ],
                 {urls: ['<all_urls>']}, ['blocking']);
@@ -267,19 +238,6 @@ chrome.test.getConfig(function(config) {
                     },
                     retval: {cancel: true}
                   },
-                  {
-                    label: 'onResponseStarted',
-                    event: 'onResponseStarted',
-                    details: {
-                      url: url,
-                      type: 'websocket',
-                      fromCache: false,
-                      ip: '127.0.0.1',
-                      statusCode: 401,
-                      statusLine: 'HTTP/1.0 401 Unauthorized',
-                      initiator: getDomain(initiators.WEB_INITIATED)
-                    }
-                  },
                   {
                     label: 'onErrorOccurred',
                     event: 'onErrorOccurred',
@@ -296,8 +254,7 @@ chrome.test.getConfig(function(config) {
                 [  // event order
                   [
                     'onBeforeRequest', 'onBeforeSendHeaders', 'onSendHeaders',
-                    'onHeadersReceived', 'onAuthRequired', 'onResponseStarted',
-                    'onErrorOccurred'
+                    'onHeadersReceived', 'onAuthRequired', 'onErrorOccurred'
                   ]
                 ],
                 {urls: ['<all_urls>']}, ['blocking']);

extensions/browser/api/web_request/web_request_proxying_websocket.cc

@@ -138,37 +138,6 @@ void WebRequestProxyingWebSocket::OnOpeningHandshakeStarted(
   forwarding_handshake_client_->OnOpeningHandshakeStarted(std::move(request));
 }
 
-void WebRequestProxyingWebSocket::OnResponseReceived(
-    network::mojom::WebSocketHandshakeResponsePtr response) {
-  DCHECK(forwarding_handshake_client_);
-
-  // response_.headers will be set in OnBeforeSendHeaders if
-  // |receiver_as_header_client_| is set.
-  if (!receiver_as_header_client_.is_bound()) {
-    response_.headers =
-        base::MakeRefCounted<net::HttpResponseHeaders>(base::StringPrintf(
-            "HTTP/%d.%d %d %s", response->http_version.major_value(),
-            response->http_version.minor_value(), response->status_code,
-            response->status_text.c_str()));
-    for (const auto& header : response->headers)
-      response_.headers->AddHeader(header->name + ": " + header->value);
-  }
-
-  response_.remote_endpoint = response->remote_endpoint;
-
-  // TODO(yhirano): OnResponseReceived is called with the original
-  // response headers. That means if OnHeadersReceived modified them the
-  // renderer won't see that modification. This is the opposite of http(s)
-  // requests.
-  forwarding_handshake_client_->OnResponseReceived(std::move(response));
-
-  if (!receiver_as_header_client_.is_bound() || response_.headers) {
-    ContinueToHeadersReceived();
-  } else {
-    waiting_for_header_client_headers_received_ = true;
-  }
-}
-
 void WebRequestProxyingWebSocket::ContinueToHeadersReceived() {
   auto continuation = base::BindRepeating(
       &WebRequestProxyingWebSocket::OnHeadersReceivedComplete,
@@ -195,16 +164,47 @@ void WebRequestProxyingWebSocket::OnConnectionEstablished(
     mojo::PendingReceiver<network::mojom::WebSocketClient> client_receiver,
     const std::string& selected_protocol,
     const std::string& extensions,
+    network::mojom::WebSocketHandshakeResponsePtr response,
     mojo::ScopedDataPipeConsumerHandle readable) {
   DCHECK(forwarding_handshake_client_);
   DCHECK(!is_done_);
   is_done_ = true;
+  websocket_ = std::move(websocket);
+  client_receiver_ = std::move(client_receiver);
+  selected_protocol_ = selected_protocol;
+  extensions_ = extensions;
+  handshake_response_ = std::move(response);
+  readable_ = std::move(readable);
+
+  response_.remote_endpoint = handshake_response_->remote_endpoint;
+
+  // response_.headers will be set in OnBeforeSendHeaders if
+  // |receiver_as_header_client_| is set.
+  if (receiver_as_header_client_.is_bound()) {
+    ContinueToCompleted();
+    return;
+  }
+
+  response_.headers =
+      base::MakeRefCounted<net::HttpResponseHeaders>(base::StringPrintf(
+          "HTTP/%d.%d %d %s", handshake_response_->http_version.major_value(),
+          handshake_response_->http_version.minor_value(),
+          handshake_response_->status_code,
+          handshake_response_->status_text.c_str()));
+  for (const auto& header : handshake_response_->headers)
+    response_.headers->AddHeader(header->name + ": " + header->value);
+
+  ContinueToHeadersReceived();
+}
+
+void WebRequestProxyingWebSocket::ContinueToCompleted() {
+  DCHECK(forwarding_handshake_client_);
+  DCHECK(is_done_);
   ExtensionWebRequestEventRouter::GetInstance()->OnCompleted(
       browser_context_, &info_, net::ERR_WS_UPGRADE);
-
   forwarding_handshake_client_->OnConnectionEstablished(
-      std::move(websocket), std::move(client_receiver), selected_protocol,
-      extensions, std::move(readable));
+      std::move(websocket_), std::move(client_receiver_), selected_protocol_,
+      extensions_, std::move(handshake_response_), std::move(readable_));
 
   // Deletes |this|.
   proxies_->RemoveProxy(this);
@@ -259,16 +259,9 @@ void WebRequestProxyingWebSocket::OnHeadersReceived(
     OnHeadersReceivedCallback callback) {
   DCHECK(receiver_as_header_client_.is_bound());
 
-  // Note: since there are different pipes used for WebSocketClient and
-  // TrustedHeaderClient, there are no guarantees whether this or
-  // OnResponseReceived are called first.
   on_headers_received_callback_ = std::move(callback);
   response_.headers = base::MakeRefCounted<net::HttpResponseHeaders>(headers);
 
-  if (!waiting_for_header_client_headers_received_)
-    return;
-
-  waiting_for_header_client_headers_received_ = false;
   ContinueToHeadersReceived();
 }
 
@@ -422,6 +415,9 @@ void WebRequestProxyingWebSocket::OnHeadersReceivedComplete(int error_code) {
   info_.AddResponseInfoFromResourceResponse(response_);
   ExtensionWebRequestEventRouter::GetInstance()->OnResponseStarted(
       browser_context_, &info_, net::OK);
+
+  if (!receiver_as_header_client_.is_bound())
+    ContinueToCompleted();
 }
 
 void WebRequestProxyingWebSocket::OnAuthRequiredComplete(

extensions/browser/api/web_request/web_request_proxying_websocket.h

@@ -60,13 +60,12 @@ class WebRequestProxyingWebSocket
   // network::mojom::WebSocketHandshakeClient methods:
   void OnOpeningHandshakeStarted(
       network::mojom::WebSocketHandshakeRequestPtr request) override;
-  void OnResponseReceived(
-      network::mojom::WebSocketHandshakeResponsePtr response) override;
   void OnConnectionEstablished(
       mojo::PendingRemote<network::mojom::WebSocket> websocket,
       mojo::PendingReceiver<network::mojom::WebSocketClient> client_receiver,
       const std::string& selected_protocol,
       const std::string& extensions,
+      network::mojom::WebSocketHandshakeResponsePtr response,
       mojo::ScopedDataPipeConsumerHandle readable) override;
 
   // network::mojom::AuthenticationHandler method:
@@ -108,6 +107,7 @@ class WebRequestProxyingWebSocket
       ExtensionWebRequestEventRouter::AuthRequiredResponse rv);
   void OnHeadersReceivedCompleteForAuth(const net::AuthChallengeInfo& auth_info,
                                         int rv);
+  void ContinueToCompleted();
 
   void PauseIncomingMethodCallProcessing();
   void ResumeIncomingMethodCallProcessing();
@@ -138,8 +138,13 @@ class WebRequestProxyingWebSocket
 
   GURL redirect_url_;
   bool is_done_ = false;
-  bool waiting_for_header_client_headers_received_ = false;
   bool has_extra_headers_;
+  mojo::PendingRemote<network::mojom::WebSocket> websocket_;
+  mojo::PendingReceiver<network::mojom::WebSocketClient> client_receiver_;
+  std::string selected_protocol_;
+  std::string extensions_;
+  network::mojom::WebSocketHandshakeResponsePtr handshake_response_ = nullptr;
+  mojo::ScopedDataPipeConsumerHandle readable_;
 
   WebRequestInfo info_;
 

services/network/public/mojom/websocket.mojom

@@ -54,21 +54,19 @@ interface WebSocketHandshakeClient {
   // Notify the renderer that the browser has started an opening handshake.
   OnOpeningHandshakeStarted(WebSocketHandshakeRequest request);
 
-  // Called when the HTTP response is received. This doesn't mean the connection
-  // is (or will be) established. This message precedes OnConnectionEstablished.
-  // |response| may contain cookie-related headers when the client has
-  // an access to raw cookie information.
-  OnResponseReceived(WebSocketHandshakeResponse response);
-
   // Called when the connection is established. |selected_protocol| is the
   // sub-protocol the server selected, or empty if no sub-protocol was selected.
   // |extensions| is the list of extensions negotiated for the connection.
   // default threshold value
+  // |response| may contain cookie-related headers when the client has
+  // an access to raw cookie information.
   // |readable| is readable datapipe to receive data from browser.
+  // TODO(yoichio): Merge |selected_protocol| and |extensions| into |response|.
   OnConnectionEstablished(pending_remote<WebSocket> socket,
                           pending_receiver<WebSocketClient> client_receiver,
                           string selected_protocol,
                           string extensions,
+                          WebSocketHandshakeResponse response,
                           handle<data_pipe_consumer> readable);
 };
 

services/network/websocket.cc

@@ -118,6 +118,8 @@ class WebSocket::WebSocketEventHandler final
  private:
   WebSocket* const impl_;
 
+  mojom::WebSocketHandshakeResponsePtr response_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(WebSocketEventHandler);
 };
 
@@ -182,7 +184,7 @@ void WebSocket::WebSocketEventHandler::OnAddChannelResponse(
   impl_->handshake_client_->OnConnectionEstablished(
       impl_->receiver_.BindNewPipeAndPassRemote(),
       impl_->client_.BindNewPipeAndPassReceiver(), selected_protocol,
-      extensions, std::move(readable));
+      extensions, std::move(response_), std::move(readable));
   impl_->receiver_.set_disconnect_handler(base::BindOnce(
       &WebSocket::OnConnectionError, base::Unretained(impl_), FROM_HERE));
   impl_->handshake_client_.reset();
@@ -310,7 +312,7 @@ void WebSocket::WebSocketEventHandler::OnFinishOpeningHandshake(
   headers_text.append("\r\n");
   response_to_pass->headers_text = headers_text;
 
-  impl_->handshake_client_->OnResponseReceived(std::move(response_to_pass));
+  response_ = std::move(response_to_pass);
 }
 
 void WebSocket::WebSocketEventHandler::OnSSLCertificateError(

third_party/blink/renderer/modules/websockets/websocket_channel_impl.cc

@@ -453,31 +453,26 @@ void WebSocketChannelImpl::OnOpeningHandshakeStarted(
   handshake_request_ = std::move(request);
 }
 
-void WebSocketChannelImpl::OnResponseReceived(
-    network::mojom::blink::WebSocketHandshakeResponsePtr response) {
-  DCHECK_EQ(GetState(), State::kConnecting);
-  NETWORK_DVLOG(1) << this << " OnResponseReceived("
-                   << response->url.GetString() << ")";
-  TRACE_EVENT_INSTANT1(
-      "devtools.timeline", "WebSocketReceiveHandshakeResponse",
-      TRACE_EVENT_SCOPE_THREAD, "data",
-      InspectorWebSocketEvent::Data(execution_context_, identifier_));
-  probe::DidReceiveWebSocketHandshakeResponse(execution_context_, identifier_,
-                                              handshake_request_.get(),
-                                              response.get());
-  handshake_request_ = nullptr;
-}
-
 void WebSocketChannelImpl::OnConnectionEstablished(
     mojo::PendingRemote<network::mojom::blink::WebSocket> websocket,
     mojo::PendingReceiver<network::mojom::blink::WebSocketClient>
         client_receiver,
     const String& protocol,
     const String& extensions,
+    network::mojom::blink::WebSocketHandshakeResponsePtr response,
     mojo::ScopedDataPipeConsumerHandle readable) {
   DCHECK_EQ(GetState(), State::kConnecting);
   NETWORK_DVLOG(1) << this << " OnConnectionEstablished(" << protocol << ", "
                    << extensions << ")";
+  TRACE_EVENT_INSTANT1(
+      "devtools.timeline", "WebSocketReceiveHandshakeResponse",
+      TRACE_EVENT_SCOPE_THREAD, "data",
+      InspectorWebSocketEvent::Data(execution_context_, identifier_));
+  probe::DidReceiveWebSocketHandshakeResponse(execution_context_, identifier_,
+                                              handshake_request_.get(),
+                                              response.get());
+  handshake_request_ = nullptr;
+
   // From now on, we will detect mojo errors via |client_receiver_|.
   handshake_client_receiver_.reset();
   client_receiver_.Bind(

third_party/blink/renderer/modules/websockets/websocket_channel_impl.h

@@ -114,14 +114,13 @@ class MODULES_EXPORT WebSocketChannelImpl final
   // network::mojom::blink::WebSocketHandshakeClient methods:
   void OnOpeningHandshakeStarted(
       network::mojom::blink::WebSocketHandshakeRequestPtr) override;
-  void OnResponseReceived(
-      network::mojom::blink::WebSocketHandshakeResponsePtr) override;
   void OnConnectionEstablished(
       mojo::PendingRemote<network::mojom::blink::WebSocket> websocket,
       mojo::PendingReceiver<network::mojom::blink::WebSocketClient>
           client_receiver,
       const String& selected_protocol,
       const String& extensions,
+      network::mojom::blink::WebSocketHandshakeResponsePtr,
       mojo::ScopedDataPipeConsumerHandle readable) override;
 
   // network::mojom::blink::WebSocketClient methods:

third_party/blink/renderer/modules/websockets/websocket_channel_impl_test.cc

@@ -237,10 +237,14 @@ class WebSocketChannelImplTest : public PageTestBase {
     auto websocket = std::make_unique<TestWebSocket>(
         websocket_to_pass.InitWithNewPipeAndPassReceiver());
 
+    auto response = network::mojom::blink::WebSocketHandshakeResponse::New();
+    response->http_version = network::mojom::blink::HttpVersion::New();
+    response->status_text = "";
+    response->headers_text = "";
     handshake_client->OnConnectionEstablished(
         std::move(websocket_to_pass),
         client_remote.InitWithNewPipeAndPassReceiver(), selected_protocol,
-        extensions, std::move(readable));
+        extensions, std::move(response), std::move(readable));
     client->Bind(std::move(client_remote));
     return websocket;
   }

third_party/blink/web_tests/FlagExpectations/disable-site-isolation-trials

@@ -12,7 +12,6 @@ crbug.com/933880 external/wpt/FileAPI/url/url-with-xhr.any.html [ Failure ]
 crbug.com/933880 external/wpt/service-workers/service-worker/request-end-to-end.https.html [ Failure ]
 crbug.com/933880 http/tests/inspector-protocol/network/interception-take-stream.js [ Failure ]
 crbug.com/933880 http/tests/inspector-protocol/network/raw-headers-for-protected-document.js [ Failure ]
-crbug.com/933880 http/tests/inspector-protocol/network/raw-headers-for-websocket.js [ Failure ]
 crbug.com/933880 http/tests/inspector-protocol/network/security-info-on-response.js [ Failure ]
 crbug.com/933880 http/tests/inspector-protocol/network/xhr-interception-auth-fail.js [ Failure ]
 crbug.com/933880 http/tests/misc/redirect-to-about-blank.html [ Timeout ]

third_party/blink/web_tests/http/tests/inspector-protocol/network/raw-headers-for-websocket-expected.txt

-Tests that cross-domain WebSocket cookies are not reported.
-
-new WebSocket('ws://127.0.0.1:8000/inspector-protocol/network/resources/cookie.pl')
-Set-Cookie: name=value
-
-new WebSocket('ws://devtools.oopif.test:8000/inspector-protocol/network/resources/cookie.pl')
-Set-Cookie: undefined
-

third_party/blink/web_tests/http/tests/inspector-protocol/network/raw-headers-for-websocket.js

-(async function(testRunner) {
-  var {page, session, dp} = await testRunner.startURL(
-      'resources/cookie.pl',
-      `Tests that cross-domain WebSocket cookies are not reported.`);
-
-  await dp.Network.enable();
-
-  await testURL('ws://127.0.0.1:8000/inspector-protocol/network/resources/cookie.pl');
-  await testURL('ws://devtools.oopif.test:8000/inspector-protocol/network/resources/cookie.pl');
-  testRunner.completeTest();
-
-  async function testURL(url) {
-    session.evaluate(`
-      // The WebSocket handshake will fail but it doesn't matter.
-      window.ws = new WebSocket('${url}');`);
-    const response = await dp.Network.onceWebSocketHandshakeResponseReceived();
-    dump(url, response);
-  }
-
-  function dump(url, response) {
-    response = response.params.response;
-    testRunner.log(`\nnew WebSocket('${url}')`);
-    testRunner.log(`Set-Cookie: ${response.headers['Set-Cookie']}`);
-  }
-})

third_party/blink/web_tests/http/tests/inspector-protocol/websocket/handshake-response-expected.txt

+Tests DidReceiveWebSocketHandshakeResponse probe.
+
+new WebSocket('ws://localhost:8880/binary-frames')
+Set-Cookie: undefined
+Connection: Upgrade
+Upgrade: websocket
+

third_party/blink/web_tests/http/tests/inspector-protocol/websocket/handshake-response.js

+(async function(testRunner) {
+  var {page, session, dp} = await testRunner.startURL(
+      'resources/cookie.pl',
+      `Tests DidReceiveWebSocketHandshakeResponse probe.`);
+
+  await dp.Network.enable();
+
+  await testURL('ws://localhost:8880/binary-frames');
+  testRunner.completeTest();
+
+  async function testURL(url) {
+    session.evaluate(`window.ws = new WebSocket('${url}');`);
+    const value = await dp.Network.onceWebSocketHandshakeResponseReceived();
+    const headers = value.params.response.headers;
+    testRunner.log(`\nnew WebSocket('${url}')`);
+    dumpHeader(headers, 'Set-Cookie');
+    dumpHeader(headers, 'Connection');
+    dumpHeader(headers, 'Upgrade');
+  }
+
+  function dumpHeader(headers, property) {
+    testRunner.log(`${property}: ${headers[property]}`);
+  }
+})