Issue 6969 - Implement abort-on-property-read snippet

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 34 matching lines @@
  */
 function toRegExp(pattern)
 {
   if (pattern.length >= 2 && pattern[0] == "/" &&
       pattern[pattern.length - 1] == "/")
   {
     return new RegExp(pattern.substring(1, pattern.length - 1));
   }
 
   return new RegExp(regexEscape(pattern));
+}
+
+/**
+ * Returns the value of the <code>cssText</code> property of the object
+ * returned by <code>getComputedStyle</code> for the given element. If the
+ * value of the <code>cssText</code> property is blank, this function computes
+ * the value out of the properties available in the object.
+ *
+ * @param {Element} element The element for which to get the computed CSS text.
+ *
+ * @returns {string} The computed CSS text.
+ */
+function getComputedCSSText(element)
+{
+  let style = getComputedStyle(element);
+  let {cssText} = style;
+
+  if (cssText)
+    return cssText;
+
+  for (let property of style)
+    cssText += `${property}: ${style[property]}; `;
+
+  return cssText.trim();
 }
 
 /**
  * Injects JavaScript code into the document using a temporary
  * <code>script</code> element.
  *
  * @param {string} code The code to inject.
  * @param {Array.<function|string>} [dependencies] A list of dependencies
  *   to inject along with the code. A dependency may be either a function or a
  *   string containing some executable code.
@@ skipping 295 matching lines @@
 
   let styleRegExp = style ? toRegExp(style) : null;
   let searchStyleRegExp = searchStyle ? toRegExp(searchStyle) : null;
 
   new MutationObserver(() =>
   {
     for (let element of document.querySelectorAll(searchSelector))
     {
       if (searchRegExp.test(element.textContent) &&
           (!searchStyleRegExp ||
-           searchStyleRegExp.test(getComputedStyle(element).cssText)))
+           searchStyleRegExp.test(getComputedCSSText(element))))
       {
         let closest = element.closest(selector);
         if (closest && (!styleRegExp ||
-                        styleRegExp.test(getComputedStyle(closest).cssText)))
+                        styleRegExp.test(getComputedCSSText(closest))))
         {
           hideElement(closest);
         }
       }
     }
   })
   .observe(document, {childList: true, characterData: true, subtree: true});
 }
 
 exports["hide-if-contains-and-matches-style"] = hideIfContainsAndMatchesStyle;
@@ skipping 45 matching lines @@
     for (let i = 0; i < times; i++)
       object + "";
 
     if (typeof dir == "function")
       dir.call(this, object);
   };
 }
 
 exports["dir-string"] = makeInjector(dirString);
 
+/**
+ * Prevents the execution of inline scripts.
+ *
+ * Only on Firefox.
+ *
+ * @param {string} [search] If specified, only scripts containing the given
+ *   string are prevented from executing. If the string begins and ends with a
+ *   slash (<code>/</code>), the text in between is treated as a regular
+ *   expression.
+ * @param {string} [selector] If specified, only scripts whose HTML element
+ *   matches the given CSS selector are prevented from executing.
+ */
+function preventInlineScripts(search = null, selector = null)
+{
+  let re = search ? toRegExp(search) : null;
+
+  document.addEventListener("beforescriptexecute", event =>
+  {
+    let {target} = event;
+
+    if ((!selector || target.matches(selector)) &&
+        (!re || re.test(target.textContent)))
+    {
+      event.preventDefault();
+    }
+  },
+  true);
+}
+
+exports["prevent-inline-scripts"] = preventInlineScripts;
+
+/**
+ * Generates a random alphanumeric ID consisting of 6 base-36 digits
+ * from the range 100000..zzzzzz (both inclusive).
+ *
+ * @returns {string} The random ID.
+ */
 function randomId()
 {
   // 2176782336 is 36^6 which mean 6 chars [a-z0-9]

[Manish Jethani, 2018/09/24 11:08:35]

Since we're not zero-padding the return value, does the number of characters
here have any significance at all?

I mean:

  (123).toString(36)

Gives `3f`. It's not 6 characters.

I think we should just do something like
`Math.random().toString(36).substring(2)`

[hub, 2018/09/25 04:28:52]

Modifying the formula so that if Math.random() returns 0 we still get 6
characters.

[Manish Jethani, 2018/09/25 12:25:22]

The updated version looks alright to me, it always generates a string that is 6
characters long, but I still wonder why it must be 6 characters only.

[hub, 2018/09/25 15:40:43]

6 is arbitrary. Long enough to be unique, but still short enough.

-  return Math.floor(Math.random() * 2176782336).toString(36);
-}
-
-/**
- * Will patch a property on the window object to abort when read.
- * It will intercept the onerror callback and block it if tagged.
- *
- * @todo handle properties of properties.
- *
- * @param {string} prop the name of the property.
- */
-function abortOnPropertyRead(...props)

[Manish Jethani, 2018/09/24 11:08:35]

