Repository navigation
console.log(proxy) invokes get trap with built-in Symbols #10731
Description
Activity
I have long been wanting to revisit the proxy inspection code to see if we can improve performance. If you'd like to look into it and come up with a PR I'd be happy to help!
@jasnell cool. Happy to. Thoughts on desired behavior?
<target>(node's default behavior), with the trap invocations avoidedProxy [<target> <handler>](node's behavior withshowProxy: true), if we can get it fast enoughProxy <target>(Chrome's behavior)
I tend to prefer Chrome's behavior if we can get it fast enough. The key challenge is that we end up having to do the isProxy check on every value and on some keys (e.g. when inspecting things like
Map), which is what leads to the significant performance hit.Also keep in mind the original motivation for the
showProxychange -- that is, to avoid other troublesome issues and side effects that occur with Proxy traps (getin particular)var o={valueOf:function(){console.log("X");return 1}} console.log(o);{ [Number: 1] valueOf: [Function: valueOf] }Not only Proxy but also normal Object.
Anyone still working on this? Maybe it's time to add a
help wantedlabel?I haven't had time to keep working on this, no. :(
Any possible workarounds to this?
Nevermind, just returning the object itself (when the get trap is triggered by a symbol) seems to do.possible workaround here: https://stackoverflow.com/a/46928371/242298
On master it will only trigger symbol calls such as:
symbol Symbol(util.inspect.custom) symbol Symbol(Symbol.toStringTag) symbol Symbol(Symbol.iterator)It would be possible to work around this specific trap but then we end up in a different one. I know this is not ideal but I do not really see a way we can address this ideally. If anyone has some ideas, please leave a comment!
I’d like to make a case for not giving Proxies special treatment. In fact I think the showProxy option should be removed entirely, but assuming that’s not an option, at the least it should not be defaulting to true in any context (like it does in the REPL) and shouldn’t be doing any additional special casing.
I understand that proxies are perceived as having a higher risk of having buggy implementations. That’s probably true; no matter how well you understand the invariants of the internal methods being trapped, it’s still pretty easy to make mistakes. It’s a low-level (from an ES perspective) API. But while there will be flawed implementations of internal methods like [[Get]], this isn’t really different from there already being accessors that throw in the wild, and since [[Get]] is the biggie, this can likely be mitigated by doing a [[Has]] check first (e.g.
Symbol.toStringTag in object), which doesn’t need to be specific to proxies.Proxies are one of the few concepts in ES which can be categorically called an implementation detail. Other “implementation details” are mostly observable, but proxies are not. Both a target and a handler object may obtain the same level of privacy as a closure/scope, and no code with access to a given object it did not create can know if it is a proxy or not, by design.
The presentation of objects as proxies in the REPL is mysterious. Even [util.inspect.custom] is ignored and Node prints details of the internal implementation instead of the realized public interface. Since it’s not uncommon for the target of a proxy to have no own enumerable properties, it often will just print
Proxy [ {}, { defineProperty: [Function: defineProperty], ...etc } ]. This presentation is particularly frustrating when proxy objects are used in a public API that should be learnable by exploring from the REPL or with logging. Nothing shown relates to the object’s interface as seen by other code.But in addition to seeming at odds with the intentions of Proxy objects, the mechanics of observing Proxy implementations in util.inspect are themselves problematic. It doesn’t just observe that a given object is a proxy exotic object — it actually obtains references to the target and handler, values which haven’t been made available to that scope. The V8 C++ API calls that are necessary for achieving the current behavior violate invariants of ECMAScript itself. If the values never escaped util, that might be okay, but presently, it actually is exploitable:
const util = require('util'); const victim = function() { return new Proxy({}, { SECRET: 'One of the few guarantees in JS: nothing outside can see this' }); }(); let secret; const invariantViolator = { [util.inspect.custom](depth, options) { const { stylize } = options; options.showProxy = true; options.stylize = value => { secret = value; options.stylize = stylize; return ''; } return victim; } }; util.inspect(invariantViolator); console.log(secret);
This exploit can be fixed pretty easily. But I think it’s symptomatic. My understanding was that V8’s C++ APIs are intended for making external functionality available to ECMAScript using ECMAScript values and ECMAScript semantics. Here though, it is being used to bypass ECMAScript semantics. The behavior seen in the above script isn’t possible in ECMAScript — but neither is distinguishing between objects which have been implemented as ordinary objects and objects implemented as proxy exotic objects.
Reacted by Anna Henningsen, Iain J. Reid and Nicolò RibaudoTo get back to the original issue: the reason why the traps are not called in the browser is that they use internals that Node.js does (and I would say should) not use. We mainly use regular JavaScript to access all the information. That way we have to trigger some of the traps. We could switch triggering one trap for another but that should not really be helpful.
We also trigger the
ownKeys,getPrototypeOfand thegetOwnPropertyDescriptortraps since we have calls that trigger those traps. I don't really think we can change much about this. If I am mistaken, please give me a hint, otherwise I would like to go ahead and close this issue as a won't fix.
@simonkcleung this is not triggered in newer Node.js versions anymore.
@bathos I have no strong opinion about the way proxy inspection currently works but to me it seems like a separate concern from the issue that proxy traps are invoked during inspection. Would you be so kind and open a separate issue about this?
Reacted by Darien Maillet ValentineI won't be hurt at all if this is closed as wont-fix. It makes sense that proxies should be written to handle various inputs safely (at least when they're publicly accessible). I think the original point remains valid, though, that Node.js' inspect functionality could avoid the issue. It might not be worth fixing (it sounds difficult), but it seems like it'll remain a gotcha for Node.js users.
If anyone stumbles upon a good idea how to prevent some traps, please leave a comment.
- added 2 commits that reference this issue
on Feb 28, 2019 With that PR no proxy trap is going to be triggered anymore.
What if I need a trap to know that an object is inspected? If proxy doesn't support it, then the trap has to be installed onto the target object directly, which is not good.
Reacted by extremeheatThis seems like an oversight to me. So how exactly are you supposed to trap the inspect symbol on a proxy? There is no way to extend a proxy.
Calling
console.log(util.inspectwith default opts) on an instance of a Proxy invokes thegettrap with some built-in Symbols. (It also invokes the trap with "inspect" and "valueOf", which is different than browser behavior but doesn't seem as problematic.)In node:
In chrome:
In FF:
In Edge:
MSEdge output
If you're expecting your object to have Symbol keys and thus handle them, that's fine, but if you're only using string keys, then this can make it necessary to add a guard clause like
if (typeof p !== "string") return o[p];within thegettrap. None of the above three browsers invoke the trap with a Symbol, by comparison.The
showProxy: trueoption forutil.inspectprevents these four calls and making it the default could be a fix, but from the comments in #6465 it is apparently too slow. However, a simpleisProxybinding (args.GetReturnValue().Set(args[0]->IsProxy())) is 10x faster than thegetProxyDetailsbinding (29 ms vs 280 ms for 1M ops), and the target can be inspected the same as a plain object (without the binding). It seems like there's some special casing that could be done cheaply to avoid invoking thegettrap with symbols, or even invoking it at all.Maybe this is a wontfix -- maybe it's only sane if symbols are handled -- but right now this seems like an addition required to make some get traps compatible with node.