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.

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

Should PrivateName be a frozen/defensible class? #68

Closed
littledan opened this issue Feb 28, 2018 · 64 comments
Closed

Should PrivateName be a frozen/defensible class? #68

littledan opened this issue Feb 28, 2018 · 64 comments

Comments

@littledan
Copy link
Member

In a meeting discussing decorators with potential implementers, @gsathya, @msaboff and @akroshg expressed some concern about the implementation complexity of adding a new PrivateName primitive. The current specification requires handling PrivateName in all sorts of contexts--this could be complexity which grows over time as the language evolves.

Is it possible to avoid exposing PrivateName directly, and instead use some kind of closures, but still get at the same kind of expressivity? Let's brainstorm API ideas.

@bschaepper
Copy link

How about adding placement "private", and working with Strings? Granted, decorators that don't care a out the distinction suffer a bit. If there is to be a "protected" later, even more so.

@gsathya
Copy link
Member

gsathya commented Mar 7, 2018

Providing a private field factory function as the last parameter to the decorator could work. For example, the @observed decorator could be updated as:

function observed({kind, key, placement, descriptor, initializer}, privateFieldMaker) {
  assert(kind == "field");
  assert(placement == "own");
  // Create a new anonymous private name as a key for a class element
  let { get, set } = privateFieldMaker();
  let underlyingDescriptor = { enumerable: false, configurable: false, writable: true };
  let underlying = { kind, key: storage, placement, descriptor: underlyingDescriptor, initializer };
  return {
    kind: "method",
    key,
    placement,
    descriptor: {
      get() { get(this); },
      set(value) {
        set(this, value);
        // Assume the @bound decorator was used on render
        window.requestAnimationFrame(this.render);
      },
      enumerable: descriptor.enumerable,
      configurable: descriptor.configurable,
    },
    extras: [underlying]
  };

@rbuckton
Copy link
Collaborator

rbuckton commented Mar 7, 2018

@littledan, why not a specially branded symbol, i.e. via Symbol.private(), but special cased to work like PrivateName is currently? Having an opaque value we could use for privileged access was important to resolving #24, per our discussion last meeting.

@rbuckton
Copy link
Collaborator

rbuckton commented Mar 7, 2018

@gsathya, you still need a key, in your updated example storage is not defined.

The factory approach also makes it impossible for a decorator to use a shared private name across multiple different declarations, which is a useful scenario (such as for branding).

@littledan
Copy link
Member Author

@gsathya Yes, that's a good half of the equation, but as @rbuckton says, I don't understand what the value of key would be in case you have an example like class C { @observed #x; }

@gsathya
Copy link
Member

gsathya commented Mar 7, 2018

oops, yes. The factory could return an object with an internal slot for the private field, so that it doesn't leak to userspace:
{ [[PrivateName]]: new PrivateName(), get() { ... }, set() { ... } }

and then update key to take key: String, Symbol or Object (where assertNotNull(Object.[[PrivateName]]))

@rbuckton
Copy link
Collaborator

rbuckton commented Mar 7, 2018

This isn't much different than just having a PrivateName global you can just new, other than its typeof would be "object" as opposed to a new primitive.

@gsathya
Copy link
Member

gsathya commented Mar 8, 2018

Yes, from a spec point of view. Now, implementations don't have to plumb through a new primitive in all the backends.

@rbuckton
Copy link
Collaborator

rbuckton commented Mar 9, 2018

I'm not opposed to a global PrivateName constructor for an object rather than a primitive, but I would be opposed to PrivateName.protoype.get/PrivateName.prototype.set for the reasons mentioned in #24.

We discussed resolving #24 through something like a PrivateName.restricted(name) that returns a copy of the private name name that can only be used to define the property but cannot be used to read/write the property to prevent leaking privilege. It's possible this could still work with get/set, assuming they threw a TypeError on the restricted copy.

@littledan
Copy link
Member Author

littledan commented Mar 9, 2018

@rbuckton That plan for #24 sounds good to me. Given that the get and set functions in the current draft spec don't close over particular private names, such analogous TypeErrors would have to be created for the current interface as well if we provide the sort of v2 upgrade we were discussing in #24. In the short term, I think the use cases can get by with WeakMap (even if a goal of this proposal is to decrease the need for WeakMap for private state).

@gsathya At the decorators native implementer meeting, we heard from V8 (@gsathya), JSC (@msaboff) and ChakraCore (@akroshg) that it seems pretty complicated to add a new primitive type for PrivateName, and it'd be preferable to avoid that. This seemed to be the strongest piece of implementation feedback about the whole proposal. If primitives are harder to plumb through systems than classes, what if we used a completely frozen class for PrivateName? It could look something like this:

PrivateName objects have two internal slots:

  • [[PrivateName]]: The actual underlying private name value
  • [[PrivateNameDescription]]: A string for the name, or undefined. E.g., for #x, it'd be "x".

PrivateName.prototype has three methods:

  • PrivateName.prototype.get takes an object as an argument and performs PrivateFieldGet with the [[PrivateName]] of the receiver and the argument.
  • PrivateName.prototype.set takes an object and value as an arguments and performs PrivateFieldSet with the [[PrivateName]] of the receiver and the arguments.
  • get PrivateName.prototype.descriptor returns [[PrivateNameDescription]]

PrivateName and PrivateName.prototype both have a [[Prototype]] of null. They are both frozen, and all of their methods are frozen.

The PrivateName constructor would not be a property of the global object, but instead passed to all decorators as the second argument. When constructed with new, it would take a single argument, which is the description, and create a new instance.

Because there is no toString method in the prototype chain, ToPropertyKey will fail (as we require it to) without requiring modifications to the algorithm. Property access is pretty complicated and pervasive throughout JavaScript, and implementers raised the issue that it would be preferable to not modify this algorithm.

It's true that PrivateNames would not be monkey-patchable, but this is entirely be design: the premise here is that it's . For this reason, the current interface proposal is not monkey-patchable. I hope it's not considered worse to switch to another, superficially different, non-monkey-patchable design just because it's based on objects rather than primitives. cc @ljharb who is interested in monkey-patching and @erights who is interested in defensible classes.

How does this plan sound?

@ljharb
Copy link
Member

ljharb commented Mar 9, 2018

The concern is that a) new functionality might be added to PrivateName in the future, requiring that it be mutable to polyfill, and b) that an implementation will ship this proposal with a bug, which a polyfill would need to be able to fix (it happens often).

