Skip to content

Tweak the exposure of cross-origin properties#2777

Merged
annevk merged 1 commit into
masterfrom
cross-origin-object-tweaks
Jul 17, 2017
Merged

Tweak the exposure of cross-origin properties#2777
annevk merged 1 commit into
masterfrom
cross-origin-object-tweaks

Conversation

@domenic

@domenic domenic commented Jun 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

This contains two separate changes:

  • It makes all cross-origin properties that would normally be enumerable
    on same-origin objects, enumerable also on WindowProxy and Location
    objects (including when accessed same-origin). This includes
    safelisted methods and attributes, browsing context name properties,
    and browsing context index properties. The motivation for making them
    non-enumerable seems to have been a mistaken impression that doing so
    would prevent a cross-origin information leak.

  • It hides window names from [[OwnPropertyKeys]](), and thus
    Object.keys(), Object.getOwnPropertyNames(), etc. This actually
    prevents a cross-origin information leak.

Closes #2753.

Marking "do not merge yet" until we are aware of the web-compatibility implications of the latter change. Writing tests should help with that.

@domenic
Tweak the exposure of cross-origin properties
205659f 
This contains two separate changes:

* It makes all cross-origin properties that would normally be enumerable
  on same-origin objects, enumerable also on WindowProxy and Location
  objects (including when accessed same-origin). This includes
  safelisted methods and attributes, browsing context name properties,
  and browsing context index properties. The motivation for making them
  non-enumerable seems to have been a mistaken impression that doing so
  would prevent a cross-origin information leak.

* It hides window names from [[OwnPropertyKeys]](), and thus
  Object.keys(), Object.getOwnPropertyNames(), etc. This actually
  prevents a cross-origin information leak.

Closes #2753.
@domenic domenic added do not merge yet Pull request must not be merged per rationale in comment normative change security/privacy There are security or privacy implications needs tests Moving the issue forward requires someone to write tests labels Jun 21, 2017
@domenic domenic mentioned this pull request Jun 28, 2017
Closed
hubot pushed a commit to WebKit/WebKit-http that referenced this pull request Jul 11, 2017
Window's [[OwnPropertyKeys]] is wrong for cross origin windows
0b7c00e 
https://bugs.webkit.org/show_bug.cgi?id=174364
<rdar://problem/33238056>

Reviewed by Brent Fulgham.

Source/WebCore:

Window's [[OwnPropertyKeys]] should not list descendant frame names
when the window is cross-origin:
- whatwg/html#2777

This aligns our behavior with Firefox and Chrome.

No new tests, updated existing test.

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::addCrossOriginPropertyNames):
(WebCore::addCrossOriginOwnPropertyNames):
(WebCore::JSDOMWindow::getOwnPropertyNames):

LayoutTests:

Update test to reflect behavior change. I verified that the test is passing in Firefox.
The test fails in Chrome because its does not expose frames indexes on the Window, and
it is incorrectly listing "assign" on Location.

* http/tests/security/cross-frame-access-enumeration.html:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@219355 268f45cc-cd09-0410-ab3c-d52691b4dbfc
annevk added a commit to web-platform-tests/wpt that referenced this pull request Jul 13, 2017
@annevk
"Cross-origin objects" are to have enumerable properties
b838fd4 
See whatwg/html#2777.
@annevk annevk mentioned this pull request Jul 13, 2017
Merged
@annevk

annevk commented Jul 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

The latter change is web-compatible (and is actually what implementations do). Tests at web-platform-tests/wpt#6538. The potential problem area would be the former change I think, but maybe it's not a big deal.

@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Jul 13, 2017
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Jul 14, 2017
@domenic

domenic commented Jul 14, 2017

Copy link
Copy Markdown
Member Author

OK cool, so this should be ready to merge. @annevk, will let you review/merge alongside #2777.

annevk
annevk approved these changes Jul 15, 2017
View reviewed changes

@annevk annevk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh forgot to explicitly mark this as reviewed. I can land and file bugs on Monday.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Jul 17, 2017
@annevk
"Cross-origin objects" are to have enumerable properties
e63fd9b 
And also, test that IDL so-called named properties are not exposed across origins (via [[OwnPropertyKeys]]()).

See whatwg/html#2777 for the corresponding HTML Standard change.
@annevk annevk merged commit 79b4fbc into master Jul 17, 2017
@annevk annevk deleted the cross-origin-object-tweaks branch July 17, 2017 08:14
hubot pushed a commit to WebKit/WebKit-http that referenced this pull request Jul 19, 2017
Make cross-origin properties enumerable
1a50f18 
https://bugs.webkit.org/show_bug.cgi?id=174576

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Re-sync tests from upstream and rebaseline to improve test coverage.

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

Source/WebCore:

Makes cross-origin properties enumerable on Window and Location objects
as per:
- whatwg/html#2777

This simplifies our code quite a bit.

No new tests, updated existing tests.

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

LayoutTests:

Update / rebaseline some tests to reflect behavior change.

* http/tests/security/cross-origin-descriptors-expected.txt:
* http/tests/security/cross-origin-descriptors.html:
* js/dom/getOwnPropertyDescriptor-expected.txt:
* js/resources/getOwnPropertyDescriptor.js:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@219659 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@bzbarsky bzbarsky mentioned this pull request Oct 31, 2017
Closed
@bzbarsky

bzbarsky commented Oct 31, 2017

Copy link
Copy Markdown
Contributor

The potential problem area would be the former change

Yeah, that part doesn't seem to be web-compatible. See #3183

ryanhaddad pushed a commit to WebKit/WebKit that referenced this pull request Dec 22, 2020
@cdumez
Window's [[OwnPropertyKeys]] is wrong for cross origin windows
3cc35bf 
https://bugs.webkit.org/show_bug.cgi?id=174364
<rdar://problem/33238056>

Reviewed by Brent Fulgham.

Source/WebCore:

Window's [[OwnPropertyKeys]] should not list descendant frame names
when the window is cross-origin:
- whatwg/html#2777

This aligns our behavior with Firefox and Chrome.

No new tests, updated existing test.

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::addCrossOriginPropertyNames):
(WebCore::addCrossOriginOwnPropertyNames):
(WebCore::JSDOMWindow::getOwnPropertyNames):

LayoutTests:

Update test to reflect behavior change. I verified that the test is passing in Firefox.
The test fails in Chrome because its does not expose frames indexes on the Window, and
it is incorrectly listing "assign" on Location.

* http/tests/security/cross-frame-access-enumeration.html:

Canonical link: https://commits.webkit.org/191196@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219355 268f45cc-cd09-0410-ab3c-d52691b4dbfc
ryanhaddad pushed a commit to WebKit/WebKit that referenced this pull request Dec 22, 2020
@cdumez
Make cross-origin properties enumerable
c0737a6 
https://bugs.webkit.org/show_bug.cgi?id=174576

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Re-sync tests from upstream and rebaseline to improve test coverage.

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

Source/WebCore:

Makes cross-origin properties enumerable on Window and Location objects
as per:
- whatwg/html#2777

This simplifies our code quite a bit.

No new tests, updated existing tests.

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

LayoutTests:

Update / rebaseline some tests to reflect behavior change.

* http/tests/security/cross-origin-descriptors-expected.txt:
* http/tests/security/cross-origin-descriptors.html:
* js/dom/getOwnPropertyDescriptor-expected.txt:
* js/resources/getOwnPropertyDescriptor.js:


Canonical link: https://commits.webkit.org/191470@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219659 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@annevk annevk annevk approved these changes

Assignees

No one assigned

Labels

normative change security/privacy There are security or privacy implications

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@domenic @annevk @bzbarsky