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

No way to resolve a Promise with a cross domain WindowProxy #536

Closed
esprehn opened this issue Nov 15, 2017 · 19 comments
Closed

No way to resolve a Promise with a cross domain WindowProxy #536

esprehn opened this issue Nov 15, 2017 · 19 comments

Comments

@esprehn
Copy link

esprehn commented Nov 15, 2017

Since resolving a promise looks for a thenable and accessing the then property on a cross domain WindowProxy throws you can't do something like:

const loadFrame = (src) => new Promise((resolve) => {
  frame.src = src;
  frame.onload = () => resolve(frame.contentWindow);
});

Perhaps cross domain WindowProxy objects should allow accessing then and have it always be undefined?

@annevk
Copy link
Member

annevk commented Nov 15, 2017

This is technically an issue with whatwg/html, but we can discuss it here. I don't care strongly.

If this also needs to work for cross-origin Location objects it would be simplest to change step 1 of https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-) and step 3 of https://html.spec.whatwg.org/#crossoriginownpropertykeys-(-o-): add "then" to the list of those three symbols.

cc @bzbarsky @cdumez @domenic

@bzbarsky
Copy link

We probably could do that. @bholley may have opinions on whether it's OK from his pov security-wise.

I just double-checked the interaction with subframes named "then", and I think it should be ok, since at https://html.spec.whatwg.org/#windowproxy-getownproperty step 5 the value would be undefined, then at step 6 we would find the named subframe, but it's not callable, so would not affect the Promise behavior.

@esprehn
Copy link
Author

esprehn commented Nov 15, 2017

(Sorry about filing in the wrong repo @annevk my bad :)

@mikewest

@bholley
Copy link

bholley commented Nov 15, 2017

Note: I'm on leave, so probably can't formulate a definitive answer here in a timeframe that would be useful. That said, it seems fine at first glance, and so I'm happy deferring to Boris.

@domenic
Copy link
Member

domenic commented Nov 16, 2017

This seems like a generally good idea to me; basically treating it the same as toStringTag/hasInstance/isConcatSpreadable. I've forgotten why those need to return a property with value undefined, instead of just saying there's no such property, but that's separate I guess.

@annevk
Copy link
Member

annevk commented Nov 16, 2017

@domenic I think that's for the same reason. So obvious operations you might apply to objects don't end up throwing. (Although those might also have been driven by web compatibility.)

@domenic
Copy link
Member

domenic commented Nov 16, 2017

No, I mean, just omitting the property would also work (i.e. avoid the throwing), so I don't remember why we made them explicitly present as undefined. Shrug

@annevk
Copy link
Member

annevk commented Nov 16, 2017

I don't understand, getting a property that does not exist will throw on a cross-origin object. E.g., try <iframe src=data:, onload=w(this.contentWindow.x)></iframe> in LDV.

@domenic
Copy link
Member

domenic commented Nov 16, 2017

Right, because we specced it to. We could spec that for these 3-4 properties, it doesn't exist and doesn't throw. Instead we specced that it does exist and doesn't throw.

@annevk
Copy link
Member

annevk commented Nov 16, 2017

@domenic
Copy link
Member

domenic commented Nov 16, 2017

Got it. Oh well, I don't think it's better in any way that's worth the churn. Just glad to have my curiousity satisfied :). Sorry for derailing the thread. Back on topic...

@yuki3 @cdumez, are you up for adding then to the list of undefined cross-origin properties? Currently it contains @@toStringTag, @@hasInstance, and @@isConcatSpreadable.

@yuki3
Copy link

yuki3 commented Nov 17, 2017

Blink's implementation is still far from the ideal state, but I can implement something so that crossOriginWindow.then returns undefined. However, I have a concern.

If we add then to HTML 7.2.3.3 CrossOriginGetOwnPropertyHelper, then then has a priority over 7.2.3.1 CrossOriginProperties, right? IIUC, it shadows a child browsing context with the name "then".

Should we deprioritize then? @@toStringTag, etc. are probably okay because they're symbols.

@yuki3
Copy link

yuki3 commented Nov 17, 2017

Oops, maybe I'm wrong. We'd like to intentionally shadow then even though it would break the backward compatibility, right? Just in case, I'd like to confirm it before implementing. Is it okay?

@annevk
Copy link
Member

annevk commented Nov 17, 2017

I'm not sure if that's what we want. It's easy enough to move step 1 of CrossOriginGetOwnPropertyHelper down and it would be more consistent with how a same-origin window behaves.

The potential for conflict would also complicate CrossOriginOwnPropertyKeys somewhat as we don't want to return duplicate keys there. We'd only want to append "then" if it's not there already.

@bzbarsky
Copy link

then then has a priority over 7.2.3.1 CrossOriginProperties, right? IIUC, it shadows a child browsing context with the name "then".

Oh, good catch. I got confused between undefined-the-value and undefined-the-descriptor when looking at https://html.spec.whatwg.org/multipage/window-object.html#windowproxy-getownproperty

I agree that this is not desirable. If there's a named subframe called "then", we should expose it in the usual way. It won't affect the Promise behavior, per #536 (comment)

@yuki3
Copy link

yuki3 commented Nov 20, 2017

I see. I understand that we wouldn't like to shadow "then". I agree that it's reasonable. I'll implement it in Blink accordingly.

annevk added a commit to whatwg/html that referenced this issue Nov 20, 2017
This allows promises to work better with cross-origin WindowProxy and Location objects.

