Merge pull request #8815 from tarruda/improve-sandbox-expose-more-modules

Improvements to sandbox mode

atom/renderer/atom_sandboxed_renderer_client.cc

@@ -11,6 +11,7 @@
 #include "atom/common/api/api_messages.h"
 #include "atom/common/native_mate_converters/string16_converter.h"
 #include "atom/common/native_mate_converters/value_converter.h"
+#include "atom/common/node_includes.h"
 #include "atom/common/options_switches.h"
 #include "atom/renderer/api/atom_api_renderer_ipc.h"
 #include "atom/renderer/atom_render_view_observer.h"
@@ -21,6 +22,8 @@
 #include "content/public/renderer/render_view.h"
 #include "content/public/renderer/render_view_observer.h"
 #include "ipc/ipc_message_macros.h"
+#include "native_mate/converter.h"
+#include "native_mate/dictionary.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebKit.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
@@ -31,7 +34,57 @@ namespace atom {
 
 namespace {
 
-const std::string kBindingKey = "binding";
+const std::string kIpcKey = "ipcNative";
+const std::string kModuleCacheKey = "native-module-cache";
+
+
+v8::Local<v8::Object> GetModuleCache(v8::Isolate* isolate) {
+  mate::Dictionary global(isolate, isolate->GetCurrentContext()->Global());
+  v8::Local<v8::Value> cache;
+
+  if (!global.GetHidden(kModuleCacheKey, &cache)) {
+    cache = v8::Object::New(isolate);
+    global.SetHidden(kModuleCacheKey, cache);
+  }
+
+  return cache->ToObject();
+}
+
+// adapted from node.cc
+v8::Local<v8::Value> GetBinding(v8::Isolate* isolate, v8::Local<v8::String> key,
+    mate::Arguments* margs) {
+  v8::Local<v8::Object> exports;
+  std::string module_key = mate::V8ToString(key);
+  mate::Dictionary cache(isolate, GetModuleCache(isolate));
+
+  if (cache.Get(module_key.c_str(), &exports)) {
+    return exports;
+  }
+
+  auto mod = node::get_builtin_module(module_key.c_str());
+
+  if (!mod) {
+    char errmsg[1024];
+    snprintf(errmsg, sizeof(errmsg), "No such module: %s", module_key.c_str());
+    margs->ThrowError(errmsg);
+    return exports;
+  }
+
+  exports = v8::Object::New(isolate);
+  DCHECK_EQ(mod->nm_register_func, nullptr);
+  DCHECK_NE(mod->nm_context_register_func, nullptr);
+  mod->nm_context_register_func(exports, v8::Null(isolate),
+      isolate->GetCurrentContext(), mod->nm_priv);
+  cache.Set(module_key.c_str(), exports);
+  return exports;
+}
+
+void InitializeBindings(v8::Local<v8::Object> binding,
+                        v8::Local<v8::Context> context) {
+  auto isolate = context->GetIsolate();
+  mate::Dictionary b(isolate, binding);
+  b.SetMethod("get", GetBinding);
+}
 
 class AtomSandboxedRenderFrameObserver : public content::RenderFrameObserver {
  public:
@@ -100,7 +153,7 @@ class AtomSandboxedRenderViewObserver : public AtomRenderViewObserver {
       mate::ConvertToV8(isolate, channel),
       mate::ConvertToV8(isolate, args)
     };
-    renderer_client_->InvokeBindingCallback(
+    renderer_client_->InvokeIpcCallback(
         context,
         "onMessage",
         std::vector<v8::Local<v8::Value>>(argv, argv + 2));
@@ -158,17 +211,13 @@ void AtomSandboxedRendererClient::DidCreateScriptContext(
       script->Run(context).ToLocalChecked());
   // Create and initialize the binding object
   auto binding = v8::Object::New(isolate);
-  api::Initialize(binding, v8::Null(isolate), context, nullptr);
+  InitializeBindings(binding, context);
   v8::Local<v8::Value> args[] = {
     binding,
     mate::ConvertToV8(isolate, preload_script)
   };
   // Execute the function with proper arguments
   ignore_result(func->Call(context, v8::Null(isolate), 2, args));
-  // Store the bindingt privately for handling messages from the main process.
-  auto binding_key = mate::ConvertToV8(isolate, kBindingKey)->ToString();
-  auto private_binding_key = v8::Private::ForApi(isolate, binding_key);
-  context->Global()->SetPrivate(context, private_binding_key, binding);
 }
 
 void AtomSandboxedRendererClient::WillReleaseScriptContext(
@@ -176,15 +225,15 @@ void AtomSandboxedRendererClient::WillReleaseScriptContext(
   auto isolate = context->GetIsolate();
   v8::HandleScope handle_scope(isolate);
   v8::Context::Scope context_scope(context);
-  InvokeBindingCallback(context, "onExit", std::vector<v8::Local<v8::Value>>());
+  InvokeIpcCallback(context, "onExit", std::vector<v8::Local<v8::Value>>());
 }
 
-void AtomSandboxedRendererClient::InvokeBindingCallback(
+void AtomSandboxedRendererClient::InvokeIpcCallback(
     v8::Handle<v8::Context> context,
     std::string callback_name,
     std::vector<v8::Handle<v8::Value>> args) {
   auto isolate = context->GetIsolate();
-  auto binding_key = mate::ConvertToV8(isolate, kBindingKey)->ToString();
+  auto binding_key = mate::ConvertToV8(isolate, kIpcKey)->ToString();
   auto private_binding_key = v8::Private::ForApi(isolate, binding_key);
   auto global_object = context->Global();
   v8::Local<v8::Value> value;

atom/renderer/atom_sandboxed_renderer_client.h

@@ -21,9 +21,9 @@ class AtomSandboxedRendererClient : public content::ContentRendererClient {
       v8::Handle<v8::Context> context, content::RenderFrame* render_frame);
   void WillReleaseScriptContext(
       v8::Handle<v8::Context> context, content::RenderFrame* render_frame);
-  void InvokeBindingCallback(v8::Handle<v8::Context> context,
-                             std::string callback_name,
-                             std::vector<v8::Handle<v8::Value>> args);
+  void InvokeIpcCallback(v8::Handle<v8::Context> context,
+                         std::string callback_name,
+                         std::vector<v8::Handle<v8::Value>> args);
   // content::ContentRendererClient:
   void RenderFrameCreated(content::RenderFrame*) override;
   void RenderViewCreated(content::RenderView*) override;

electron.gyp

@@ -452,6 +452,8 @@
             'browserify',
             '--',
             'lib/sandboxed_renderer/init.js',
+            '-r',
+            './lib/sandboxed_renderer/api/exports/electron.js:electron',
             '-o',
             '<@(_outputs)',
           ],

filenames.gypi

@@ -51,6 +51,7 @@
       'lib/common/api/module-list.js',
       'lib/common/api/native-image.js',
       'lib/common/api/shell.js',
+      'lib/common/atom-binding-setup.js',
       'lib/common/init.js',
       'lib/common/parse-features-string.js',
       'lib/common/reset-search-paths.js',
@@ -67,7 +68,6 @@
       'lib/renderer/api/desktop-capturer.js',
       'lib/renderer/api/exports/electron.js',
       'lib/renderer/api/ipc-renderer.js',
-      'lib/renderer/api/ipc-renderer-setup.js',
       'lib/renderer/api/module-list.js',
       'lib/renderer/api/remote.js',
       'lib/renderer/api/screen.js',
@@ -78,8 +78,8 @@
       'lib/renderer/extensions/web-navigation.js',
     ],
     'browserify_entries': [
-      'lib/renderer/api/ipc-renderer-setup.js',
       'lib/sandboxed_renderer/init.js',
+      'lib/sandboxed_renderer/api/exports/electron.js',
     ],
     'isolated_context_browserify_entries': [
       'lib/renderer/window-setup.js',

lib/common/atom-binding-setup.js

@@ -0,0 +1,13 @@
+module.exports = function atomBindingSetup (binding, processType) {
+  return function atomBinding (name) {
+    try {
+      return binding(`atom_${processType}_${name}`)
+    } catch (error) {
+      if (/No such module/.test(error.message)) {
+        return binding(`atom_common_${name}`)
+      } else {
+        throw error
+      }
+    }
+  }
+}

lib/common/init.js

@@ -1,18 +1,6 @@
 const timers = require('timers')
 
-const {binding} = process
-
-process.atomBinding = function (name) {
-  try {
-    return binding(`atom_${process.type}_${name}`)
-  } catch (error) {
-    if (/No such module/.test(error.message)) {
-      return binding(`atom_common_${name}`)
-    } else {
-      throw error
-    }
-  }
-}
+process.atomBinding = require('./atom-binding-setup')(process.binding, process.type)
 
 // setImmediate and process.nextTick makes use of uv_check and uv_prepare to
 // run the callbacks, however since we only run uv loop on requests, the

lib/renderer/api/ipc-renderer-setup.js

@@ -1,40 +0,0 @@
-// Any requires added here need to be added to the browserify_entries array
-// in filenames.gypi so they get built into the preload_bundle.js bundle
-
-module.exports = function (ipcRenderer, binding) {
-  ipcRenderer.send = function (...args) {
-    return binding.send('ipc-message', args)
-  }
-
-  ipcRenderer.sendSync = function (...args) {
-    return JSON.parse(binding.sendSync('ipc-message-sync', args))
-  }
-
-  ipcRenderer.sendToHost = function (...args) {
-    return binding.send('ipc-message-host', args)
-  }
-
-  ipcRenderer.sendTo = function (webContentsId, channel, ...args) {
-    if (typeof webContentsId !== 'number') {
-      throw new TypeError('First argument has to be webContentsId')
-    }
-
-    ipcRenderer.send('ELECTRON_BROWSER_SEND_TO', false, webContentsId, channel, ...args)
-  }
-
-  ipcRenderer.sendToAll = function (webContentsId, channel, ...args) {
-    if (typeof webContentsId !== 'number') {
-      throw new TypeError('First argument has to be webContentsId')
-    }
-
-    ipcRenderer.send('ELECTRON_BROWSER_SEND_TO', true, webContentsId, channel, ...args)
-  }
-
-  const removeAllListeners = ipcRenderer.removeAllListeners.bind(ipcRenderer)
-  ipcRenderer.removeAllListeners = function (...args) {
-    if (args.length === 0) {
-      throw new Error('Removing all listeners from ipcRenderer will make Electron internals stop working.  Please specify a event name')
-    }
-    removeAllListeners(...args)
-  }
-}

lib/renderer/api/ipc-renderer.js

@@ -5,6 +5,41 @@ const v8Util = process.atomBinding('v8_util')
 
 // Created by init.js.
 const ipcRenderer = v8Util.getHiddenValue(global, 'ipc')
-require('./ipc-renderer-setup')(ipcRenderer, binding)
+
+ipcRenderer.send = function (...args) {
+  return binding.send('ipc-message', args)
+}
+
+ipcRenderer.sendSync = function (...args) {
+  return JSON.parse(binding.sendSync('ipc-message-sync', args))
+}
+
+ipcRenderer.sendToHost = function (...args) {
+  return binding.send('ipc-message-host', args)
+}
+
+ipcRenderer.sendTo = function (webContentsId, channel, ...args) {
+  if (typeof webContentsId !== 'number') {
+    throw new TypeError('First argument has to be webContentsId')
+  }
+
+  ipcRenderer.send('ELECTRON_BROWSER_SEND_TO', false, webContentsId, channel, ...args)
+}
+
+ipcRenderer.sendToAll = function (webContentsId, channel, ...args) {
+  if (typeof webContentsId !== 'number') {
+    throw new TypeError('First argument has to be webContentsId')
+  }
+
+  ipcRenderer.send('ELECTRON_BROWSER_SEND_TO', true, webContentsId, channel, ...args)
+}
+
+const removeAllListeners = ipcRenderer.removeAllListeners.bind(ipcRenderer)
+ipcRenderer.removeAllListeners = function (...args) {
+  if (args.length === 0) {
+    throw new Error('Removing all listeners from ipcRenderer will make Electron internals stop working.  Please specify a event name')
+  }
+  removeAllListeners(...args)
+}
 
 module.exports = ipcRenderer

lib/sandboxed_renderer/api/exports/electron.js

@@ -0,0 +1,8 @@
+Object.defineProperties(exports, {
+  ipcRenderer: {
+    enumerable: true,
+    get: function () {
+      return require('../ipc-renderer')
+    }
+  }
+})

lib/sandboxed_renderer/api/ipc-renderer.js

@@ -0,0 +1,20 @@
+const ipcRenderer = require('../../renderer/api/ipc-renderer')
+
+const v8Util = process.atomBinding('v8_util')
+const ipcNative = process.atomBinding('ipc')
+
+// AtomSandboxedRendererClient will look for the "ipcNative" hidden object when
+// invoking the `onMessage`/`onExit` callbacks. We could reuse "ipc" and assign
+// `onMessage`/`onExit` directly to `ipcRenderer`, but it is better to separate
+// private/public APIs.
+v8Util.setHiddenValue(global, 'ipcNative', ipcNative)
+
+ipcNative.onMessage = function (channel, args) {
+  ipcRenderer.emit(channel, {sender: ipcRenderer}, ...args)
+}
+
+ipcNative.onExit = function () {
+  process.emit('exit')
+}
+
+module.exports = ipcRenderer

lib/sandboxed_renderer/init.js

@@ -2,31 +2,42 @@
 // in filenames.gypi so they get built into the preload_bundle.js bundle
 
 /* eslint no-eval: "off" */
-/* global binding, preloadPath, process, Buffer */
+/* global binding, preloadPath, Buffer */
 const events = require('events')
 
-const ipcRenderer = new events.EventEmitter()
-const proc = new events.EventEmitter()
-// eval in window scope:
-// http://www.ecma-international.org/ecma-262/5.1/#sec-10.4.2
-const geval = eval
-
-require('../renderer/api/ipc-renderer-setup')(ipcRenderer, binding)
-
-binding.onMessage = function (channel, args) {
-  ipcRenderer.emit(channel, ...args)
-}
+process.atomBinding = require('../common/atom-binding-setup')(binding.get, 'renderer')
 
-binding.onExit = function () {
-  proc.emit('exit')
+const v8Util = process.atomBinding('v8_util')
+// The `lib/renderer/api/ipc-renderer.js` module looks for the ipc object in the
+// "ipc" hidden value
+v8Util.setHiddenValue(global, 'ipc', new events.EventEmitter())
+// The process object created by browserify is not an event emitter, fix it so
+// the API is more compatible with non-sandboxed renderers.
+for (let prop of Object.keys(events.EventEmitter.prototype)) {
+  if (process.hasOwnProperty(prop)) {
+    delete process[prop]
+  }
 }
+Object.setPrototypeOf(process, events.EventEmitter.prototype)
 
+const electron = require('electron')
 const preloadModules = new Map([
-  ['electron', {
-    ipcRenderer: ipcRenderer
-  }]
+  ['electron', electron]
 ])
 
+// Fetch the preload script. This needs to be done through the browser process
+// since we may not have filesystem access in a sandboxed renderer.
+let preloadSrc = electron.ipcRenderer.sendSync('ELECTRON_BROWSER_READ_FILE', preloadPath)
+if (preloadSrc.err) {
+  throw new Error(preloadSrc.err)
+}
+
+// Pass different process object to the preload script(which should not have
+// access to things like `process.atomBinding`).
+const preloadProcess = new events.EventEmitter()
+process.on('exit', () => preloadProcess.emit('exit'))
+
+// This is the `require` function that will be visible to the preload script
 function preloadRequire (module) {
   if (preloadModules.has(module)) {
     return preloadModules.get(module)
@@ -34,26 +45,31 @@ function preloadRequire (module) {
   throw new Error('module not found')
 }
 
-// Fetch the source for the preload
-let preloadSrc = ipcRenderer.sendSync('ELECTRON_BROWSER_READ_FILE', preloadPath)
-if (preloadSrc.err) {
-  throw new Error(preloadSrc.err)
-}
-
-// Wrap the source into a function receives a `require` function as argument.
-// Browserify bundles can make use of this, as explained in:
-// https://github.com/substack/node-browserify#multiple-bundles
+// Wrap the script into a function executed in global scope. It won't have
+// access to the current scope, so we'll expose a few objects as arguments:
 //
-// For example, the user can create a browserify bundle with:
+// - `require`: The `preloadRequire` function
+// - `process`: The `preloadProcess` object
+// - `Buffer`: Browserify `Buffer` implementation
+// - `global`: The window object, which is aliased to `global` by browserify.
+//
+// Browserify bundles can make use of an external require function as explained
+// in https://github.com/substack/node-browserify#multiple-bundles, so electron
+// apps can use multi-module preload scripts in sandboxed renderers.
+//
+// For example, the user can create a bundle with:
 //
 //     $ browserify -x electron preload.js > renderer.js
 //
 // and any `require('electron')` calls in `preload.js` will work as expected
-// since browserify won't try to include `electron` in the bundle and will fall
-// back to the `preloadRequire` function above.
+// since browserify won't try to include `electron` in the bundle, falling back
+// to the `preloadRequire` function above.
 let preloadWrapperSrc = `(function(require, process, Buffer, global) {
 ${preloadSrc.data}
 })`
 
+// eval in window scope:
+// http://www.ecma-international.org/ecma-262/5.1/#sec-10.4.2
+const geval = eval
 let preloadFn = geval(preloadWrapperSrc)
-preloadFn(preloadRequire, proc, Buffer, global)
+preloadFn(preloadRequire, preloadProcess, Buffer, global)

vendor/node

@@ -1 +1 @@
-Subproject commit 0f84d972a1b48b7da361f9717ff43349a7946abd
+Subproject commit a6663598aa78832e7955cb93c51a098eac787abb