From b5bd87cbdecc98c155d55405df6ed623c93f20f4 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 2 Jun 2020 00:42:46 +0200
Subject: [PATCH 1/4] util: restrict custom inspect function + vm.Context
 interaction

When `util.inspect()` is called on an object with a custom inspect
function, and that object is from a different `vm.Context`,
that function will not receive any arguments that access
context-specific data anymore.
---
 doc/api/util.md                    |  5 ++
 lib/internal/util/inspect.js       | 37 ++++++++++++--
 test/parallel/test-util-inspect.js | 82 ++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 3 deletions(-)

diff --git a/doc/api/util.md b/doc/api/util.md
index 8cdb7b6d001000..9a0c8cb2f95ed7 100644
--- a/doc/api/util.md
+++ b/doc/api/util.md
@@ -398,6 +398,11 @@ stream.write('With ES6');
 <!-- YAML
 added: v0.3.0
 changes:
+  - version: REPLACEME
+    pr-url: ???
+    description: If `object` is from a different `vm.Context` now, a custom
+                 inspection function on it will not receive context-specific
+                 arguments anymore.
   - version:
      - v13.13.0
      - v12.17.0
diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index fbc0a4ee2d6b6e..3178b9938a293b 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
@@ -12,6 +12,7 @@ const {
   DatePrototypeToString,
   ErrorPrototypeToString,
   Float32Array,
+  FunctionPrototypeCall,
   FunctionPrototypeToString,
   JSONStringify,
   Map,
@@ -25,6 +26,7 @@ const {
   Number,
   NumberIsNaN,
   NumberPrototypeValueOf,
+  Object,
   ObjectAssign,
   ObjectCreate,
   ObjectDefineProperty,
@@ -37,6 +39,7 @@ const {
   ObjectPrototypeHasOwnProperty,
   ObjectPrototypePropertyIsEnumerable,
   ObjectSeal,
+  ObjectSetPrototypeOf,
   RegExp,
   RegExpPrototypeToString,
   Set,
@@ -212,8 +215,8 @@ const ansi = new RegExp(ansiPattern, 'g');
 
 let getStringWidth;
 
-function getUserOptions(ctx) {
-  return {
+function getUserOptions(ctx, targetValue) {
+  const ret = {
     stylize: ctx.stylize,
     showHidden: ctx.showHidden,
     depth: ctx.depth,
@@ -228,6 +231,33 @@ function getUserOptions(ctx) {
     getters: ctx.getters,
     ...ctx.userOptions
   };
+
+  // Typically, the target value will be an instance of `Object`. If that is
+  // *not* the case, the object may come from another vm.Context, and we want
+  // to avoid passing it objects from this Context in that case, so we remove
+  // the prototype from the returned object itself + the `stylize()` function,
+  // and remove all other non-primitives, including non-primitive user options.
+  if (!(targetValue instanceof Object)) {
+    ObjectSetPrototypeOf(ret, null);
+    for (const key of ObjectKeys(ret)) {
+      if ((typeof ret[key] === 'object' || typeof ret[key] === 'function') &&
+          ret[key] !== null) {
+        delete ret[key];
+      }
+    }
+    ret.stylize = ObjectSetPrototypeOf((value, flavour) => {
+      let stylized;
+      try {
+        stylized = ctx.stylize(value, flavour);
+      } catch {}
+
+      if (typeof stylized !== 'string') return value;
+      // `stylized` is a string as it should be, which is safe to pass along.
+      return stylized;
+    }, null);
+  }
+
+  return ret;
 }
 
 /**
@@ -726,7 +756,8 @@ function formatValue(ctx, value, recurseTimes, typedArray) {
       // This makes sure the recurseTimes are reported as before while using
       // a counter internally.
       const depth = ctx.depth === null ? null : ctx.depth - recurseTimes;
-      const ret = maybeCustom.call(context, depth, getUserOptions(ctx));
+      const ret = FunctionPrototypeCall(
+        maybeCustom, context, depth, getUserOptions(ctx, context));
       // If the custom inspection method returned `this`, don't go into
       // infinite recursion.
       if (ret !== context) {
diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index 1700e4d5c636aa..1c0871ee9a0b31 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -2901,3 +2901,85 @@ assert.strictEqual(
     "'aaaa'... 999996 more characters"
   );
 }
+
+{
+  // Verify that util.inspect() invokes custom inspect functions on objects
+  // from other vm.Contexts but does not pass data from its own Context to that
+  // function.
+  const target = vm.runInNewContext(`
+    ({
+      [Symbol.for('nodejs.util.inspect.custom')](depth, ctx) {
+        this.depth = depth;
+        this.ctx = ctx;
+        try {
+          this.stylized = ctx.stylize('🐈');
+        } catch (e) {
+          this.stylizeException = e;
+        }
+        return this.stylized;
+      }
+    })
+  `, Object.create(null));
+  assert.strictEqual(target.ctx, undefined);
+
+  {
+    // Subtest 1: Just try to inspect the object with default options.
+    assert.strictEqual(util.inspect(target), '🐈');
+    assert.strictEqual(typeof target.ctx, 'object');
+    const objectGraph = fullObjectGraph(target);
+    assert(!objectGraph.has(Object));
+    assert(!objectGraph.has(Function));
+  }
+
+  {
+    // Subtest 2: Use a stylize function that returns a non-primitive.
+    const output = util.inspect(target, {
+      stylize: common.mustCall((str) => {
+        return {};
+      })
+    });
+    assert.strictEqual(output, '🐈');
+    assert.strictEqual(typeof target.ctx, 'object');
+    const objectGraph = fullObjectGraph(target);
+    assert(!objectGraph.has(Object));
+    assert(!objectGraph.has(Function));
+  }
+
+  {
+    // Subtest 3: Use a stylize function that throws an exception.
+    const output = util.inspect(target, {
+      stylize: common.mustCall((str) => {
+        throw new Error('oops');
+      })
+    });
+    assert.strictEqual(output, '🐈');
+    assert.strictEqual(typeof target.ctx, 'object');
+    const objectGraph = fullObjectGraph(target);
+    assert(!objectGraph.has(Object));
+    assert(!objectGraph.has(Function));
+  }
+
+  function fullObjectGraph(value) {
+    const graph = new Set([value]);
+
+    for (const entry of graph) {
+      if ((typeof entry !== 'object' && typeof entry !== 'function') ||
+          entry === null) {
+        continue;
+      }
+
+      graph.add(Object.getPrototypeOf(entry));
+      const descriptors = Object.values(Object.getOwnPropertyDescriptors(entry));
+      for (const descriptor of descriptors) {
+        graph.add(descriptor.value);
+        graph.add(descriptor.set);
+        graph.add(descriptor.get);
+      }
+    }
+
+    return graph;
+  }
+
+  // Consistency check.
+  assert(fullObjectGraph(global).includes(Function.prototype));
+}

From 583c1bd1ca1e33b9bde1e6cf9246900057e393ba Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 2 Jun 2020 00:52:54 +0200
Subject: [PATCH 2/4] fixup! util: restrict custom inspect function +
 vm.Context interaction

---
 doc/api/util.md                    | 2 +-
 lib/internal/util/inspect.js       | 8 +++++---
 test/parallel/test-util-inspect.js | 5 +++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/doc/api/util.md b/doc/api/util.md
index 9a0c8cb2f95ed7..59945545d7a189 100644
--- a/doc/api/util.md
+++ b/doc/api/util.md
@@ -399,7 +399,7 @@ stream.write('With ES6');
 added: v0.3.0
 changes:
   - version: REPLACEME
-    pr-url: ???
+    pr-url: https://github.com/nodejs/node/pull/33690
     description: If `object` is from a different `vm.Context` now, a custom
                  inspection function on it will not receive context-specific
                  arguments anymore.
diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index 3178b9938a293b..e5d487d4f6939c 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
@@ -215,7 +215,7 @@ const ansi = new RegExp(ansiPattern, 'g');
 
 let getStringWidth;
 
-function getUserOptions(ctx, targetValue) {
+function getUserOptions(ctx, isCrossContext) {
   const ret = {
     stylize: ctx.stylize,
     showHidden: ctx.showHidden,
@@ -237,7 +237,7 @@ function getUserOptions(ctx, targetValue) {
   // to avoid passing it objects from this Context in that case, so we remove
   // the prototype from the returned object itself + the `stylize()` function,
   // and remove all other non-primitives, including non-primitive user options.
-  if (!(targetValue instanceof Object)) {
+  if (isCrossContext) {
     ObjectSetPrototypeOf(ret, null);
     for (const key of ObjectKeys(ret)) {
       if ((typeof ret[key] === 'object' || typeof ret[key] === 'function') &&
@@ -756,8 +756,10 @@ function formatValue(ctx, value, recurseTimes, typedArray) {
       // This makes sure the recurseTimes are reported as before while using
       // a counter internally.
       const depth = ctx.depth === null ? null : ctx.depth - recurseTimes;
+      const isCrossContext =
+        proxy !== undefined || !(context instanceof Object);
       const ret = FunctionPrototypeCall(
-        maybeCustom, context, depth, getUserOptions(ctx, context));
+        maybeCustom, context, depth, getUserOptions(ctx, isCrossContext));
       // If the custom inspection method returned `this`, don't go into
       // infinite recursion.
       if (ret !== context) {
diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index 1c0871ee9a0b31..b558cdd21b59a7 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -2969,7 +2969,8 @@ assert.strictEqual(
       }
 
       graph.add(Object.getPrototypeOf(entry));
-      const descriptors = Object.values(Object.getOwnPropertyDescriptors(entry));
+      const descriptors = Object.values(
+        Object.getOwnPropertyDescriptors(entry));
       for (const descriptor of descriptors) {
         graph.add(descriptor.value);
         graph.add(descriptor.set);
@@ -2981,5 +2982,5 @@ assert.strictEqual(
   }
 
   // Consistency check.
-  assert(fullObjectGraph(global).includes(Function.prototype));
+  assert(fullObjectGraph(global).has(Function.prototype));
 }

From 8544be7df300e012b2abfbc509a7b8576ab8ea5c Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 6 Jun 2020 17:33:06 +0200
Subject: [PATCH 3/4] fixup! util: restrict custom inspect function +
 vm.Context interaction

---
 lib/internal/util/inspect.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index e5d487d4f6939c..2c37e581964e7f 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
@@ -248,7 +248,7 @@ function getUserOptions(ctx, isCrossContext) {
     ret.stylize = ObjectSetPrototypeOf((value, flavour) => {
       let stylized;
       try {
-        stylized = ctx.stylize(value, flavour);
+        stylized = `${ctx.stylize(value, flavour)}`;
       } catch {}
 
       if (typeof stylized !== 'string') return value;

From 7b0b27bac2ab5e8ca3831def6d76cd2189924836 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Mon, 8 Jun 2020 20:43:09 +0200
Subject: [PATCH 4/4] fixup! fixup! util: restrict custom inspect function +
 vm.Context interaction

---
 test/parallel/test-util-inspect.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index b558cdd21b59a7..a16acc64de3456 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -2938,7 +2938,7 @@ assert.strictEqual(
         return {};
       })
     });
-    assert.strictEqual(output, '🐈');
+    assert.strictEqual(output, '[object Object]');
     assert.strictEqual(typeof target.ctx, 'object');
     const objectGraph = fullObjectGraph(target);
     assert(!objectGraph.has(Object));