I don’t think a frozen class is a great solution for these reasons.

@littledan
Copy link
Member Author

@ljharb I don't see a great way to monkey-patch on more functionality or bug fixes to the current API either, yet we arrived at it to meet @bmeck's "hard privacy" goals. What do you think?

@ljharb
Copy link
Member

ljharb commented Mar 10, 2018

One way might be a transpiler step that wraps called decorators in another function, that receives whatever PrivateName objects are available and provides them, wrapped/modified, to the actual decorator.

@littledan
Copy link
Member Author

@ljharb Does the possibility of this transpiler step mean that it would be OK to have PrivateName be a frozen class?

@ljharb
Copy link
Member

ljharb commented Mar 12, 2018

Hmm - if unfrozen, would every non-function object accessible to a decorator be newly created every time?

@littledan
Copy link
Member Author

littledan commented Mar 13, 2018

@ljharb Well, that's another alternative. But I don't see how that would enable monkey-patching.

It would help to understand a little more what goals you have here. @bmeck has explained that, for his use cases, it needs to not be possible to change or intercept (e.g., by monkey patching) the behavior of getting and setting private names. Are you asking for the exact opposite, that you can make that change? Or, are you asking for something more superficial about avoiding frozen built-in objects?

@ljharb
Copy link
Member

ljharb commented Mar 13, 2018

I’ll try to clarify; since decorators are functions, users shouldn’t be able to use one to get access to mutate anything that could alter the behavior of other decorators. Since decorators are also syntax, it should be possible to transpile the decoration itself such that missing or broken features can be correctly provided to the intended decorator function.

As long as all the characteristics of arguments passed to a decorator function by decoration syntax are replicable in JavaScript, and as long as there’s no shared (between decorations) mutable objects provided to decorators that could violate @bmeck’s concerns, then mine are met as well. Sorry if I’ve been unclear previously, i hope this makes sense :-)

@littledan
Copy link
Member Author

I think a transpiler would be able to send other objects instead of the built-in frozen PrivateName objects to decorators. Based on that, it seems to me like the design in #68 (comment) would meet your requirements--not by polyfills alone, but by cooperation with a transpiler. Does that match your understanding?

@ljharb
Copy link
Member

ljharb commented Mar 13, 2018

That seems right.

@littledan
Copy link
Member Author

