Fixed #4 - Disable rewrite option for all but internal redirect

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -699,60 +699,55 @@
  * @param {string} [domains]
  *   Domains that the filter is restricted to, e.g. "foo.com|bar.com|~baz.com"
  * @param {boolean} [thirdParty]
  *   Defines whether the filter should apply to third-party or first-party
  *   content only
  * @param {string} [sitekeys]
  *   Public keys of websites that this filter should apply to
  * @param {?string} [rewrite]
- *   The (optional) rule specifying how to rewrite the URL. See
- *   RegExpFilter.prototype.rewrite.
- * @param {?string} [resourceName]
  *   The name of the internal resource to which to rewrite the
  *   URL. e.g. if the value of the <code>rewrite</code> parameter is
  *   <code>abp-resource:blank-html</code>, this should be
  *   <code>blank-html</code>.
  * @constructor
  * @augments ActiveFilter
  */
 function RegExpFilter(text, regexpSource, contentType, matchCase, domains,
-                      thirdParty, sitekeys, rewrite, resourceName)
+                      thirdParty, sitekeys, rewrite)
 {
   ActiveFilter.call(this, text, domains);
 
   if (contentType != null)
     this.contentType = contentType;
   if (matchCase)
     this.matchCase = matchCase;
   if (thirdParty != null)
     this.thirdParty = thirdParty;
   if (sitekeys != null)
     this.sitekeySource = sitekeys;
   if (rewrite != null)
     this.rewrite = rewrite;
-  if (resourceName)
-    this.resourceName = resourceName;
 
   if (regexpSource.length >= 2 &&
       regexpSource[0] == "/" &&
       regexpSource[regexpSource.length - 1] == "/")
   {
     // The filter is a regular expression - convert it immediately to
     // catch syntax errors
     let regexp = new RegExp(regexpSource.substr(1, regexpSource.length - 2),
                             this.matchCase ? "" : "i");
     Object.defineProperty(this, "regexp", {value: regexp});
   }
   else
   {
     // Patterns like /foo/bar/* exist so that they are not treated as regular
     // expressions. We drop any superfluous wildcards here so our optimizations
     // can kick in.
-    if (this.rewrite == null || this.resourceName)
+    if (this.rewrite)

[Manish Jethani, 2019/04/16 05:52:58]

This actually has a somewhat opposite of the intended effect.

The intention was: If either there is no `$rewrite` or `$rewrite` is
`abp-resource:`, then do this. After the changes here, if there is `$rewrite` it
will always be `abp-resource:`, consequently the check here is redundant now. We
can simply remove the `if` condition.

[hub, 2019/04/16 12:23:06]

Good catch.

       regexpSource = regexpSource.replace(/^\*+/, "").replace(/\*+$/, "");
 
     if (!this.matchCase && isLiteralPattern(regexpSource))
       regexpSource = regexpSource.toLowerCase();
 
     // No need to convert this filter to regular expression yet, do it on demand
     this.pattern = regexpSource;
   }
@@ -782,22 +777,19 @@
   /**
    * Regular expression to be used when testing against this filter
    * @type {RegExp}
    */
   get regexp()
   {
     let value = null;
 
-    let {pattern, rewrite, resourceName} = this;
-    if ((rewrite != null && !resourceName) || !isLiteralPattern(pattern))
-    {
-      value = new RegExp(filterToRegExp(pattern, rewrite != null),
-                         this.matchCase ? "" : "i");
-    }
+    let {pattern} = this;
+    if (!isLiteralPattern(pattern))
+      value = new RegExp(filterToRegExp(pattern), this.matchCase ? "" : "i");
 
     Object.defineProperty(this, "regexp", {value});
     return value;
   },
   /**
    * Content types the filter applies to, combination of values from
    * RegExpFilter.typeMap
    * @type {number}
@@ -837,30 +829,23 @@
 
     Object.defineProperty(
       this, "sitekeys", {value: sitekeys, enumerable: true}
     );
     return this.sitekeys;
   },
 
   /**
-   * The rule specifying how to rewrite the URL.
-   * The syntax is similar to the one of String.prototype.replace().
-   * @type {?string}
-   */
-  rewrite: null,
-
-  /**
    * The name of the internal resource to which to rewrite the
    * URL. e.g. if the value of the <code>rewrite</code> property is

[Manish Jethani, 2019/04/16 05:52:58]

I think we can replace `<code>rewrite</code> property` with
`<code>$rewrite</code> option` here because otherwise it would be wrong.

[hub, 2019/04/16 12:23:06]

Done.

    * <code>abp-resource:blank-html</code>, this should be
    * <code>blank-html</code>.
    * @type {?string}
    */
-  resourceName: null,
+  rewrite: null,
 
   /**
    * Tests whether the URL matches this filter
    * @param {string} location URL to be tested
    * @param {number} typeMask bitmask of content / request types to match
    * @param {string} [docDomain] domain name of the document that loads the URL
    * @param {boolean} [thirdParty] should be true if the URL is a third-party
    *                               request
@@ -989,17 +974,16 @@
   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 resourceName = null;
   let options;
   let match = text.includes("$") ? Filter.optionsRegExp.exec(text) : null;
   if (match)
   {
     options = match[1].split(",");
     text = match.input.substr(0, match.index);
     for (let option of options)
     {
@@ -1055,50 +1039,35 @@
             collapse = !inverse;
             break;
           case "sitekey":
             if (!value)
               return new InvalidFilter(origText, "filter_unknown_option");
             sitekeys = value.toUpperCase();
             break;
           case "rewrite":
-            if (value == null)
-              return new InvalidFilter(origText, "filter_unknown_option");
-            rewrite = value;
-            if (value.startsWith("abp-resource:"))
-              resourceName = value.substr("abp-resource:".length);
+            if (!value || !value.startsWith("abp-resource:"))

[Manish Jethani, 2019/04/16 05:52:58]

It doesn't matter _that_ much, but I'd just like to point out that this is an
additional change in behavior. A filter like `foo$rewrite` would given an
"unknown option" error previously, while now it would give a "invalid rewrite"
error. Maybe that's fine.

If we want to stick with the old behavior, then we can leave the check for
`value == null` as it is and check for `!value.startsWith(...)` as an additional
check.

[hub, 2019/04/16 12:23:06]

I think I'm gonna go the safe route. Change was intentional but maybe that's not
a good idea for this fix right now.

+              return new InvalidFilter(origText, "filter_invalid_rewrite");
+            rewrite = value.substr("abp-resource:".length);
             break;
           default:
             return new InvalidFilter(origText, "filter_unknown_option");
         }
       }
     }
   }
 
-  // For security reasons, never match $rewrite filters
-  // against requests that might load any code to be executed.
-  // Unless it is to an internal resource.
-  if (rewrite != null && !resourceName)
-  {
-    if (contentType == null)
-      ({contentType} = RegExpFilter.prototype);
-    contentType &= ~(RegExpFilter.typeMap.SCRIPT |
-                     RegExpFilter.typeMap.SUBDOCUMENT |
-                     RegExpFilter.typeMap.OBJECT |
-                     RegExpFilter.typeMap.OBJECT_SUBREQUEST);
-  }
-
   try
   {
     if (blocking)
     {
       if (csp && Filter.invalidCSPRegExp.test(csp))
         return new InvalidFilter(origText, "filter_invalid_csp");
 
-      if (resourceName)
+      if (rewrite)
       {
         if (text[0] == "|" && text[1] == "|")
         {
           if (!domains && thirdParty != false)
             return new InvalidFilter(origText, "filter_invalid_rewrite");
         }
         else if (text[0] == "*")
         {
@@ -1107,17 +1076,17 @@
         }
         else
         {
           return new InvalidFilter(origText, "filter_invalid_rewrite");
         }
       }
 
       return new BlockingFilter(origText, text, contentType, matchCase, domains,
-                                thirdParty, sitekeys, rewrite, resourceName,
+                                thirdParty, sitekeys, rewrite,
                                 collapse, csp);
     }
     return new WhitelistFilter(origText, text, contentType, matchCase, domains,
                                thirdParty, sitekeys);
   }
   catch (e)
   {
     return new InvalidFilter(origText, "filter_invalid_regexp");
@@ -1168,36 +1137,33 @@
  * @param {string} text see {@link Filter Filter()}
  * @param {string} regexpSource see {@link RegExpFilter RegExpFilter()}
  * @param {number} [contentType] see {@link RegExpFilter RegExpFilter()}
  * @param {boolean} [matchCase] see {@link RegExpFilter RegExpFilter()}
  * @param {string} [domains] see {@link RegExpFilter RegExpFilter()}
  * @param {boolean} [thirdParty] see {@link RegExpFilter RegExpFilter()}
  * @param {string} [sitekeys] see {@link RegExpFilter RegExpFilter()}
  * @param {?string} [rewrite]
- *   The (optional) rule specifying how to rewrite the URL. See
- *   RegExpFilter.prototype.rewrite.
- * @param {?string} [resourceName]
  *   The name of the internal resource to which to rewrite the
  *   URL. e.g. if the value of the <code>rewrite</code> parameter is
  *   <code>abp-resource:blank-html</code>, this should be
  *   <code>blank-html</code>.
  * @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
  * @constructor
  * @augments RegExpFilter
  */
 function BlockingFilter(text, regexpSource, contentType, matchCase, domains,
-                        thirdParty, sitekeys, rewrite, resourceName,
+                        thirdParty, sitekeys, rewrite,
                         collapse, csp)
 {
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
-                    thirdParty, sitekeys, rewrite, resourceName);
+                    thirdParty, sitekeys, rewrite);
 
   if (collapse != null)
     this.collapse = collapse;
 
   if (csp != null)
     this.csp = csp;
 }
 exports.BlockingFilter = BlockingFilter;
@@ -1220,30 +1186,17 @@
 
   /**
    * Rewrites an URL.
    * @param {string} url the URL to rewrite
    * @return {string} the rewritten URL, or the original in case of failure
    */
   rewriteUrl(url)
   {
-    if (this.resourceName)
-      return resourceMap.get(this.resourceName) || 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;
+    return resourceMap.get(this.rewrite) || url;
   }
 });
 
 /**
  * Class for whitelist filters
  * @param {string} text see {@link Filter Filter()}
  * @param {string} regexpSource see {@link RegExpFilter RegExpFilter()}
  * @param {number} [contentType] see {@link RegExpFilter RegExpFilter()}