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

Discoverability of wrapper prototypes #173

Closed
mhofman opened this issue Jan 30, 2022 · 16 comments · Fixed by #235
Closed

Discoverability of wrapper prototypes #173

mhofman opened this issue Jan 30, 2022 · 16 comments · Fixed by #235

Comments

@mhofman
Copy link
Member

mhofman commented Jan 30, 2022

#172 made me realize that this proposal introduces 2 new hidden intrinsics for the wrapper prototypes, aka that are only discoverable by calling the Iterator.from and AsyncIterator.from methods with well formed iterators.

Since the spec still doesn't provide a unified way to discover user exposed intrinsics (see @ljharb get intrinsic proposal), we'd like to make sure these are discoverable by walking the globals (combination of getOwnPropertyDescriptors and getPrototypeOf)

@ljharb
Copy link
Member

ljharb commented Jan 30, 2022

Iterator.prototype already exists, it’s not newly introduced. I believe the only new one is the AsyncIterator prototype.

I don’t think it makes sense to need to expose the sync one on a global, and i don’t think it makes sense for the two not to be consistently on or not on the global.

@mhofman
Copy link
Member Author

mhofman commented Jan 30, 2022

No I'm talking about %WrapForValidIteratorPrototype% and %WrapForValidAsyncIteratorPrototype%, which are the protos of objects returned by respectively Iterator.from and AsyncIterator.from. These are new hidden intrinsics, and we'd like not to see any more of those.

@ljharb
Copy link
Member

ljharb commented Jan 30, 2022

Isn’t that Iterator.prototype and AsyncIterator.prototype, respectively?

@mhofman
Copy link
Member Author

mhofman commented Jan 30, 2022

Not from my reading. These are new prototype intrinsics that have as [[Prototype]] the Iterator.prototype and AsyncIterator.prototype objects.

@ljharb
Copy link
Member

ljharb commented Jan 30, 2022

and where would these go? All builtin iterator prototype objects (except the newly exposed ones on Iterator/AsyncIterator) are “hidden” - matchAll’s was the most recently added.

I think that if we want a convention of exposing these, they should all be done at once, setting a precedent for future ones as well, rather than trying to draw a line in the sand of “old” hidden ones and “new” exposed ones.

(note that the get intrinsic proposal explicitly is not solving discoverability of intrinsics)

@erights
Copy link

erights commented Jul 21, 2022

@michaelficarra you asked me to add an issue to the proposal to capture my objection just now in the tc39 plenary. Does this issue cover it? It seems so to me. What else should we capture?

@michaelficarra
Copy link
Member

@erights I asked you to capture your opinion on this issue. In particular, I'm not interested in making these new intrinsics reachable in this way, so I'd like to figure out how we can not block this proposal on the addition of something like getIntrinsics given that it's at an earlier stage. I feel like we shouldn't make requirements like these that add a dependency from a later-stage proposal to an earlier-stage one.

@erights
Copy link

erights commented Jul 21, 2022

I do consider it a requirement to, as we say, "not break the web". Introducing new intrinsics that are not discoverable by old code, and that thereby makes currently secure sites insecure, is a breaking change that should block this proposal, until this proposal is fixed.

@zloirock
Copy link
Contributor

@erights could you show examples of code that the current proposal can break?

@zloirock
Copy link
Contributor

@erights ?

@zloirock
Copy link
Contributor

About the current issue - I agree with @ljharb. Here is no difference, for example, with %ArrayIteratorPrototype% that we can get as Object.getPrototypeOf([].values()) or any other iterator prototypes.

We can get %WrapForValidIteratorPrototype% as Object.getPrototypeOf(Iterator.from({ next() { } })), %IteratorHelperPrototype% as Object.getPrototypeOf([].values().map(() => {})), etc.

@erights
Copy link

erights commented Sep 26, 2022

@erights could you show examples of code that the current proposal can break?

Yes. If this PR were implemented and deployed, but Hardened JS https://github.com/endojs/endo/blob/master/packages/ses was not changed to incorporate the code that @michaelficarra posted at https://github.com/tc39/proposal-iterator-helpers/pull/235/files , then Hardened JS would be broken. These hidden primordials would still be unfrozen after lockdown(). Code running in two separate compartments that should be isolated from each other could use these shared unfrozen intrinsics to communicate info and direct references to objects (capabilities) to each other.

@zloirock
Copy link
Contributor

@erights it looks like an issue of EndoJS. Such moments should have been foreseen by its architecture since they are common for many new JS essences. Because of such moments, I'm skeptical about frozen JS environments.

@mhofman
Copy link
Member Author

mhofman commented Sep 26, 2022

To be clear, new hidden intrinsics (defined as not discoverable through a simple property / prototype walk from the global) are not a problem for such frozen environments and libraries implementing them, as long as these hidden intrinsics are only ever reachable through new APIs, which should be automatically denied by these libraries (and are by EndoJS / SES-shim). The real problem is when exposing new hidden intrinsics through existing APIs or syntax.

Edit: I realize this is contradicting my original issue, but this is to clarify what I believe is the conclusion of the discussion and compromise reached at the last plenary.

@erights
Copy link

erights commented Sep 26, 2022

I'm skeptical about frozen JS environments.

And please remain skeptical. In fact, please try to find things we may have overlooked, like this or otherwise, that may compromise our security. I try my best to remain skeptical too. But if you do find something, please let us know.

@zloirock
Copy link
Contributor

@erights sure -)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment