Issue 6592 - Implement $rewrite filter option

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -765,16 +765,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)
     {
@@ -812,30 +813,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");
   }
@@ -888,27 +891,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

[kzar, 2018/05/15 14:08:58]

Mind making this comment useful? (Same below.)

[hub, 2018/05/15 16:16:31]

Done.

[kzar, 2018/05/16 09:22:40]

Thanks, looking way better now.

  * @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.
@@ -916,17 +922,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,
+
+  /**
+   * Rewrites an URL.
+   * @param {string} url the URL to rewrite
+   * @return {string} the rewritten URL, or the original in case of failure
+   */
+  rewriteUrl(url)
+  {
+    try
+    {
+      let rewrittenUrl = new URL(url.replace(this.regexp, this.rewrite), url);
+      if (rewrittenUrl.origin == new URL(url).origin)
+        return rewrittenUrl.href;
+    }
+    catch (e)
+    {
+    }
+
+    return url;

[kzar, 2018/05/15 14:24:57]

Why do we return `url` on failure? Shouldn't we return `false` or similar
instead? That would be cheaper to check in the onBeforeRequest listener. (Yea,
we'd also have to tweak the above logic to check that `url != rewrittenUrl`.)

[Sebastian Noack, 2018/05/15 14:40:34]

Even if no error is thrown and no origin change was detected, the URL might be
the same after the substitution. If we distinguish between those scenarios here,
we'd also have to handle them separately in the calling code.

[kzar, 2018/05/15 14:44:23]

Not if we tweak the above logic, so we don't return the rewritten URL if either
the origins don't match or if the URLs are identical.

[Sebastian Noack, 2018/05/15 14:51:02]

Sure, we could add another check here, in order to only return a string if it
differs from the original URL, and then in the calling code check for null
instead of checking whether the value changed. But I don't see how that makes
anything simpler (or faster).

[kzar, 2018/05/15 14:53:58]

Well it seems pointless to return the existing URL if the origin didn't match
(or replace threw) only to then compare that URL with the existing one
afterwards. But I guess it's not expected to be too common a case so I guess I
don't insist.

[Manish Jethani, 2018/05/15 15:18:12]

These are not the only cases in which the rewrite doesn't happen (the above
examples are failures), it could also be that there's a filter like
/(trackingId=)\d+/$rewrite=$10 (replace any tracking ID with 0), in which case
it would "fail" for "trackingId=0" (technically successful but nothing changed).
I think explicitly comparing the output with the input is a bit weird for this
case.

[hub, 2018/05/15 16:16:31]

The original idea is to not block the request in case of error. A gentle
failure.

+  }
 });
 
 /**
  * Class for whitelist filters
  * @param {string} text see Filter()
  * @param {string} regexpSource see RegExpFilter()
  * @param {number} [contentType] see RegExpFilter()
  * @param {boolean} [matchCase] see RegExpFilter()