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

Strict mode assignment on frozen objects #11

Open
dead-claudia opened this issue Jan 6, 2019 · 8 comments
Open

Strict mode assignment on frozen objects #11

dead-claudia opened this issue Jan 6, 2019 · 8 comments

Comments

@dead-claudia
Copy link

This was first brought up starting approximately here.

Currently, the proposal requires this:

var sym1 = Symbol.private("sym1")
var sym2 = Symbol.private("sym2")
var object = {[sym1]: 1}

Object.freeze(object)
try {
	object[sym1] = 2 // succeeds
	object[sym2] = 3 // throws
} finally {
	console.log(object[sym1]) // logs 2
	console.log(object[sym2]) // logs `undefined`
}

This is thanks to Object.freeze not including the symbol in its list of keys. However, assignment of private symbol keys still requires IsExtensible(object) to return true, or else it throws.

I see three routes here we could take:

  • We freeze private symbols on the object, too, grabbing them through the proxy target as necessary. This way, it's consistent across the board, and engines can do this relatively cheaply already. This papers over much of the inconsistency with private symbols, making it much more like normal property descriptors in practice. This is probably the least invasive viable change, and it only affects a few operations.
    • This would result in both assignments throwing, not just the second.
  • We just tell everyone that you can't add new private symbols on frozen objects, but you can still change existing ones, unlike with normal properties. (Or in other words, it works like sealing an object.) This is the easiest way out and is what the proposal does now, but IMHO it's also the worst of the three options.
    • This would result in the second assignment throwing, but not the first.
  • We skip the IsExtensible check in the appropriate places, so new private properties can still be added to frozen objects. This gives private symbols weak map-like semantics at the cost of inconsistency with the existing property semantics.
    • This would result in both assignments succeeding, not just the first.

I like the first option the best as it's the most property-like and diverges from "public" symbols the least, but the third still is compelling for other usability reasons. Note that in the case of the first, you could have issues if you call a method on a frozen object that tries to modify a private property, but those hazards already exist with public properties. The appropriate workaround would just be to set the private property to an object containing the appropriate data if you want to support the current instance being frozen.

/cc @erights (who may have an opinion here)

@erights
Copy link

erights commented Jan 6, 2019

I do. In the long thread at tc39/proposal-class-fields#183 (comment) I finally noticed that all of the private symbol proposals still suffer from fatal confused deputy attacks. I don't think this direction can work at all.

@bathos-wistia
Copy link

bathos-wistia commented Jan 6, 2019

@erights Is this to say that any solution that doesn’t strictly prevent allocating private fields except by way of invoking an associated constructor is off the table?

Is there a possible future for non-constructor-based private fields if they were a distinct feature from constructor-based private fields, rather than being an alternative proposal?

(It seems like one group wants a feature that does x and y, and another wants a feature that does x and z, where y and z are fundamentally incompatible concepts. But I believe an x+y feature and an x+z feature could nonetheless coexist.)

@erights
Copy link

erights commented Jan 6, 2019

I'm not sure I understand your question. Is https://github.com/erights/PLNSOWNSF a constructor-based approach?

Note that I just now wrote this repository in a hurry and it is still rough ;)

@bathos-wistia
Copy link

bathos-wistia commented Jan 6, 2019

By constructor-based, I meant something like “fields which cannot be declared except through class syntax, and which cannot be allocated ‘on’ an object except by [[Construct]]ing that class’s constructor”.

PLNSOWNSF is not an example of that. It appears to realize both the y (footgun-free private class-based fields) and z features (freeeeedom) while having them share a common x primitive (private field). Seems like everybody’s needs get covered! 👍

@bathos-wistia
Copy link

BTW, "PrivateName-like special..." seems to start with PNLS, not PLNS :)

@erights
Copy link

erights commented Jan 7, 2019

@bathos-wistia Done. Thanks!

@dead-claudia
Copy link
Author

@erights The "confused deputy" vulnerability is really not seen as a problem in this proposal, since by design it doesn't care what type the object is. It's more a fundamental difference in viewpoint, and if you really care about the type of an object and want to block confused-deputy attacks, you can check for the presence of a private symbol on an object much like how people commonly do if (this instanceof Foo) throw new TypeError("`this` must be a `Foo`!") now. It's just not the default behavior.

I feel your criticism here is off-topic in this issue, though: I was specifically referring to an unusual edge case with symbols that didn't align with properties and was rather counter-intuitive. Yours is really a criticism of the proposal itself, which is much more meta than this.

@erights
Copy link

erights commented Jan 9, 2019

If you want to classify it as a criticism of the proposal, that's fine with me. Either way, it remains a severe problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants