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

Enumerable cross-origin properties don't seem to be web-compatible #3183

Closed
bzbarsky opened this Issue Oct 31, 2017 · 10 comments

Comments

4 participants
@bzbarsky
Collaborator

bzbarsky commented Oct 31, 2017

Updating Firefox to #2777 caused a web compat issue: if a cross-origin window makes its way into jQuery.extend, that function will go into an infinite loop. This is turning up as a problem on actual sites. See https://bugzilla.mozilla.org/show_bug.cgi?id=1412741 for details.

I will be backing out the corresponding changes from Firefox.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky
Collaborator

bzbarsky commented Oct 31, 2017

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 31, 2017

Collaborator

And @cdumez because he was working on this stuff in WebKit...

Collaborator

bzbarsky commented Oct 31, 2017

And @cdumez because he was working on this stuff in WebKit...

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Oct 31, 2017

Contributor

I may be able to reproduce in WebKit ToT. No infinite loops but login fails and I get this error in console:
RangeError: Maximum call stack size exceeded.

Contributor

cdumez commented Oct 31, 2017

I may be able to reproduce in WebKit ToT. No infinite loops but login fails and I get this error in console:
RangeError: Maximum call stack size exceeded.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 31, 2017

Collaborator

Right, I should have said "infinite recursion", not "infinite loop". In Gecko the infinite recursion didn't run out of stack space in the amount of time I was willing to wait...

Collaborator

bzbarsky commented Oct 31, 2017

Right, I should have said "infinite recursion", not "infinite loop". In Gecko the infinite recursion didn't run out of stack space in the amount of time I was willing to wait...

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 31, 2017

Member

We'll try to fix this ASAP. Sorry to put everyone through this.

To help us understand: this problem would also occur with a same-origin window, right? It's just bad luck that the code in question is only passing cross-origin windows and expecting them to work?

Member

domenic commented Oct 31, 2017

We'll try to fix this ASAP. Sorry to put everyone through this.

To help us understand: this problem would also occur with a same-origin window, right? It's just bad luck that the code in question is only passing cross-origin windows and expecting them to work?

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 31, 2017

Collaborator

To help us understand: this problem would also occur with a same-origin window, right?

No, because the recursive step in jQuery.extend only happens for things that test true for Array.isArray or jQuery.isPlainObject. And jQuery.isPlainObject is false for same-origin windows (not least because toString.call( obj ) !== "[object Object]" returns true). But it's true for cross-origin windows, because that test returns false and the object has a null proto. See https://code.jquery.com/jquery-3.2.1.js around line 300.

The problem would occur with an object graph that contains plain objects, has loops, and has non-writable properties so the loops don't get broken by extend() as it traverses the object graph...

Collaborator

bzbarsky commented Oct 31, 2017

To help us understand: this problem would also occur with a same-origin window, right?

No, because the recursive step in jQuery.extend only happens for things that test true for Array.isArray or jQuery.isPlainObject. And jQuery.isPlainObject is false for same-origin windows (not least because toString.call( obj ) !== "[object Object]" returns true). But it's true for cross-origin windows, because that test returns false and the object has a null proto. See https://code.jquery.com/jquery-3.2.1.js around line 300.

The problem would occur with an object graph that contains plain objects, has loops, and has non-writable properties so the loops don't get broken by extend() as it traverses the object graph...

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 31, 2017

Collaborator

Note that after the backout, Firefox will go back to its previous behavior: indexed properties will be enumerable, but other things will not.

Collaborator

bzbarsky commented Oct 31, 2017

Note that after the backout, Firefox will go back to its previous behavior: indexed properties will be enumerable, but other things will not.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 1, 2017

Member

@bzbarsky but other things would still be enumerable on same-origin objects, right?

Member

annevk commented Nov 1, 2017

@bzbarsky but other things would still be enumerable on same-origin objects, right?

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Nov 1, 2017

Collaborator

Yes, everything will remain as it was for same-origin objects.

Collaborator

bzbarsky commented Nov 1, 2017

Yes, everything will remain as it was for same-origin objects.

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Nov 1, 2017

Contributor

Will back out change from WebKit as well (https://bugs.webkit.org/show_bug.cgi?id=179117).

Contributor

cdumez commented Nov 1, 2017

Will back out change from WebKit as well (https://bugs.webkit.org/show_bug.cgi?id=179117).

hubot pushed a commit to WebKit/webkit that referenced this issue Nov 1, 2017

cdumez@apple.com
Regression(r219659): Can no longer log into ifttt.com using Google ac…
…count

https://bugs.webkit.org/show_bug.cgi?id=179117

Reviewed by Geoffrey Garen.

LayoutTests/imported/w3c:

Rebaseline WPT tests.

* web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:
* web-platform-tests/html/browsers/the-window-object/window-indexed-properties-expected.txt:

Source/WebCore:

After r219659, it is no longer possible to log into ifttt.com using a Google
account:
- Signed into a Google account already
- Visit https://ifttt.com/login
- Click "Continue with Google"
- Select the signed in account

It turns out that this change to the HTML specification was not Web-compatible:
See https://bugzilla.mozilla.org/show_bug.cgi?id=1412741 & whatwg/html#3183

This patch reverts r219659 for now until we agree on what behavior should get
specified.

No new tests, rebaselined existing tests.

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess):
(WebCore::JSDOMWindow::getOwnPropertySlotByIndex):
(WebCore::JSDOMWindow::getOwnPropertyNames):
* bindings/js/JSLocationCustom.cpp:
(WebCore::getOwnPropertySlotCommon):
(WebCore::JSLocation::getOwnPropertyNames):

LayoutTests:

Update / rebaseline existing test.

* http/tests/security/cross-origin-descriptors-expected.txt:
* http/tests/security/cross-origin-descriptors.html:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@224287 268f45cc-cd09-0410-ab3c-d52691b4dbfc

annevk added a commit that referenced this issue Nov 2, 2017

Enumerate less cross-origin properties
In 205659f we made all properties on cross-origin objects enumerable, equivalent to their same-origin object counterparts.

However, this turned out not be web-compatible. This makes them unenumerable again with the exception of array index property names, which need to be enumerable.

Tests: ...

Fixes #3183.

@domenic domenic closed this in #3186 Nov 6, 2017

domenic added a commit that referenced this issue Nov 6, 2017

Enumerate fewer cross-origin properties
In 205659f we made all properties on
cross-origin objects enumerable, equivalent to their same-origin object
counterparts.

However, this turned out not be web-compatible. This makes them
non-enumerable again with the exception of array index property names,
which need to be enumerable.

Tests: web-platform-tests/wpt#8045

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