Enable websocket filtering via SubresourceFilter

third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp

Index: third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
diff --git a/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp b/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
index 089e79056e6db6d4035b8c01e96af14484c6d5a6..c595f3157d39579b0cdbb01a17ec565e27c3d08a 100644
--- a/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
+++ b/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
@@ -33,12 +33,15 @@
 #include <memory>
 #include "core/dom/DOMArrayBuffer.h"
 #include "core/dom/ExecutionContext.h"
+#include "core/dom/TaskRunnerHelper.h"
 #include "core/fileapi/FileReaderLoader.h"
 #include "core/fileapi/FileReaderLoaderClient.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/LocalFrameClient.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "core/inspector/InspectorInstrumentation.h"
+#include "core/loader/DocumentLoader.h"
+#include "core/loader/SubresourceFilter.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/MixedContentChecker.h"
 #include "core/loader/ThreadableLoadingContext.h"
@@ -184,6 +187,22 @@ bool DocumentWebSocketChannel::connect(const KURL& url,
     protocol.split(", ", true, protocols);
   }
 
+  // If the connection needs to be filtered, asynchronously fail. Note that

[engedy, 2017/03/06 14:12:46]

nit: Let's make this comment more specific, as it leaves the reader wondering
what a `synchronous security error` is, and why subresource filtering cannot be
done synchronously.

It looks like we are just using this to avoid triggering throwing a
SecurityError in [1], as that code expects the only reason for synchronously
failing here would be because of Mixed Content checks? If so, could you please
elaborate on this specifically here?

1:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/websoc...

[Charlie Harrison, 2017/03/06 14:48:23]

It's based on that and based on the fact that we don't want to block the worker
thread by posting a message. I'll fix up the comment.

+  // returning "true" just indicates that this was not synchronous security
+  // error.
+  if (shouldDisallowConnection(url)) {
+    TaskRunnerHelper::get(TaskType::Networking, document())
+        ->postTask(
+            BLINK_FROM_HERE,
+            WTF::bind(
+                &DocumentWebSocketChannel::failWithClosureCode,
+                wrapPersistent(this), CloseEventCodePolicyViolation,

[engedy, 2017/03/06 14:12:46]

nit: Is this error code really appropriate here?

According to [1], code 1008 is sent by an endpoint in a `Close control frame` if
it has received a message that violates its policy.

Would code 1006 not be more appropriate? It is "designated for use in
applications expecting a status code to indicate that the connection was closed
abnormally, e.g., without sending or receiving a Close control frame."

1: https://tools.ietf.org/html/rfc6455#section-7.4

[Charlie Harrison, 2017/03/06 14:48:23]

Hm yeah I think you're right. Since we never open a connection I think 1006
might be the best option.

yhirano: WDYT?

+                String("Connection disallowed by the subresource filter"),
+                WarningMessageLevel,
+                WTF::passed(SourceLocation::create(String(), 0, 0, nullptr))));
+    return true;
+  }
+
   // TODO(kinuko): document() should return nullptr if we don't
   // have valid document/frame that returns non-empty interface provider.
   if (document() && document()->frame() &&
@@ -296,6 +315,15 @@ void DocumentWebSocketChannel::close(int code, const String& reason) {
 void DocumentWebSocketChannel::fail(const String& reason,
                                     MessageLevel level,
                                     std::unique_ptr<SourceLocation> location) {
+  return failWithClosureCode(CloseEventCodeAbnormalClosure, reason, level,
+                             std::move(location));
+}
+
+void DocumentWebSocketChannel::failWithClosureCode(
+    unsigned short code,
+    const String& reason,
+    MessageLevel level,
+    std::unique_ptr<SourceLocation> location) {
   NETWORK_DVLOG(1) << this << " fail(" << reason << ")";
   // m_handle and m_client can be null here.
 
@@ -314,7 +342,7 @@ void DocumentWebSocketChannel::fail(const String& reason,
     m_client->didError();
   // |reason| is only for logging and should not be provided for scripts,
   // hence close reason must be empty.
-  handleDidClose(false, CloseEventCodeAbnormalClosure, String());
+  handleDidClose(false, code, String());
   // handleDidClose may delete this object.
 }
 
@@ -681,6 +709,18 @@ void DocumentWebSocketChannel::didFailLoadingBlob(
   // |this| can be deleted here.
 }
 
+bool DocumentWebSocketChannel::shouldDisallowConnection(const KURL& url) {
+  if (!m_handle)

[engedy, 2017/03/06 14:12:46]

Can this ever evaluate to false? If not, let's make this a DCHECK.

[Charlie Harrison, 2017/03/06 14:48:23]

Done.

+    return false;
+  DocumentLoader* loader = document()->loader();
+  if (!loader)
+    return false;
+  SubresourceFilter* subresourceFilter = loader->subresourceFilter();
+  if (!subresourceFilter)
+    return false;
+  return !subresourceFilter->allowWebSocketConnection(url);
+}
+
 DEFINE_TRACE(DocumentWebSocketChannel) {
   visitor->trace(m_blobLoader);
   visitor->trace(m_messages);