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

Add HostEnsureCanAddPrivateElement definition. #8198

Merged
merged 6 commits into from Aug 22, 2022

Conversation

mgaudet
Copy link
Contributor

@mgaudet mgaudet commented Aug 16, 2022

Disallow the addition of private fields to Location and WindowProxy object

Fixes #7952

(See WHATWG Working Mode: Changes for more details.)


/acknowledgements.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/webappapis.html ( diff )

Disallow the addition of private fields to Location and WindowProxy object

Fixes whatwg#7952
@mgaudet
Copy link
Contributor Author

mgaudet commented Aug 16, 2022

(There is a test which will need to be updated and moved to WPT here https://phabricator.services.mozilla.com/D141877 )

@mgaudet
Copy link
Contributor Author

mgaudet commented Aug 16, 2022

A note: I will be heading on parental leave at any time now; if need be please move on with this without me, and I will return to this work come my return.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Thank you! Since you mentioned your leave, I pushed a quick fixup commit myself instead of doing a review with my nits. Normatively everything looks great.

The only blocker here then is making sure we have tests in the web platform tests repo. A particularly tricky case I noticed is if you make classes that inherit from WindowProxy or Location; let's be sure that is tested too.

Other optional additions:

  • Would you like to add yourself to the acknowledgments section?
  • We could explain the motivation for this in a <p class="note"> after the algorithm, to help future readers.

abstract operation. User agents must use the following implementation: <ref spec=JAVASCRIPT>

<ol>
<li><p>If <var>O</var> is a <code>WindowProxy</code> object, or <span>implements</span>
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little unsure on the most rigorous way to phrase this for WindowProxy. "implements" doesn't work since it generally applies to platform objects; Window is a platform object, but WindowProxy is not. Maybe we should say "has a [[Window]] internal slot"?

I also was thinking maybe we should check Window objects, but upon consideraiton WindowProxy seems right. This algorithm will only ever get called with WindowProxy, right? Nobody is going to be able to get ahold of a Window except spec text, and the spec text in html.spec.whatwg.org/#the-windowproxy-exotic-object will never hand the WindowProxy to someone who might trigger this, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I waffled a bit on that front myself; I came to a similar conclusion. My understanding is that no user code should ever see a Window object, and so specifying this hook in terms of the WindowProxy made sense to me. Editorially I wasn't sure of the best way to write it; thanks for your review.

@mgaudet
Copy link
Contributor Author

mgaudet commented Aug 17, 2022

Thank you! Since you mentioned your leave, I pushed a quick fixup commit myself instead of doing a review with my nits. Normatively everything looks great.

Thank you very much!

The only blocker here then is making sure we have tests in the web platform tests repo. A particularly tricky case I noticed is if you make classes that inherit from WindowProxy or Location; let's be sure that is tested too.

I'll try to resuscitate my test case tomorrow, and add that extra dimension as well.

Other optional additions:

* Would you like to add yourself to the acknowledgments section?

* We could explain the motivation for this in a `<p class="note">` after the algorithm, to help future readers.

I'll work on this if I can tomorrow.

Thanks again!

@mgaudet
Copy link
Contributor Author

mgaudet commented Aug 17, 2022

A particularly tricky case I noticed is if you make classes that inherit from WindowProxy or Location; let's be sure that is tested too.

I was playing around with this: is it actually possible to construct a class which extends from WindowProxy/Location at all?

  • window / window.location aren't constructors directly.
  • Their prototypes have a .constructor property, however, it seems that we cannot actually use said constructor; new (class extends window.location.__proto__.constructor {}) / new (class extends window.__proto__.constructor {}) throws an Illegal Constructor error.

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 18, 2022
…tion

This tests the HostEnsureCanAddPrivateElement host hook, to be added as part of
this pull request: whatwg/html#8198

Differential Revision: https://phabricator.services.mozilla.com/D141877

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1785309
gecko-commit: fb112dce4448cab35b0113ee28abe382128ee451
gecko-reviewers: arai
@mgaudet
Copy link
Contributor Author

mgaudet commented Aug 18, 2022

The WPT test case for this hook is inbound; see https://bugzilla.mozilla.org/show_bug.cgi?id=1785309 and web-platform-tests/wpt#35520

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 19, 2022
…st-hook definition r=arai

This tests the HostEnsureCanAddPrivateElement host hook, to be added as part of
this pull request: whatwg/html#8198

Differential Revision: https://phabricator.services.mozilla.com/D141877
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 19, 2022
…tion

This tests the HostEnsureCanAddPrivateElement host hook, to be added as part of
this pull request: whatwg/html#8198

Differential Revision: https://phabricator.services.mozilla.com/D141877

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1785309
gecko-commit: fb112dce4448cab35b0113ee28abe382128ee451
gecko-reviewers: arai
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Access to private class members in the windows object is not checked
2 participants