The problem with accepting a variable number of arguments here is that we can't
extend the function to add more arguments that specify some conditions. It would
also break compatibility with uBO, so that if uBO adds an additional argument we
won't be able to add it for compatibility.

So I think we should just keep it compatible with uBO and take only one
argument.

[hub, 2018/09/25 04:28:52]

I didn't have uBO compatibility in mind. I just allow a list of properties as an
argument as to be able to just group all in one filter.

This was a suggestion from Amr. But I'm ok to revert to one propertie per
invocation if you think it's better.

[Manish Jethani, 2018/09/25 12:25:22]

The thing is that the moment ABP starts using the technique that works for uBO,
they will circumvent it, and uBO may respond with an updated snippet. Then we'll
also have to implement the updated technique, which might have additional
parameters. Since the snippet has the same name as the uBO version, it would be
better to just stay fully compatible.

So I think at least for now it's better to just keep it at a single parameter.

[hub, 2018/09/25 15:40:43]

fair enough. will fix it.

[Manish Jethani, 2018/10/22 14:21:01]

Happy to proceed whenever you're ready.

[hub, 2018/10/22 14:52:49]

This was fixed in Patch Set 5

[Manish Jethani, 2018/10/22 20:21:54]

Sorry, I missed this. I'll look at this patch tomorrow for sure.

-{
-  if (props.length == 0)
+  // 60466176 is 36^5
+  // 2176782336 - 60466176 = 2116316160. This ensure to always have 6
+  // chars even if Math.random() returns its minimum value 0.0
+  //
+  return Math.floor(Math.random() * 2116316160 + 60466176).toString(36);
+}
+
+function wrapPropertyAccess(object, property, descriptor)
+{
+  let currentDescriptor = Object.getOwnPropertyDescriptor(object, property);
+  if (currentDescriptor && !currentDescriptor.configurable)
+    return false;
+
+  Object.defineProperty(object, property, descriptor);
+  return true;
+}
+
+/**
+ * Patches a property on the window object to abort execution when the
+ * property is read.
+ *
+ * No error is be printed to the console.
+ *
+ * The idea originates from
+ * {@link https://github.com/uBlockOrigin/uAssets/blob/80b195436f8f8d78ba713237bfc268ecfc9d9d2b/filters/resources.txt#L1703 uBlock Origin}.
+ *
+ * @param {string} property The name of the property.
+ */
+function abortOnPropertyRead(property)
+{
+  if (!property)
     return;
 
-  let magic = randomId();
-
-  let abort = function()
-  {
-    throw new ReferenceError(magic);
-  };
+  let rid = randomId();
+
+  function abort()
+  {
+    throw new ReferenceError(rid);
+  }
 
   let {onerror} = window;
-  window.onerror = (msg, ...args) =>

[Manish Jethani, 2018/09/24 11:08:35]

Let's rename this to `message` and `...rest`. About the `...rest` bit, I think
we have a convention that if it's "rest of the arguments" then we use the name
`rest`.

[hub, 2018/09/25 04:28:51]

Done.

-  {
-    if (typeof msg === "string" && msg.indexOf(magic) != -1)

[Manish Jethani, 2018/09/24 11:08:35]

We could use `String.includes` instead of `String.indexOf`.

[Manish Jethani, 2018/09/24 11:08:35]

Strict equality is unnecessary here.

[hub, 2018/09/25 04:28:51]

Done.

[hub, 2018/09/25 04:28:52]

Done.

-      return true;
-    if (onerror)

[Manish Jethani, 2018/09/24 11:08:35]

Just to be safe, we should check `typeof onerror == "function"` here.

[hub, 2018/09/25 04:28:51]

Done.

-      return onerror(msg, ...args);

[Manish Jethani, 2018/09/24 11:08:35]

Let's just also set the `this` for this call, so `onerror.call(this, message,
...args)`.

[hub, 2018/09/25 04:28:52]

Done.

-  };
-
-  let patch = function(o, p)
-  {
-    // simple property
-    let d = Object.getOwnPropertyDescriptor(o, p);
-    if (!d || d.get !== abort)

[Manish Jethani, 2018/09/24 11:08:35]

Strict inequality is unnecessary here.

[hub, 2018/09/25 04:28:51]

Done.

-      Object.defineProperty(o, p, {get: abort, set() {}});
-  };
-  for (let prop of props)
-    patch(window, prop);
-}
-
-exports["abort-on-property-read"] = makeInjector(abortOnPropertyRead, randomId);
+  if (wrapPropertyAccess(window, property, {get: abort, set() {}}))
+  {
+    window.onerror = (message, ...rest) =>
+    {
+      if (typeof message == "string" && message.includes(rid))
+        return true;
+      if (typeof onerror == "function")
+        return (() => {}).call.call(onerror, this, message, ...rest);
+    };
+  }
+}
+
+exports["abort-on-property-read"] = makeInjector(abortOnPropertyRead,
+                                                 wrapPropertyAccess,
+                                                 randomId);