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

Normative: allow host exotic objects to reject private fields #2807

Merged
merged 2 commits into from
Aug 12, 2022

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Jun 26, 2022

It is possible to add private fields to existing objects with the return override trick, although this is not good practice. This PR adds a new carveout allowing hosts such as HTML to reject the addition of private fields on exotic objects they define (by throwing). This is intended particularly for use with the WindowProxy exotic object, which is super weird. See discussion in whatwg/html#7952 - the upshot is that supporting private fields on WindowProxy is a lot of potentially security-sensitive work in implementations for very little benefit.

It's conceivable that we would want to expose this ability to userland code at some point, or to non-browser hosts, but that can be done transparently on top of this if we ever decide to. This PR only exposes this functionality to web browser hosts, and only for host-defined exotic objects.

@bakkot bakkot added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. labels Jun 26, 2022
@mgaudet
Copy link

mgaudet commented Jun 27, 2022

Strong Support from Gecko/SpiderMonkey here.

At least in Gecko, WindowProxy is a particularly problematic object to apply a private field to:

  • window appears to be a single object with identity; but this is an illusion maintained, with varying degrees of pain, by the engine.
  • As a cross-origin objects, Window, WindowProxy and Location objects need to go through some fairly gross transmutation on navigation.
  • While, in principle, a WeakMap based implementation of private fields wouldn't be subject to these challenges, in practice, at least some engines have chosen implementation strategies that look more like fields for performance; and WindowProxy hasn't historically supported arbitrary properties being added to it.

@mgaudet
Copy link

mgaudet commented Jun 27, 2022

