[Extensions] More bindings improvements

extensions/renderer/resources/binding.js

Index: extensions/renderer/resources/binding.js
diff --git a/extensions/renderer/resources/binding.js b/extensions/renderer/resources/binding.js
index 60d85a97afa38731403fb18afb2e0350f3b12598..9967eaf88694202306e4158af18ae6b06a72cece 100644
--- a/extensions/renderer/resources/binding.js
+++ b/extensions/renderer/resources/binding.js
@@ -4,6 +4,8 @@
 
 var Event = require('event_bindings').Event;
 var forEach = require('utils').forEach;
+// Note: Beware sneaky getters/setters when using GetAvailbility(). Use safe/raw
+// variables as arguments.
 var GetAvailability = requireNative('v8_context').GetAvailability;
 var exceptionHandler = require('uncaught_exception_handler');
 var lastError = require('lastError');
@@ -236,7 +238,7 @@ Binding.prototype = {
     // Binding.generate.
     // Additionally, since the schema is an object returned from a native
     // handler, its properties don't have the custom getters/setters that a page
-    // may have put on Object.prototype.
+    // may have put on Object.prototype, and the object is frozen by v8.
     var schema = schemaRegistry.GetSchema(this.apiName_);
 
     function shouldCheckUnprivileged() {
@@ -366,9 +368,10 @@ Binding.prototype = {
 
         var apiFunction = {};
         apiFunction.definition = functionDef;
-        apiFunction.name = schema.namespace + '.' + functionDef.name;
+        var apiFunctionName = schema.namespace + '.' + functionDef.name;
+        apiFunction.name = apiFunctionName;

[robwu, 2016/04/20 10:49:17]

If you're concerned about being affected by prototype poisoning, use
var apiFunction = $Object.create(null);

Then you'll get an object that doesn't inherit from any object.

[Devlin, 2016/04/20 13:44:38]

This is what https://codereview.chromium.org/1902003003/ does for the Binding
object, which helps.

We need to be careful where we do this because some of these objects are
actually exposed to extension developers, and someone may be relying on object
behavior for the API objects (e.g., perhaps someone checks
chrome.runtime.hasOwnProperty rather than for undefined).

Long-term we should do an audit of pretty much all of these files and make sure
that none are exposing anything they shouldn't, but, well, there's about 129 of
them, so I've been starting with the low-hanging fruit.

[robwu, 2016/04/20 15:06:35]

|apiFunction| is an internal object, so it shouldn't matter. For exposed
objects, it's possible to start with a null prototype and then update the
prototype to $Object.self ($Object.setPrototypeOf(obj, $Object.self); - todo
export setPrototypeOf in safe_builtins.cc).

 
-        if (!GetAvailability(apiFunction.name).is_available ||

[asargent_no_longer_on_chrome, 2016/04/19 23:06:48]

ugh, javascript

I fear we will this mistake again someday soon - it's so easy to forget about
getters/setters

[Devlin, 2016/04/19 23:18:22]

Quite :/  I've been talking with Adam Klein and gotten a few fun tips for how to
help out in some areas.  The problem, of course, is that there are roughly 129
js files that could potentially be attack surfaces.  Obviously, binding.js and
others are juicier, but I'm nervous we'll have issues with others as well.

+        if (!GetAvailability(apiFunctionName).is_available ||
             (checkUnprivileged && !isSchemaAccessAllowed(functionDef))) {
           this.apiFunctions_.registerUnavailable(functionDef.name);
           return;