From 34d988f122c206da692b5c39ec5342fec22b60ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= <targos@protonmail.com>
Date: Fri, 16 Mar 2018 15:23:39 +0100
Subject: [PATCH] vm: move options checks from C++ to JS

Also introduces stronger type validations for options passed to vm
functions.

PR-URL: https://github.com/nodejs/node/pull/19398
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 doc/api/vm.md                               |   3 +
 lib/internal/bootstrap/loaders.js           |  13 +-
 lib/internal/vm/module.js                   |   4 +
 lib/vm.js                                   | 223 +++++++-----
 src/env.h                                   |   1 -
 src/node_contextify.cc                      | 358 +++++++-------------
 src/node_contextify.h                       |  11 +-
 test/parallel/test-vm-basic.js              | 111 +++---
 test/parallel/test-vm-cached-data.js        |  10 +-
 test/parallel/test-vm-context.js            |  28 +-
 test/parallel/test-vm-is-context.js         |   7 -
 test/parallel/test-vm-options-validation.js |  78 +++++
 test/parallel/test-vm-timeout.js            |  20 +-
 13 files changed, 457 insertions(+), 410 deletions(-)
 create mode 100644 test/parallel/test-vm-options-validation.js

diff --git a/doc/api/vm.md b/doc/api/vm.md
index 25efc85cf66..3712fa4f2b1 100644
--- a/doc/api/vm.md
+++ b/doc/api/vm.md
@@ -618,6 +618,9 @@ console.log(globalVar);
 <!-- YAML
 added: v0.3.1
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/19398
+    description: The `sandbox` option can no longer be a function.
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/19016
     description: The `codeGeneration` option is supported now.
diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js
index fff82cd0e08..828370f6ead 100644
--- a/lib/internal/bootstrap/loaders.js
+++ b/lib/internal/bootstrap/loaders.js
@@ -88,12 +88,7 @@
     };
   }
 
-  // Minimal sandbox helper
   const ContextifyScript = process.binding('contextify').ContextifyScript;
