From d32033bdbcdb20c7ef75793003c707b51e165749 Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Fri, 11 Dec 2020 13:01:40 -0800 Subject: [PATCH] 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 + docs/api/structures/ipc-main-invoke-event.md | 1 + docs/api/web-contents.md | 2 +- lib/browser/api/web-contents.js | 19 ++++++------ lib/browser/remote/server.ts | 14 ++++----- .../browser/api/electron_api_web_contents.cc | 31 +++++++++++++------ shell/browser/api/electron_api_web_contents.h | 2 +- .../api/remote/remote_callback_freer.cc | 25 ++++++++------- .../common/api/remote/remote_callback_freer.h | 3 ++ shell/common/gin_helper/event_emitter.cc | 5 ++- spec-main/api-ipc-main-spec.ts | 23 ++++++++++++++ .../snapshots/proxy-window-open.snapshot.txt | 15 ++++++--- spec-main/guest-window-manager-spec.ts | 3 ++ typings/internal-ambient.d.ts | 2 +- typings/internal-electron.d.ts | 5 +++ 15 files changed, 105 insertions(+), 46 deletions(-) diff --git a/docs/api/structures/ipc-main-event.md b/docs/api/structures/ipc-main-event.md index f222de35b86dc..ae32dc46a6888 100644 --- a/docs/api/structures/ipc-main-event.md +++ b/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 diff --git a/docs/api/structures/ipc-main-invoke-event.md b/docs/api/structures/ipc-main-invoke-event.md index 235b219c2d6ea..b765791e8258c 100644 --- a/docs/api/structures/ipc-main-invoke-event.md +++ b/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 diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index 366760ec2c6e6..cacbfad77a2bf 100644 --- a/docs/api/web-contents.md +++ b/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[] diff --git a/lib/browser/api/web-contents.js b/lib/browser/api/web-contents.js index 7a8385e59014b..492db24e200b3 100644 --- a/lib/browser/api/web-contents.js +++ b/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); }; }; diff --git a/lib/browser/remote/server.ts b/lib/browser/remote/server.ts index 2ed49817a8c0b..038e83a550af3 100644 --- a/lib/browser/remote/server.ts +++ b/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) { diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index fa521e25e643b..fd23541eb715a 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/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 frame, const std::string& channel, v8::Local 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 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 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; diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index 92a6ef63a7683..c8ea073b5b783 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -276,7 +276,7 @@ class WebContents : public gin_helper::TrackableObject, bool SendIPCMessageToFrame(bool internal, bool send_to_all, - int32_t frame_id, + v8::Local frame, const std::string& channel, v8::Local args); diff --git a/shell/common/api/remote/remote_callback_freer.cc b/shell/common/api/remote/remote_callback_freer.cc index 40dd4a706d6cb..e61c38ae6cabd 100644 --- a/shell/common/api/remote/remote_callback_freer.cc +++ b/shell/common/api/remote/remote_callback_freer.cc @@ -17,22 +17,25 @@ namespace electron { // static void RemoteCallbackFreer::BindTo(v8::Isolate* isolate, v8::Local 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 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) {} @@ -40,16 +43,14 @@ RemoteCallbackFreer::RemoteCallbackFreer(v8::Isolate* isolate, 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 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 electron_renderer; + rfh->GetRemoteAssociatedInterfaces()->GetInterface(&electron_renderer); + electron_renderer->DereferenceRemoteJSCallback(context_id_, object_id_); Observe(nullptr); } diff --git a/shell/common/api/remote/remote_callback_freer.h b/shell/common/api/remote/remote_callback_freer.h index 7e9d95a2e06d3..6c43a162808ef 100644 --- a/shell/common/api/remote/remote_callback_freer.h +++ b/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 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 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_; diff --git a/shell/common/gin_helper/event_emitter.cc b/shell/common/gin_helper/event_emitter.cc index ebb52d2ac90ca..710147b54f156 100644 --- a/shell/common/gin_helper/event_emitter.cc +++ b/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 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; } diff --git a/spec-main/api-ipc-main-spec.ts b/spec-main/api-ipc-main-spec.ts index 3f54527a5b497..9b3aef4474e97 100644 --- a/spec-main/api-ipc-main-spec.ts +++ b/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'); + }); }); }); diff --git a/spec-main/fixtures/snapshots/proxy-window-open.snapshot.txt b/spec-main/fixtures/snapshots/proxy-window-open.snapshot.txt index 66020217d7f5c..c0d1fb2d2ab79 100644 --- a/spec-main/fixtures/snapshots/proxy-window-open.snapshot.txt +++ b/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", diff --git a/spec-main/guest-window-manager-spec.ts b/spec-main/guest-window-manager-spec.ts index c8b9f216f50af..9745dbd428924 100644 --- a/spec-main/guest-window-manager-spec.ts +++ b/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'; } diff --git a/typings/internal-ambient.d.ts b/typings/internal-ambient.d.ts index 04c73b94c1dbd..3f99dfc10e895 100644 --- a/typings/internal-ambient.d.ts +++ b/typings/internal-ambient.d.ts @@ -40,7 +40,7 @@ declare namespace NodeJS { requestGarbageCollectionForTesting(): void; createIDWeakMap(): ElectronInternal.KeyWeakMap; createDoubleIDWeakMap(): 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[]; diff --git a/typings/internal-electron.d.ts b/typings/internal-electron.d.ts index 5e6a9d51f3b40..99ccb44d9b681 100644 --- a/typings/internal-electron.d.ts +++ b/typings/internal-electron.d.ts @@ -35,6 +35,11 @@ declare namespace Electron { _initiallyShown: boolean; } + interface WebPreferences { + guestInstanceId?: number; + openerId?: number; + } + interface SerializedError { message: string; stack?: string,