Issue 6592 - Implement $rewrite filter option

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -766,16 +766,17 @@
 
   let contentType = null;
   let matchCase = null;
   let domains = null;
   let sitekeys = null;
   let thirdParty = null;
   let collapse = null;
   let csp = null;
+  let rewrite = null;
   let options;
   let match = (text.indexOf("$") >= 0 ? Filter.optionsRegExp.exec(text) : null);
   if (match)
   {
     options = match[1].split(",");
     text = match.input.substr(0, match.index);
     for (let option of options)
     {
@@ -813,30 +814,32 @@
       else if (option == "~THIRD_PARTY")
         thirdParty = false;
       else if (option == "COLLAPSE")
         collapse = true;
       else if (option == "~COLLAPSE")
         collapse = false;
       else if (option == "SITEKEY" && value)
         sitekeys = value.toUpperCase();
+      else if (option == "REWRITE" && value)
+        rewrite = value;
       else
         return new InvalidFilter(origText, "filter_unknown_option");
     }
   }
 
   try
   {
     if (blocking)
     {
       if (csp && Filter.invalidCSPRegExp.test(csp))
         return new InvalidFilter(origText, "filter_invalid_csp");
 
       return new BlockingFilter(origText, text, contentType, matchCase, domains,
-                                thirdParty, sitekeys, collapse, csp);
+                                thirdParty, sitekeys, collapse, csp, rewrite);
     }
     return new WhitelistFilter(origText, text, contentType, matchCase, domains,
                                thirdParty, sitekeys);
   }
   catch (e)
   {
     return new InvalidFilter(origText, "filter_invalid_regexp");
   }
@@ -889,27 +892,30 @@
  * @param {boolean} matchCase see RegExpFilter()
  * @param {string} domains see RegExpFilter()
  * @param {boolean} thirdParty see RegExpFilter()
  * @param {string} sitekeys see RegExpFilter()
  * @param {boolean} collapse
  *   defines whether the filter should collapse blocked content, can be null
  * @param {string} [csp]
  *   Content Security Policy to inject when the filter matches
+ * @param {string} [rewrite]
+ *   The rewrite expression
  * @constructor
  * @augments RegExpFilter
  */
 function BlockingFilter(text, regexpSource, contentType, matchCase, domains,
-                        thirdParty, sitekeys, collapse, csp)
+                        thirdParty, sitekeys, collapse, csp, rewrite)
 {
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
                     thirdParty, sitekeys);
 
   this.collapse = collapse;
   this.csp = csp;
