Sanitize inheritance in callers of utils.expose

chrome/renderer/resources/extensions/enterprise_platform_keys/key_pair.js

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 var utils = require('utils');
 var intersect = require('platformKeys.utils').intersect;
 var keyModule = require('platformKeys.Key');
 var Key = keyModule.Key;
 var KeyType = keyModule.KeyType;
 var KeyUsage = keyModule.KeyUsage;
 
 /**
  * Implementation of WebCrypto.KeyPair used in enterprise.platformKeys.
+ *

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

I don't think this is typical style.  Please revert (everywhere).

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

Done.

  * @param {ArrayBuffer} publicKeySpki The Subject Public Key Info in DER
  *   encoding.
  * @param {KeyAlgorithm} algorithm The algorithm identifier.
  * @param {KeyUsage[]} usages The allowed key usages.
  * @constructor
  */
-var KeyPairImpl = function(publicKeySpki, algorithm, usages) {
+function KeyPairImpl(publicKeySpki, algorithm, usages) {
   this.publicKey = new Key(KeyType.public,
                            publicKeySpki,
                            algorithm,
                            intersect([KeyUsage.verify], usages),
                            true /* extractable */);
   this.privateKey = new Key(KeyType.private,
                             publicKeySpki,
                             algorithm,
                             intersect([KeyUsage.sign], usages),
                             false /* not extractable */);
-};
+}
+$Object.setPrototypeOf(KeyPairImpl, null);
+$Object.setPrototypeOf(KeyPairImpl.prototype, null);

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

Why do we need this?

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

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.

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

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]

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]

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]

Done.

 
 function KeyPair() {
   privates(KeyPair).constructPrivate(this, arguments);
 }
 utils.expose(KeyPair, KeyPairImpl, {
   readonly: [
     'publicKey',
     'privateKey',
   ],
 });
 
 exports.$set('KeyPair', KeyPair);