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

Handling of "then" on cross-origin windows doesn't match what we agreed to do or browsers #4251

Closed
bzbarsky opened this issue Dec 21, 2018 · 4 comments
Labels
security/privacy There are security or privacy implications topic: multiple globals

Comments

@bzbarsky
Copy link
Contributor

Consider this code, where "win" is a cross-origin window that has a subframe named "then":

Object.getOwnPropertyDescriptor(win, "then")

What should this return?

Per spec as currently written, we land in https://html.spec.whatwg.org/multipage/window-object.html#windowproxy-getownproperty and:

  1. Just sets W
  2. P == "then" is not an array index, step skipped.
  3. IsPlatformObjectSameOrigin is false.
  4. CrossOriginGetPropertyHelper returns a descriptor with [[Value]]: undefined
  5. This property descriptor is returned.

But what browsers implement, and wpt tests for, and we agreed on, iirc, is that in this case the property descriptor's [[Value]] should be the subframe window.

@annevk @domenic

@bzbarsky
Copy link
Contributor Author

Looks like this was wrong in the original PR for #3242

@annevk annevk added good first issue Ideal for someone new to a WHATWG standard or software project topic: multiple globals labels Dec 23, 2018
@annevk
Copy link
Member

annevk commented Dec 23, 2018

So to fix this, we need to remove the "then" case from step 3 of CrossOriginGetOwnPropertyHelper. Then we need to add a "then" case near the end of WindowProxy's and Location's [[GetOwnProperty]]. Alternatively we could move that entire step (step 3 of CrossOriginGetOwnPropertyHelper) to end of these two algorithms.

It might be worthwhile to look into a shorthand for

PropertyDescriptor{ [[Value]]: undefined, [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true }

too to avoid the duplication of that, but perhaps only if there's more uses of it throughout the platform.

This seems like a good first issue for someone to try and tackle.

@bzbarsky
Copy link
Contributor Author

In the Gecko implementation, I moved all of step 3 into a separate algorithm that is invoked from both ends of [[GetOwnProperty]], for the moment...

annevk added a commit that referenced this issue Jan 3, 2019
This isn't strictly needed for @@toStringTag, @@hasInstance, and @@isConcatSpreadable, but "then" can be overridden and therefore cannot be handled earlier on. Handling them all at the same time leads to cleaner code.

Tests: already tested by wpt.

Fixes #4251.
@annevk annevk removed the good first issue Ideal for someone new to a WHATWG standard or software project label Jan 3, 2019
@annevk
Copy link
Member

annevk commented Jan 3, 2019

I ended up posting a PR for this myself, despite "good first issue", as it concerns security, so removing that label.

@annevk annevk added the security/privacy There are security or privacy implications label Jan 3, 2019
annevk added a commit that referenced this issue Jan 7, 2019
This isn't strictly needed for @@toStringTag, @@hasInstance, and @@isConcatSpreadable, but "then" can be overridden and therefore cannot be handled earlier on. Handling them all at the same time leads to cleaner code.

Tests: already tested by wpt.

Fixes #4251.
alice pushed a commit to alice/html that referenced this issue Jan 7, 2019
This isn't strictly needed for @@toStringTag, @@hasInstance, and @@isConcatSpreadable, but "then" can be overridden and therefore cannot be handled earlier on. Handling them all at the same time leads to cleaner code.

Tests: already tested by wpt.

Fixes whatwg#4251.
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
This isn't strictly needed for @@toStringTag, @@hasInstance, and @@isConcatSpreadable, but "then" can be overridden and therefore cannot be handled earlier on. Handling them all at the same time leads to cleaner code.

Tests: already tested by wpt.

Fixes whatwg#4251.
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
This isn't strictly needed for @@toStringTag, @@hasInstance, and @@isConcatSpreadable, but "then" can be overridden and therefore cannot be handled earlier on. Handling them all at the same time leads to cleaner code.

Tests: already tested by wpt.

Fixes whatwg#4251.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security/privacy There are security or privacy implications topic: multiple globals
Development

No branches or pull requests

2 participants