Enable websocket filtering via SubresourceFilter

third_party/WebKit/Source/core/loader/SubresourceFilter.cpp

Index: third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
diff --git a/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp b/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
index fe3c77e3abe26747c54b368c3344455a9011c1bf..b9aadfc839bef83d916818b5855708ff6c6dc7cd 100644
--- a/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
+++ b/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
@@ -4,8 +4,11 @@
 
 #include "core/loader/SubresourceFilter.h"
 
+#include "core/dom/TaskRunnerHelper.h"
 #include "core/loader/DocumentLoader.h"
-#include "public/platform/WebDocumentSubresourceFilter.h"
+#include "platform/WebTaskRunner.h"
+#include "platform/weborigin/KURL.h"
+#include "public/platform/WebTraceLocation.h"
 
 namespace blink {
 
@@ -32,20 +35,41 @@ bool SubresourceFilter::allowLoad(
   // Pair<url string, context> -> LoadPolicy.
   WebDocumentSubresourceFilter::LoadPolicy loadPolicy =
       m_subresourceFilter->getLoadPolicy(resourceUrl, requestContext);
-  if (reportingPolicy == SecurityViolationReportingPolicy::Report) {
-    switch (loadPolicy) {
-      case WebDocumentSubresourceFilter::Allow:
-        break;
-      case WebDocumentSubresourceFilter::Disallow:
-        m_subresourceFilter->reportDisallowedLoad();
-      // fall through
-      case WebDocumentSubresourceFilter::WouldDisallow:
-        m_documentLoader->didObserveLoadingBehavior(
-            WebLoadingBehaviorSubresourceFilterMatch);
-        break;
-    }
-  }
+  if (reportingPolicy == SecurityViolationReportingPolicy::Report)
+    notifyLoad(resourceUrl, loadPolicy);
+  return loadPolicy != WebDocumentSubresourceFilter::Disallow;
+}
+
+bool SubresourceFilter::allowWebSocketConnection(const KURL& resourceUrl) {
+  // TODO(csharrison): Should probably have a new API for this in
+  // WebDocumentSubresourceFilter rather than sending subresource context.

[pkalinnikov, 2017/03/02 12:33:53]

Right, there is no RequestContext for WebSocket. We would need to do either of:

1. Add a getLoadPolicyForWebSocket(url) method to WebDocumentSubresourceFilter.
2. Introduce a SubresourceFilter-specific "resource type", similar to the one in
the CL https://codereview.chromium.org/2700553002. The type would be a short
version of RequestContext, with some types merged together. Plus, it would
include the WebSocket type. I would have the same values as
s_f::proto::ElementType.
3. Move the WebRequestResourceType (from the CL in option 2) to content/
(probably rename it), and use it just like in option 2.

The 2 and 3 seem more canonical, but the 1 is shorter. I don't have preferences
for now. Charles, WDYT?

[Charlie Harrison, 2017/03/02 14:48:01]

I slightly prefer (1), but (2) is also ok to me, though I hate adding an extra
layer of abstraction when the alternative is not so bad.

+  WebDocumentSubresourceFilter::LoadPolicy loadPolicy =
+      m_subresourceFilter->getLoadPolicy(
+          resourceUrl, WebURLRequest::RequestContextSubresource);

[pkalinnikov, 2017/03/02 12:33:53]

Using this type temporarily looks good. Note that this means that the websocket
type becomes equivalent to OTHER, although it's just a subset of those. We need
a separate type in a follow-up CL (see options above). We would also need a new
proto::ElementType to be able to create rules targeting this specific type.

[Charlie Harrison, 2017/03/02 14:48:00]

Acknowledged.

+
+  // Post a task to notify this load to avoid unduly blocking the worker
+  // thread.
+  TaskRunnerHelper::get(TaskType::Networking, m_documentLoader->frame())

[pkalinnikov, 2017/03/02 12:33:53]

Is this intended that allowWebSocketConnection reports (via notifyLoad) always,
but allowLoad reports only if the |reportingPolicy| permits to do so? Shouldn't
this be consistent?

[Charlie Harrison, 2017/03/02 14:48:01]

This was intentional. The reason is that ReportingPolicy basically stems from
whether or not the resource sent to allowLoad is a speculative preload
(generated by the preload scanner). We don't really have speculative websocket
connections so we should always report.

I've added a comment to clarify this.

+      ->postTask(BLINK_FROM_HERE,
+                 WTF::bind(&SubresourceFilter::notifyLoad, wrapPersistent(this),
+                           resourceUrl, loadPolicy));
   return loadPolicy != WebDocumentSubresourceFilter::Disallow;
 }
 
+void SubresourceFilter::notifyLoad(
+    const KURL& resourceUrl,
+    WebDocumentSubresourceFilter::LoadPolicy loadPolicy) {
+  switch (loadPolicy) {
+    case WebDocumentSubresourceFilter::Allow:
+      break;
+    case WebDocumentSubresourceFilter::Disallow:
+      m_subresourceFilter->reportDisallowedLoad();
+    // fall through
+    case WebDocumentSubresourceFilter::WouldDisallow:
+      m_documentLoader->didObserveLoadingBehavior(
+          WebLoadingBehaviorSubresourceFilterMatch);
+      break;
+  }
+}
+
 }  // namespace blink