[Extensions Bindings] Avoid passing the event filter to JS

[Extensions Bindings] Avoid passing the event filter to JS

Currently, in JS bindings, we pass the event filter to JS, and then pass
it back to C++ to find matching event listeners. This can cause problems
because the object could be tweaked by monkey-patching, and we aren't
correctly checking.

Instead of updating the checks to safely interact with the object, just
avoid passing it to JS altogether. Instead, pass the ids of the relevant
listeners directly to JS. This avoids the security issue and is also
more performant.

Add a regression test.

BUG=702946
Review-Url: https://codereview.chromium.org/2924683002
Cr-Commit-Position: refs/heads/master@{#478742}
Committed: https://chromium.googlesource.com/chromium/src/+/229f3b010af25c9bc38c01cfc7c403d8b5fdecf0

[Devlin, 2017-06-05 20:55:09]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 20:56:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 21:52:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 21:52:23]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-08 03:20:02]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 03:20:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 03:22:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 03:22:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2017-06-09 18:49:19]

Description was changed from

==========
bindings fix
BUG=
==========

to

==========
[Extensions Bindings] Avoid passing the event filter to JS

Currently, in JS bindings, we pass the event filter to JS, and then pass
it back to C++ to find matching event listeners. This can cause problems
because the object could be tweaked by monkey-patching, and we aren't
correctly checking.

Instead of updating the checks to safely interact with the object, just
avoid passing it to JS altogether. Instead, pass the ids of the relevant
listeners directly to JS. This avoids the security issue and is also
more performant.

Add a regression test.

BUG=702946
==========

[Devlin, 2017-06-09 18:50:24]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 18:50:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-06-09 18:56:50]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 18:57:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 20:29:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 20:29:03]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-09 20:31:25]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[Devlin, 2017-06-09 20:31:34]

Patchset #1 (id:1) has been deleted

[Devlin, 2017-06-09 20:31:43]

Patchset #1 (id:20001) has been deleted

[Devlin, 2017-06-09 20:31:49]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-06-09 20:31:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-06-09 20:31:55]

Patchset #1 (id:40001) has been deleted

[Devlin, 2017-06-09 20:32:40]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2017-06-09 20:32:40]

Mind taking a look?  Not strictly related to native bindings (and in fact
automatically fixed when we convert), but probably best to not wait for that.

[commit-bot: I haz the power, 2017-06-09 22:29:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 22:29:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lazyboy, 2017-06-10 00:22:09]

lgtm once jbroman@'s happy.

https://codereview.chromium.org/2924683002/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/bindings/event_filter/background.js
(right):