OK, thanks for your consideration here. @caridy, @diervo @rbuckton and I discussed switching to the design in #68 (comment) and are in agreement about it, so I think we should write this up in spec-ese and land it. I'd welcome any more review feedback from implementers, as this change is motivated by their feedback.

@littledan
Copy link
Member Author

Would anyone be interested in writing the specification text for this new interface, as described in #68 (comment) ?

@littledan
Copy link
Member Author

In the March 2018 TC39 meeting, the idea of a non-frozen PrivateName class was brought up and discussed among @bmeck, @erights and others. Integrity would be ensured similarly to other objects--run code "at the beginning" to save any important functions off to the side, "before" they might get overwritten. @bmeck, do you see any issues for this approach?

I think the next step here would be to draw up some straw-person non-frozen PrivateName object spec text, and then discuss it in some more detail in a call.

@bmeck
Copy link
Member

bmeck commented Apr 3, 2018

@littledan I still don't see that as hard private, is there a reason we can't use the well know functions approach that are not member methods? That was what I thought we were still looking at.

@littledan
Copy link
Member Author

OK, let's follow up in the call with Mark.

littledan added a commit that referenced this issue Apr 6, 2018
This patch changes PrivateName from a primitive type to a non-frozen
object. Rather than passing the `get` and `set` functions to decorators,
the PrivateName constructor is instead passed, and is no longer present
as a property of the global object.

The change is hoped to reduce implementation complexity compared to the
previous primitive type semantics, and describes a possibility which
was discussed at the March 2018 TC39 meeting.

Applications which need an unmodified version of PrivateName's methods
are recommended to make a copy of them early on before anything modifies
them, similarly to any other JS built-in.

Addresses #68
@littledan
Copy link
Member Author

At this point, everyone I've talked to or heard from is in agreement about it being fine to use an object rather than a primitive for PrivateName. The main point of contention here remains whether the PrivateName should be frozen or not. In a call between me, @erights, @rbuckton and @bmeck , we discussed this question. @rbuckton and @bmeck expressed a desire to have this more locked down; @erights and I couldn't really understand what that would defend against exactly. Maybe it would make sense as a defense in depth, even if it's not an absolute defense. We'll think more about the threat model and discuss more soon.

littledan added a commit that referenced this issue Apr 18, 2018
This patch changes PrivateName from a primitive type to a non-frozen
object. Rather than passing the `get` and `set` functions to decorators,
the PrivateName constructor is instead passed, and is no longer present
as a property of the global object.

The change is hoped to reduce implementation complexity compared to the
previous primitive type semantics, and describes a possibility which
was discussed at the March 2018 TC39 meeting.

Applications which need an unmodified version of PrivateName's methods
are recommended to make a copy of them early on before anything modifies
them, similarly to any other JS built-in.

Addresses #68
@jridgewell
Copy link
Member