Tests: (please write some).

Fixes: whatwg/dom#536.
@annevk
Copy link
Member

annevk commented Nov 20, 2017

I created a PR that should match the discussion above. Anyone keen on writing tests?

@yuki3
Copy link

yuki3 commented Nov 21, 2017

Just FYI, I landed a patch in Blink.
https://chromium-review.googlesource.com/c/chromium/src/+/779319

(I know that it's imperfect from a lot of point of views, and wouldn't pass the coming WPTs. But crossOriginObject.then returns undefined at least.)

MXEBot pushed a commit to mirror/chromium that referenced this issue Nov 23, 2017
According to the discussion (and request) at
  whatwg/dom#536
this patch adds 'then' as a cross origin accessible property
returning 'undefined'.

HTML 7.2.3.3 CrossOriginGetOwnPropertyHelper ( O, P )
https://html.spec.whatwg.org/multipage/browsers.html#crossorigingetownpropertyhelper-(-o,-p-)
is not yet updated, but it should be quite soon.  The proposed
change is at
  whatwg/html#3242

Bug: 
Change-Id: Icb72ba01ccf8eabaad607b61805411d1b4845f86
Reviewed-on: https://chromium-review.googlesource.com/779319
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518158}
@bzbarsky
Copy link

bzbarsky commented Feb 7, 2018

I wrote some tests in https://bug1435596.bmoattachments.org/attachment.cgi?id=8949101

Chrome fails most of the relevant wpt already, unfortunately, which makes behavior around this stuff really hard to test there.

domenic pushed a commit to whatwg/html that referenced this issue Feb 7, 2018
This allows promises to work better with cross-origin WindowProxy and Location objects.

Fixes whatwg/dom#536.
kisg pushed a commit to paul99/webkit-mips that referenced this issue Oct 1, 2018
https://bugs.webkit.org/show_bug.cgi?id=190094

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Rebaseline WPT test now that more checks are passing.

* web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:

Source/WebCore:

Make crossOriginObject.then undefined for promises. This allows promises to work better with cross-origin WindowProxy
and Location objects.

Specification:
- whatwg/html#3242
- whatwg/dom#536

This aligns our behavior with Blink and Gecko.

No new tests, rebaselined existing test.

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess):
(WebCore::addCrossOriginWindowOwnPropertyNames):
* bindings/js/JSLocationCustom.cpp:
(WebCore::getOwnPropertySlotCommon):
(WebCore::addCrossOriginLocationOwnPropertyNames):

LayoutTests:

Update existing tests to reflect behavior change.

* http/tests/navigation/process-swap-window-open-expected.txt:
* http/tests/navigation/process-swap-window-open.html:
* http/wpt/cross-origin-window-policy/resources/utils.js:
(testCrossOriginOption):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@236661 268f45cc-cd09-0410-ab3c-d52691b4dbfc
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
This allows promises to work better with cross-origin WindowProxy and Location objects.

Fixes whatwg/dom#536.
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=190094

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Rebaseline WPT test now that more checks are passing.

* web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:

Source/WebCore:

Make crossOriginObject.then undefined for promises. This allows promises to work better with cross-origin WindowProxy
and Location objects.

Specification:
- whatwg/html#3242
- whatwg/dom#536

This aligns our behavior with Blink and Gecko.

No new tests, rebaselined existing test.

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess):
(WebCore::addCrossOriginWindowOwnPropertyNames):
* bindings/js/JSLocationCustom.cpp:
(WebCore::getOwnPropertySlotCommon):
(WebCore::addCrossOriginLocationOwnPropertyNames):

LayoutTests:

Update existing tests to reflect behavior change.

* http/tests/navigation/process-swap-window-open-expected.txt:
* http/tests/navigation/process-swap-window-open.html:
* http/wpt/cross-origin-window-policy/resources/utils.js:
(testCrossOriginOption):


Canonical link: https://commits.webkit.org/205090@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236661 268f45cc-cd09-0410-ab3c-d52691b4dbfc
robertknight added a commit to hypothesis/client that referenced this issue Sep 7, 2021
…wsers

Fix an issue where the sidebar failed to appear in Safari 11 and Chrome
63 when returning a cross-origin Window from a Promise `then` callback.
In these browsers an exception is triggered when the browser tries to
test if the return value is a Promise that can be unwrapped.

The issue was resolved in more recent browsers by adding an undefined
`then` property to cross origin objects. [1]

[1] whatwg/dom#536
robertknight added a commit to hypothesis/client that referenced this issue Sep 7, 2021
…wsers

Fix an issue where the sidebar failed to appear in Safari 11 and Chrome
63 when returning a cross-origin Window from a Promise `then` callback.
In these browsers an exception is triggered when the browser tries to
test if the return value is a Promise that can be unwrapped.

The issue was resolved in more recent browsers by adding an undefined
`then` property to cross origin objects. [1]

[1] whatwg/dom#536
robertknight added a commit to hypothesis/client that referenced this issue Sep 8, 2021
…wsers

Fix an issue where the sidebar failed to appear in Safari 11 and Chrome
63 when returning a cross-origin Window from a Promise `then` callback.
In these browsers an exception is triggered when the browser tries to
test if the return value is a Promise that can be unwrapped.

The issue was resolved in more recent browsers by adding an undefined
`then` property to cross origin objects. [1]

[1] whatwg/dom#536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants