From a32cbe159749c645bf2da6b2af46f9b732b416e0 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Feb 2019 16:07:07 +0100 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/26241 Fixes: https://github.com/nodejs/node/issues/10731 Fixes: https://github.com/nodejs/node/issues/26231 Refs: https://github.com/nodejs/node/issues/25212 Refs: https://github.com/nodejs/node/issues/24765 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- lib/internal/util/inspect.js | 24 ++++++---- test/parallel/test-util-inspect-proxy.js | 60 +++++++++++++++++++----- 2 files changed, 62 insertions(+), 22 deletions(-) diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index 9a1a0f9f03bb34..40bc76f817b99a 100644 --- 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 index adc0810722d553..9aaa8fb13eeb71 100644 --- 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.