(Sorry everyone, but this is a long response. It'll probably take you 15min to read and understand 😬. I tried to break it up into digestible sections)

I think using objects with methods is a huge usability improvement over passing in the get/set helper functions into the decorator call. However, we've lost all privacy encapsulation for dynamic privates in this change.

To illustrate, let's imagine the @observed decorator, and various users of that decorator.

function observed(propDescriptor) {
  // Let's pretend the observed prop is a private `#x`.
  assert(propDescriptor.kind === 'field');
  assert(typeof propDescriptor.key === 'object'); // Because I can't instanceof...
  assert(propDescriptor.placement === 'own');

  // Also, it's really difficult to create a new `PrivateName`.
  const backing = new propDescriptor.key.constructor('backing');

  // We're going to turn `#x` into an accessor, backed by `backing`.
  const init = propDescriptor.initializer;
  delete propDescriptor.initializer;
  propDescriptor.kind = 'method';
  propDescriptor.descriptor = {
    get() {
      return backing.get(this);
    },
    set(v) {
      backing.set(this, v);
    }
  }

  // Let's setup `backing` on the class.
  const backingDesc = {
    kind: 'field',
    key: backing,
    placement: 'own',
    initializer: init,
    descriptor: {},
  }
  propDescriptor.extras = [backingDesc];
}

If I've not made any coding mistakes, let's assume this is the proper usage of dynamic privates with decorators.

Levels of Privacy

Now, there are three levels of privacy encapsulation I associate with this:

  1. Common usage does not leak.
    • An example of common usage being my @observed decorator above, using .get and .set.
  2. Decorators cannot be attacked into leaking privates.
  3. Uncommon usage does not leak.

The current API does not provide privacy encapsulation at any of these levels. Let's walk through some scenarios to explain.

Level 1 Privacy

Scenario: I am a library author who uses @observed in my codebase. I trust @observed because I've reviewed it. I trust my own code because I wrote it. I distrust the developers who use my library, so I must use privates to encapsulate my classes.

The issue though, it that the developers using my library can extract any dynamic private I use. To explain, imagine the following code written by that developer:

const privatesDevWantsToExtract = new Set();

function monkeyWithPrivates(propDesc) {
  // Gain access to PrivateName
  const PrivateName = propDesc.key.constructor;
  const proto = PrivateName.prototype;

  // Now developer owns every dynamic private
  const { get: getter, set: setter } = proto;
  proto.get = function(obj) {
    if (devWantsToExtractFrom(obj)) {
      privatesDevWantsToExtract.add(this);
    }
    return getter.call(this, obj);
  };
  proto.set = function(obj, val) {
    if (devWantsToExtractFrom(obj)) {
      privatesDevWantsToExtract.add(this);
    }
    setter.call(this, obj, val);
  }
}
class X {
  @monkeyWithPrivates
  #x = 1;
}

// Later, dev imports my library's classes and uses them as necessary
// to extract the privates I `@observed`.

So with some setup, the developer has weakened my library's encapsulation, all because I used @observed. And remember, @observed is just using the common paths of dynamic privates.

Level 2 Privacy

Scenario: I am the author of a decorator library that provides @brand that adds a common brand to classes. I trust my decorator code, because I wrote it. I don't necessarily trust the libraries that use my decorator, some of them may be trying to attack another library that uses @brand.

(Note, this is @rbuckton's scenario #24).

In this case, there are two users of my decorator library. One is an innocent library developer, who uses my decorator to do something. The other is an attacker, who's trying to extract the private field from the innocent dev. This is done by directly calling my @brand decorator and not using it as an actual decorator.

// attacker
import { brand } from 'decoratorLib';

brand({
  // this obj looks like a ElementDescriptor, or ClassDescriptor, etc.
});

Again, see #24 for further explanation.

Level 3 Privacy

Scenario: I am an inexperienced developer writing a decorator to do something. I do not trust my own code, because I don't know any better. I do not trust anyone else, because I don't know any better.

This is a "robustness" scenario. It's here to mean that there is no way for me to accidentally leak my private state because of my poorly written decorator code.

Full details of possible leakers was provided by @rbuckton in #68 (comment). There may be more...

Back to Discussion

There are three levels of privacy encapsulation:

  1. Common usage does not leak.
  2. Decorators cannot be attacked into leaking privates.
  3. Uncommon usage does not leak.

Unfortunately, the current API does not provide guarantees of encapsulation at any of these levels. That's extremely worrying to me. It's essentially no better than a throwing WeakMap API.

I think Level 1 Privacy is stage blocking. It is critical that we get this one right. If we can't guarantee at least this, we shouldn't even be providing an API for decorators to interact with private state.

Level 2 Privacy is something we should consider in our API, but its not blocking.

And Level 3 Privacy will be so difficult to fully solve, it may be OK if we don't do anything about it. We should at least look into what we think might be common footguns.

Solutions to Different Levels of Privacy

First, everything is solvable using a frozen class. But frozen class instances feel icky. I'm fine with someone mucking around with an instance after they've created it. I just want to ensure that they start out with a pristine, unpatched instance during construction.

So, if we've rejected fozen classes, here are some solutions that don't require it:

Level 1 Privacy Solutions

There were 2 weaknesses with my with my @observed decorator:

  1. Cannot construct a PrivateName without getting access to a syntactic private name element key.
  2. Inheritance off of a monkey-patchable prototype.

We can allow constructing private names through syntax: private #x declarations, or something similar. This is guaranteed to construct using the %PrivateName% constructor. Second, we can install the original %PrivateNameGet% and %PrivateNameSet% as own (regular data) properties on the new instance.

This would guarantee the "common path" is safe, at the expense of monkey-patching. If you want to fix a bug with dynamic private usage, you're free to call into some monkey patcher with your already constructed instance.

Level 2 Privacy Solutions

@rbuckton's already gone into great detail in #24. Essentially, it boils down to providing a key property on the instance, which does not have access to .get and .set.

Level 3 Privacy Solutions

This requires looking at any of the leakers from #68 (comment), and installing own properties to prevent monkey-patchable prototype lookups. It's a lot of work for what may be uncommon code usages.

Rejection of Monkey Patching

Unfortunately, allowing monkey-patching on the prototype and then inheriting from the prototype is incompatible with privacy encapsulation. Any override of .get or .set that's inherited by a newly constructed instance means there are no privacy guarantees.

From #68 (comment), @ljharb highlighted two circumstances where he wants monkey-patching:

  1. new functionality might be added to PrivateName in the future, requiring that it be mutable to polyfill
  2. that an implementation will ship this proposal with a bug, which a polyfill would need to be able to fix

In circumstance 1, this is solved by passing the instance to a polyfiller after construction. The class instance itself is un-frozen, and you're free to muck with it however you want. But the difference is only you can muck with it, not everyone who has access to %PrivateNamePrototype%.

Circumstance 2, though, is only half solved. If I care about a bug, I'm free to monkey patch it like Circumstance 1. But, I can't force someone else's already-written code to call this patcher function. It will continue to have this bug until they update to call the patcher, or the bug is fixed.

But I'm ok with that. It's difficult for me to imagine a scenario where regular syntatic usage of a private will be safe from bugs, but dynamic usage will have one.

// Syntatic usage
class X {
  #x = 1; //
  method() {
    this.#x++;
  }
}

Specifically, I imagine both the syntatic and dynamic usage will suffer the same bug, in which case everything is screwed anyways.

@ljharb
Copy link
Member

ljharb commented May 26, 2018

That is a very well written analysis, and i agree that my two polyfillng concerns would be sufficiently met; and if i had to choose between polyfillability and robust encapsulation, i would take encapsulation every time - especially for this particular feature.

@littledan
Copy link
Member Author

// Also, it's really difficult to create a new PrivateName.
const backing = new propDescriptor.key.constructor('backing');

The PrivateName constructor is passed as the second argument to all decorators. You could use this with instanceof if you wanted to, but typeof works here too.

monkeyWithPrivates

What's left out of this code sample is a mechanism for keying on the actual private name that you care about and keying your monkey-patched get/set calls based on that. I think this will end up being too difficult to make the "attack" practical.

A decorator library which wants to defend against this "attack" can read out the original get and set methods earlier. This is just like Array.prototype.map: You can monkey-patch that function to "leak" random arrays, but you won't know where they came from or what to do with them, and it can be "defended against" by trying to copy that function out earlier.

Level 2 Privacy

We described in #24 how this could be developed as a follow-on proposal. I don't see anything in this version of decorators that makes the follow-on proposal difficult. Are you saying that it's required that we put this capability in decorators v1? I've been trying to keep v1 pretty minimal.

I am an inexperienced developer writing a decorator to do something. I do not trust my own code, because I don't know any better. I do not trust anyone else, because I don't know any better.

I'm not really sure how to function as a developer given these constraints... I think we should assume decorator authors are able to learn the decorator API. If you don't trust yourself, then how are you going to make sure you don't write the private name in some other public place that you aren't supposed to (for example, a global variable, if you accidentally omit a var declaration)?

We can allow constructing private names through syntax: private #x declarations, or something similar.

@caridy brought up something similar, but I'm not sure what you two are proposing. Once you use that syntax, how would you access the name?

Any override of .get or .set that's inherited by a newly constructed instance means there are no privacy guarantees.

This is really hyperbole. There are the same privacy guarantees as any other method call, which you also expect to be encapsulated in a sense. If you make use of the original method, then there is no leak. I still think the "leak" is impractical to take advantage of, as explained above.

I agree that monkey patching is not so important, for the reasons you state. However, I don't see a reasonable other way to present this object--allowing monkey-patching is just following the conventions of JavaScript in general, for better or worse.

@jridgewell
Copy link
Member

The PrivateName constructor is passed as the second argument to all decorators. You could use this with instanceof if you wanted to, but typeof works here too.

Hm. Looking into it, only to class decorators get passed it, element decorators get %PrivateNameGet% and %PrivateNameSet%. I figured that was an error from a previous draft, so didn't think anything was passed to the decorator call.

What's left out of this code sample is a mechanism for keying on the actual private name that you care about... I think this will end up being too difficult to make the "attack" practical.

It depends on how a particular private is exposed. If it only takes me creating an instance and calling a single method to key the private I want, then don't underestimate the lengths people will go through to do it. I know I've written some very stupid code before to get the context of calls and wrap state.

A decorator library which wants to defend against this "attack" can read out the original get and set methods earlier.

This is outright incorrect. If the library can't control when its started, it can't guarantee anything about the builtins. Extracting methods off the builtin is only marginally more secure, but is not anywhere near absolutely secure.

It's pretty trivial to patch the builtins before a require or import.

We described in #24 how this could be developed as a follow-on proposal. I don't see anything in this version of decorators that makes the follow-on proposal difficult.

Technically it can be a follow on, but that'd be pretty poor experience. If we can correct this in the initial version, and never expose a bad practice to begin with, that's the better option.

I'm not really sure how to function as a developer given these constraints... I think we should assume decorator authors are able to learn the decorator API.

We certainly can't 100% secure dynamic private names. But things like instanceof or '' + priv leaking the private name is unexpected for even intermediate coders. Hell, I consider myself to be advanced JS, and I wouldn't have spotted the instanceof leak.

Once you use that syntax, how would you access the name?

If we do private x, then just x.get. If it has to be private #x, then probably #x.get?

This is really hyperbole. There are the same privacy guarantees as any other method call, which you also expect to be encapsulated in a sense... allowing monkey-patching is just following the conventions of JavaScript in general, for better or worse.

I don't think so. We've never had a construct in JS that's meant to perfectly encapsulate, so I think exploring new ideas here is warranted. Otherwise dynamic private will always be the less secure version of lexical private.

@littledan
Copy link
Member Author

If we do private x, then just x.get. If it has to be private #x, then probably #x.get?

I don't understand what you are proposing here. Have you written it up anywhere?

@littledan
Copy link
Member Author

Hm. Looking into it, only to class decorators get passed it, element decorators get %PrivateNameGet% and %PrivateNameSet%. I figured that was an error from a previous draft, so didn't think anything was passed to the decorator call.

Oops, the error there was that this location should also pass in %PrivateName% instead.

@caridy
Copy link

caridy commented Jun 20, 2018

I agree with @jridgewell analysis as I have said it multiple times. I think we are rushing with the PrivateName constructor. I believe we can drop it for now, until we find the right solution, at the end of the day, we have multiple options today (WeakMap, symbols, _, etc) to offer storage mechanism per instance, and the only tricky part is the initialization, but I think we should offer a way to do on-demand initialization.

I don't understand why are we rushing this part! I always thought that we will be able to solve this one with the generalization of the private fields in the future. Can we drop this feature for now before the next meeting?

In the past, @littledan mentioned that since we have to pass the private name representation to the decorator when decorating a private field, it was ok to allow them to create new privates, but the more I think of it, the more I believe we can get away with giving them a get and set method for each private field, and that simplifies everything from the privacy point of view, and probably ergonomics as well.

@littledan
Copy link
Member Author

I still don't understand how we can make it work with get and set methods. I feel like I have not explained myself well about this in the past. Maybe we could have a call to discuss this particular issue?

@littledan
Copy link
Member Author

Metaprogramming based on PrivateName objects would be beneficial the first time around, and likely not subsumed by some kind of declaration form. If we want more integrity, we can do it without any real drastic changes to the current proposal, and without creating a frozen class.

Why PrivateName now and for the future

@caridy raised the question about whether we really need PrivateName. This breaks down into two parts:

Can we leave PrivateName for later?

It's hard for me to see how this would work. A few issues here:

  • Introducing PrivateName "later" poses a web compatibility risk
    • Class decorators get an elements Array of all of the things inside of the class. If we don't include PrivateName, it's not clear what would go in this Array. Would we just omit class elements that are private? If we do that, then existing decorators on existing classes will see an observable change when we later upgrade to supporting some kind of PrivateName.
  • PrivateName is pretty essential to a bunch of big use cases for decorators and private, e.g.,
    • For a decorator which uses underlying, non-exposed storage (e.g., @observed on a public field), a WeakMap isn't quite enough--there also has to be a way to install the object as a key in the WeakMap when an instance is created. The current decorator proposal only lets you insert fields, not code thunks, though it has been proposed.
    • PrivateName allows decorators to expose more broad access to private fields or methods, e.g., for testing, friendship (see friend.js).

Would PrivateName be subsumed by private name declarations?

It's hard for me to see how this would work either. The difficult comes from private #foo needing to be two declarations in one: A declaration of the mapping in the private name scope, to map #foo to the underlying name, and something which is somehow usable in ordinary lexical scope to refer to the name for metaprogramming purposes. Once the latter is available, most of the complexity of the PrivateName interface is still there somewhere, even if you don't have the constructor.

  • Either way, the API is exposed: PrivateNames aren't just used for their get and set methods--they are also used to create fields or methods on objects. For this sort of usage, there has to be some sort of value which represents the private name itself. A random object which has get and set properites wouldn't serve the purpose. An internal slot pointing to the underlying private name (which the get and set methods also access) would do--this is the core of the current PrivateName proposal, as well as the one at the bottom of this post.
  • If we go with private #foo;: To refer to the private name itself, maybe we could use the syntax #foo. That syntax is also under consideration for private element shorthand. I've been trying to avoid anything which would conflict with that possibility.
  • If we go with private foo;: To refer to the private name itself, the syntax foo could be used. Then, I guess, for usages, we'd use this.#foo. The syntax private foo breaks the idea that # is part of the name; I've been hoping that thinking of # as part of the name itself will make it easier to remember to include the #.

Overall, I think private name declarations could make sense as a complement to explicit PrivateName manipulation, but not as a replacement. In particular, I think it'd usually make sense to not make the PrivateName object available when such a declaration is made, and only instead make its private name binding available in the #-based scoping.

Alternative API shape for integrity

@jridgewell expressed continued concern about integrity issues related to PrivateName.

Skepticism

I've said that I'm not so convinced we need to do anything here, since

  • Without going and freezing much more than just PrivateName, there are very few integrity guarantees on just about anything you might do.
  • If you can create a new Realm, or if we have the get-originals proposal, you can go and get its PrivateName methods and call those, when you care about integrity a lot.

But my top concern is, I don't want to start making one-off "defensible" deep-frozen classes before we've thought about what that means in general and how it would apply to other things. I'm worried we'll make mistakes that are impossible to fix, and PrivateName would be a one-off wart, rather than the start of a clean pattern.

But, if we do it anyway:

Some of you have suggested we make an object with own methods in place of PrivateName. That'd meet the twin goals of being immune to leaks through monkey-patching as well as being something that fits roughly within current JS idioms. Here's what it'd look like:

Each privateName object is an ordinary object with the following:

  • An internal slot [[PrivateName]] which has the underlying Private Name value (i.e., what the scope maps to)
  • An own property get, which is the function (shared between instances) %PrivateNameGet%. This function expects a privateName object as its receiver and looks up its [[PrivateName]] internal slot, taking the object to read as an argument.
  • An own property set, which is the function (shared between instances) %PrivateNameSet%. Works analogously.
  • An own property description which is a String, e.g., something which is syntactically #x would have the description "x".
  • [[Prototype]] is null, which means that .toString() throws (since it's not a property key), +, etc. isn't monkey-patchable in a way shared between instances.
  • The privateName object is ordinary, mutable, etc, as are the methods.
  • Each time the system gives a privateName object to JS, it creates a new, fresh one, rather than reusing an old one.
  • There is no constructor property on the objects, and the privateName function passed into decorators is just a function which vends one of these objects.

I believe the above design would reach what Justin calls "Level 3" privacy.

Conclusion

It would be difficult and strange to delay reifying PrivateName once we have both decorators and private class features. PrivateName metaprogramming and declarations are inherently pretty different things. If we really want, we could make some tweaks to PrivateName objects to avoid certain monkey-patching scenarios.

littledan added a commit that referenced this issue Jul 10, 2018
This patch changes PrivateName objects to have a null prototype
and own methods, as described in

#68 (comment)

I'm not sure whether we want to do the change, but I'm writing it up
so that we can think more carefully about whether we want to go in
this direction. This patch is the most concrete, reasonable thing
I can imagine in the direction of more integrity. It's also a relatively
small change vs the previous proposal.

Some results of the change:
- It's not possible to effectively monkey-patch anything in particular
  to intercept private name accesses; you can only change one in
  particular which you already have access to.
- There may be slightly more memory consuption due to the privateName
  objects having more own properties, but not that much, as the methods
  remain not bound to the receiver.

Closes #68
@caridy
Copy link

caridy commented Jul 10, 2018

@littledan I think that covers a lot of ground, and solves must of my concerns. I still think that the factory function can be removed for now. Let's discuss that later today.

@littledan
Copy link
Member Author

@caridy Do you think that we'd remove the factory function, but still have PrivateName objects as I described above?

@caridy
Copy link

caridy commented Jul 10, 2018

@caridy Do you think that we'd remove the factory function, but still have PrivateName objects as I described above?

Yes! that seems like a good chunk to defer for the second iteration.

@rbuckton
Copy link
Collaborator

rbuckton commented Jul 10, 2018

If we were to add lexical private name declarations (i.e. private #foo;), we could support private names in decorators using an additional syntax like nameof #foo, which would return an opaque value used to define a private name (possibly just an object with a [PrivateName] internal slot):

private #foo;

export function addFoo(descriptor) {
  descriptor.extras = [{
    key: nameof #foo,
    placement: "static",
    ...
  }];
  descriptor.finisher = (ctor) => {
    // no .get or .set, just use it:
    ctor.#foo = 1;
  };
  return descriptor;
}

With this there's nothing to monkeypatch, nothing that can be mutated to violate security, and also aleviates my concern around leaking in #24.

Also, nameof could have other uses. I've often wanted it in other cases, like:

function foo() {
  console.log(nameof foo); // prints: 'foo'
}

@rbuckton
Copy link
Collaborator

Although, nameof is not strictly necessary if we opt to have #foo (outside of a property access) result in the opaque value, though that would step on private element shorthand.

@caridy
Copy link

caridy commented Jul 10, 2018

To be clear, I'm only suggesting that we remove the second argument of the decorator (the PrivateName constructor or factory method), in favor of just not providing that sugar at the moment. It is true that users can still create new private names at will using something similar to this:

function privateName() {
  let pn;
  function decorator(descriptor) { pn = descriptor.key };
  class Bogus { @decorator #p }
  return pn;
}

And that is actually fine, it is a very low level mechanism that correspond to the complexity associated to the meta programming aspect of the decorators. Eventually, we can work on the generalization proposal for private names, and decide if we want to provide other mechanism to facilitate the creation of such objects.

@littledan
Copy link
Member Author

@rbuckton Thanks for those suggestions--I hadn't thought of an operator to get the name before, and agree that this is a plausible extension.

@caridy Thanks for the clarification. I see your point--it doesn't sound so bad to remove the PrivateName constructor actually when you put it that way. Another reason that comes to mind is, when we have a decorator standard library, the PrivateName constructor could be part of that rather than a parameter to the decorator. Does anyone have hesitations about the change to remove the PrivateName constructor as the second parameter of decorators?

@nicolo-ribaudo
Copy link
Member

@caridy Even if the decorators doesn't get the PrivateName constructor as a parameter, it can still be modified using privateName().__proto__. Thus, not passing it doesn't solve the security problem.

@littledan
Copy link
Member Author

@nicolo-ribaudo I think the "security" concern is addressed by #124, and not passing the PrivateName constructor is a more of a measure to future-proof for better aesthetics/less redundancy.

@nicolo-ribaudo
Copy link
Member

Oh I thought that it was proposed as an alternative to that PR. I'm ok with removing that parameter for now then.

littledan added a commit that referenced this issue Jul 11, 2018
This patch changes PrivateName objects to have a null prototype
and own methods, as described in

#68 (comment)

I'm not sure whether we want to do the change, but I'm writing it up
so that we can think more carefully about whether we want to go in
this direction. This patch is the most concrete, reasonable thing
I can imagine in the direction of more integrity. It's also a relatively
small change vs the previous proposal.

Some results of the change:
- It's not possible to effectively monkey-patch anything in particular
  to intercept private name accesses; you can only change one in
  particular which you already have access to.
- There may be slightly more memory consuption due to the privateName
  objects having more own properties, but not that much, as the methods
  remain not bound to the receiver.

Closes #68
littledan added a commit that referenced this issue Jul 11, 2018
This patch changes PrivateName objects to have a null prototype
and own methods, as described in

#68 (comment)

I'm not sure whether we want to do the change, but I'm writing it up
so that we can think more carefully about whether we want to go in
this direction. This patch is the most concrete, reasonable thing
I can imagine in the direction of more integrity. It's also a relatively
small change vs the previous proposal.

Some results of the change:
- It's not possible to effectively monkey-patch anything in particular
  to intercept private name accesses; you can only change one in
  particular which you already have access to.
- There may be slightly more memory consuption due to the privateName
  objects having more own properties, but not that much, as the methods
  remain not bound to the receiver.

Closes #68
littledan added a commit that referenced this issue Jul 28, 2018
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 added a commit that referenced this issue Aug 3, 2018
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests