-
Notifications
You must be signed in to change notification settings - Fork 106
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.
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
PrivateName design: null-proto with methods vs. object-with-functions #129
Comments
The problem with this is that well-known symbols (e.g.
Having
This feels a little bit strange to me, since every native JavaScript function can be used on a different |
Agree with @nicolo-ribaudo on his first two points.
I'm neutral on the idea, but wouldn't this increase memory usage? I can't think of another constructor/factory that pre-binds the methods. |
Can you give an example?
I am supremely surprised that decorator-descriptors have a toStringTag, given that property descriptors do not.
People seem to have been misled by my use of the word "bound". There's no binding involved. What I meant was closures. E.g. let x = 5;
function get() {
return x;
}
function set() {
x = 5;
} These functions cannot be used on a different this value using call/apply, because they don't reference this at all. Same for many existing functions in the spec, e.g. the promise resolving functions.
It's unclear to me how that would be important when writing a decorator. |
@rbuckton's comment, and my follow up. Allowing
Isn't that effectively the same thing?
Which promise resolving functions? |
@jridgewell the ThenFinally and CatchFinally functions, for example. |
Excellent, I see now what you mean by promise resolving functions. Doing a simple grep, we currently have the following that create closed over functions:
Proxy.revokable = function(target, handler) {
const proxy = new Proxy(target, handlers);
function revoke() {
proxy.[[ProxyTarget]] = null;
proxy.[[ProxyHandler]] = null;
}
return { proxy, revoke };
} So at least there's precedent. But it kinda feels weird... |
It’s basically just spec fiction to represent closures ¯\_(ツ)_/¯ |
There are a bunch of different issues here, which I'd like to treat separately (unfortunately they were conflated into a single patch). Internal slot on the objectA PrivateName is more than just a way to get and set it--it can also be used as a Do you have an idea for how we would get at the underlying private name value in the case of using bound get and set functions instead? It feels a bit hacky to me to reach into the closure to get out its internal slot--what if the get and set functions somehow differ in their internal slot? It seems cleaner to me to ignore those functions in finding the underlying private name. Bound functionsIf we do have an internal slot on the object, and if (ergonomically) it usually makes sense to call this as a method on the private name object, it seems like bound functions are sort of unnecessary. It doesn't hurt to save a couple allocations--I'd like to avoid introducing cases where private fields have much more runtime overhead than public fields (though this probably wouldn't be that much--just two closures at class definition time). If there's a good reason to make these bound functions in addition to having the internal slot in the private name object, I'd be fine to reconsider, but it seems more complicated than optimal. toStringTagLots of decorators end up being interested in figuring out whether the key is a Private Name or not. One way to check this is
My understanding is that the "branding" mechanism that @domenic , @allenwb and others would prefer is not based on adding more things that query internal slots, but rather All the same things are going on for decorator descriptors, and that's why these have a Note, property descriptors and iteration results don't have I'd be fine replacing the EDIT: See #56 for earlier discussion of "branding" and decorator descriptors. null prototype@jridgewell gave good cross-references above. I'm not as strident as @jridgewell and @rbuckton on protecting against this sort of hazard as being completely critical--there's no way we could absolutely protect against all ways programmers could leak names accidentally, and it doesn't seem like an ocap issue. However, it seems like a realistic, practical programming risk--you could accidentally leak things this way, and that could lead to broken privacy in practice. Given that it's pretty easy to defend against with a null prototype, I'd say, why not? |
I see. In that case, it seems like the design should lean more toward a class instance. So, the shared functions should go on a prototype, and not use a null prototype.
Because it's inconsistent with the rest of the language, for (as you point out) dubious gain. The pattern for method sharing is not this weird null-proto-with-triple-equals-functions-and-internal-slots, but classes, with prototypes, constructors, and methods on the prototypes. That's the pattern that should be used for PrivateName. The above thread cites "module namespace exotic objects" as a precedent for PrivateName's weird design. But that's not a good precedent. First of all, PrivateNames are not exotic. Second, as has been explained previously, the design of module namespace exotic objects is specially tuned to support a single ability, dotted access to module imported bindings. What you have here is instead first-class domain objects in your problem domain, which are best represented by a class. Right now this proposal seems to be attempting to smuggle in some kind of pseudo-defensible-objects framework, unprecedented in the language, and use it for one of your domain objects. That's a much larger discussion, that needs to happen with full committee participation. I anticipate this being a big topic if this proposal is going for stage 3 advancement. |
OK, looking forward to committee discussion! I hope we can settle on one of these two possibilities. If you want to come to the decorator call as well, you'd be welcome; let me know and I'll send you an invite. |
Timeout, I havent read all the threads linked to this issue but why is this a blocker for decorators and not https://github.com/tc39/proposal-class-fields |
@pwhissell Also, I guess you mean https://github.com/tc39/proposal-class-fields rather then https://github.com/tc39/proposal-private-methods. |
@trotyl
thanks, i edited my post! |
There would be compatibility concerns if decorators were introduced without any private field support, and then we tried to add it later. |
We discussed this issue in the July 2018 TC39 meeting. Various people expressed interest in having some sort of integrity properties, but we didn't come to a strong conclusion that we should definitely have these properties, or what form they should take exactly. On balance, some kind of integrity for PrivateName seems like a reasonable idea to me. Although reliable isolation in general requires additional work, it doesn't hurt to have some integrity here, and it seems like it would be nice in practice to have some of these issues already guarded against, given the practical difficulty of being "first" and installing the protection. In the July 2018 TC39 meeting, I asked about whether this should be part of a general defensible classes proposal. I haven't heard back yet from anyone who's interested in championing defensible classes. If someone is interested in pushing that forward, I would love to discuss this all with you in detail. Short of that, I'd suggest that we not hold back decorating private class elements for a general defensible classes mechanism. We have all the mechanisms we need in the ordinary JavaScript object toolbox to make PrivateName defensible, and decorators will any class declaration to opt into this sort of user-defined defensibility. Unless someone is working on a defensible classes proposal now, I'm thinking that we should do our best here to make PrivateName as defensible-style as we can, check this against the intuitions of committee members for what defensible classes might eventually be, and forge ahead. @trotyl In addition to the compatibility concerns @ljharb mentions, we also expect decorators for private class elements to be pretty useful. In the July 2018 TC39 meeting, we discussed again whether it makes sense to have private class elements be decoratable, and I felt like we had renewed consensus for the current path of including this functionality. Note that this isn't the only issue we're still discussing for decorators--the ordering export vs decorators (#69) is still a hot topic for debate. |
Some initial thoughts on defensible classes. I understand that the immediate purpose of wanting clarity on defensible classes is to get clarity on what PrivateName should be. But since this group are the right experts, I'd also like to verify that everything in the following sketch could be implemented by a user-written @def
class Point {
#x, #y;
constructor(x, y) {
#x = +x;
#y = +y;
}
toString() {
return `<${this.#x},${this.#y}>`;
}
add(other) {
return new Point(this.#x + other.#x, this.#y + other.#y);
}
} The
With all of this locked down, multiple mutually suspicious adversaries can share both the Point class, and share some of the instances it made, without violating any of the following invariants. Many of these invariants will simply be true of classes in general, but it is interesting to write them down. None of these are true for vanilla ES6 classes that use public properties rather than private state.
Attn @warner @tribble @jfparadis |
I'm not sure this is a capability that's needed by default for every use case. A nondefensible class could produce defensible instances, and a defensible class seems like it could produce nondefensible instances (forcing the |
I'm trying to make the simple case simple and easy. The user can still express all these other combinations by other means. A non-exotic non-frozen object whose non-frozenness is part of its intended semantics cannot be defensively consistent, since any of its clients can freeze it. A non-exotic object has no way to refuse to be frozen. |
All the smart contract code we're writing at Agoric freezes everything that could possibly matter. This significantly reduces the number of things we need to worry about. |
I'm not contesting that there's plenty of use cases that need a defensible class that produces a defensible instance - i'm suggesting that that might be too strong a requirement for the class itself - which is just the constructor function and its prototype - to be considered "defensible". |
@erights Thanks so much for this prompt and thorough guidance. All of this makes sense to me. I am happy that we can accomplish all of this with the current object model and decorators. Two things which were not obvious to me, but which sound totally reasonable, were:
I share your intuition that the wrapping for instance freezing is unfortunate; I think we could consider addressing this in a follow-on proposal for a decorator to replace the construct behavior. For PrivateName, I'd say that we don't need to emulate this wrapping. What do you think? |
This patch makes PrivateName a deeply frozen, "defensible" class, whereas in previous iterations, it was represented as a primitive, an ordinary class, and finally an object with own methods and a null prototype. The goal of using a defensible class by default, as opposed to being an ordinary class that users can freeze, is to protect privacy by deafult against complex scenarios. In modern JavaScript code, a decorator which others depend on may be implemented in a deep dependency and unable to capture/freeze the original value of PrivateName.prototype proerties. As a result, monkey-patching of that object can make it difficult to preserve privacy of decorated private class elements. A defensible-by-default PrivateName achieves the goal. See [1] for past discussion. The details of the PrivateName class are as follows, based on advice [2] from Mark Miller: - The constructor, prototype, and all methods are frozen objects. - Instances are frozen. - The constructor and prototype have null [[Prototype]] values. - The constructor, when called, throws a TypeError, matching the decision [3] to not expose the PrivateName constructor. If we were to support new-ing the PrivateName constructor, the semantics would be such that instance is frozen only if new.target === PrivateName. [1] #68 [2] #129 (comment) [3] #68 (comment)
@littledan Say decorators already existed, would it not be possible to define/implement "privateness" of members afterwards? |
@pwhissell the list of class elements would suddenly change to include private members, so class decorators which depended on only public members being in the list could easily break. |
@ljharb
|
@pwhissell the difference is the implementation of |
@ljharb but the user has to make a change in his class to make some fields private once "privateness" exists. This user's decorators will always work untill he changes his class to make some members private in which case I believe he is also responsible for changing the isPublic method. I just feel that any future keyword that describes class members would have the same effect, "static" or "final" for example. |
The word "defensible" is tricky. Let's start with "defensive" with the standard being defensive consistency:
The class and its instances represent some abstraction. For the class to be defensively consistent, the instances it produces should themselves be defensively consistent. If an instance gets corrupted, then the class has not provided good service. The point of defensive consistency is it enables us to reason more about the abstraction and less about how the abstraction is implemented. That's why my example above is followed by a long list of invariants --- guaranteed properties that stay true despite fallible clients. Since a non-frozen non-exotic object cannot resist being frozen by its fallible clients, its non-frozenness cannot be part of its intended API surface. |
@erights I'm so happy people like you are thinking about security subjects like these because the are important issues and the structure and implementation of visibility should definitely be part of a proposal. Even if how privateness is implemented can change how users implement their visibility dependant decorators, I'm still becoming more and more convinced that member visibility shouldn't be a blocker for decorators. What do you think? |
This patch makes PrivateName a deeply frozen, "defensible" class, whereas in previous iterations, it was represented as a primitive, an ordinary class, and finally an object with own methods and a null prototype. The goal of using a defensible class by default, as opposed to being an ordinary class that users can freeze, is to protect privacy by deafult against complex scenarios. In modern JavaScript code, a decorator which others depend on may be implemented in a deep dependency and unable to capture/freeze the original value of PrivateName.prototype proerties. As a result, monkey-patching of that object can make it difficult to preserve privacy of decorated private class elements. A defensible-by-default PrivateName achieves the goal. See [1] for past discussion. The details of the PrivateName class are as follows, based on advice [2] from Mark Miller: - The constructor, prototype, and all methods are frozen objects. - Instances are frozen. - The constructor and prototype have null [[Prototype]] values. - The constructor, when called, throws a TypeError, matching the decision [3] to not expose the PrivateName constructor. If we were to support new-ing the PrivateName constructor, the semantics would be such that instance is frozen only if new.target === PrivateName. [1] #68 [2] #129 (comment) [3] #68 (comment)
@erights, @FUDCo, @warner and I had a conversation about PrivateName as a defensible class. @erights reiterated his reasoning from #129 (comment) , which explains why not just the prototype should be frozen but also instances and methods. We concluded that we should also give the methods null prototypes. |
um, i disagree with that last one - then they can’t be used with .call, .apply, or .bind, or .toString, etc, without borrowing methods. They’ll also lose toStringTag, and the name accessor. |
This isn't an argument for or against the change, but the PrivateName constructor also has a null prototype, in tip of tree. |
To make PrivateName a truly defensible class, this patch makes it so that no mutable objects are reachable from it, per #129 (comment) Closes #129
Just want to make clear here the conclusion I stated in #139 : That none of the [[Prototype]] links should be severed, including that of the constructor. Why was the constructor ever treated differently than the methods? |
@erights Thanks for the summary and detailed look at this issue. We used a null prototype to prevent cases that @jridgewell and @rbuckton raised, e.g., if someone does However, I have never been sure that it's necessary to defend against that particular case. We will need to do some education anyway--it's always possible to leak these names, e.g., by passing a name as an argument to another function; there are other ways to tell whether something is a PrivateName. |
The more general problem is that I do not suggest we do the following, but it is worth making the observation. A less violent way to address this one case would be a special case of @ljharb 's suggestion at #139 (comment) : Define a frozen Rather, we need to make clear:
|
Thanks @erights for this thorough analysis and practical suggestion for what we can do today. Let's review this idea in this coming week's decorators call, in addition to continuing to discuss it here in this thread. |
@erights Quick question, is it a problem if these method/constructor objects are frozen and don't permit rewriting away the link to |
Previously, some prototypes were set to `null`. This patch reverts all prototypes to their natural value, following analysis by Mark Miller that such a change is appropriate from a data security perspective. See #129
Previously, some prototypes were set to `null`. This patch reverts all prototypes to their natural value, following analysis by Mark Miller that such a change is appropriate from a data security perspective. See #129
We've settled on using a prototype which is deeply frozen for PrivateName. |
The private name stuff has evolved a lot recently, but I think I'm caught up. The latest is that you create an object with:
privateName.get.call(otherPrivateName)
will work).This seems like a strange design that's halfway between a class and an object.
Have you considered instead just using an object that contains two closures? That is, a normal ordinary object, with two functions that each themselves have an internal slot pointing to the private name.
The main user-observable differences would be:
privateName.get.call(otherPrivateName)
will be equivalent toprivateName.get()
;this
is ignored, as they are functions, not methods)This seems like a much more normal design to me.
The text was updated successfully, but these errors were encountered: