Repository: electron/electron
Pull request: fix: `postMessage` crash with invalid transferrable

#### Description of Change

Refs https://github.com/microsoft/vscode/issues/192175

Second effort at this with feedback incorporated from https://github.com/electron/electron/pull/41504/files#r1516814893

Fixes a potential crash in `utilityProcess.postMessage` when calling with an invalid transferable. This happened when passing e.g. an (unsupported) ArrayBuffer, because we try to unwrap everything in the transferrables list and upstream's [WrapperInfo ](https://source.chromium.org/chromium/chromium/src/+/main:gin/wrapper_info.cc;l=10-16;drc=0e9a0b6e9bb6ec59521977eec805f5d0bca833e0) logic doesn't take into account that an object might have the correct number of internal fields but not be a `WrappableBase`.

This PR also removes a check in `message_port.cc` for at least one argument being passed. The type of message is any per spec, and thus null or undefined is acceptable. As it was, if null or undefined were passed the error wouldn't be thrown regardless, as those values can be converted to v8::Value.

#### Checklist
<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->

- [x] PR description included and stakeholders cc'd
- [x] `npm test` passes
- [x] tests are [changed or added](https://github.com/electron/electron/blob/main/docs/development/testing.md)
- [x] [PR release notes](https://github.com/electron/clerk/blob/main/README.md) describe the change in a way relevant to app developers, and are [capitalized, punctuated, and past tense](https://github.com/electron/clerk/blob/main/README.md#examples).

#### Release Notes

Notes: Fixed a potential crash in `utilityProcess.postMessage` when calling with an invalid transferable.


Changed files:
filename: shell/browser/api/electron_api_utility_process.cc
patch: @@ -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<v8::Value> 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<v8::Value> transferables;
   std::vector<gin::Handle<MessagePort>> wrapped_ports;
   if (args->GetNext(&transferables)) {
+    std::vector<v8::Local<v8::Value>> 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;
     }
   }

filename: shell/browser/api/message_port.cc
patch: @@ -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<v8::Value>& val) {
-  if (!val->IsObject())
-    return false;
-
-  v8::Local<v8::Object> port = val.As<v8::Object>();
-
-  if (port->InternalFieldCount() != gin::kNumberOfInternalFields)
-    return false;
-
-  const auto* info = static_cast<gin::WrapperInfo*>(
-      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<v8::Value> 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<v8::Value> 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;

filename: shell/common/gin_helper/wrappable.cc
patch: @@ -10,6 +10,23 @@
 
 namespace gin_helper {
 
+bool IsValidWrappable(const v8::Local<v8::Value>& val,
+                      const gin::WrapperInfo* wrapper_info) {
+  if (!val->IsObject())
+    return false;
+
+  v8::Local<v8::Object> port = val.As<v8::Object>();
+  if (port->InternalFieldCount() != gin::kNumberOfInternalFields)
+    return false;
+
+  const gin::WrapperInfo* info = static_cast<gin::WrapperInfo*>(
+      port->GetAlignedPointerFromInternalField(gin::kWrapperInfoIndex));
+  if (info != wrapper_info)
+    return false;
+
+  return true;
+}
+
 WrappableBase::WrappableBase() = default;
 
 WrappableBase::~WrappableBase() {

filename: shell/common/gin_helper/wrappable.h
patch: @@ -11,6 +11,9 @@
 
 namespace gin_helper {
 
+bool IsValidWrappable(const v8::Local<v8::Value>& obj,
+                      const gin::WrapperInfo* wrapper_info);
+
 namespace internal {
 
 void* FromV8Impl(v8::Isolate* isolate, v8::Local<v8::Value> val);

filename: spec/api-ipc-spec.ts
patch: @@ -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();