-  function runInThisContext(code, options) {
-    const script = new ContextifyScript(code, options);
-    return script.runInThisContext();
-  }
 
   // Set up NativeModule
   function NativeModule(id) {
@@ -205,11 +200,9 @@
     this.loading = true;
 
     try {
-      const fn = runInThisContext(source, {
-        filename: this.filename,
-        lineOffset: 0,
-        displayErrors: true
-      });
+      const script = new ContextifyScript(source, this.filename);
+      // Arguments: timeout, displayErrors, breakOnSigint
+      const fn = script.runInThisContext(-1, true, false);
       const requireFn = this.id.startsWith('internal/deps/') ?
         NativeModule.requireForDeps :
         NativeModule.require;
diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js
index 9af071ce28c..a64ccfe3b2d 100644
--- a/lib/internal/vm/module.js
+++ b/lib/internal/vm/module.js
@@ -59,6 +59,10 @@ class Module {
 
     let context;
     if (options.context !== undefined) {
+      if (typeof options.context !== 'object' || options.context === null) {
+        throw new ERR_INVALID_ARG_TYPE('options.context', 'object',
+                                       options.context);
+      }
       if (isContext(options.context)) {
         context = options.context;
       } else {
diff --git a/lib/vm.js b/lib/vm.js
index 47eb1b851f0..5a5130d7c9c 100644
--- a/lib/vm.js
+++ b/lib/vm.js
@@ -24,63 +24,109 @@
 const {
   ContextifyScript,
   kParsingContext,
-
   makeContext,
   isContext: _isContext,
 } = process.binding('contextify');
 
 const {
   ERR_INVALID_ARG_TYPE,
-  ERR_MISSING_ARGS
+  ERR_OUT_OF_RANGE
 } = require('internal/errors').codes;
-
-// The binding provides a few useful primitives:
-// - Script(code, { filename = "evalmachine.anonymous",
-//                  displayErrors = true } = {})
-//   with methods:
-//   - runInThisContext({ displayErrors = true } = {})
-//   - runInContext(sandbox, { displayErrors = true, timeout = undefined } = {})
-// - makeContext(sandbox)
-// - isContext(sandbox)
-// From this we build the entire documented API.
+const { isUint8Array } = require('internal/util/types');
 
 class Script extends ContextifyScript {
-  constructor(code, options) {
+  constructor(code, options = {}) {
+    code = `${code}`;
+    if (typeof options === 'string') {
+      options = { filename: options };
+    }
+    if (typeof options !== 'object' || options === null) {
+      throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
+    }
+
+    const {
+      filename = 'evalmachine.<anonymous>',
+      lineOffset = 0,
+      columnOffset = 0,
+      cachedData,
+      produceCachedData = false,
+      [kParsingContext]: parsingContext
+    } = options;
+
+    if (typeof filename !== 'string') {
+      throw new ERR_INVALID_ARG_TYPE('options.filename', 'string', filename);
+    }
+    validateInteger(lineOffset, 'options.lineOffset');
+    validateInteger(columnOffset, 'options.columnOffset');
+    if (cachedData !== undefined && !isUint8Array(cachedData)) {
+      throw new ERR_INVALID_ARG_TYPE('options.cachedData',
+                                     ['Buffer', 'Uint8Array'], cachedData);
+    }
+    if (typeof produceCachedData !== 'boolean') {
+      throw new ERR_INVALID_ARG_TYPE('options.produceCachedData', 'boolean',
+                                     produceCachedData);
+    }
+
     // Calling `ReThrow()` on a native TryCatch does not generate a new
     // abort-on-uncaught-exception check. A dummy try/catch in JS land
     // protects against that.
     try {
-      super(code, options);
+      super(code,
+            filename,
+            lineOffset,
+            columnOffset,
+            cachedData,
+            produceCachedData,
+            parsingContext);
     } catch (e) {
       throw e; /* node-do-not-add-exception-line */
     }
   }
+
+  runInThisContext(options) {
+    const { breakOnSigint, args } = getRunInContextArgs(options);
+    if (breakOnSigint && process.listenerCount('SIGINT') > 0) {
+      return sigintHandlersWrap(super.runInThisContext, this, args);
+    } else {
+      return super.runInThisContext(...args);
+    }
+  }
+
+  runInContext(contextifiedSandbox, options) {
+    validateContext(contextifiedSandbox);
+    const { breakOnSigint, args } = getRunInContextArgs(options);
+    if (breakOnSigint && process.listenerCount('SIGINT') > 0) {
+      return sigintHandlersWrap(super.runInContext, this,
+                                [contextifiedSandbox, ...args]);
+    } else {
+      return super.runInContext(contextifiedSandbox, ...args);
+    }
+  }
+
+  runInNewContext(sandbox, options) {
+    const context = createContext(sandbox, getContextOptions(options));
+    return this.runInContext(context, options);
+  }
 }
 
-const realRunInThisContext = Script.prototype.runInThisContext;
-const realRunInContext = Script.prototype.runInContext;
-
-Script.prototype.runInThisContext = function(options) {
-  if (options && options.breakOnSigint && process.listenerCount('SIGINT') > 0) {
-    return sigintHandlersWrap(realRunInThisContext, this, [options]);
-  } else {
-    return realRunInThisContext.call(this, options);
+function validateContext(sandbox) {
+  if (typeof sandbox !== 'object' || sandbox === null) {
+    throw new ERR_INVALID_ARG_TYPE('contextifiedSandbox', 'Object', sandbox);
   }
-};
-
-Script.prototype.runInContext = function(contextifiedSandbox, options) {
-  if (options && options.breakOnSigint && process.listenerCount('SIGINT') > 0) {
-    return sigintHandlersWrap(realRunInContext, this,
-                              [contextifiedSandbox, options]);
-  } else {
-    return realRunInContext.call(this, contextifiedSandbox, options);
+  if (!_isContext(sandbox)) {
+    throw new ERR_INVALID_ARG_TYPE('contextifiedSandbox', 'vm.Context',
+                                   sandbox);
   }
-};
+}
 
-Script.prototype.runInNewContext = function(sandbox, options) {
-  const context = createContext(sandbox, getContextOptions(options));
-  return this.runInContext(context, options);
-};
+function validateInteger(prop, propName) {
+  if (!Number.isInteger(prop)) {
+    throw new ERR_INVALID_ARG_TYPE(propName, 'integer', prop);
+  }
+  if ((prop >> 0) !== prop) {
+    throw new ERR_OUT_OF_RANGE(propName, '32-bit integer', prop);
+  }
+}
 
 function validateString(prop, propName) {
   if (prop !== undefined && typeof prop !== 'string')
@@ -97,6 +143,39 @@ function validateObject(prop, propName) {
     throw new ERR_INVALID_ARG_TYPE(propName, 'Object', prop);
 }
 
+function getRunInContextArgs(options = {}) {
+  if (typeof options !== 'object' || options === null) {
+    throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
+  }
+
+  let timeout = options.timeout;
+  if (timeout === undefined) {
+    timeout = -1;
+  } else if (!Number.isInteger(timeout) || timeout <= 0) {
+    throw new ERR_INVALID_ARG_TYPE('options.timeout', 'a positive integer',
+                                   timeout);
+  }
+
+  const {
+    displayErrors = true,
+    breakOnSigint = false
+  } = options;
+
+  if (typeof displayErrors !== 'boolean') {
+    throw new ERR_INVALID_ARG_TYPE('options.displayErrors', 'boolean',
+                                   displayErrors);
+  }
+  if (typeof breakOnSigint !== 'boolean') {
+    throw new ERR_INVALID_ARG_TYPE('options.breakOnSigint', 'boolean',
+                                   breakOnSigint);
+  }
+
+  return {
+    breakOnSigint,
+    args: [timeout, displayErrors, breakOnSigint]
+  };
+}
+
 function getContextOptions(options) {
   if (options) {
     validateObject(options.contextCodeGeneration,
@@ -123,57 +202,43 @@ function getContextOptions(options) {
 }
 
 function isContext(sandbox) {
-  if (arguments.length < 1) {
-    throw new ERR_MISSING_ARGS('sandbox');
+  if (typeof sandbox !== 'object' || sandbox === null) {
+    throw new ERR_INVALID_ARG_TYPE('sandbox', 'Object', sandbox);
   }
-
-  if (typeof sandbox !== 'object' && typeof sandbox !== 'function' ||
-      sandbox === null) {
-    throw new ERR_INVALID_ARG_TYPE('sandbox', 'object', sandbox);
-  }
-
   return _isContext(sandbox);
 }
 
 let defaultContextNameIndex = 1;
-function createContext(sandbox, options) {
-  if (sandbox === undefined) {
-    sandbox = {};
-  } else if (isContext(sandbox)) {
+function createContext(sandbox = {}, options = {}) {
+  if (isContext(sandbox)) {
     return sandbox;
   }
 
-  if (options !== undefined) {
-    if (typeof options !== 'object' || options === null) {
-      throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
-    }
-    validateObject(options.codeGeneration, 'options.codeGeneration');
-    options = {
-      name: options.name,
-      origin: options.origin,
-      codeGeneration: typeof options.codeGeneration === 'object' ? {
-        strings: options.codeGeneration.strings,
-        wasm: options.codeGeneration.wasm,
-      } : undefined,
-    };
-    if (options.codeGeneration !== undefined) {
-      validateBool(options.codeGeneration.strings,
-                   'options.codeGeneration.strings');
-      validateBool(options.codeGeneration.wasm,
-                   'options.codeGeneration.wasm');
-    }
-    if (options.name === undefined) {
-      options.name = `VM Context ${defaultContextNameIndex++}`;
-    } else if (typeof options.name !== 'string') {
-      throw new ERR_INVALID_ARG_TYPE('options.name', 'string', options.name);
-    }
-    validateString(options.origin, 'options.origin');
-  } else {
-    options = {
-      name: `VM Context ${defaultContextNameIndex++}`
-    };
+  if (typeof options !== 'object' || options === null) {
+    throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
   }
-  makeContext(sandbox, options);
+
+  const {
+    name = `VM Context ${defaultContextNameIndex++}`,
+    origin,
+    codeGeneration
+  } = options;
+
+  if (typeof name !== 'string') {
+    throw new ERR_INVALID_ARG_TYPE('options.name', 'string', options.name);
+  }
+  validateString(origin, 'options.origin');
+  validateObject(codeGeneration, 'options.codeGeneration');
+
+  let strings = true;
+  let wasm = true;
+  if (codeGeneration !== undefined) {
+    ({ strings = true, wasm = true } = codeGeneration);
+    validateBool(strings, 'options.codeGeneration.strings');
+    validateBool(wasm, 'options.codeGeneration.wasm');
+  }
+
+  makeContext(sandbox, name, origin, strings, wasm);
   return sandbox;
 }
 
@@ -200,6 +265,7 @@ function sigintHandlersWrap(fn, thisArg, argsArray) {
 }
 
 function runInContext(code, contextifiedSandbox, options) {
+  validateContext(contextifiedSandbox);
   if (typeof options === 'string') {
     options = {
       filename: options,
@@ -226,6 +292,9 @@ function runInNewContext(code, sandbox, options) {
 }
 
 function runInThisContext(code, options) {
+  if (typeof options === 'string') {
+    options = { filename: options };
+  }
   return createScript(code, options).runInThisContext(options);
 }
 
diff --git a/src/env.h b/src/env.h
index a688b069242..e6a4e1dc05f 100644
--- a/src/env.h
+++ b/src/env.h
@@ -239,7 +239,6 @@ struct PackageConfig {
   V(port_string, "port")                                                      \
   V(preference_string, "preference")                                          \
   V(priority_string, "priority")                                              \
-  V(produce_cached_data_string, "produceCachedData")                          \
   V(promise_string, "promise")                                                \
   V(pubkey_string, "pubkey")                                                  \
   V(query_string, "query")                                                    \
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 42f66885bb8..c71e9b57896 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -40,6 +40,7 @@ using v8::FunctionTemplate;
 using v8::HandleScope;
 using v8::IndexedPropertyHandlerConfiguration;
 using v8::Integer;
+using v8::Isolate;
 using v8::Just;
 using v8::Local;
 using v8::Maybe;
@@ -96,8 +97,8 @@ Local<Name> Uint32ToName(Local<Context> context, uint32_t index) {
 
 ContextifyContext::ContextifyContext(
     Environment* env,
-    Local<Object> sandbox_obj, Local<Object> options_obj) : env_(env) {
-  Local<Context> v8_context = CreateV8Context(env, sandbox_obj, options_obj);
+    Local<Object> sandbox_obj, const ContextOptions& options) : env_(env) {
+  Local<Context> v8_context = CreateV8Context(env, sandbox_obj, options);
   context_.Reset(env->isolate(), v8_context);
 
   // Allocation failure or maximum call stack size reached
@@ -129,7 +130,7 @@ Local<Value> ContextifyContext::CreateDataWrapper(Environment* env) {
 Local<Context> ContextifyContext::CreateV8Context(
     Environment* env,
     Local<Object> sandbox_obj,
-    Local<Object> options_obj) {
+    const ContextOptions& options) {
   EscapableHandleScope scope(env->isolate());
   Local<FunctionTemplate> function_template =
       FunctionTemplate::New(env->isolate());
@@ -179,45 +180,15 @@ Local<Context> ContextifyContext::CreateV8Context(
                           env->contextify_global_private_symbol(),
                           ctx->Global());
 
-  Local<Value> name =
-      options_obj->Get(env->context(), env->name_string())
-          .ToLocalChecked();
-  CHECK(name->IsString());
-  Utf8Value name_val(env->isolate(), name);
-
-  Local<Value> codegen = options_obj->Get(env->context(),
-      FIXED_ONE_BYTE_STRING(env->isolate(), "codeGeneration"))
-          .ToLocalChecked();
-
-  if (!codegen->IsUndefined()) {
-    CHECK(codegen->IsObject());
-    Local<Object> codegen_obj = codegen.As<Object>();
-
-    Local<Value> allow_code_gen_from_strings =
-      codegen_obj->Get(env->context(),
-          FIXED_ONE_BYTE_STRING(env->isolate(), "strings"))
-      .ToLocalChecked();
-    ctx->AllowCodeGenerationFromStrings(
-        allow_code_gen_from_strings->IsUndefined() ||
-            allow_code_gen_from_strings->IsTrue());
-
-    Local<Value> allow_wasm_code_gen = codegen_obj->Get(env->context(),
-        FIXED_ONE_BYTE_STRING(env->isolate(), "wasm"))
-      .ToLocalChecked();
-    ctx->SetEmbedderData(ContextEmbedderIndex::kAllowWasmCodeGeneration,
-        Boolean::New(env->isolate(), allow_wasm_code_gen->IsUndefined() ||
-            allow_wasm_code_gen->IsTrue()));
-  }
+  Utf8Value name_val(env->isolate(), options.name);
+  ctx->AllowCodeGenerationFromStrings(options.allow_code_gen_strings->IsTrue());
+  ctx->SetEmbedderData(ContextEmbedderIndex::kAllowWasmCodeGeneration,
+                       options.allow_code_gen_wasm);
 
   ContextInfo info(*name_val);
 
-  Local<Value> origin =
-      options_obj->Get(env->context(),
-                       FIXED_ONE_BYTE_STRING(env->isolate(), "origin"))
-          .ToLocalChecked();
-  if (!origin->IsUndefined()) {
-    CHECK(origin->IsString());
-    Utf8Value origin_val(env->isolate(), origin);
+  if (!options.origin.IsEmpty()) {
+    Utf8Value origin_val(env->isolate(), options.origin);
     info.origin = *origin_val;
   }
 
@@ -238,12 +209,12 @@ void ContextifyContext::Init(Environment* env, Local<Object> target) {
 }
 
 
+// makeContext(sandbox, name, origin, strings, wasm);
 void ContextifyContext::MakeContext(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  if (!args[0]->IsObject()) {
-    return env->ThrowTypeError("sandbox argument must be an object.");
-  }
+  CHECK_EQ(args.Length(), 5);
+  CHECK(args[0]->IsObject());
   Local<Object> sandbox = args[0].As<Object>();
 
   // Don't allow contextifying a sandbox multiple times.
@@ -252,8 +223,21 @@ void ContextifyContext::MakeContext(const FunctionCallbackInfo<Value>& args) {
           env->context(),
           env->contextify_context_private_symbol()).FromJust());
 
-  Local<Object> options = args[1].As<Object>();
-  CHECK(options->IsObject());
+  ContextOptions options;
+
+  CHECK(args[1]->IsString());
+  options.name = args[1].As<String>();
+
+  CHECK(args[2]->IsString() || args[2]->IsUndefined());
+  if (args[2]->IsString()) {
+    options.origin = args[2].As<String>();
+  }
+
+  CHECK(args[3]->IsBoolean());
+  options.allow_code_gen_strings = args[3].As<Boolean>();
+
+  CHECK(args[4]->IsBoolean());
+  options.allow_code_gen_wasm = args[4].As<Boolean>();
 
   TryCatch try_catch(env->isolate());
   ContextifyContext* context = new ContextifyContext(env, sandbox, options);
@@ -277,7 +261,6 @@ void ContextifyContext::IsContext(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
   CHECK(args[0]->IsObject());
-
   Local<Object> sandbox = args[0].As<Object>();
 
   Maybe<bool> result =
@@ -729,101 +712,6 @@ MaybeLocal<Context> GetContextArg(Environment* env,
   return context;
 }
 
-namespace {
-
-Maybe<bool> GetDisplayErrorsArg(Environment* env,
-                                Local<Value> options) {
-  if (options->IsUndefined() || options->IsString()) {
-    return Just(true);
-  }
-  if (!options->IsObject()) {
-    env->ThrowTypeError("options must be an object");
-    return Nothing<bool>();
-  }
-
-  Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "displayErrors");
-  MaybeLocal<Value> maybe_value =
-      options.As<Object>()->Get(env->context(), key);
-  if (maybe_value.IsEmpty())
-    return Nothing<bool>();
-
-  Local<Value> value = maybe_value.ToLocalChecked();
-  if (value->IsUndefined())
-    return Just(true);
-
-  return value->BooleanValue(env->context());
-}
-
-MaybeLocal<String> GetFilenameArg(Environment* env,
-                                  Local<Value> options) {
-  Local<String> defaultFilename =
-      FIXED_ONE_BYTE_STRING(env->isolate(), "evalmachine.<anonymous>");
-
-  if (options->IsUndefined()) {
-    return defaultFilename;
-  }
-  if (options->IsString()) {
-    return options.As<String>();
-  }
-  if (!options->IsObject()) {
-    env->ThrowTypeError("options must be an object");
-    return Local<String>();
-  }
-
-  Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "filename");
-  MaybeLocal<Value> maybe_value =
-      options.As<Object>()->Get(env->context(), key);
-  if (maybe_value.IsEmpty())
-    return MaybeLocal<String>();
-
-  Local<Value> value = maybe_value.ToLocalChecked();
-  if (value->IsUndefined())
-    return defaultFilename;
-  return value->ToString(env->context());
-}
-
-MaybeLocal<Uint8Array> GetCachedData(Environment* env,
-                                     Local<Value> options) {
-  if (!options->IsObject()) {
-    return MaybeLocal<Uint8Array>();
-  }
-
-  MaybeLocal<Value> maybe_value =
-      options.As<Object>()->Get(env->context(), env->cached_data_string());
-  if (maybe_value.IsEmpty())
-    return MaybeLocal<Uint8Array>();
-
-  Local<Value> value = maybe_value.ToLocalChecked();
-  if (value->IsUndefined()) {
-    return MaybeLocal<Uint8Array>();
-  }
-
-  if (!value->IsUint8Array()) {
-    env->ThrowTypeError("options.cachedData must be a Buffer instance");
-    return MaybeLocal<Uint8Array>();
-  }
-
-  return value.As<Uint8Array>();
-}
-
-Maybe<bool> GetProduceCachedData(Environment* env,
-                                 Local<Value> options) {
-  if (!options->IsObject()) {
-    return Just(false);
-  }
-
-  MaybeLocal<Value> maybe_value =
-      options.As<Object>()->Get(env->context(),
-                                env->produce_cached_data_string());
-  if (maybe_value.IsEmpty())
-    return Nothing<bool>();
-
-  Local<Value> value = maybe_value.ToLocalChecked();
-  return Just(value->IsTrue());
-}
-
-}  // anonymous namespace
-
 class ContextifyScript : public BaseObject {
  private:
   Persistent<UnboundScript> script_;
@@ -855,48 +743,67 @@ class ContextifyScript : public BaseObject {
   }
 
 
-  // args: code, [options]
   static void New(const FunctionCallbackInfo<Value>& args) {
     Environment* env = Environment::GetCurrent(args);
+    Isolate* isolate = env->isolate();
+    Local<Context> context = env->context();
 
-    if (!args.IsConstructCall()) {
-      return env->ThrowError("Must call vm.Script as a constructor.");
+    CHECK(args.IsConstructCall());
+
+    const int argc = args.Length();
+    CHECK_GE(argc, 2);
+
+    CHECK(args[0]->IsString());
+    Local<String> code = args[0].As<String>();
+
+    CHECK(args[1]->IsString());
+    Local<String> filename = args[1].As<String>();
+
+    Local<Integer> line_offset;
+    Local<Integer> column_offset;
+    Local<Uint8Array> cached_data_buf;
+    bool produce_cached_data = false;
+    Local<Context> parsing_context = context;
+
+    if (argc > 2) {
+      // new ContextifyScript(code, filename, lineOffset, columnOffset
+      //                      cachedData, produceCachedData, parsingContext)
+      CHECK_EQ(argc, 7);
+      CHECK(args[2]->IsNumber());
+      line_offset = args[2].As<Integer>();
+      CHECK(args[3]->IsNumber());
+      column_offset = args[3].As<Integer>();
+      if (!args[4]->IsUndefined()) {
+        CHECK(args[4]->IsUint8Array());
+        cached_data_buf = args[4].As<Uint8Array>();
+      }
+      CHECK(args[5]->IsBoolean());
+      produce_cached_data = args[5]->IsTrue();
+      if (!args[6]->IsUndefined()) {
+        CHECK(args[6]->IsObject());
+        ContextifyContext* sandbox =
+            ContextifyContext::ContextFromContextifiedSandbox(
+                env, args[6].As<Object>());
+        CHECK_NE(sandbox, nullptr);
+        parsing_context = sandbox->context();
+      }
+    } else {
+      line_offset = Integer::New(isolate, 0);
+      column_offset = Integer::New(isolate, 0);
     }
 
     ContextifyScript* contextify_script =
         new ContextifyScript(env, args.This());
 
-    TryCatch try_catch(env->isolate());
-    Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
-    Local<String> code =
-        args[0]->ToString(env->context()).FromMaybe(Local<String>());
-
-    Local<Value> options = args[1];
-    MaybeLocal<String> filename = GetFilenameArg(env, options);
-    MaybeLocal<Integer> lineOffset = GetLineOffsetArg(env, options);
-    MaybeLocal<Integer> columnOffset = GetColumnOffsetArg(env, options);
-    MaybeLocal<Uint8Array> cached_data_buf = GetCachedData(env, options);
-    Maybe<bool> maybe_produce_cached_data = GetProduceCachedData(env, options);
-    MaybeLocal<Context> maybe_context = GetContextArg(env, options);
-    if (try_catch.HasCaught()) {
-      no_abort_scope.Close();
-      try_catch.ReThrow();
-      return;
-    }
-
-    bool produce_cached_data = maybe_produce_cached_data.ToChecked();
-
     ScriptCompiler::CachedData* cached_data = nullptr;
-    Local<Uint8Array> ui8;
-    if (cached_data_buf.ToLocal(&ui8)) {
-      ArrayBuffer::Contents contents = ui8->Buffer()->GetContents();
+    if (!cached_data_buf.IsEmpty()) {
+      ArrayBuffer::Contents contents = cached_data_buf->Buffer()->GetContents();
+      uint8_t* data = static_cast<uint8_t*>(contents.Data());
       cached_data = new ScriptCompiler::CachedData(
-          static_cast<uint8_t*>(contents.Data()) + ui8->ByteOffset(),
-          ui8->ByteLength());
+          data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
     }
 
-    ScriptOrigin origin(filename.ToLocalChecked(), lineOffset.ToLocalChecked(),
-                        columnOffset.ToLocalChecked());
+    ScriptOrigin origin(filename, line_offset, column_offset);
     ScriptCompiler::Source source(code, origin, cached_data);
     ScriptCompiler::CompileOptions compile_options =
         ScriptCompiler::kNoCompileOptions;
@@ -904,10 +811,12 @@ class ContextifyScript : public BaseObject {
     if (source.GetCachedData() != nullptr)
       compile_options = ScriptCompiler::kConsumeCodeCache;
 
-    Context::Scope scope(maybe_context.FromMaybe(env->context()));
+    TryCatch try_catch(isolate);
+    Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
+    Context::Scope scope(parsing_context);
 
     MaybeLocal<UnboundScript> v8_script = ScriptCompiler::CompileUnboundScript(
-        env->isolate(),
+        isolate,
         &source,
         compile_options);
 
@@ -917,13 +826,12 @@ class ContextifyScript : public BaseObject {
       try_catch.ReThrow();
       return;
     }
-    contextify_script->script_.Reset(env->isolate(),
-                                     v8_script.ToLocalChecked());
+    contextify_script->script_.Reset(isolate, v8_script.ToLocalChecked());
 
     if (compile_options == ScriptCompiler::kConsumeCodeCache) {
       args.This()->Set(
           env->cached_data_rejected_string(),
-          Boolean::New(env->isolate(), source.GetCachedData()->rejected));
+          Boolean::New(isolate, source.GetCachedData()->rejected));
     } else if (produce_cached_data) {
       const ScriptCompiler::CachedData* cached_data =
         ScriptCompiler::CreateCodeCache(v8_script.ToLocalChecked(), code);
@@ -937,7 +845,7 @@ class ContextifyScript : public BaseObject {
       }
       args.This()->Set(
           env->cached_data_produced_string(),
-          Boolean::New(env->isolate(), cached_data_produced));
+          Boolean::New(isolate, cached_data_produced));
     }
   }
 
@@ -948,86 +856,55 @@ class ContextifyScript : public BaseObject {
   }
 
 
-  // args: [options]
   static void RunInThisContext(const FunctionCallbackInfo<Value>& args) {
     Environment* env = Environment::GetCurrent(args);
 
-    // Assemble arguments
-    TryCatch try_catch(args.GetIsolate());
-    Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[0]);
-    Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[0]);
-    Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[0]);
-    if (try_catch.HasCaught()) {
-      try_catch.ReThrow();
-      return;
-    }
+    CHECK_EQ(args.Length(), 3);
 
-    int64_t timeout = maybe_timeout.ToChecked();
-    bool display_errors = maybe_display_errors.ToChecked();
-    bool break_on_sigint = maybe_break_on_sigint.ToChecked();
+    CHECK(args[0]->IsNumber());
+    int64_t timeout = args[0]->IntegerValue(env->context()).FromJust();
+
+    CHECK(args[1]->IsBoolean());
+    bool display_errors = args[1]->IsTrue();
+
+    CHECK(args[2]->IsBoolean());
+    bool break_on_sigint = args[2]->IsTrue();
 
     // Do the eval within this context
-    EvalMachine(env, timeout, display_errors, break_on_sigint, args,
-                &try_catch);
+    EvalMachine(env, timeout, display_errors, break_on_sigint, args);
   }
 
-  // args: sandbox, [options]
   static void RunInContext(const FunctionCallbackInfo<Value>& args) {
     Environment* env = Environment::GetCurrent(args);
 
-    int64_t timeout;
-    bool display_errors;
-    bool break_on_sigint;
-
-    // Assemble arguments
-    if (!args[0]->IsObject()) {
-      return env->ThrowTypeError(
-          "contextifiedSandbox argument must be an object.");
-    }
+    CHECK_EQ(args.Length(), 4);
 
+    CHECK(args[0]->IsObject());
     Local<Object> sandbox = args[0].As<Object>();
-    {
-      TryCatch try_catch(env->isolate());
-      Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[1]);
-      Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[1]);
-      Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[1]);
-      if (try_catch.HasCaught()) {
-        try_catch.ReThrow();
-        return;
-      }
-
-      timeout = maybe_timeout.ToChecked();
-      display_errors = maybe_display_errors.ToChecked();
-      break_on_sigint = maybe_break_on_sigint.ToChecked();
-    }
-
     // Get the context from the sandbox
     ContextifyContext* contextify_context =
         ContextifyContext::ContextFromContextifiedSandbox(env, sandbox);
-    if (contextify_context == nullptr) {
-      return env->ThrowTypeError(
-          "sandbox argument must have been converted to a context.");
-    }
+    CHECK_NE(contextify_context, nullptr);
 
     if (contextify_context->context().IsEmpty())
       return;
 
-    {
-      TryCatch try_catch(env->isolate());
-      // Do the eval within the context
-      Context::Scope context_scope(contextify_context->context());
-      EvalMachine(contextify_context->env(),
-                  timeout,
-                  display_errors,
-                  break_on_sigint,
-                  args,
-                  &try_catch);
+    CHECK(args[1]->IsNumber());
+    int64_t timeout = args[1]->IntegerValue(env->context()).FromJust();
 
-      if (try_catch.HasCaught()) {
-        try_catch.ReThrow();
-        return;
-      }
-    }
+    CHECK(args[2]->IsBoolean());
+    bool display_errors = args[2]->IsTrue();
+
+    CHECK(args[3]->IsBoolean());
+    bool break_on_sigint = args[3]->IsTrue();
+
+    // Do the eval within the context
+    Context::Scope context_scope(contextify_context->context());
+    EvalMachine(contextify_context->env(),
+                timeout,
+                display_errors,
+                break_on_sigint,
+                args);
   }
 
   static void DecorateErrorStack(Environment* env, const TryCatch& try_catch) {
@@ -1072,14 +949,13 @@ class ContextifyScript : public BaseObject {
                           const int64_t timeout,
                           const bool display_errors,
                           const bool break_on_sigint,
-                          const FunctionCallbackInfo<Value>& args,
-                          TryCatch* try_catch) {
+                          const FunctionCallbackInfo<Value>& args) {
     if (!ContextifyScript::InstanceOf(env, args.Holder())) {
       env->ThrowTypeError(
           "Script methods can only be called on script instances.");
       return false;
     }
-
+    TryCatch try_catch(env->isolate());
     ContextifyScript* wrapped_script;
     ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder(), false);
     Local<UnboundScript> unbound_script =
@@ -1115,10 +991,10 @@ class ContextifyScript : public BaseObject {
       env->isolate()->CancelTerminateExecution();
     }
 
-    if (try_catch->HasCaught()) {
+    if (try_catch.HasCaught()) {
       if (!timed_out && !received_signal && display_errors) {
         // We should decorate non-termination exceptions
-        DecorateErrorStack(env, *try_catch);
+        DecorateErrorStack(env, try_catch);
       }
 
       // If there was an exception thrown during script execution, re-throw it.
@@ -1126,7 +1002,7 @@ class ContextifyScript : public BaseObject {
       // letting try_catch catch it.
       // If execution has been terminated, but not by one of the watchdogs from
       // this invocation, this will re-throw a `null` value.
-      try_catch->ReThrow();
+      try_catch.ReThrow();
 
       return false;
     }
diff --git a/src/node_contextify.h b/src/node_contextify.h
index cf3e6452fd0..1075bc5c686 100644
--- a/src/node_contextify.h
+++ b/src/node_contextify.h
@@ -9,6 +9,13 @@
 namespace node {
 namespace contextify {
 
+struct ContextOptions {
+  v8::Local<v8::String> name;
+  v8::Local<v8::String> origin;
+  v8::Local<v8::Boolean> allow_code_gen_strings;
+  v8::Local<v8::Boolean> allow_code_gen_wasm;
+};
+
 class ContextifyContext {
  protected:
   Environment* const env_;
@@ -17,11 +24,11 @@ class ContextifyContext {
  public:
   ContextifyContext(Environment* env,
                     v8::Local<v8::Object> sandbox_obj,
-                    v8::Local<v8::Object> options_obj);
+                    const ContextOptions& options);
 
   v8::Local<v8::Value> CreateDataWrapper(Environment* env);
   v8::Local<v8::Context> CreateV8Context(Environment* env,
-      v8::Local<v8::Object> sandbox_obj, v8::Local<v8::Object> options_obj);
+      v8::Local<v8::Object> sandbox_obj, const ContextOptions& options);
   static void Init(Environment* env, v8::Local<v8::Object> target);
 
   static bool AllowWasmCodeGeneration(
diff --git a/test/parallel/test-vm-basic.js b/test/parallel/test-vm-basic.js
index bf1532cacc7..2dfc1e9cb72 100644
--- a/test/parallel/test-vm-basic.js
+++ b/test/parallel/test-vm-basic.js
@@ -24,60 +24,85 @@ const common = require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
-// Test 1: vm.runInNewContext
-const sandbox = {};
-let result = vm.runInNewContext(
-  'foo = "bar"; this.typeofProcess = typeof process; typeof Object;',
-  sandbox
-);
-assert.deepStrictEqual(sandbox, {
-  foo: 'bar',
-  typeofProcess: 'undefined',
-});
-assert.strictEqual(result, 'function');
+// vm.runInNewContext
+{
+  const sandbox = {};
+  const result = vm.runInNewContext(
+    'foo = "bar"; this.typeofProcess = typeof process; typeof Object;',
+    sandbox
+  );
+  assert.deepStrictEqual(sandbox, {
+    foo: 'bar',
+    typeofProcess: 'undefined',
+  });
+  assert.strictEqual(result, 'function');
+}
 
-// Test 2: vm.runInContext
-const sandbox2 = { foo: 'bar' };
-const context = vm.createContext(sandbox2);
-result = vm.runInContext(
-  'baz = foo; this.typeofProcess = typeof process; typeof Object;',
-  context
-);
-assert.deepStrictEqual(sandbox2, {
-  foo: 'bar',
-  baz: 'bar',
-  typeofProcess: 'undefined'
-});
-assert.strictEqual(result, 'function');
+// vm.runInContext
+{
+  const sandbox = { foo: 'bar' };
+  const context = vm.createContext(sandbox);
+  const result = vm.runInContext(
+    'baz = foo; this.typeofProcess = typeof process; typeof Object;',
+    context
+  );
+  assert.deepStrictEqual(sandbox, {
+    foo: 'bar',
+    baz: 'bar',
+    typeofProcess: 'undefined'
+  });
+  assert.strictEqual(result, 'function');
+}
 
-// Test 3: vm.runInThisContext
-result = vm.runInThisContext(
-  'vmResult = "foo"; Object.prototype.toString.call(process);'
-);
-assert.strictEqual(global.vmResult, 'foo');
-assert.strictEqual(result, '[object process]');
-delete global.vmResult;
+// vm.runInThisContext
+{
+  const result = vm.runInThisContext(
+    'vmResult = "foo"; Object.prototype.toString.call(process);'
+  );
+  assert.strictEqual(global.vmResult, 'foo');
+  assert.strictEqual(result, '[object process]');
+  delete global.vmResult;
+}
 
-// Test 4: vm.runInNewContext
-result = vm.runInNewContext(
-  'vmResult = "foo"; typeof process;'
-);
-assert.strictEqual(global.vmResult, undefined);
-assert.strictEqual(result, 'undefined');
+// vm.runInNewContext
+{
+  const result = vm.runInNewContext(
+    'vmResult = "foo"; typeof process;'
+  );
+  assert.strictEqual(global.vmResult, undefined);
+  assert.strictEqual(result, 'undefined');
+}
 
-// Test 5: vm.createContext
-const sandbox3 = {};
-const context2 = vm.createContext(sandbox3);
-assert.strictEqual(sandbox3, context2);
+// vm.createContext
+{
+  const sandbox = {};
+  const context = vm.createContext(sandbox);
+  assert.strictEqual(sandbox, context);
+}
 
-// Test 6: invalid arguments
+// Run script with filename
+{
+  const script = 'throw new Error("boom")';
+  const filename = 'test-boom-error';
+  const context = vm.createContext();
+
+  function checkErr(err) {
+    return err.stack.startsWith('test-boom-error:1');
+  }
+
+  assert.throws(() => vm.runInContext(script, context, filename), checkErr);
+  assert.throws(() => vm.runInNewContext(script, context, filename), checkErr);
+  assert.throws(() => vm.runInThisContext(script, filename), checkErr);
+}
+
+// Invalid arguments
 [null, 'string'].forEach((input) => {
   common.expectsError(() => {
     vm.createContext({}, input);
   }, {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "options" argument must be of type object. ' +
+    message: 'The "options" argument must be of type Object. ' +
              `Received type ${typeof input}`
   });
 });
diff --git a/test/parallel/test-vm-cached-data.js b/test/parallel/test-vm-cached-data.js
index 5302d73c2df..e3947d1ae6c 100644
--- a/test/parallel/test-vm-cached-data.js
+++ b/test/parallel/test-vm-cached-data.js
@@ -1,5 +1,5 @@
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const vm = require('vm');
 const spawnSync = require('child_process').spawnSync;
@@ -84,8 +84,12 @@ function testRejectSlice() {
 testRejectSlice();
 
 // It should throw on non-Buffer cachedData
-assert.throws(() => {
+common.expectsError(() => {
   new vm.Script('function abc() {}', {
     cachedData: 'ohai'
   });
-}, /^TypeError: options\.cachedData must be a Buffer instance$/);
+}, {
+  code: 'ERR_INVALID_ARG_TYPE',
+  type: TypeError,
+  message: /must be one of type Buffer or Uint8Array/
+});
diff --git a/test/parallel/test-vm-context.js b/test/parallel/test-vm-context.js
index fd6a66f3927..31ed69da93f 100644
--- a/test/parallel/test-vm-context.js
+++ b/test/parallel/test-vm-context.js
@@ -64,19 +64,25 @@ try {
 // This is outside of catch block to confirm catch block ran.
 assert.strictEqual(gh1140Exception.toString(), 'Error');
 
-// GH-558, non-context argument segfaults / raises assertion
-const nonContextualSandboxErrorMsg =
-  /^TypeError: contextifiedSandbox argument must be an object\.$/;
-const contextifiedSandboxErrorMsg =
-    /^TypeError: sandbox argument must have been converted to a context\.$/;
+const nonContextualSandboxError = {
+  code: 'ERR_INVALID_ARG_TYPE',
+  type: TypeError,
+  message: /must be of type Object/
+};
+const contextifiedSandboxError = {
+  code: 'ERR_INVALID_ARG_TYPE',
+  type: TypeError,
+  message: /must be of type vm\.Context/
+};
+
 [
-  [undefined, nonContextualSandboxErrorMsg],
-  [null, nonContextualSandboxErrorMsg], [0, nonContextualSandboxErrorMsg],
-  [0.0, nonContextualSandboxErrorMsg], ['', nonContextualSandboxErrorMsg],
-  [{}, contextifiedSandboxErrorMsg], [[], contextifiedSandboxErrorMsg]
+  [undefined, nonContextualSandboxError],
+  [null, nonContextualSandboxError], [0, nonContextualSandboxError],
+  [0.0, nonContextualSandboxError], ['', nonContextualSandboxError],
+  [{}, contextifiedSandboxError], [[], contextifiedSandboxError]
 ].forEach((e) => {
-  assert.throws(() => { script.runInContext(e[0]); }, e[1]);
-  assert.throws(() => { vm.runInContext('', e[0]); }, e[1]);
+  common.expectsError(() => { script.runInContext(e[0]); }, e[1]);
+  common.expectsError(() => { vm.runInContext('', e[0]); }, e[1]);
 });
 
 // Issue GH-693:
diff --git a/test/parallel/test-vm-is-context.js b/test/parallel/test-vm-is-context.js
index a762622c67a..2749a14834b 100644
--- a/test/parallel/test-vm-is-context.js
+++ b/test/parallel/test-vm-is-context.js
@@ -35,13 +35,6 @@ for (const valToTest of [
   });
 }
 
-common.expectsError(() => {
-  vm.isContext();
-}, {
-  code: 'ERR_MISSING_ARGS',
-  type: TypeError
-});
-
 assert.strictEqual(vm.isContext({}), false);
 assert.strictEqual(vm.isContext([]), false);
 
diff --git a/test/parallel/test-vm-options-validation.js b/test/parallel/test-vm-options-validation.js
new file mode 100644
index 00000000000..e00a14e4b9e
--- /dev/null
+++ b/test/parallel/test-vm-options-validation.js
@@ -0,0 +1,78 @@
+'use strict';
+
+const common = require('../common');
+const vm = require('vm');
+
+const invalidArgType = {
+  type: TypeError,
+  code: 'ERR_INVALID_ARG_TYPE'
+};
+
+const outOfRange = {
+  type: RangeError,
+  code: 'ERR_OUT_OF_RANGE'
+};
+
+common.expectsError(() => {
+  new vm.Script('void 0', 42);
+}, invalidArgType);
+
+[null, {}, [1], 'bad', true, 0.1].forEach((value) => {
+  common.expectsError(() => {
+    new vm.Script('void 0', { lineOffset: value });
+  }, invalidArgType);
+
+  common.expectsError(() => {
+    new vm.Script('void 0', { columnOffset: value });
+  }, invalidArgType);
+});
+
+common.expectsError(() => {
+  new vm.Script('void 0', { lineOffset: Number.MAX_SAFE_INTEGER });
+}, outOfRange);
+
+common.expectsError(() => {
+  new vm.Script('void 0', { columnOffset: Number.MAX_SAFE_INTEGER });
+}, outOfRange);
+
+common.expectsError(() => {
+  new vm.Script('void 0', { filename: 123 });
+}, invalidArgType);
+
+common.expectsError(() => {
+  new vm.Script('void 0', { produceCachedData: 1 });
+}, invalidArgType);
+
+[[0], {}, true, 'bad', 42].forEach((value) => {
+  common.expectsError(() => {
+    new vm.Script('void 0', { cachedData: value });
+  }, invalidArgType);
+});
+
+{
+  const script = new vm.Script('void 0');
+  const sandbox = vm.createContext();
+
+  function assertErrors(options) {
+    common.expectsError(() => {
+      script.runInThisContext(options);
+    }, invalidArgType);
+
+    common.expectsError(() => {
+      script.runInContext(sandbox, options);
+    }, invalidArgType);
+
+    common.expectsError(() => {
+      script.runInNewContext({}, options);
+    }, invalidArgType);
+  }
+
+  [null, 'bad', 42].forEach(assertErrors);
+  [{}, [1], 'bad', null, -1, 0, NaN].forEach((value) => {
+    assertErrors({ timeout: value });
+  });
+  [{}, [1], 'bad', 1, null].forEach((value) => {
+    assertErrors({ displayErrors: value });
+    assertErrors({ breakOnSigint: value });
+  });
+}
diff --git a/test/parallel/test-vm-timeout.js b/test/parallel/test-vm-timeout.js
index 51239ffed58..859992e99ba 100644
--- a/test/parallel/test-vm-timeout.js
+++ b/test/parallel/test-vm-timeout.js
@@ -24,25 +24,15 @@ require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
-// Test 1: Timeout of 100ms executing endless loop
+// Timeout of 100ms executing endless loop
 assert.throws(function() {
   vm.runInThisContext('while(true) {}', { timeout: 100 });
 }, /^Error: Script execution timed out\.$/);
 
-// Test 2: Timeout must be >= 0ms
-assert.throws(function() {
-  vm.runInThisContext('', { timeout: -1 });
-}, /^RangeError: timeout must be a positive number$/);
-
-// Test 3: Timeout of 0ms
-assert.throws(function() {
-  vm.runInThisContext('', { timeout: 0 });
-}, /^RangeError: timeout must be a positive number$/);
-
-// Test 4: Timeout of 1000ms, script finishes first
+// Timeout of 1000ms, script finishes first
 vm.runInThisContext('', { timeout: 1000 });
 
-// Test 5: Nested vm timeouts, inner timeout propagates out
+// Nested vm timeouts, inner timeout propagates out
 assert.throws(function() {
   const context = {
     log: console.log,
@@ -54,7 +44,7 @@ assert.throws(function() {
   throw new Error('Test 5 failed');
 }, /Script execution timed out\./);
 
-// Test 6: Nested vm timeouts, outer timeout is shorter and fires first.
+// Nested vm timeouts, outer timeout is shorter and fires first.
 assert.throws(function() {
   const context = {
     runInVM: function(timeout) {
@@ -65,7 +55,7 @@ assert.throws(function() {
   throw new Error('Test 6 failed');
 }, /Script execution timed out\./);
 
-// Test 7: Nested vm timeouts, inner script throws an error.
+// Nested vm timeouts, inner script throws an error.
 assert.throws(function() {
   const context = {
     runInVM: function(timeout) {