Issue 5283 - Add support for $websocket and $webrtc

test/abp2blocklist.js

Index: test/abp2blocklist.js
===================================================================
--- a/test/abp2blocklist.js
+++ b/test/abp2blocklist.js
@@ -79,27 +79,27 @@
     testRules(test, ["test.com#@#whatever", "##whatever"], []);
     testRules(test, ["~test.com##whatever"], []);
 
     test.done();
   },
 
   testRequestFilters: function(test)
   {
-    testRules(test, ["/foo", "||test.com", "http://example.com/foo"], [
+    testRules(test, ["/foo", "||test.com", "|http://example.com/foo"], [

[Manish Jethani, 2017/05/31 02:51:43]

We can talk about this. As I said earlier, I don't see why the string "http" at
the beginning of the filter text should be treated as a special case. For
example, "httpseverywhere.com" will also match "ftp://httpseverywhere.com" but
it shouldn't.

[Sebastian Noack, 2017/05/31 07:09:26]

A pattern of "http://example.com" should match any URL that contains this
substring. This is what Adblock Plus (without Content Blockers) does, and some
filter lists may rely on this behavior.

However, "example.com" should not match "ftp://example.com" or
"file:///home/user/example.com/foo.txt", as Adblock Plus is used to ignore these
protocols, since anything potentially blocked in a local document or one
downloaded via FTP is usually undesired.

Currently this is achieved by prefixing such filters with "^https?://.*". But if
the protocol is already given in the filter we have to omit the additional
prefix because we'd otherwise translate filters like "http://example.com" to
"ĥttps?://.*https://example.com" which would no longer match http://example.com.

I see how this is a problem now where we want to consider WebSockets, also
because we cannot just use "^(http|ws)s?://.*" as there is no alternation in
regular expressions used by Content Blockers. Also this workaround wasn't
ultimately accurrate as in case of filters like "h*://example.com" or
"s://example" we'd still add a redundant protocol part.

So rather than always adding a protocol part to filters without implied
beginning, we should never do so. But the question still is what is about ftp://
and file:// URLs. Does they go through the Content Blocker anyway? I actually
never confirmed that, so perhaps this workaround was redundant from the
beginning. Otherwise, there is also the option to just add an exception rule for
ftp:// and/or file:// URLs to the generated lists.

[Manish Jethani, 2017/05/31 07:22:46]

I see what you mean. In that case we should check for "https?://" and not "http"
to be more specific.

Also I think we shouldn't block HTTP URLs for a filter that's supposed to
specifically block a WebSocket URL, for example (this would be especially
problematic for filters without a hostname part). I think it's a good idea to
continue adding the URL scheme. We're almost there, I can handle the case where
the scheme is already part of the URL pattern (i.e. the above case).

Let me post another patch.

[Sebastian Noack, 2017/05/31 08:05:46]

How about this:

* If the filter neither starts with | nor contains ://
  * If the typeMask includes neither WEBSOCKET not WEBRTC
    * Generate a single rule prefixed with ^https?://.*
  * If the typeMask includes only WEBSOCKET
    * Generate a single rule prefixed with ^wss?://.*
  * If the typeMask includes only WEBRTC
    * Generate 2 rules, prefixed with: ^stuns?:.*, ^turns?:.*
  * If the typeMask includes WEBSOCKET and any type other than WEBRTC
    * Generate 2 rules, prefixed with: ^wss?://.*, ^https?://.*
  * If the typeMask includes WEBRTC and any type other than WEBSOCKET
    * Generate 3 rules, prefixed with: ^stuns?:.*, ^turns?:.*, ^https?://.*
  * If the typeMask includes WEBSOCKET, WEBRTC and any other type
    * Generate a single rule without addional protocol handler
* Else
  * Generate a single rule without addional protocol handler

And in case file:// and/or ftp:// is an issue, we just add exception rules.

[Manish Jethani, 2017/06/02 08:02:36]

These rules are both complicated and wrong. For example, the URL scheme for
WebRTC does not include the two slashes.
We would have to split the resource types here. e.g. "raw" for the WebRTC rule
and other types for the non-WebRTC rule. We can do this, I even did it at first,
but do we really need this? It's unnecessarily complicated. Let's call this a
limitation of this program, but we can simply not support this type of filter,
at least for now. i.e. if you mix WebSocket or WebRTC with other types, we only
generate a rule for the other types. I think this is OK, we can improve on this
later if we need to.
Then it would match any URL scheme, which may be fine in practice, it's just
weird to do this only sometimes and not other times.

I have a much better solution with my latest patch.
I think we should try to do as accurate a translation of ABP filters to content
blocker rules as we can. This may make the code more complicated but it would be
worth it.

[Sebastian Noack, 2017/06/02 10:58:50]

Note that "raw" is for all requests initiated by code, this includes
XMLHTTPREQUEST, WEBSOCKET and WEBRTC. However, if we prefix the pattern with
^wss?://.*, ^stuns?:.* or ^turns?:.* we could potentially even omit the resource
type because the URL pattern already implies a subset of "raw". Similarly when a
filter doesn't use any type options, and therefore should match all resource
types, we can just omit the resource type along the protocol prefix as well. So
the only scenario in which we'd further have to split the rules are filters like
||example.com$websocket,image. These are rare enough that I wouldn't mind the
addional rules produced, on the other hand, if this will get too complicated a
limitation here might be acceptable too. But filters with no type option are
quite common, and should be applied to WEBSOCKET and WEBRTC as well.
 
Well, Adblock Plus generally ignores the protocol when matching filters which
don't explicitly include the protocol part. But non-HTTP/WS/RTC URLs are ignored
before filter matching takes place. So IMO ignoring the protocol here as well,
and then ignore these protocols by an exception rule which we add automatically,
doesn't seem any less accurrate to me.

[Manish Jethani, 2017/07/03 18:34:52]

Makes sense, done.
Alright, I agree.

       {trigger: {"url-filter": "^https?://.*/foo",
                  "resource-type": ["image", "style-sheet", "script", "font",
                                    "media", "raw", "document"]},
        action: {type: "block"}},
       {trigger: {"url-filter": "^https?://([^/]+\\.)?test\\.com",
                  "url-filter-is-case-sensitive": true,
                  "resource-type": ["image", "style-sheet", "script", "font",
                                    "media", "raw", "document"]},
        action: {type: "block"}},
-      {trigger: {"url-filter": "http://example\\.com/foo",
+      {trigger: {"url-filter": "^http://example\\.com/foo",
                  "resource-type": ["image", "style-sheet", "script", "font",
                                    "media", "raw", "document"]},
        action: {type: "block"}}
     ]);
 
     testRules(test, ["||example.com"], [
       {trigger: {"url-filter": "^https?://([^/]+\\.)?example\\.com",
                  "url-filter-is-case-sensitive": true,
@@ -209,28 +209,32 @@
   },
 
   testRequestTypeMapping: function(test)
   {
     testRules(
       test,
       ["1", "2$image", "3$stylesheet", "4$script", "5$font", "6$media",
        "7$popup", "8$object", "9$object_subrequest", "10$xmlhttprequest",
-       "11$ping", "12$subdocument", "13$other", "14$IMAGE",
-       "15$script,PING,Popup", "16$~image"],
+       "11$websocket", "12$webrtc",
+       "13$ping", "14$subdocument", "15$other", "16$IMAGE",
+       "17$script,PING,Popup", "18$~image"],
       [["image", "style-sheet", "script", "font", "media", "raw", "document" ],
        ["image"],
        ["style-sheet"],
        ["script"],
        ["font"],
        ["media"],
        ["popup"],
        ["media"],
        ["raw"],
        ["raw"],
+       ["raw"], // WebSocket
+       ["raw"], // WebRTC: STUN
+       ["raw"], // WebRTC: TURN
        ["raw"],
        ["document"],
        ["raw"],
        ["image"],
        ["script", "popup", "raw" ],
        ["style-sheet", "script", "font", "media", "raw", "document"]],
       rules => rules.map(rule => rule.trigger["resource-type"])
     );
@@ -269,10 +273,32 @@
   testUnicode: function(test)
   {
     testRules(test, ["$domain=🐈.cat"], ["*xn--zn8h.cat"],
               rules => rules[0]["trigger"]["if-domain"]);
     testRules(test, ["🐈$domain=🐈.cat"], []);
     testRules(test, ["###🐈"], []);
 
     test.done();
+  },
+
+  testWebSocket: function(test)
+  {
+    testRules(test, ["foo$websocket"], [
+      {trigger: {"url-filter": "^wss?://.*foo", "resource-type": ["raw"]},
+       action: {type: "block"}}
+    ]);
+
+    test.done();
+  },
+
+  testWebRTC: function(test)
+  {
+    testRules(test, ["foo$webrtc"], [
+      {trigger: {"url-filter": "^stuns?:.*foo", "resource-type": ["raw"]},
+       action: {type: "block"}},
+      {trigger: {"url-filter": "^turns?:.*foo", "resource-type": ["raw"]},
+       action: {type: "block"}}
+    ]);
+
+    test.done();
   }
 };