Sanitize inheritance in callers of utils.expose

Sanitize inheritance in callers of utils.expose

- Let all private classes inherit from the null prototype.
- Use safe built-ins.
- Minor drive-by fixes (code style).

BUG=603748, 591164
Committed: https://crrev.com/931719c51866a7c2e272114517cc52aa6581adff
Cr-Commit-Position: refs/heads/master@{#390510}

[robwu, 2016-04-23 16:14:00]

Description was changed from

==========
Sanitize inheritance in callers of utils.expose

- Let all private classes inherit from the null prototype.
- Use safe built-ins.
- Minor drive-by fixes (code style).

BUG=603748
==========

to

==========
Sanitize inheritance in callers of utils.expose

- Let all private classes inherit from the null prototype.
- Use safe built-ins.
- Minor drive-by fixes (code style).

BUG=603748,591164
==========

[robwu, 2016-04-23 16:14:20]

rob@robwu.nl changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-04-26 22:41:25]

(It looks like you published, but I didn't see an email for this one.  Hopefully
I'm not reviewing before it's ready.)

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js
(right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js:14: *
I don't think this is typical style.  Please revert (everywhere).

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js:34:
$Object.setPrototypeOf(KeyPairImpl.prototype, null);
Why do we need this?

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/platform_keys/key.js (right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/platform_keys/key.js:51: function KeyBase()
{
nit: let's do "function KeyBase() {}" (one line)

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view/chrome_web_view.js (right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view/chrome_web_view.js:41:
subEventName, opt_argSchemas, opt_eventOptions, webViewInstanceId);
nit: prefer to line these params up (multiple lines is fine, but same
indentation for arguments)

[robwu, 2016-04-26 23:14:33]

On 2016/04/26 22:41:25, Devlin wrote:
> (It looks like you published, but I didn't see an email for this one. 
Hopefully
> I'm not reviewing before it's ready.)

Weird. Did you check your spam bin? I found one codereview mail in my spam bin
last week.

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js
(right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js:14: *
On 2016/04/26 22:41:25, Devlin wrote:
> I don't think this is typical style.  Please revert (everywhere).

Done.

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js:34:
$Object.setPrototypeOf(KeyPairImpl.prototype, null);
On 2016/04/26 22:41:25, Devlin wrote:
> Why do we need this?

By default, instances inherit from Object, which could be modified by the
extension. Therefore the extension bindings cannot and should not rely on
prototype methods and use $Object, $Function, etc. Nulling __proto__ is the most
effective way to make sure that extension bindings stop relying on unreliable
inherit methods.

I added a DCHECk to utils.expose that asserts that all private classes do not
inherit from the public Object, and consistently reset all prototypes of private
classes to null to not trigger the assertion.

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/platform_keys/key.js (right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/platform_keys/key.js:51: function KeyBase()
{
On 2016/04/26 22:41:25, Devlin wrote:
> nit: let's do "function KeyBase() {}" (one line)

Done.

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view/chrome_web_view.js (right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view/chrome_web_view.js:41:
subEventName, opt_argSchemas, opt_eventOptions, webViewInstanceId);
On 2016/04/26 22:41:25, Devlin wrote:
> nit: prefer to line these params up (multiple lines is fine, but same
> indentation for arguments)

Done.

[Devlin, 2016-04-27 00:10:25]

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js
(right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js:34:
$Object.setPrototypeOf(KeyPairImpl.prototype, null);
On 2016/04/26 23:14:33, robwu wrote:
> On 2016/04/26 22:41:25, Devlin wrote:
> > Why do we need this?
> 
> By default, instances inherit from Object, which could be modified by the
> extension. Therefore the extension bindings cannot and should not rely on
> prototype methods and use $Object, $Function, etc. Nulling __proto__ is the
most
> effective way to make sure that extension bindings stop relying on unreliable
> inherit methods.
> 
> I added a DCHECk to utils.expose that asserts that all private classes do not
> inherit from the public Object, and consistently reset all prototypes of
private
> classes to null to not trigger the assertion.

I should have been more clear.  Why do we need to set the prototype on both
KeyPairImpl and KeyPairImpl.prototype?

[robwu, 2016-04-27 09:09:21]

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js
(right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js:34:
$Object.setPrototypeOf(KeyPairImpl.prototype, null);
On 2016/04/27 00:10:25, Devlin wrote:
> On 2016/04/26 23:14:33, robwu wrote:
> > On 2016/04/26 22:41:25, Devlin wrote:
> > > Why do we need this?
> > 
> > By default, instances inherit from Object, which could be modified by the
> > extension. Therefore the extension bindings cannot and should not rely on
> > prototype methods and use $Object, $Function, etc. Nulling __proto__ is the
> most
> > effective way to make sure that extension bindings stop relying on
unreliable
> > inherit methods.
> > 
> > I added a DCHECk to utils.expose that asserts that all private classes do
not
> > inherit from the public Object, and consistently reset all prototypes of
> private
> > classes to null to not trigger the assertion.
> 
> I should have been more clear.  Why do we need to set the prototype on both
> KeyPairImpl and KeyPairImpl.prototype?

On prototype for the reason above.
On the constructor to avoid issues like Binding.create = ...

At the moment there is no immediate need for voiding the constructor's
__proto__, but maybe in the future a static method is added and then this change
will be useful.

[Devlin, 2016-04-27 17:33:02]

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js
(right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js:34:
$Object.setPrototypeOf(KeyPairImpl.prototype, null);
On 2016/04/27 09:09:21, robwu wrote:
> On 2016/04/27 00:10:25, Devlin wrote:
> > On 2016/04/26 23:14:33, robwu wrote:
> > > On 2016/04/26 22:41:25, Devlin wrote:
> > > > Why do we need this?
> > > 
> > > By default, instances inherit from Object, which could be modified by the
> > > extension. Therefore the extension bindings cannot and should not rely on
> > > prototype methods and use $Object, $Function, etc. Nulling __proto__ is
the
> > most
> > > effective way to make sure that extension bindings stop relying on
> unreliable
> > > inherit methods.
> > > 
> > > I added a DCHECk to utils.expose that asserts that all private classes do
> not
> > > inherit from the public Object, and consistently reset all prototypes of
> > private
> > > classes to null to not trigger the assertion.
> > 
> > I should have been more clear.  Why do we need to set the prototype on both
> > KeyPairImpl and KeyPairImpl.prototype?
> 
> On prototype for the reason above.
> On the constructor to avoid issues like Binding.create = ...
> 
> At the moment there is no immediate need for voiding the constructor's
> __proto__, but maybe in the future a static method is added and then this
change
> will be useful.

Hmm... I'm a bit torn on this.

A few of the v8 folks recoiled at the idea of making a ctor not inherit from
function prototype, and it does indeed seem very weird.  On the other hand, this
makes it less likely someone's gonna do something wrong later.  But we also have
that risk on every other line of JS, so I'm not sure it's very compelling.

I'm inclined to say that we should keep the ctors themselves as inheriting from
functions, and enforce that if anyone wants to add a static method, it should be
done using $Object.defineProperty, as we do in binding.js.

[robwu, 2016-04-28 20:27:50]

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js
(right):

https://codereview.chromium.org/1915753002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js:34:
$Object.setPrototypeOf(KeyPairImpl.prototype, null);
On 2016/04/27 17:33:02, Devlin wrote:
> On 2016/04/27 09:09:21, robwu wrote:
> > On 2016/04/27 00:10:25, Devlin wrote:
> > > On 2016/04/26 23:14:33, robwu wrote:
> > > > On 2016/04/26 22:41:25, Devlin wrote:
> > > > > Why do we need this?
> > > > 
> > > > By default, instances inherit from Object, which could be modified by
the
> > > > extension. Therefore the extension bindings cannot and should not rely
on
> > > > prototype methods and use $Object, $Function, etc. Nulling __proto__ is
> the
> > > most
> > > > effective way to make sure that extension bindings stop relying on
> > unreliable
> > > > inherit methods.
> > > > 
> > > > I added a DCHECk to utils.expose that asserts that all private classes
do
> > not
> > > > inherit from the public Object, and consistently reset all prototypes of
> > > private
> > > > classes to null to not trigger the assertion.
> > > 
> > > I should have been more clear.  Why do we need to set the prototype on
both
> > > KeyPairImpl and KeyPairImpl.prototype?
> > 
> > On prototype for the reason above.
> > On the constructor to avoid issues like Binding.create = ...
> > 
> > At the moment there is no immediate need for voiding the constructor's
> > __proto__, but maybe in the future a static method is added and then this
> change
> > will be useful.
> 
> Hmm... I'm a bit torn on this.
> 
> A few of the v8 folks recoiled at the idea of making a ctor not inherit from
> function prototype, and it does indeed seem very weird.  On the other hand,
this
> makes it less likely someone's gonna do something wrong later.  But we also
have
> that risk on every other line of JS, so I'm not sure it's very compelling.
> 
> I'm inclined to say that we should keep the ctors themselves as inheriting
from
> functions, and enforce that if anyone wants to add a static method, it should
be
> done using $Object.defineProperty, as we do in binding.js.

Done.

[Devlin, 2016-04-28 21:21:21]

lgtm

https://codereview.chromium.org/1915753002/diff/60001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1915753002/diff/60001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:26: var ports = {__proto__:null};
nit: space after ':'

[robwu, 2016-04-28 21:32:17]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-04-28 21:32:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1915753002/#ps80001
(title: "Nit: space after ':'")

[commit-bot: I haz the power, 2016-04-28 21:32:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915753002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915753002/80001

[commit-bot: I haz the power, 2016-04-28 22:29:38]

Description was changed from

==========
Sanitize inheritance in callers of utils.expose

- Let all private classes inherit from the null prototype.
- Use safe built-ins.
- Minor drive-by fixes (code style).

BUG=603748,591164
==========

to

==========
Sanitize inheritance in callers of utils.expose

- Let all private classes inherit from the null prototype.
- Use safe built-ins.
- Minor drive-by fixes (code style).

BUG=603748,591164
==========

[commit-bot: I haz the power, 2016-04-28 22:29:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-30 17:22:07]

Patchset 5 (id:??) landed as
https://crrev.com/931719c51866a7c2e272114517cc52aa6581adff
Cr-Commit-Position: refs/heads/master@{#390510}

[commit-bot: I haz the power, 2016-04-30 17:58:01]

Description was changed from

==========
Sanitize inheritance in callers of utils.expose

- Let all private classes inherit from the null prototype.
- Use safe built-ins.
- Minor drive-by fixes (code style).

BUG=603748,591164
==========

to

==========
Sanitize inheritance in callers of utils.expose

- Let all private classes inherit from the null prototype.
- Use safe built-ins.
- Minor drive-by fixes (code style).

BUG=603748,591164

Committed: https://crrev.com/931719c51866a7c2e272114517cc52aa6581adff
Cr-Commit-Position: refs/heads/master@{#390510}
==========