https://codereview.chromium.org/2924683002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/bindings/event_filter/background.js:5:
Object.defineProperty(Object.prototype, 'windowExposedByDefault',
nit: put a comment about the intent of this custom getter.

https://codereview.chromium.org/2924683002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/bindings/event_filter/background.js:6:
{enumerable: true, get() {  return 'hahaha'; }});
nit: remove one of the two spaces before "return"

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:144:
CHECK(array->CreateDataProperty(context, i++, v8::Integer::New(isolate, id))
I'll defer to Jeremy for this one.

[jbroman, 2017-06-12 15:48:51]

lgtm

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:130: EventFilter& event_filter =
g_event_filter.Get();
nit: const&? (use of a singleton across threads like this is kinda scary, but
non-const ref makes me thing this is going to mutate)

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:144:
CHECK(array->CreateDataProperty(context, i++, v8::Integer::New(isolate, id))
On 2017/06/10 at 00:22:08, lazyboy wrote:
> I'll defer to Jeremy for this one.

This looks good to me.

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:182: std::vector<v8::Local<v8::Value>>
arguments;
super-nit: could at least reserve the buffer here, since it's of constant size.
Or this could just be an array on the stack:

v8::Local<v8::Value> args[] = {
    gin::StringToSymbol(context->isolate(), event_name),
    content::V8ValueConverter::create()->ToV8Value(event_args,
                                                   context->v8_context()),
    listener_ids
};
context->module_system()->CallModuleMethodSafe(
    kEventBindings, "dispatchEvent", arraysize(args), args);

(don't feel obligated as it's arguably premature optimization, but to my eyes
it's also slightly simpler)

[Devlin, 2017-06-12 16:34:40]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 16:35:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 16:38:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 16:38:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2017-06-12 17:01:54]

https://codereview.chromium.org/2924683002/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/bindings/event_filter/background.js
(right):

https://codereview.chromium.org/2924683002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/bindings/event_filter/background.js:5:
Object.defineProperty(Object.prototype, 'windowExposedByDefault',
On 2017/06/10 00:22:08, lazyboy wrote:
> nit: put a comment about the intent of this custom getter.

Done.

https://codereview.chromium.org/2924683002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/bindings/event_filter/background.js:6:
{enumerable: true, get() {  return 'hahaha'; }});
On 2017/06/10 00:22:08, lazyboy wrote:
> nit: remove one of the two spaces before "return"

Good catch!  Done.

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:130: EventFilter& event_filter =
g_event_filter.Get();
On 2017/06/12 15:48:51, jbroman wrote:
> nit: const&? (use of a singleton across threads like this is kinda scary, but
> non-const ref makes me thing this is going to mutate)

Done.

https://codereview.chromium.org/2924683002/diff/100001/extensions/renderer/ev...
extensions/renderer/event_bindings.cc:182: std::vector<v8::Local<v8::Value>>
arguments;
On 2017/06/12 15:48:51, jbroman wrote:
> super-nit: could at least reserve the buffer here, since it's of constant
size.
> Or this could just be an array on the stack:
> 
> v8::Local<v8::Value> args[] = {
>     gin::StringToSymbol(context->isolate(), event_name),
>     content::V8ValueConverter::create()->ToV8Value(event_args,
>                                                    context->v8_context()),
>     listener_ids
> };
> context->module_system()->CallModuleMethodSafe(
>     kEventBindings, "dispatchEvent", arraysize(args), args);
> 
> (don't feel obligated as it's arguably premature optimization, but to my eyes
> it's also slightly simpler)

sgtm.  Little more complicated since we have to pull out
V8ValueConverter::create() (that returns a raw ptr, not a unique ptr - that's
annoyed me enough that I'll investigate converting), but overall I'd agree it's
a little simpler.  Thanks for the suggestion!

[Devlin, 2017-06-12 18:43:42]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 18:44:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-06-12 20:22:55]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2017-06-12 20:22:59]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-06-12 20:23:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
lazyboy@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2924683002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-12 20:23:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 20:29:13]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1497298979822100,
"parent_rev": "39fbf000fd660ce059a62d68b02f51cc0384ca0c", "commit_rev":
"229f3b010af25c9bc38c01cfc7c403d8b5fdecf0"}

[commit-bot: I haz the power, 2017-06-12 20:29:27]

Description was changed from

==========
[Extensions Bindings] Avoid passing the event filter to JS

Currently, in JS bindings, we pass the event filter to JS, and then pass
it back to C++ to find matching event listeners. This can cause problems
because the object could be tweaked by monkey-patching, and we aren't
correctly checking.

Instead of updating the checks to safely interact with the object, just
avoid passing it to JS altogether. Instead, pass the ids of the relevant
listeners directly to JS. This avoids the security issue and is also
more performant.

Add a regression test.

BUG=702946
==========

to

==========
[Extensions Bindings] Avoid passing the event filter to JS

Currently, in JS bindings, we pass the event filter to JS, and then pass
it back to C++ to find matching event listeners. This can cause problems
because the object could be tweaked by monkey-patching, and we aren't
correctly checking.

Instead of updating the checks to safely interact with the object, just
avoid passing it to JS altogether. Instead, pass the ids of the relevant
listeners directly to JS. This avoids the security issue and is also
more performant.

Add a regression test.

BUG=702946

Review-Url: https://codereview.chromium.org/2924683002
Cr-Commit-Position: refs/heads/master@{#478742}
Committed:
https://chromium.googlesource.com/chromium/src/+/229f3b010af25c9bc38c01cfc7c4...
==========

[commit-bot: I haz the power, 2017-06-12 20:29:28]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/229f3b010af25c9bc38c01cfc7c4...

[Devlin, 2017-06-13 00:11:41]

A revert of this CL (patchset #4 id:140001) has been created in
https://codereview.chromium.org/2936673004/ by rdevlin.cronin@chromium.org.

The reason for reverting is: New test seems to be flaky on Mac...

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.9_T....