From 8d0c21fd24d9d931db7d738489e7dd8ff00a8112 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 2 Jun 2020 00:42:46 +0200
Subject: [PATCH] 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.

PR-URL: https://github.com/nodejs/node/pull/33690
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 doc/api/util.md                    |  5 ++
 lib/internal/util/inspect.js       | 39 ++++++++++++--
 test/parallel/test-util-inspect.js | 83 ++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 3 deletions(-)

diff --git a/doc/api/util.md b/doc/api/util.md
index c5f33475c822c9..a5c405077e506a 100644
--- a/doc/api/util.md
+++ b/doc/api/util.md
@@ -414,6 +414,11 @@ stream.write('With ES6');
 <!-- YAML
 added: v0.3.0
 changes:
+  - version: REPLACEME
+    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.
   - version: v12.17.0
     pr-url: https://github.com/nodejs/node/pull/32392
     description: The `maxStringLength` option is supported now.
diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index de164f28ef5814..e6bd5ee542a075 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
@@ -12,6 +12,7 @@ const {
   DatePrototypeToString,
   ErrorPrototypeToString,
   Float32Array,
+  FunctionPrototypeCall,
   FunctionPrototypeToString,
   Int16Array,
   JSONStringify,
@@ -26,6 +27,7 @@ const {
   Number,
   NumberIsNaN,
   NumberPrototypeValueOf,
+  Object,
   ObjectAssign,
   ObjectCreate,
   ObjectDefineProperty,
@@ -38,6 +40,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, isCrossContext) {
+  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 (isCrossContext) {
+    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;
 }
 
 /**
@@ -723,7 +753,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 ret = maybeCustom.call(context, depth, getUserOptions(ctx));
+      const isCrossContext =
+        proxy !== undefined || !(context instanceof Object);
+      const ret = FunctionPrototypeCall(
+        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 383f592bca4132..d63ad3a1440b11 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -2874,3 +2874,86 @@ 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, '[object Object]');
+    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).has(Function.prototype));
+}