fix: restrict sendToFrame to same-process frames by default (#26875) (#…

…26927)

* fix: restrict sendToFrame to same-process frames by default (#26875)

* missed a conflict

* fix build

* fix build again

* fix usage of defer

docs/api/structures/ipc-main-event.md

@@ -1,5 +1,6 @@
 # IpcMainEvent Object extends `Event`
 
+* `processId` Integer - The internal ID of the renderer process that sent this message
 * `frameId` Integer - The ID of the renderer frame that sent this message
 * `returnValue` any - Set this to the value to be returned in a synchronous message
 * `sender` WebContents - Returns the `webContents` that sent the message

docs/api/structures/ipc-main-invoke-event.md

@@ -1,4 +1,5 @@
 # IpcMainInvokeEvent Object extends `Event`
 
+* `processId` Integer - The internal ID of the renderer process that sent this message
 * `frameId` Integer - The ID of the renderer frame that sent this message
 * `sender` WebContents - Returns the `webContents` that sent the message

docs/api/web-contents.md

@@ -1586,7 +1586,7 @@ app.whenReady().then(() => {
 
 #### `contents.sendToFrame(frameId, channel, ...args)`
 
-* `frameId` Integer
+* `frameId` Integer | [number, number]
 * `channel` String
 * `...args` any[]
 

lib/browser/api/web-contents.js

@@ -167,29 +167,29 @@ WebContents.prototype._sendInternalToAll = function (channel, ...args) {
 
   return this._send(internal, sendToAll, channel, args);
 };
-WebContents.prototype.sendToFrame = function (frameId, channel, ...args) {
+WebContents.prototype.sendToFrame = function (frame, channel, ...args) {
   if (typeof channel !== 'string') {
     throw new Error('Missing required channel argument');
-  } else if (typeof frameId !== 'number') {
-    throw new Error('Missing required frameId argument');
+  } else if (!(typeof frame === 'number' || Array.isArray(frame))) {
+    throw new Error('Missing required frame argument (must be number or array)');
   }
 
   const internal = false;
   const sendToAll = false;
 
-  return this._sendToFrame(internal, sendToAll, frameId, channel, args);
+  return this._sendToFrame(internal, sendToAll, frame, channel, args);
 };
-WebContents.prototype._sendToFrameInternal = function (frameId, channel, ...args) {
+WebContents.prototype._sendToFrameInternal = function (frame, channel, ...args) {
   if (typeof channel !== 'string') {
     throw new Error('Missing required channel argument');
-  } else if (typeof frameId !== 'number') {
-    throw new Error('Missing required frameId argument');
+  } else if (!(typeof frame === 'number' || Array.isArray(frame))) {
+    throw new Error('Missing required frame argument (must be number or array)');
   }
 
   const internal = true;
   const sendToAll = false;
 
-  return this._sendToFrame(internal, sendToAll, frameId, channel, args);
+  return this._sendToFrame(internal, sendToAll, frame, channel, args);
 };
 
 // Following methods are mapped to webFrame.
@@ -445,8 +445,9 @@ WebContents.prototype.loadFile = function (filePath, options = {}) {
 };
 
 const addReplyToEvent = (event) => {
+  const { processId, frameId } = event;
   event.reply = (...args) => {
-    event.sender.sendToFrame(event.frameId, ...args);
+    event.sender.sendToFrame([processId, frameId], ...args);
   };
 };
 

lib/browser/remote/server.ts

@@ -276,7 +276,7 @@ const fakeConstructor = (constructor: Function, name: string) =>
   });
 
 // Convert array of meta data from renderer into array of real values.
-const unwrapArgs = function (sender: electron.WebContents, frameId: number, contextId: string, args: any[]) {
+const unwrapArgs = function (sender: electron.WebContents, frameId: [number, number], contextId: string, args: any[]) {
   const metaToValue = function (meta: MetaTypeFromRenderer): any {
     switch (meta.type) {
       case 'nativeimage':
@@ -331,7 +331,7 @@ const unwrapArgs = function (sender: electron.WebContents, frameId: number, cont
         v8Util.setHiddenValue(callIntoRenderer, 'location', meta.location);
         Object.defineProperty(callIntoRenderer, 'length', { value: meta.length });
 
-        v8Util.setRemoteCallbackFreer(callIntoRenderer, frameId, contextId, meta.id, sender);
+        v8Util.setRemoteCallbackFreer(callIntoRenderer, frameId[0], frameId[1], contextId, meta.id, sender);
         rendererFunctions.set(objectId, callIntoRenderer);
         return callIntoRenderer;
       }
@@ -480,7 +480,7 @@ handleRemoteCommand('ELECTRON_BROWSER_CURRENT_WEB_CONTENTS', function (event, co
 });
 
 handleRemoteCommand('ELECTRON_BROWSER_CONSTRUCTOR', function (event, contextId, id, args) {
-  args = unwrapArgs(event.sender, event.frameId, contextId, args);
+  args = unwrapArgs(event.sender, [event.processId, event.frameId], contextId, args);
   const constructor = objectsRegistry.get(id);
 
   if (constructor == null) {
@@ -491,7 +491,7 @@ handleRemoteCommand('ELECTRON_BROWSER_CONSTRUCTOR', function (event, contextId,
 });
 
 handleRemoteCommand('ELECTRON_BROWSER_FUNCTION_CALL', function (event, contextId, id, args) {
-  args = unwrapArgs(event.sender, event.frameId, contextId, args);
+  args = unwrapArgs(event.sender, [event.processId, event.frameId], contextId, args);
   const func = objectsRegistry.get(id);
 
   if (func == null) {
@@ -508,7 +508,7 @@ handleRemoteCommand('ELECTRON_BROWSER_FUNCTION_CALL', function (event, contextId
 });
 
 handleRemoteCommand('ELECTRON_BROWSER_MEMBER_CONSTRUCTOR', function (event, contextId, id, method, args) {
-  args = unwrapArgs(event.sender, event.frameId, contextId, args);
+  args = unwrapArgs(event.sender, [event.processId, event.frameId], contextId, args);
   const object = objectsRegistry.get(id);
 
   if (object == null) {
@@ -519,7 +519,7 @@ handleRemoteCommand('ELECTRON_BROWSER_MEMBER_CONSTRUCTOR', function (event, cont
 });
 
 handleRemoteCommand('ELECTRON_BROWSER_MEMBER_CALL', function (event, contextId, id, method, args) {
-  args = unwrapArgs(event.sender, event.frameId, contextId, args);
+  args = unwrapArgs(event.sender, [event.processId, event.frameId], contextId, args);
   const object = objectsRegistry.get(id);
 
   if (object == null) {
@@ -536,7 +536,7 @@ handleRemoteCommand('ELECTRON_BROWSER_MEMBER_CALL', function (event, contextId,
 });
 
 handleRemoteCommand('ELECTRON_BROWSER_MEMBER_SET', function (event, contextId, id, name, args) {
-  args = unwrapArgs(event.sender, event.frameId, contextId, args);
+  args = unwrapArgs(event.sender, [event.processId, event.frameId], contextId, args);
   const obj = objectsRegistry.get(id);
 
   if (obj == null) {

shell/browser/api/electron_api_web_contents.cc

@@ -2354,7 +2354,7 @@ bool WebContents::SendIPCMessageWithSender(bool internal,
 
 bool WebContents::SendIPCMessageToFrame(bool internal,
                                         bool send_to_all,
-                                        int32_t frame_id,
+                                        v8::Local<v8::Value> frame,
                                         const std::string& channel,
                                         v8::Local<v8::Value> args) {
   blink::CloneableMessage message;
@@ -2363,17 +2363,30 @@ bool WebContents::SendIPCMessageToFrame(bool internal,
         gin::StringToV8(isolate(), "Failed to serialize arguments")));
     return false;
   }
-  auto frames = web_contents()->GetAllFrames();
-  auto iter = std::find_if(frames.begin(), frames.end(), [frame_id](auto* f) {
-    return f->GetRoutingID() == frame_id;
-  });
-  if (iter == frames.end())
-    return false;
-  if (!(*iter)->IsRenderFrameLive())
+  int32_t frame_id;
+  int32_t process_id;
+  if (gin::ConvertFromV8(isolate(), frame, &frame_id)) {
+    process_id = web_contents()->GetMainFrame()->GetProcess()->GetID();
+  } else {
+    std::vector<int32_t> id_pair;
+    if (gin::ConvertFromV8(isolate(), frame, &id_pair) && id_pair.size() == 2) {
+      process_id = id_pair[0];
+      frame_id = id_pair[1];
+    } else {
+      isolate()->ThrowException(v8::Exception::Error(gin::StringToV8(
+          isolate(),
+          "frameId must be a number or a pair of [processId, frameId]")));
+      return false;
+    }
+  }
+
+  auto* rfh = content::RenderFrameHost::FromID(process_id, frame_id);
+  if (!rfh || !rfh->IsRenderFrameLive() ||
+      content::WebContents::FromRenderFrameHost(rfh) != web_contents())
     return false;
 
   mojo::AssociatedRemote<mojom::ElectronRenderer> electron_renderer;
-  (*iter)->GetRemoteAssociatedInterfaces()->GetInterface(&electron_renderer);
+  rfh->GetRemoteAssociatedInterfaces()->GetInterface(&electron_renderer);
   electron_renderer->Message(internal, send_to_all, channel, std::move(message),
                              0 /* sender_id */);
   return true;

shell/browser/api/electron_api_web_contents.h

@@ -276,7 +276,7 @@ class WebContents : public gin_helper::TrackableObject<WebContents>,
 
   bool SendIPCMessageToFrame(bool internal,
                              bool send_to_all,
-                             int32_t frame_id,
+                             v8::Local<v8::Value> frame,
                              const std::string& channel,
                              v8::Local<v8::Value> args);
 

shell/common/api/remote/remote_callback_freer.cc

@@ -17,39 +17,40 @@ namespace electron {
 // static
 void RemoteCallbackFreer::BindTo(v8::Isolate* isolate,
                                  v8::Local<v8::Object> target,
+                                 int process_id,
                                  int frame_id,
                                  const std::string& context_id,
                                  int object_id,
                                  content::WebContents* web_contents) {
-  new RemoteCallbackFreer(isolate, target, frame_id, context_id, object_id,
-                          web_contents);
+  new RemoteCallbackFreer(isolate, target, process_id, frame_id, context_id,
+                          object_id, web_contents);
 }
 
 RemoteCallbackFreer::RemoteCallbackFreer(v8::Isolate* isolate,
                                          v8::Local<v8::Object> target,
+                                         int process_id,
                                          int frame_id,
                                          const std::string& context_id,
                                          int object_id,
                                          content::WebContents* web_contents)
     : ObjectLifeMonitor(isolate, target),
       content::WebContentsObserver(web_contents),
+      process_id_(process_id),
       frame_id_(frame_id),
       context_id_(context_id),
       object_id_(object_id) {}
 
 RemoteCallbackFreer::~RemoteCallbackFreer() = default;
 
 void RemoteCallbackFreer::RunDestructor() {
-  auto frames = web_contents()->GetAllFrames();
-  auto iter = std::find_if(frames.begin(), frames.end(), [this](auto* f) {
-    return f->GetRoutingID() == frame_id_;
-  });
-
-  if (iter != frames.end() && (*iter)->IsRenderFrameLive()) {
-    mojo::AssociatedRemote<mojom::ElectronRenderer> electron_renderer;
-    (*iter)->GetRemoteAssociatedInterfaces()->GetInterface(&electron_renderer);
-    electron_renderer->DereferenceRemoteJSCallback(context_id_, object_id_);
-  }
+  auto* rfh = content::RenderFrameHost::FromID(process_id_, frame_id_);
+  if (!rfh || !rfh->IsRenderFrameLive() ||
+      content::WebContents::FromRenderFrameHost(rfh) != web_contents())
+    return;
+
+  mojo::AssociatedRemote<mojom::ElectronRenderer> electron_renderer;
+  rfh->GetRemoteAssociatedInterfaces()->GetInterface(&electron_renderer);
+  electron_renderer->DereferenceRemoteJSCallback(context_id_, object_id_);
 
   Observe(nullptr);
 }

shell/common/api/remote/remote_callback_freer.h

@@ -17,6 +17,7 @@ class RemoteCallbackFreer : public ObjectLifeMonitor,
  public:
   static void BindTo(v8::Isolate* isolate,
                      v8::Local<v8::Object> target,
+                     int process_id,
                      int frame_id,
                      const std::string& context_id,
                      int object_id,
@@ -25,6 +26,7 @@ class RemoteCallbackFreer : public ObjectLifeMonitor,
  protected:
   RemoteCallbackFreer(v8::Isolate* isolate,
                       v8::Local<v8::Object> target,
+                      int process_id,
                       int frame_id,
                       const std::string& context_id,
                       int object_id,
@@ -37,6 +39,7 @@ class RemoteCallbackFreer : public ObjectLifeMonitor,
   void RenderViewDeleted(content::RenderViewHost*) override;
 
  private:
+  int process_id_;
   int frame_id_;
   std::string context_id_;
   int object_id_;

shell/common/gin_helper/event_emitter.cc

@@ -5,6 +5,7 @@
 #include "shell/common/gin_helper/event_emitter.h"
 
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
 #include "shell/browser/api/event.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/object_template_builder.h"
@@ -67,8 +68,10 @@ v8::Local<v8::Object> CreateNativeEvent(
   Dictionary dict(isolate, event);
   dict.Set("sender", sender);
   // Should always set frameId even when callback is null.
-  if (frame)
+  if (frame) {
     dict.Set("frameId", frame->GetRoutingID());
+    dict.Set("processId", frame->GetProcess()->GetID());
+  }
   return event;
 }
 

spec-main/api-ipc-main-spec.ts

@@ -46,6 +46,7 @@ describe('ipc main module', () => {
   });
 
   describe('ipcMain.on', () => {
+    afterEach(() => { ipcMain.removeAllListeners('test-echo'); });
     it('is not used for internals', async () => {
       const appPath = path.join(fixtures, 'api', 'ipc-main-listeners');
       const electronPath = process.execPath;
@@ -59,5 +60,27 @@ describe('ipc main module', () => {
       output = JSON.parse(output);
       expect(output).to.deep.equal(['error']);
     });
+
+    it('can be replied to', async () => {
+      ipcMain.on('test-echo', (e, arg) => {
+        e.reply('test-echo', arg);
+      });
+
+      const w = new BrowserWindow({
+        show: false,
+        webPreferences: {
+          nodeIntegration: true
+        }
+      });
+      w.loadURL('about:blank');
+      const v = await w.webContents.executeJavaScript(`new Promise((resolve, reject) => {
+        const { ipcRenderer } = require('electron')
+        ipcRenderer.send('test-echo', 'hello')
+        ipcRenderer.on('test-echo', (e, v) => {
+          resolve(v)
+        })
+      })`);
+      expect(v).to.equal('hello');
+    });
   });
 });

spec-main/fixtures/snapshots/proxy-window-open.snapshot.txt

@@ -3,7 +3,8 @@
     "top=5,left=10,resizable=no",
     {
       "sender": "[WebContents]",
-      "frameId": 1
+      "frameId": 1,
+      "processId": "placeholder-process-id"
     },
     "about:blank",
     "frame name",
@@ -36,7 +37,8 @@
     "zoomFactor=2,resizable=0,x=0,y=10",
     {
       "sender": "[WebContents]",
-      "frameId": 1
+      "frameId": 1,
+      "processId": "placeholder-process-id"
     },
     "about:blank",
     "frame name",
@@ -68,7 +70,8 @@
     "backgroundColor=gray,webPreferences=0,x=100,y=100",
     {
       "sender": "[WebContents]",
-      "frameId": 1
+      "frameId": 1,
+      "processId": "placeholder-process-id"
     },
     "about:blank",
     "frame name",
@@ -100,7 +103,8 @@
     "x=50,y=20,title=sup",
     {
       "sender": "[WebContents]",
-      "frameId": 1
+      "frameId": 1,
+      "processId": "placeholder-process-id"
     },
     "about:blank",
     "frame name",
@@ -130,7 +134,8 @@
     "show=false,top=1,left=1",
     {
       "sender": "[WebContents]",
-      "frameId": 1
+      "frameId": 1,
+      "processId": "placeholder-process-id"
     },
     "about:blank",
     "frame name",

spec-main/guest-window-manager-spec.ts

@@ -94,6 +94,9 @@ function stringifySnapshots (snapshots: any, pretty = false) {
     if (key === 'openerId' && typeof value === 'number') {
       return 'placeholder-opener-id';
     }
+    if (key === 'processId' && typeof value === 'number') {
+      return 'placeholder-process-id';
+    }
     if (key === 'returnValue') {
       return 'placeholder-guest-contents-id';
     }

typings/internal-ambient.d.ts

@@ -40,7 +40,7 @@ declare namespace NodeJS {
     requestGarbageCollectionForTesting(): void;
     createIDWeakMap<V>(): ElectronInternal.KeyWeakMap<number, V>;
     createDoubleIDWeakMap<V>(): ElectronInternal.KeyWeakMap<[string, number], V>;
-    setRemoteCallbackFreer(fn: Function, frameId: number, contextId: String, id: number, sender: any): void
+    setRemoteCallbackFreer(fn: Function, processId: number, frameId: number, contextId: String, id: number, sender: any): void
     weaklyTrackValue(value: any): void;
     clearWeaklyTrackedValues(): void;
     getWeaklyTrackedValues(): any[];