Issue 6622 - Implement $rewrite filter option

lib/requestBlocker.js

Index: lib/requestBlocker.js
===================================================================
--- a/lib/requestBlocker.js
+++ b/lib/requestBlocker.js
@@ -65,20 +65,21 @@
 
   for (let type in browser.webRequest.ResourceType)
     yield resourceTypes.get(browser.webRequest.ResourceType[type]) || "OTHER";
 
   // WEBRTC gets addressed through a workaround, even if the webRequest API is
   // lacking support to block this kind of a request.
   yield "WEBRTC";
 
-  // POPUP, CSP and ELEMHIDE filters aren't mapped to resource types.
+  // POPUP, CSP, REWRITE and ELEMHIDE filters aren't mapped to resource types.

[Sebastian Noack, 2018/05/02 15:04:59]

Nit: As this list growth, listing those filter options redundantly in the
comment doesn't seem to add much value. How about changing the comment to "These
filter types aren't mapped to resource types."?

[hub, 2018/05/03 01:18:02]

Done.

   yield "POPUP";
   yield "ELEMHIDE";
   yield "CSP";
+  yield "REWRITE";
 }());
 
 function getDocumentInfo(page, frame, originUrl)
 {
   return [
     extractHostFromFrame(frame, originUrl),
     getKey(page, frame, originUrl),
     !!checkWhitelisted(page, frame, originUrl,
@@ -189,22 +190,37 @@
     return;
 
   let type = resourceTypes.get(details.type) || "OTHER";
   let [docDomain, sitekey, specificOnly] = getDocumentInfo(page, frame,
                                                            originUrl);
   let [filter, urlString, thirdParty] = matchRequest(url, type, docDomain,
                                                      sitekey, specificOnly);
 
-  getRelatedTabIds(details).then(tabIds =>
+  let log = logType => getRelatedTabIds(details).then(tabIds =>
   {
-    logRequest(tabIds, urlString, type, docDomain,
+    logRequest(tabIds, urlString, logType, docDomain,
                thirdParty, sitekey, specificOnly, filter);
   });
 
+  if (filter instanceof BlockingFilter && filter.rewrite)

[Manish Jethani, 2018/04/30 20:24:58]

I think maybe rewrite should be a separate module in the extension, similar to
lib/csp.js. REWRITE filters should be looked up specifically, and only if the
URL isn't already blocked (so the onBeforeRequest handler for rewrite filters
should be added afterwards).

[hub, 2018/05/01 18:32:36]

This is a blocking filter with the rewrite option. Doing as you suggest would
require another call to matchRequest() which is IMHO costly (but necessary).

[Sebastian Noack, 2018/05/02 15:04:59]

I agree that having the logic here is fine. Otherwise, we have to run some logic
(whitelisting checks, etc.) twice on every request. In case of CSP, this is less
an issue since the onBeforeRequest handler there is only registered for requests
of the type main_frame and sub_frame, besides the logic there diverges even more
from what we are doing here.

[Manish Jethani, 2018/05/02 16:13:45]

Suppose there are two filters:

  /([^/]+\/)?bar$/$rewrite=lambda
  /foo/

The URL https://example.com/foo/bar would get rewritten to
https://example.com/lambda instead of getting blocked. Is this OK? There's no
way to add an exception based on the URL itself.

Furthermore, because filters are sorted in random order (see
lib/filterListener.js), it will not be possible to predict whether the URL will
get rewritten or blocked. This is not going to be fun.

[Sebastian Noack, 2018/05/02 16:40:09]

If we want to give precedence to blocking filters over $rewrite filters, we
could do another filter match without REWRITE in the type mask if we found a
$rewrite filter here. Still better than running the whole logic again in another
onBeforeRequest listener.

But I'm inclined to just accept undefined behavior in case of conflicting
$rewrite and blocking filters, for now (in favor of a simpler and more efficient
implementation). I wouldn't insist though. Dave, what do you think?

[Manish Jethani, 2018/05/02 17:45:50]

Yeah, I think that's fair.

I think defaultMatcher.matchesAny will have to take another argument called
"rewrites": if it's true, the matcher will return a rewrite filter or null,
otherwise a non-rewrite filter or null.

Like in the _checkEntryMatch function:

  _checkEntryMatch(keyword, location, typeMask, docDomain, thirdParty, sitekey,
                   specificOnly, rewrites = false)
  {
    ...

    if (!!filter.rewrite == rewrites &&
        filter.matches(location, typeMask, docDomain, thirdParty, sitekey))
    {
      return filter;
    }

    ...
  }

Maybe the values in filterByKeyword should be two lists (rewrite and
non-rewrite) instead of a single list.

[hub, 2018/05/03 01:18:02]

I'm trying to actually keep it simple here.

[Manish Jethani, 2018/05/03 02:51:16]

I'm OK with letting the filters with a rewrite option compete with other
filters, but it will be difficult to debug (especially for filter authors) when
a rewrite filter doesn't work sometimes (and only sometimes).

But yeah in the interest of keeping things simple I'm OK with this.

[Sebastian Noack, 2018/05/03 15:55:47]

I don't quite get what we need another argument for. Can't we control through
the typeMask whether $rewrite filters should be considered or not?

[Manish Jethani, 2018/05/04 10:01:59]

If the typeMask doesn't contain REWRITE it'll still match on other bases, like
SCRIPT or STYLESHEET. If we want to say explicitly that it shouldn't match
rewrite filters we need an additional parameter.

For example a filter may be $script,rewrite=$1, it'll match any script requests.

[Sebastian Noack, 2018/05/04 15:17:16]

Gotya. In fact REWRITE isn't even a flag in the type mask which makes sense. So
yeah, if we want to enable filter matches ignoring $rewrite filters, in order to
give precedence to blocking filters, we'd need a new argument in matchesAny().
But I'm still inclined to keep things simple for now, and accept/document
undefined behavior in case of conflicting filters. If this should become a
problem in the future we can revisit it then.

[Manish Jethani, 2018/05/04 15:37:47]

Makes sense.

+  {
+    let rewritten = filter.doRewrite(urlString);
+    if (rewritten && rewritten != urlString)
+    {
+      log("REWRITE");
+
+      return {redirectUrl: rewritten};
+    }
+    // we couldn't do the rewrite, so just let it through.
+    return;
+  }
+
+  log(type);

[Sebastian Noack, 2018/05/02 15:04:59]

I'd rather move the logic around so that we don't need that log() function.

  getRelatedTabIds(details).then(tabIds =>
  {
    logRequest(tabIds, urlString, type, docDomain,
               thirdParty, sitekey, specificOnly, filter);
  });

  if (filter instanceof BlockingFilter)
  {
    if (filter.rewrite)
    {
      ...
    }

    return {cancel: true};
  }

[hub, 2018/05/03 01:18:03]

Done in a different way.

+
   if (filter instanceof BlockingFilter)
     return {cancel: true};
 }, {urls: ["<all_urls>"]}, ["blocking"]);
 
 port.on("filters.collapse", (message, sender) =>
 {
   let {page, frame} = sender;