From 879c9322ceb80cea6ee1dd7fd893597a84bd6cf1 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Tue, 9 Jun 2020 09:25:39 -0500
Subject: [PATCH] vm: allow proxy callbacks to throw
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes: https://github.com/nodejs/node/issues/33806

PR-URL: https://github.com/nodejs/node/pull/33808
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 src/node_contextify.cc                        | 12 +++++------
 .../test-vm-context-property-forwarding.js    | 20 +++++++++++++++++++
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 69663afe423f58..536d761c637928 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -422,7 +422,7 @@ void ContextifyContext::PropertySetterCallback(
     args.GetReturnValue().Set(false);
   }
 
-  ctx->sandbox()->Set(ctx->context(), property, value).Check();
+  USE(ctx->sandbox()->Set(ctx->context(), property, value));
 }
 
 // static
@@ -440,9 +440,10 @@ void ContextifyContext::PropertyDescriptorCallback(
   Local<Object> sandbox = ctx->sandbox();
 
   if (sandbox->HasOwnProperty(context, property).FromMaybe(false)) {
-    args.GetReturnValue().Set(
-        sandbox->GetOwnPropertyDescriptor(context, property)
-            .ToLocalChecked());
+    Local<Value> desc;
+    if (sandbox->GetOwnPropertyDescriptor(context, property).ToLocal(&desc)) {
+      args.GetReturnValue().Set(desc);
+    }
   }
 }
 
@@ -485,8 +486,7 @@ void ContextifyContext::PropertyDefinerCallback(
           desc_for_sandbox->set_configurable(desc.configurable());
         }
         // Set the property on the sandbox.
-        sandbox->DefineProperty(context, property, *desc_for_sandbox)
-            .Check();
+        USE(sandbox->DefineProperty(context, property, *desc_for_sandbox));
       };
 
   if (desc.has_get() || desc.has_set()) {
diff --git a/test/parallel/test-vm-context-property-forwarding.js b/test/parallel/test-vm-context-property-forwarding.js
index 800a9fa2438988..53d38c1467a5a2 100644
--- a/test/parallel/test-vm-context-property-forwarding.js
+++ b/test/parallel/test-vm-context-property-forwarding.js
@@ -43,3 +43,23 @@ assert.deepStrictEqual(pd_actual, pd_expected);
 assert.strictEqual(ctx2[1], 5);
 delete ctx2[1];
 assert.strictEqual(ctx2[1], undefined);
+
+// https://github.com/nodejs/node/issues/33806
+{
+  const ctx = vm.createContext();
+
+  Object.defineProperty(ctx, 'prop', {
+    get() {
+      return undefined;
+    },
+    set(val) {
+      throw new Error('test error');
+    },
+  });
+
+  assert.throws(() => {
+    vm.runInContext('prop = 42', ctx);
+  }, {
+    message: 'test error',
+  });
+}