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

[BridgeAR]

This prevents any proxy traps from being called while inspecting
proxy objects. That guarantees a side-effect free way of inspecting
proxies.

That aligns the behavior to the one in chrome. Firefox always inspects
the proxy as if showProxy would be set to true and thus has no side-effects
either.

Refs: #25212
Refs: #24765
Fixes: #10731
Fixes: #26231

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2690/pipeline

[addaleax]

Is this semver-major? It looks that way to me.

[BridgeAR]

@addaleax it should be a patch. inspect() is meant to be side-effect-free. As such it was actually a problem that some traps could be triggered in some cases (which ones changed multiple times). It was also not possible to inspect general proxies so far due to that (see the referenced issues).

It would be semver-major if some code relied on a specific trap from being triggered. But since the internals changed often, there has never been any guarantee to if and what traps would be triggered.

[BridgeAR]

But if anyone wants it to be semver-major, that's totally fine to me.

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/20934/
Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/296/

Update: There is no performance regression!

[bathos-wistia]

This PR says “fix proxy inspection” but it entirely disables real proxy inspection, substitutes proxies for their target objects during inspection, and breaks existing inspect.custom implementations on proxies.

[BridgeAR]

@bathos-wistia I am not sure I can follow.

it entirely disables real proxy inspection

Inspecting proxies is possible as before. Just set the showProxy option to true. The only difference is that calling console.*(), util.inspect() or util.format() on a proxy won't trigger the traps anymore (which prevented multiple proxies from being inspectable in the first place).

substitutes proxies for their target objects during inspection

That is the same what chrome does during inspection and it seems the only way to prevent any proxy traps from being triggered during inspection.

breaks existing inspect.custom implementations on proxies.

What traps do you currently rely upon? We never had any guarantee of triggering a specific trap, especially not at a specific point of inspection. What traps got triggered and when changed a couple of times. So relying on any of that is very brittle and could break at any point of time.

It would be good to have an example of what you want to do. I guess it's still possible to do what you want to achieve :)

[Hakerh400]

it should be a patch. inspect() is meant to be side-effect-free.

While it sounds reasonable to not trigger proxy traps on inspection, making inspect side-effect-free seems to be a lot harder task. With this patch, the following code:

const p = {
  constructor: new Proxy(() => {}, {
    get(){
      console.log('side effect');
    }
  })
};

console.log('before');
util.inspect(p);
console.log('after');

results in

before
side effect
side effect
after

[bathos-wistia]

Inspecting proxies is possible as before. Just set the showProxy option to true. The only difference is that calling console.*(), util.inspect() or util.format() on a proxy won't trigger the traps anymore (which prevented multiple proxies from being inspectable in the first place).

Previously, showProxy was a flag which made inspection replace proxies with [ target, handlers ], but if it was false, you could inspect proxy objects themselves. After this change, showProxy being true does the same thing, but showProxy being false causes the proxy object to be replaced with target. The target is not the proxy. It may not have anything to do with the surface implemented by the proxy object at all. The proxy and its target are two different objects, and with this change, the former object cannot be inspected, regardless of inspect config.

I get that it’s intentional, but this is a significant change.

What traps do you currently rely upon? We never had any guarantee of triggering a specific trap, especially not at a specific point of inspection.

[[Get]] for inspect.custom. Or more accurately, usually none, since I’ve rarely needed to implement custom [[Get]]. But the crux is those custom inspect methods need to be called with the correct receiver. That receiver is the proxy, not the proxy’s target.

What traps got triggered and when changed a couple of times. So relying on any of that is very brittle and could break at any point of time.

Custom inspect methods have been honored on proxies so long as the showProxy option wasn’t set to true.

---

It would be good to have an example of what you want to do. I guess it's still possible to do what you want to achieve :)

In my particular case, if, when Symbol.for('nodejs.util.inspect.custom') is found on the target, it still got called with the proxy as receiver, that would prevent breakage. That wouldn’t necessarily prevent breakage for all folks.

[BridgeAR]

@Hakerh400 good point. We still have a couple of accesses that are possible but this prevents pretty much all common and reported cases. Checking for a proxy on every property access seems overkill to me. The same applies for getters. We mainly prevent those but we still access e.g., the size property of a set or the name property for functions and so on.

[addaleax]

Btw, I would agree that “util: disable proxy inspection” or something similar would be a better title, given that it does change behaviour in a way that could both be seen as correct or incorrect, depending on one’s point of view.

[targos]

+1 to semver-major.

I'm in favor of a change like this. It makes the behavior of inspection much more predictable.
IMO There can't be one good way to inspect the proxy itself. Most uses of proxies that I've seen in the real world just don't implement all the traps that would be necessary to show what the proxy "looks like" to its user.

[BridgeAR]

@bathos-wistia if I understand correct, you want the context of the custom inspection not to be set to the target but to the proxy as before. I'll update the PR accordingly.

[bathos-wistia]

@targos Would you agree that there is value to providing an explicit escape hatch for the implementors who do know how to implement well-behaved and complete proxies? Custom inspect was able to fulfill that role in the past.

if I understand correct, you want the context of the custom inspection not to be set to the target but to the proxy as before. I'll update the PR accordingly.

yep! thank you ❤️

[BridgeAR]

@bathos-wistia the context is now the same as before when using custom inspect on proxy objects.

[bathos-wistia]

Super appreciated! For my usage, this will no longer represent a breaking change.

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/20945/

@nodejs/util @targos @addaleax PTAL

[jdalton]

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?

[BridgeAR]

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.

[jdalton]

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?

[BridgeAR]

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?

[jdalton]

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.

[BridgeAR]

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?

[jdalton]

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

[BridgeAR]

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

[jdalton]

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

[BridgeAR]

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.
  }
})

[BridgeAR]

@nodejs/tsc PTAL since this is marked as semver-major (@targos was your comment a LG?).

@nodejs/util this could also still use some general reviews.

[mcollina]

LGTM

[mcollina]

I agree, this is semver-major.

[BridgeAR]

Rebased due to conflicts.

CI https://ci.nodejs.org/job/node-test-pull-request/21049/

[BridgeAR]

Resumed CI https://ci.nodejs.org/job/node-test-pull-request/21054/ ✔️

[BridgeAR]

Landed in a32cbe1 🎉

[Trott]

Did this get the CITGM run mentioned in #26241 (comment)? (If not, maybe do one after-the-fact?)

[BridgeAR]

@Trott uh, no, I forgot about it. Thanks for the reminder!

CITGM (including this PR) https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1750/
CITGM (without this PR) https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1746/

[BridgeAR]

I checked CITGM and I can not relate anything to this PR. But it seems like we should check CITGM in general again as fastify v2 and cheerio seem broken and that's something new (but I do not know what introduced the issue).

[tylerlong]

This is a breaking change as shown in #31989
I cannot figure out an alternative approach