(There's more complexity here around processes too; this is a link to a discussion last time I tried to sort this out )

@ljharb
Copy link
Member

ljharb commented Jun 27, 2022

@mgaudet is "adding arbitrary properties" something different than window.a = 'b'?

@bathos
Copy link
Contributor

bathos commented Jun 28, 2022

oof. this will break so much for me. (i honestly feel sick right now. years of work.)

@bakkot
Copy link
Contributor Author

bakkot commented Jun 28, 2022

@bathos You... have lots of code which relies on sticking private fields on WindowProxy? Why?

@devsnek
Copy link
Member

devsnek commented Jun 28, 2022

@ljharb windowproxy is not the same as window. window.a = 'b' sets a property on window, via the windowproxy. return override would allow putting the private field directly on the windowproxy.

@Jack-Works
Copy link
Member

Can we add it in Annex.B? I don't like the idea of rejecting private fields on some object while you cannot do it in the user land, it brings inconsistency into the language. I agree there is some complexity in WindowProxy but it'll be better to limit it on the Web.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 28, 2022

Given that it's a host hook, it's already optional even if it's not in Annex B: no host is enforced to make that hook reject private properties. Also, there is some desire of moving away from Annex B: #1595

@bathos
Copy link
Contributor

bathos commented Jun 28, 2022

@bathos You... have lots of code which relies on sticking private fields on WindowProxy? Why?

on arbitrary objects among which WindowProxy objects and other host exotics are guaranteed to appear. I don’t intend to make a case for preserving capabilities that nobody but me was making use of again (e.g. extends null) tho — just pour one out for my alien code and I’ll be happy.

@mgaudet
Copy link

mgaudet commented Jun 28, 2022

Sadly, right now those host exotics already behave differently between different browsers, particularly in the face of navigation and origin checks (see this comment), and in some engines have a form of data-loss.

This PR would allow HTML to cause these objects to always throw, standardizing behaviour and avoiding data loss.

@phoddie
Copy link

phoddie commented Jun 28, 2022

This change does appear to be very focused on the needs of the browser. If there isn't a clear need for this change outside the browser, I agree with @Jack-Works that this feels most appropriate for Annex B to start.

@bakkot
Copy link
Contributor Author

bakkot commented Jun 28, 2022

I would really like to avoid putting new things into Annex B (we actually had plenary consensus on that, at one point). Having both the original algorithm and a diff against that algorithm in the same document really tends to trip people up, for very little benefit. Hosts which don't want to do anything special with this host hook can just use the default behavior, I would think.

If the concern is just about whether people on non-browser platforms can rely on the return override trick always working, I'd be open to adding a normative note saying this should be restricted to browsers, I guess.

@phoddie
Copy link

phoddie commented Jun 28, 2022

@bakkot, fair point. I do recall the Annex B plenary discussion. If I understand this particular change correctly, it is motivated by one specific host exotic object that is only present in the browser, not host exotic objects in general. Or is there a use contemplated beyond WindowProxy?

@bakkot
Copy link
Contributor Author

bakkot commented Jun 28, 2022

If I understand this particular change correctly, it is motivated by one specific host exotic object that is only present in the browser, not host exotic objects in general.

That's right. Though it wouldn't surprise me to learn of another host or implementation with a similarly weird built-in where they'd want to reject private fields.

@phoddie
Copy link

phoddie commented Jun 28, 2022

If it remains just for one specific host exotic object for the web, I would prefer using Annex B.

Though it wouldn't surprise me to learn of another host or implementation with a similarly weird built-in where they'd want to reject private fields.

Agreed. That would be interesting. FWIW, I'm not aware of any in the Moddable SDK, which is both an unusual host and engine implementation.

@waldemarhorwat
Copy link

If this is a browser-specific thing not needed in the language in general, it should be in Annex B.

@bakkot
Copy link
Contributor Author

bakkot commented Jun 28, 2022

It's not that this is a browser-specific facility; it's that browsers are the only host we are currently aware of which need this facility. But there's nothing special about this facility which makes it only relevant to browsers.

@ljharb
Copy link
Member

ljharb commented Jun 28, 2022

It seems prudent to restrict it to browsers until we learn of a non-browser use case; we can always open it up later.

@bakkot
Copy link
Contributor Author

bakkot commented Jun 28, 2022

I am fine with restricting this to browsers if we'd prefer, but I would still strongly prefer to do that by a normative comment on the host hook, rather than by putting this in Annex B. I would very much like to avoid ever putting anything new in Annex B; it's been a source of a great deal of pain.

@ljharb
Copy link
Member

ljharb commented Jun 28, 2022

Totally agree.

@Jack-Works
Copy link
Member

I would very much like to avoid ever putting anything new in Annex B; it's been a source of a great deal of pain.

The pain comes from the content of Annex B, not from where you put it, I want to limit it normatively for Web-only, so Annex B looks a good place.

@bakkot
Copy link
Contributor Author

bakkot commented Jun 29, 2022

The pain comes from the content of Annex B, not from where you put it

No, it comes from the organization of the document. Having a separate annex that has the actual algorithms, as a diff against the main specification, is extremely annoying. I have several times implemented something based off the main spec and then had to go back and fix it when I remembered the main spec is a lie.

@Jack-Works
Copy link
Member

adding a normative note saying this should be restricted to browsers

Then let's do this

@nicolo-ribaudo
Copy link
Member

Are there any other behaviours that non-browsers are disallowed to implement?

@bakkot
Copy link
Contributor Author

bakkot commented Jul 4, 2022

Yes: MakeJobCallback has exactly the same note as I've just added in the last commit, and [[IsHTMLDDA]] says

the document.all object in web browsers is a host-defined exotic object with this slot that exists for web compatibility purposes. There are no other known examples of this type of object and implementations should not create any with the exception of document.all.

@mhofman
Copy link
Member

mhofman commented Jul 10, 2022

I had missed the fact the discussion had moved to a dedicated issue here. As I stated before, we have a use case that requires virtualizing WeakMap, which is currently impossible in the face of the return override + private fields, who together ended up effectively elevating WeakMap from an API to an undeniable syntax based power.

I would prefer expressing this as a slot on objects which cannot be used in this fashion. Basically replace the call to HostEnsureCanAddPrivateElement in this PR with some TBD slot presence check, and a note clarifying that user code should not rely on adding private fields to objects they didn't create.

In the future I want to propose an options bag to Object.create() which sets this slot, so that a program can create objects which have this behavior (but cannot confer this behavior to objects it doesn't create).

@kriskowal
Copy link
Member

We should not create a normative exception specifically for the web. We should not preclude a host from emulating any other host, particularly non-web hosts emulating web hosts.

@ljharb
Copy link
Member

ljharb commented Jul 14, 2022

@kriskowal i'm not sure why it obviates it? a non-web host emulating the web has the HTML spec to comply with. I'm interested in preventing non-web hosts that are NOT constrained by HTML from having this exotic behavior.

If we eventually added such a host hook, then we would obviously remove the restriction at that time. We can always loosen restrictions later; we can rarely tighten them later.

@caridy
Copy link
Contributor

caridy commented Jul 21, 2022

A proposed solution here to support virtualization of the window and location is to introduce a new API similar to Object.seal(o), and its corresponding predicate Object.isSealed(o). I haven't think about the name yet. This API will simply brand the object to prevent any attempt to add private fields. It is not clear to me whether or not this will cause the process of adding new private fields to throw, or not, I suspect it will throw.

It is also not clear to me what will happen if we do this. If Bar extends Foo, and Foo use the API in the constructor to prevent subclasses to add private fields, and what if some frameworks and libraries deliberately do this on their base-class so their objects can be wrap with proxies, which are known to fail in the presence of private fields.

@bathos
Copy link
Contributor

bathos commented Jul 21, 2022

@caridy If this were to happen without a corresponding fourteenth object internal method and corresponding reflection and trap support, we wouldn’t just be losing syntactic weak association, we’d be losing a sound meta object protocol altogether. While I am sad to hear the intention is to pursue these web-breaking changes, I hope that if it occurs the integrity of the protocol will be accounted for.

If it’s unclear why: the two existing spots where the “protocol” gets mangled — IsArray and GetFunctionRealm + revoked PEOs — are only nuisances because they cannot be exploited to effect state changes to/through the target that the target cannot mediate. Because this hypothetical new operation would effect a state change to the target, if it cannot be mediated / denied by it like every other such state change, it’d create a real rupture in the “message boundary”. That’s probably not the right term for this concept — unfortunately I don’t know the formal CS terminology for this stuff, apologies if I’m bungling it. I’d guess @allenwb would know how to describe what I mean much more clearly. Whatever it’s properly called, currently it’s still sound (and is a thing of real beauty imo!)

(Also if unclear: private fields themselves didn’t similarly rupture it because their state “belongs to” the holder of the field, not the objects. Syntactic inversion of key (the object) and map (the field) is very convenient, but the effective information model (prob the wrong term again ... visibility model?) is fields having objects, not objects having fields.)

@caridy
Copy link
Contributor

caridy commented Jul 21, 2022

@bathos I didn't quite understand all that, can you elaborate more? maybe with some examples?

@mhofman
Copy link
Member

mhofman commented Jul 26, 2022

@caridy, I also initially considered a way to set this flag like sealing / preventing extensions, however I believe the use cases do not support such a strong power, and I'm also concerned about allowing any piece of code with a reference to the object from "flipping the switch". That is why in my proposal, this power is reserved to the object creator, through an Object.create options bag. That doesn't solve your proxy use case, but we could similarly add an options to the proxy object constructor. I could be convinced to drop the Object.create option in favor of only a proxy option and reserve this behavior to exotic objects. Regardless, I really don't see a reason for this behavior to be allowed to change over the lifetime of an object, which would bring in the complexity of meta object protocols.

@syg
Copy link
Contributor

syg commented Jul 26, 2022

The web use case certainly does not justify adding a new MOP "seal" operation, and I share @mhofman's concerns in #2807 (comment).

We should not preclude a host from emulating any other host, particularly non-web hosts emulating web hosts.

@kriskowal It is not my understanding that this is a design goal that shares consensus. I do not share it.

That said in this case I find neither of the "restrict it to browsers" or "let any host do this" argument compelling. A special internal slot to the level of specificity of [[IsHTMLDDA]] is gross, but also WFM. A more generally named internal slot is "fine" but I don't want to suggest I'm in favor of adding a new MOP operation later.

@bakkot bakkot force-pushed the no-window-private branch 2 times, most recently from e6f7c0c to 039d735 Compare August 2, 2022 21:32
@bakkot
Copy link
Contributor Author

bakkot commented Aug 2, 2022

I've update this PR so that the guard for browser-based hosts is now inline at the call sites, rather than in a paragraph in the hook. I've also added a note to Annex B for this and the other browser-specific host hook (in a separate commit). The hooks themselves remain outside of Annex B.

@tc39/ecma262-editors this is ready for review/merging.

@bathos
Copy link
Contributor

bathos commented Aug 3, 2022

@caridy I think @mhofman likely conveyed it there better than I could have. The creation-time-only approach he suggested would keep it squarely in the “weird but sound” zone of IsHTMLDDA & %eval% where an eternal characteristic of an object directs syntactic effects without implying anything new about internal method contracts as far as I can tell.

@bakkot bakkot added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Aug 3, 2022
@caridy
Copy link
Contributor

caridy commented Aug 3, 2022

@caridy I think @mhofman likely conveyed it there better than I could have. The creation-time-only approach he suggested would keep it squarely in the “weird but sound” zone of IsHTMLDDA & %eval% where an eternal characteristic of an object directs syntactic effects without implying anything new about internal method contracts as far as I can tell.

I withdraw my concern in favor of a creation-time-only decoration mechanism.

@@ -6817,6 +6817,8 @@ <h1>
<dl class="header">
</dl>
<emu-alg>
1. If the host is a web browser, then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first time that I see something like this in an algo step, I have mix feelings about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phoddie requested that this be Annex B only, so that non-browser hosts wouldn't have to read it at all. But I really do not want to put any more things in Annex B. So this was the compromise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do think it seems weird, and I think it's far more appropriate to have the check inside the host hook, this is "fine" imo - and I believe this represents pushback from @phoddie (who may want to clarify)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must agreed that having this in the algos rather than in a separate sections that nobody looks at is a lot better, I was just hesitant because this is the first time I have seen this condition in the spec.

@Jack-Works
Copy link
Member

can we also limit the object to WindowProxy and no more else?

@bakkot
Copy link
Contributor Author

bakkot commented Aug 4, 2022

It may also be needed for location; the HTML spec will have to figure that out and specify the implementation of this hook on their side. I don't think we need to try to impose restrictions on how they do that. This is a specification, not a contract.

@bakkot bakkot added the editor call to be discussed in the next editor call label Aug 9, 2022
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm editorially

@bakkot bakkot added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.