Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

util: prevent proxy traps being triggered by .inspect() #26241

Closed
wants to merge 1 commit into from
Closed

util: prevent proxy traps being triggered by .inspect() #26241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions lib/internal/util/inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved
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) {
Expand All @@ -523,11 +528,10 @@ function formatValue(ctx, value, recurseTimes, typedArray) {
// This makes sure the recurseTimes are reported as before while using
Copy link
Member

@jdalton jdalton Feb 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this PR the above ☝️ maybeCustom = value[customInspectSymbol] now uses the unwrapped value to get the customInspectSymbol value. However, in usage I've seen folks rely on the value[customInspectSymbol] result being that of the get trap for customInspectSymbol. Would you be up for checking it behind a try-catch?

Copy link
Member Author

@BridgeAR BridgeAR Feb 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would contradict the actual intention of this PR: prevent traps from being called similar to browsers. Otherwise we'll keep on getting requests about proxied values not being inspectable.

Relying on the get trap for anything like that is never a good idea. Some people asked for changing traps e.g., from a normal access to checking the descriptor and that could also happen anytime.

I'll run CITGM after the security release is done to see if we find any impacted modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core of what they're trying to accomplish is provide a customizer without one needing to be bolted on to the value(s) being inspected. Perhaps that could be an inspect option?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be possible but is really superior to adding the custom inspect symbol? The symbol is easily available (Symbol.for('nodejs.util.inspect.custom')) and adding it to the concrete code makes sure everyone is aware of what happens by looking at the code (it's less implicit). Can you post an example where a generic option would be beneficial?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit is that you can customize inspection of objects you don't own. Some may not like bolting on symbol properties to objects they don't own because maybe the objects could be frozen, or have their own traps to contend with.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a customizer option is a way for the dev inspecting to suggest how objects should be inspected

I absolutely agree and that is the crux in this case: using a generic custom inspect function as the dev inspecting the object is the same as inspecting the value from the custom inspect function directly. Or do I miss something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the cases where no customization is needed the customizer could defer to inspect feeding it the context/depth from the customizer invocation arguments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIC that's always possible without a custom inspect function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a custom function the user would lack ctx and depth.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a customizer option ctx and depth would always be indentical to the defaults combined with the passed through options (and some internal state that is going to be removed soon). The reason for that is that it would never come below the main option part as we would not know for what part the customizer should be used.

util.inspect(object, {
  customInspect(value, depth, ctx) {
    // Will always trigger with `value === object` and nothing else.
    // `depth === util.inspect.defaultOptions.depth`
    // `ctx` ==> is in this case identical to `util.inspect.defaultOptions` besides some internal state that is about to be removed.
  }
})

// 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);
}
Expand Down
60 changes: 48 additions & 12 deletions test/parallel/test-util-inspect-proxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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.
Expand Down