util: fix proxy inspection

This prevents any proxy traps from being called while inspecting proxy objects. That guarantees a side-effect free way of inspecting proxies. PR-URL: #26241 Fixes: #10731 Fixes: #26231 Refs: #25212 Refs: #24765 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · Feb 28, 2019 · a32cbe1 · a32cbe1
1 parent 9edce1e
commit a32cbe1
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 22 deletions.
diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
@@ -495,18 +495,23 @@ function formatValue(ctx, value, recurseTimes, typedArray) {
     return ctx.stylize('null', 'null');
   }
 
+  // Memorize the context for custom inspection on proxies.
+  const context = value;
+  // Always check for proxies to prevent side effects and to prevent triggering
+  // any proxy handlers.
+  const proxy = getProxyDetails(value);
+  if (proxy !== undefined) {
+    if (ctx.showProxy && ctx.stop === undefined) {
+      return formatProxy(ctx, proxy, recurseTimes);
+    }
+    value = proxy[0];
+  }
+
   if (ctx.stop !== undefined) {
     const name = getConstructorName(value, ctx) || value[Symbol.toStringTag];
     return ctx.stylize(`[${name || 'Object'}]`, 'special');
   }
 
-  if (ctx.showProxy) {
-    const proxy = getProxyDetails(value);
-    if (proxy !== undefined) {
-      return formatProxy(ctx, proxy, recurseTimes);
-    }
-  }
-
   // Provide a hook for user-specified inspect functions.
   // Check that value is an object with an inspect function on it.
   if (ctx.customInspect) {
@@ -523,11 +528,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(value, depth, plainCtx);
-
+      const ret = maybeCustom.call(context, depth, plainCtx);
       // If the custom inspection method returned `this`, don't go into
       // infinite recursion.
-      if (ret !== value) {
+      if (ret !== context) {
         if (typeof ret !== 'string') {
           return formatValue(ctx, ret, recurseTimes);
         }

diff --git a/test/parallel/test-util-inspect-proxy.js b/test/parallel/test-util-inspect-proxy.js
@@ -8,11 +8,35 @@ const { internalBinding } = require('internal/test/binding');
 const processUtil = internalBinding('util');
 const opts = { showProxy: true };
 
-const target = {};
+let proxyObj;
+let called = false;
+const target = {
+  [util.inspect.custom](depth, { showProxy }) {
+    if (showProxy === false) {
+      called = true;
+      if (proxyObj !== this) {
+        throw new Error('Failed');
+      }
+    }
+    return [1, 2, 3];
+  }
+};
 const handler = {
-  get: function() { throw new Error('Getter should not be called'); }
+  getPrototypeOf() { throw new Error('getPrototypeOf'); },
+  setPrototypeOf() { throw new Error('setPrototypeOf'); },
+  isExtensible() { throw new Error('isExtensible'); },
+  preventExtensions() { throw new Error('preventExtensions'); },
+  getOwnPropertyDescriptor() { throw new Error('getOwnPropertyDescriptor'); },
+  defineProperty() { throw new Error('defineProperty'); },
+  has() { throw new Error('has'); },
+  get() { throw new Error('get'); },
+  set() { throw new Error('set'); },
+  deleteProperty() { throw new Error('deleteProperty'); },
+  ownKeys() { throw new Error('ownKeys'); },
+  apply() { throw new Error('apply'); },
+  construct() { throw new Error('construct'); }
 };
-const proxyObj = new Proxy(target, handler);
+proxyObj = new Proxy(target, handler);
 
 // Inspecting the proxy should not actually walk it's properties
 util.inspect(proxyObj, opts);
@@ -23,19 +47,31 @@ const details = processUtil.getProxyDetails(proxyObj);
 assert.strictEqual(target, details[0]);
 assert.strictEqual(handler, details[1]);
 
-assert.strictEqual(util.inspect(proxyObj, opts),
-                   'Proxy [ {}, { get: [Function: get] } ]');
+assert.strictEqual(
+  util.inspect(proxyObj, opts),
+  'Proxy [ [ 1, 2, 3 ],\n' +
+  '  { getPrototypeOf: [Function: getPrototypeOf],\n' +
+  '    setPrototypeOf: [Function: setPrototypeOf],\n' +
+  '    isExtensible: [Function: isExtensible],\n' +
+  '    preventExtensions: [Function: preventExtensions],\n' +
+  '    getOwnPropertyDescriptor: [Function: getOwnPropertyDescriptor],\n' +
+  '    defineProperty: [Function: defineProperty],\n' +
+  '    has: [Function: has],\n' +
+  '    get: [Function: get],\n' +
+  '    set: [Function: set],\n' +
+  '    deleteProperty: [Function: deleteProperty],\n' +
+  '    ownKeys: [Function: ownKeys],\n' +
+  '    apply: [Function: apply],\n' +
+  '    construct: [Function: construct] } ]'
+);
 
 // Using getProxyDetails with non-proxy returns undefined
 assert.strictEqual(processUtil.getProxyDetails({}), undefined);
 
-// This will throw because the showProxy option is not used
-// and the get function on the handler object defined above
-// is actually invoked.
-assert.throws(
-  () => util.inspect(proxyObj),
-  /^Error: Getter should not be called$/
-);
+// Inspecting a proxy without the showProxy option set to true should not
+// trigger any proxy handlers.
+assert.strictEqual(util.inspect(proxyObj), '[ 1, 2, 3 ]');
+assert(called);
 
 // Yo dawg, I heard you liked Proxy so I put a Proxy
 // inside your Proxy that proxies your Proxy's Proxy.