From 301f7b4e6467ff15908fe646bc4c1c25da80a792 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Thu, 17 Apr 2025 18:23:23 +0200 Subject: [PATCH] fix: `postMessage` crash with invalid transferrable (#46667) * fix: postMessage crash with invalid transferrable Co-authored-by: Shelley Vohr * chore: address review feedback Co-authored-by: Charles Kerr Co-authored-by: Shelley Vohr --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr --- .../api/electron_api_utility_process.cc | 23 ++++++++++- shell/browser/api/message_port.cc | 39 +++++-------------- shell/common/gin_helper/wrappable.cc | 17 ++++++++ shell/common/gin_helper/wrappable.h | 3 ++ spec/api-ipc-spec.ts | 31 +++++++++++++++ 5 files changed, 82 insertions(+), 31 deletions(-) diff --git a/shell/browser/api/electron_api_utility_process.cc b/shell/browser/api/electron_api_utility_process.cc index 573d38448e16..d9ff0996d91d 100644 --- a/shell/browser/api/electron_api_utility_process.cc +++ b/shell/browser/api/electron_api_utility_process.cc @@ -330,6 +330,9 @@ void UtilityProcessWrapper::PostMessage(gin::Arguments* args) { return; blink::TransferableMessage transferable_message; + gin_helper::ErrorThrower thrower(args->isolate()); + + // |message| is any value that can be serialized to StructuredClone. v8::Local message_value; if (args->GetNext(&message_value)) { if (!electron::SerializeV8Value(args->isolate(), message_value, @@ -342,9 +345,25 @@ void UtilityProcessWrapper::PostMessage(gin::Arguments* args) { v8::Local transferables; std::vector> wrapped_ports; if (args->GetNext(&transferables)) { + std::vector> wrapped_port_values; + if (!gin::ConvertFromV8(args->isolate(), transferables, + &wrapped_port_values)) { + thrower.ThrowTypeError("transferables must be an array of MessagePorts"); + return; + } + + for (size_t i = 0; i < wrapped_port_values.size(); ++i) { + if (!gin_helper::IsValidWrappable(wrapped_port_values[i], + &MessagePort::kWrapperInfo)) { + thrower.ThrowTypeError( + base::StrCat({"Port at index ", base::NumberToString(i), + " is not a valid port"})); + return; + } + } + if (!gin::ConvertFromV8(args->isolate(), transferables, &wrapped_ports)) { - gin_helper::ErrorThrower(args->isolate()) - .ThrowTypeError("Invalid value for transfer"); + thrower.ThrowTypeError("Passed an invalid MessagePort"); return; } } diff --git a/shell/browser/api/message_port.cc b/shell/browser/api/message_port.cc index 7d48119484cb..4ea7f19e8dd3 100644 --- a/shell/browser/api/message_port.cc +++ b/shell/browser/api/message_port.cc @@ -17,6 +17,7 @@ #include "shell/common/gin_helper/dictionary.h" #include "shell/common/gin_helper/error_thrower.h" #include "shell/common/gin_helper/event_emitter_caller.h" +#include "shell/common/gin_helper/wrappable.h" #include "shell/common/node_includes.h" #include "shell/common/v8_util.h" #include "third_party/abseil-cpp/absl/container/flat_hash_set.h" @@ -26,25 +27,6 @@ namespace electron { -namespace { - -bool IsValidWrappable(const v8::Local& val) { - if (!val->IsObject()) - return false; - - v8::Local port = val.As(); - - if (port->InternalFieldCount() != gin::kNumberOfInternalFields) - return false; - - const auto* info = static_cast( - port->GetAlignedPointerFromInternalField(gin::kWrapperInfoIndex)); - - return info && info->embedder == gin::kEmbedderNativeGin; -} - -} // namespace - gin::WrapperInfo MessagePort::kWrapperInfo = {gin::kEmbedderNativeGin}; MessagePort::MessagePort() = default; @@ -77,16 +59,14 @@ void MessagePort::PostMessage(gin::Arguments* args) { blink::TransferableMessage transferable_message; gin_helper::ErrorThrower thrower(args->isolate()); + // |message| is any value that can be serialized to StructuredClone. v8::Local message_value; - if (!args->GetNext(&message_value)) { - thrower.ThrowTypeError("Expected at least one argument to postMessage"); - return; - } - - if (!electron::SerializeV8Value(args->isolate(), message_value, - &transferable_message)) { - // SerializeV8Value sets an exception. - return; + if (args->GetNext(&message_value)) { + if (!electron::SerializeV8Value(args->isolate(), message_value, + &transferable_message)) { + // SerializeV8Value sets an exception. + return; + } } v8::Local transferables; @@ -100,7 +80,8 @@ void MessagePort::PostMessage(gin::Arguments* args) { } for (unsigned i = 0; i < wrapped_port_values.size(); ++i) { - if (!IsValidWrappable(wrapped_port_values[i])) { + if (!gin_helper::IsValidWrappable(wrapped_port_values[i], + &MessagePort::kWrapperInfo)) { thrower.ThrowTypeError("Port at index " + base::NumberToString(i) + " is not a valid port"); return; diff --git a/shell/common/gin_helper/wrappable.cc b/shell/common/gin_helper/wrappable.cc index 1d0f45e004e4..5b4a0369441c 100644 --- a/shell/common/gin_helper/wrappable.cc +++ b/shell/common/gin_helper/wrappable.cc @@ -10,6 +10,23 @@ namespace gin_helper { +bool IsValidWrappable(const v8::Local& val, + const gin::WrapperInfo* wrapper_info) { + if (!val->IsObject()) + return false; + + v8::Local port = val.As(); + if (port->InternalFieldCount() != gin::kNumberOfInternalFields) + return false; + + const gin::WrapperInfo* info = static_cast( + port->GetAlignedPointerFromInternalField(gin::kWrapperInfoIndex)); + if (info != wrapper_info) + return false; + + return true; +} + WrappableBase::WrappableBase() = default; WrappableBase::~WrappableBase() { diff --git a/shell/common/gin_helper/wrappable.h b/shell/common/gin_helper/wrappable.h index 531d446f7673..76c0faeeebf0 100644 --- a/shell/common/gin_helper/wrappable.h +++ b/shell/common/gin_helper/wrappable.h @@ -11,6 +11,9 @@ namespace gin_helper { +bool IsValidWrappable(const v8::Local& obj, + const gin::WrapperInfo* wrapper_info); + namespace internal { void* FromV8Impl(v8::Isolate* isolate, v8::Local val); diff --git a/spec/api-ipc-spec.ts b/spec/api-ipc-spec.ts index 88eaaf4a0b30..ceda0926e261 100644 --- a/spec/api-ipc-spec.ts +++ b/spec/api-ipc-spec.ts @@ -236,6 +236,23 @@ describe('ipc module', () => { expect(ev.senderFrame.routingId).to.equal(w.webContents.mainFrame.routingId); }); + it('throws when the transferable is invalid', async () => { + const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, contextIsolation: false } }); + w.loadURL('about:blank'); + const p = once(ipcMain, 'port'); + await w.webContents.executeJavaScript(`(${function () { + try { + const buffer = new ArrayBuffer(10); + // @ts-expect-error + require('electron').ipcRenderer.postMessage('port', '', [buffer]); + } catch (e) { + require('electron').ipcRenderer.postMessage('port', { error: (e as Error).message }); + } + }})()`); + const [, msg] = await p; + expect(msg.error).to.eql('Invalid value for transfer'); + }); + it('can communicate between main and renderer', async () => { const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, contextIsolation: false } }); w.loadURL('about:blank'); @@ -411,6 +428,20 @@ describe('ipc module', () => { expect(port2).not.to.be.null(); }); + it('should not throw when supported values are passed as message', () => { + const { port1 } = new MessageChannelMain(); + + // @ts-expect-error - this shouldn't crash. + expect(() => { port1.postMessage(); }).to.not.throw(); + + expect(() => { port1.postMessage(undefined); }).to.not.throw(); + expect(() => { port1.postMessage(42); }).to.not.throw(); + expect(() => { port1.postMessage(false); }).to.not.throw(); + expect(() => { port1.postMessage([]); }).to.not.throw(); + expect(() => { port1.postMessage('hello'); }).to.not.throw(); + expect(() => { port1.postMessage({ hello: 'goodbye' }); }).to.not.throw(); + }); + it('throws an error when an invalid parameter is sent to postMessage', () => { const { port1 } = new MessageChannelMain();