+  this.rewrite = rewrite;
 }
 exports.BlockingFilter = BlockingFilter;
 
 BlockingFilter.prototype = extend(RegExpFilter, {
   type: "blocking",
 
   /**
    * Defines whether the filter should collapse blocked content.
@@ -917,17 +923,43 @@
    * @type {boolean}
    */
   collapse: null,
 
   /**
    * Content Security Policy to inject for matching requests.
    * @type {string}
    */
-  csp: null
+  csp: null,
+
+  /**
+   * The rewrite expression
+   * @type {string}
+   */
+  rewrite: null,
+
+  /**
+   * Perform the URL rewrite and check the origin.

[Manish Jethani, 2018/05/04 22:50:48]

Does the detail about checking the origin really need to be exposed in this way
in the documentation, especially in the title? I think we can just skip this
part, just say "Perform the URL rewrite".

Also if we follow the rest of the file it should be "Performs the URL rewrite".

Since the same filter will be applied to multiple URLs, it should really be
"Performs a URL rewrite".

In my opinion we can just make it "Rewrites a URL".

[hub, 2018/05/07 20:11:56]

Done.

+   * @param {string} urlString the string URL to rewrite

[Manish Jethani, 2018/05/04 22:50:48]

We can just say "the URL to rewrite"

Also shall we just call this parameter "url"? It's called "urlString" on the
extension side because there's already a "url" variable, otherwise it's usually
cleaner to just call it "url". Let me know what you think.

[hub, 2018/05/07 20:11:55]

"url" is fine by me. Done.

+   * @returns {string?} the rewritten URL, or the orginal in case of failure.

[Manish Jethani, 2018/05/04 22:50:49]

Nit: original URL

Also: (1) the type should be {string} now (no question mark); (2) let's make it
@return rather than @returns to be consistent with the rest of the file; (3)
let's drop the period at the end, again for consistency.

[hub, 2018/05/07 20:11:55]

Done.

+   */
+  rewriteUrl(urlString)
+  {
+    let rewritten = urlString.replace(this.regexp, this.rewrite);

[Manish Jethani, 2018/05/04 22:50:48]

Does this work with Unicode domains? For example https://i❤️.ws/ and the filter
"/(https?:\/\/i❤️\.ws\/js\/)(bundle\.js)$/$rewrite=$1example.js"

[Manish Jethani, 2018/05/05 00:51:38]

Never mind, it won't. I've filed Trac #6647 for this.

I've confirmed that it does work for normal Unicode characters. In practice this
should be enough, at least for now.

[Sebastian Noack, 2018/05/05 17:07:07]

How about this?

  let rewritten = new URL(urlString.replace(this.regexp, this.rewrite),
                          urlString);

That way if the result of the substitution is a relative URL, it will be
resolved relative to the original URL. This would also aid filters like that:

  ||example.com/foo^$rewrite=/bar

What do you think?

[Manish Jethani, 2018/05/05 23:58:39]

Yeah, I like the idea.

So it would be something like:

  let rewritten = urlString.replace(this.regexp, this.rewrite);
  if (new URL(rewritten, urlString).origin == new URL(urlString).origin)
    return rewritten;

[Manish Jethani, 2018/05/06 00:45:41]

Oh well, redirectUrl can't be relative. It'll have to be more like:

  let {href, origin} = new URL(urlString.replace(this.regexp, this.rewrite),
                               urlString);
  if (origin == new URL(urlString).origin)
    return href;

Unfortunately this means we'll be returning an IDN-encoded version of the
rewritten URL, and that's the one that'll appear in the DevTools panel. If we're
going to switch entirely to IDN encoding anyway [1], we might as well wait until
then for consistency. If we make the change later to support relative URLs it'll
still be backwards compatible with any existing filters.

Also if we're going to match against IDN-encoded URLs then this function can
accept the URL object itself (which already exists at the caller's end) so it
doesn't have to create a new one.

So it's going to be:

  let {href, origin} = new URL(url.href.replace(this.regexp, this.rewrite),
                               url);
  if (origin == url.origin)
    return href;

But this can be done later so for now we can just not support relative URLs
here.

[1]: https://issues.adblockplus.org/ticket/6647#comment:6

[Sebastian Noack, 2018/05/06 13:04:25]

As far as I can tell, if we return an IDN-encoded URL here, everything should
still work as expected. If its just for the devtools panel, where the
IDN-encoded rewritten URL might appear inconsistent, this is fine for the time
being, IMO, if we are going to switch entirely to IDN encoding anyway.

Alternatively, we could switch to IDN encoding entirely before introducing the
$rewrite filter option. Or we could convert the URL returned here, when it is
logged to the devtools panel in the calling code, for the time being.

[Manish Jethani, 2018/05/06 13:59:31]

Sorry, here if we accept a URL object as a parameter then we should also return
the new URL object. The caller can easily do String() on it or access the href
property explicitly. The return value is guaranteed to be non-null.

[Manish Jethani, 2018/05/06 13:59:31]

Yeah, after checking EasyList and EasyList China [1] at least I can say that
this would have no impact in practice. I'll see if I can run a script on all the
EasyList filter lists and maybe some of the other popular ones.

[1]: https://issues.adblockplus.org/ticket/6647#comment:8

[hub, 2018/05/07 20:11:55]

I'll leave it like that for now.

[Sebastian Noack, 2018/05/08 06:57:30]

Since we are on the verge of landing the change moving to punycode entirely,
anything that still holds us back from supporting relative URLs here?

[hub, 2018/05/08 15:54:30]

it is done now.

+    try
+    {
+      if (new URL(rewritten).origin == new URL(urlString).origin)

[Manish Jethani, 2018/05/04 22:50:48]

We should probably have a function in lib/common.js:

  function originsMatch(url1, url2)
  {
    try
    {
      return new URL(url1).origin == new URL(url2).origin;
    }
    catch (e)
    {
    }
  }

And then:

  if (!originsMatch(rewritten, url))
    return url;

  return rewritten;

[Manish Jethani, 2018/05/04 23:41:46]

I ran a test to see if separating it out into its own function has any effect on
performance. TL;DR negative.

Here's the test:

(function ()
{
  let randomString = () => Math.random().toString(32).substr(2);
  let randomURL = () => "https://" +
                        (Math.random() < .5 ? (randomString() + ".") : "") +
                        "example.com/foo";
                                             
  let array = new Array(1000000);            
  for (let i = 0; i < array.length; i++)
    array[i] = Math.random() < .5 ? ":" : randomURL();

  let count = 0;
  let start = performance.now();

  for (let i = 0; i < array.length; i++)
  {
    try
    {
      if (new URL("https://example.com/").origin == new URL(array[i]).origin)
        count++;
    }
    catch (error)
    {
    }
  }

  console.log("time:", performance.now() - start);
  console.log("count:", count);
})();

I ran this in the background page console on both Chrome and Firefox. Then I
declared the following function:

function originsMatch(url1, url2)
{ 
  try
  { 
    return new URL(url1).origin == new URL(url2).origin;
  }
  catch (error)
  {
  }
}

And then I modified the test to call this function instead. The result is that
it actually slows down a bit (but very slightly).

So I think the only benefit of creating a separate function is that it makes the
code "cleaner". I don't feel very strongly about this, it's your call.

[Sebastian Noack, 2018/05/05 13:34:43]

As long as we only need this functionality in one place, I'd rather keep the
logic here than moving it into a function (in some other module).

[hub, 2018/05/07 20:11:55]

agreed.

+        return rewritten;
+    }
+    catch (e)
+    {
+    }
+
+    return urlString;
+  }
 });
 
 /**
  * Class for whitelist filters
  * @param {string} text see Filter()
  * @param {string} regexpSource see RegExpFilter()
  * @param {number} contentType see RegExpFilter()
  * @param {boolean} matchCase see RegExpFilter()