Issue 7294 - Pass necessary functions to strip-fetch-query-parameter

lib/content/snippets.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 843 matching lines @@
 
 /**
  * Strips a query string parameter from <code>fetch()</code> calls.
  *
  * @param {string} name The name of the parameter.
  * @param {?string} [urlPattern] An optional pattern that the URL must match.
  */
 function stripFetchQueryParameter(name, urlPattern = null)
 {
   let fetch_ = window.fetch;
+  if (typeof fetch_ != "function")

[Manish Jethani, 2019/02/21 08:15:43]

Being defensive, just in case fetch does't exist (for whatever reason). It
does't hurt.

+    return;
+
   let urlRegExp = urlPattern ? toRegExp(urlPattern) : null;
   window.fetch = function fetch(...args)
   {
     let [source] = args;
     if (typeof source == "string" &&
         (!urlRegExp || urlRegExp.test(source)))
     {
       let url = new URL(source);
       url.searchParams.delete(name);
       args[0] = url.href;
     }
-    return fetch_.apply(this, args);
+
+    return (() => {}).apply.call(fetch_, this, args);

[Manish Jethani, 2019/02/21 08:15:43]

This is because you can have a function with a null prototype. Again, just being
defensive. We don't want our snippets to break any web pages.

[a.giammarchi, 2019/02/21 08:27:35]

IMO this is super ugly and it doesn't prevent possible issues via
Function.prototype.apply overrides.

I believe we should stick with the standard and use `return fetch_(...args);`
because `fetch`, like any other global context, doesn't need a context to
execute so it'd be silly to expect one.

Moreover, the fetch *we* trap, doesn't want to pass through any context 'cause
it's supposed to be the original global one which already acts like that.

If you disagree, at least please consider using `fetch.apply.call(fetch, this,
args)` so that no runtime arrow is needed each time, but again, any
Function.prototype.apply override would easily catch that, no matter if it's an
Arrow or not.

Alternatively, we could secure execution through call/apply traps
http://webreflection.blogspot.com/2011/10/bind-apply-and-call-trap.html
but that requires some shared utility which means bigger refactory.

[Manish Jethani, 2019/02/21 08:57:16]

This is not about overrides but rather about the possibility that the prototype
can be null.

  function fetch() {
    // Custom implementation of fetch
  }
  fetch.__proto__ = null; // For some reason the developer decided to null this
out

Our code would bomb here, unless we use the original version of apply.
What if the fetch on the page is a custom implementation? How do we know that
it's the standard one? Who are we to decide how the fetch function should work?
Our wrapper should simply pass on the this, even if it's null or undefined. It
should be transparent.
I do disagree but I also think this part of the change is unrelated. I can
revert this part. Let me do that.

[Manish Jethani, 2019/02/21 09:00:36]

Done.

   };
 }
 
-exports["strip-fetch-query-parameter"] =
-  makeInjector(stripFetchQueryParameter);
+exports["strip-fetch-query-parameter"] = makeInjector(stripFetchQueryParameter,
+                                                      toRegExp, regexEscape);