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

Editorial: introduce %ObjProto_valueOf% for usage by the Location exotic object #398

Closed
wants to merge 1 commit into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Feb 19, 2016

See whatwg/html#638 (comment) for
context.

@@ -3720,6 +3731,8 @@
</tbody>
</table>
</emu-table>
<!-- If ToObject is ever made hookable, we need to alert the editors of the HTML Standard as
that'll effect the security model of the Location exotic object. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

effect -> affect
'll -> will (more formal?)

Copy link
Member

Choose a reason for hiding this comment

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

I think this comment should be removed. I don't want to start documenting these sorts of dependencies in an ad-hoc way using comments. If it's important to do let's figure out a better way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's important. New hooks in ECMAScript have resulted in security issues in browsers at least once (@@toPrimitive and Location) and also tend to cause breakage due to "cross-origin objects" throwing on unknown property access (the way to fix this is by adding e.g., @@toPrimitive with value undefined on WindowProxy).

Happy to do whatever you think is best here.

I'll fix the issues @UltCombo pointed out once we figure out a plan.

Copy link
Member

Choose a reason for hiding this comment

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

What does "hookable" mean in this context? Like if I can swap it out with my own function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the @@toPrimitive example I mentioned. But also @@toStringTag, @@hasInstance, and @@isConcatSpreadable require special behavior for cross-origin objects. Basically any change where before we just followed an internal code path and now we need to call into userland.

Copy link
Member

Choose a reason for hiding this comment

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

Who is the intended audience for such a note? ECMAScript language designers or implementors?

Copy link
Member

Choose a reason for hiding this comment

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

I still don't think a comment is the right choice here - if it's to be included, it should be an actual informative note. But I think the should be in HTML, not in ECMA262. Essentially any change to ECMA262 that adds observable behavior has a chance of violating the constraints (security or otherwise) of an ECMAScript embedding. I don't think this one special case is worth documenting in ECMA262 unless there is something particularly onerous here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that the ECMAScript engine is updated but there is no notification going downstream. And then suddenly downstream has a security bug because they didn't pay attention to something they don't normally have to pay attention to.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zenparsing both.

Copy link
Member

Choose a reason for hiding this comment

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

I think adding a NOTE instead of a comment seems reasonable. I don't agree with the logic that "changing anything in ES could break the web security model, so we shouldn't bother documenting anything." We have an explicit ask from an implementer (see linked thread) for such a note to exist in the spec. Documenting such security-impactful pitfalls seems important.

@annevk
Copy link
Member Author

annevk commented Feb 19, 2016

It was suggested to me that the warning should maybe also exist in the valueOf section given the dependency. (Again, see context for this comment.) WDYT?

@anba
Copy link
Contributor

anba commented Feb 22, 2016

If this gets merged, it also needs some kind of note/comment/justification why %ObjProto_valueOf% was added as an intrinsic, because currently the list of intrinsics is limited to the ones actually used in the ECMAScript spec.

For example ECMA-402 does not require that %StringProto_includes% and %ArrayProto_indexOf% (https://tc39.github.io/ecma402/#sec-well-known-intrinsic-objects) are defined as intrinsics in ECMA-262, instead it defines both functions as intrinsics for the intl-spec.

@annevk
Copy link
Member Author

annevk commented Feb 23, 2016

@anba this seems similar to adding the Ordinary* abstract operations to me. ECMA-402 should perhaps upstream theirs too.

There could perhaps be a section that lists the accommodations made for downstream, such as [[GlobalThisValue]].

Anyway, I'd appreciate some kind of definitive answer or clear indication of that this is going to take longer, since this PR is currently blocking security work in HTML that really should land since it's a huge improvement over what we have today.

@allenwb
Copy link
Member

allenwb commented Feb 23, 2016

This sort of note to future editors does not belong in an ECMASpec specification document. If TC39 needs to create a community memory it can do it with an working document or an issue that is never closed.

But I don't really think it is TC39's responsibility to keep track of these sorts of concerns for all past, present, and future important host platforms. In general TC39 spec. writers have no way to know about all such dependencies or when they might change on the host side. It seems more appropriate for the browser spec. writers to create and maintain a document that lists all their assumptions about ECMASCript and any essential invariants that they depend upon. Produce such a document and point TC39 at it and it will undoutably be considered as we evaluate proposed changes to ECMA-262.

@zenparsing
Copy link
Member

@allenwb

This sort of note to future editors does not belong in an ECMASpec specification document.

If you're referring to the note about ToObject, I completely agree.

Currently we manage such downstream risks (web compatibility, security, etc.) with "community memory". It might be an improvement to create some informal document which keeps track of known downstream risks.

@annevk
Copy link
Member Author

annevk commented Feb 23, 2016

Sounds good. I'll remove the note then.

@bterlson
Copy link
Member

@anba I'm sympathetic to that idea but I'm not sure I want to open the door to tabulating all inbound references. Git history will show when/why it was added, perhaps that's good enough for now? Otherwise let's think more about solving this in a general case rather than adding a one-off comment.

@bterlson
Copy link
Member

Committed as 28c5237.

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

Successfully merging this pull request may close these issues.

7 participants