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 418 matching lines @@
   {
     for (let i = 0; i < times; i++)
       object + "";
 
     if (typeof dir == "function")
       dir.call(this, object);
   };
 }
 
 exports["dir-string"] = makeInjector(dirString);
+
+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)
+    return;
+
+  let magic = randomId();
+
+  let abort = function()
+  {
+    throw new ReferenceError(magic);
+  };
+
+  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);