Handle proxy object prototypes more correctly

[jasnell]

Refs: #2291

[kentonv]

Can you give an example (JavaScript code) where this makes a difference? I don't really understand from the description.

[jasnell]

class Foo {}
Object.freeze(Foo)
Object.isFrozen(Foo);  // true... Foo is non-extensible
const P = new Proxy(Foo, {getPrototypeOf() { return Array }}) // try changing the prototype
Object.getPrototypeOf(P); // throws!

TypeError: 'getPrototypeOf' on proxy: proxy target is non-extensible but the trap did not return its actual prototype
    at Function.getPrototypeOf (<anonymous>)

We are not performing the additional check so our code would return Array rather than throwing in this case

[jasnell]

The motivation here, as far as I understand it, is that if the target is sealed or frozen (aka non-extensible) then the Proxy trap is not really supposed to be able to change the prototype that it returns as that could change the behavior of the proxied object in a way that violates the seal. For our purposes, however, it's not entirely certain we need to worry about that check, and implementing the check at the c++ layer would require patching v8, which I'm trying to avoid here. If we need it, then so be it... In that case I'll just patch v8 to expose v8's JSProxy::GetPrototypeOf implementation as a new method on v8::Proxy

[kentonv]

I dunno, we might need it. Keep in mind we follow prototype chains manually in JSRPC, and then invoke the method we find. Will we potentially be violating the seal when we do that?

I don't really understand this explanation though. Isn't your code strictly doing things that someone could do in pure JS? It shouldn't be possible to violate a seal or freeze from pure JS, should it? Again, example code would really help in understanding this.

If we can't be really sure we understand this then I would say we'd better go the route of patching V8 with a GetPrototypeJS() that actually runs V8's logic.

[jasnell]

Isn't your code strictly doing things that someone could do in pure JS? It shouldn't be possible to violate a seal or freeze from pure JS, should it?

That's the point. Reporting a different prototype when the object is sealed violates the seal and is why v8 has this additional check. The example I showed is the error as it exists in browsers and node.js. What I don't have a good sense of is just how critical this check is. It's certainly an acceptable argument that If We Don't Know, err on the side of caution. It'll likely have to wait until I'm back from PTO tho unless you'd like to take a stab at the v8 patch

[kentonv]

Wait sorry, when I looked before I missed your first comment somehow and only saw the second.

[kentonv]

OK, looking at your code example helps. But it confuses me because in my opinion the V8 behavior seems wrong to me.

In my mind a Proxy is just a convenient way to define a wrapper around some other object that intercepts and redirects some or all functionality. The wrapper is a different object from the target. The reason a Proxy has a target at all is mostly convenience -- so that functionality you don't explicitly override can fall back to operating on the target.

In that model, it makes no sense to me that because the target happens to be frozen, the proxy would be restricted in what it's allowed to override.

But I am probably not understanding something about proxies.

Still not totally sure what to do here though. I worry that if we implement this "wrong" it could come back to bite us later and will be hard to fix.

[jasnell]

Still not totally sure what to do here though. I worry that if we implement this "wrong" it could come back to bite us later and will be hard to fix.

yep, I share this concern. I'm just not sure how likely the issue is to come up in a practical sense. I think I will just go ahead and do the v8 patch but it will have to be when I'm back.

[kentonv]

Put another way, I don't understand why creating a Proxy that overrides the prototype is being treated as equivalent to re-assigning the prototype on the underlying object. The Proxy is not the same object, and it should be allowed to have whatever prototype it wants IMO.

Maybe we just go ahead with this.

[kentonv]

Can you link to V8's code for this, for comparison?

[jasnell]

https://github.com/v8/v8/blob/main/src/objects/objects.cc#L1379-L1432

[harrishancock]

Is it possible to construct a cyclic object which could cause this to infinitely recurse?

[jasnell]

It might be but I would think that our existing cpu limit mitigations would handle that case. That said, I can't think of a way off hand to create a cyclic proxy on the target.

[kentonv]

I'm going to go ahead and approve this.

I'm still a little worried about the difference in behavior when the target is frozen, but I actually question whether the V8 behavior is even correct. In any case it doesn't seem like it creates any kind of problem that we can't solve with a compat flag later.

[jasnell]

How about this... for now, we can emit a periodic warning log if the proxy is returning a different prototype. This will at least give us a sense of whether anyone is potentially hitting this. I agree we can fix this up with a compat flag later if absolutely necessary

[kentonv]

(Clicked merge since James is on vacation.)