Bug 167920 - Symbols exposed on cross-origin Window / Location objects should be configurable
Summary: Symbols exposed on cross-origin Window / Location objects should be configurable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2017-02-06 19:55 PST by Chris Dumez
Modified: 2017-02-06 20:48 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.25 KB, patch)
2017-02-06 19:57 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.50 KB, patch)
2017-02-06 20:46 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-02-06 19:55:13 PST
Symbols exposed on cross-origin Window / Location objects should be configurable:
- https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-) (Step 1)

Firefox behaves as per specification.
Comment 1 Chris Dumez 2017-02-06 19:57:51 PST
Created attachment 300779 [details]
Patch
Comment 2 Chris Dumez 2017-02-06 20:03:08 PST
With this bug and Bug 167917 fixed, 100% pass rate on:
- http://w3c-test.org/html/browsers/origin/cross-origin-objects/cross-origin-objects.html
Comment 3 Ryosuke Niwa 2017-02-06 20:12:35 PST
Comment on attachment 300779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300779&action=review

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:100
>      if (propertyName == exec->propertyNames().toStringTagSymbol || propertyName == exec->propertyNames().hasInstanceSymbol || propertyName == exec->propertyNames().isConcatSpreadableSymbol) {

Are these only symbols that could ever exist on the window object?
It seems a bit fragile to have a hard-coded list here.
Comment 4 Chris Dumez 2017-02-06 20:27:53 PST
(In reply to comment #3)
> Comment on attachment 300779 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300779&action=review
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:100
> >      if (propertyName == exec->propertyNames().toStringTagSymbol || propertyName == exec->propertyNames().hasInstanceSymbol || propertyName == exec->propertyNames().isConcatSpreadableSymbol) {
> 
> Are these only symbols that could ever exist on the window object?
> It seems a bit fragile to have a hard-coded list here.

These are the only ones that we expose on Window / Location objects that are *cross-origin*. They are listed in the spec here:
https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-) (Step 1).
Comment 5 Chris Dumez 2017-02-06 20:31:06 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 300779 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=300779&action=review
> > 
> > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:100
> > >      if (propertyName == exec->propertyNames().toStringTagSymbol || propertyName == exec->propertyNames().hasInstanceSymbol || propertyName == exec->propertyNames().isConcatSpreadableSymbol) {
> > 
> > Are these only symbols that could ever exist on the window object?
> > It seems a bit fragile to have a hard-coded list here.
> 
> These are the only ones that we expose on Window / Location objects that are
> *cross-origin*. They are listed in the spec here:
> https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-) (Step
> 1).

This is how the HTML specification and our code operates for cross-origin Window / Location objects, we explicitly whitelist things we want to expose. The list of properties exposed cross-origin on these objects is also hard-coded.
Comment 6 Ryosuke Niwa 2017-02-06 20:33:19 PST
Comment on attachment 300779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300779&action=review

>>>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:100
>>>>      if (propertyName == exec->propertyNames().toStringTagSymbol || propertyName == exec->propertyNames().hasInstanceSymbol || propertyName == exec->propertyNames().isConcatSpreadableSymbol) {
>>> 
>>> Are these only symbols that could ever exist on the window object?
>>> It seems a bit fragile to have a hard-coded list here.
>> 
>> These are the only ones that we expose on Window / Location objects that are *cross-origin*. They are listed in the spec here:
>> https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-) (Step 1).
> 
> This is how the HTML specification and our code operates for cross-origin Window / Location objects, we explicitly whitelist things we want to expose. The list of properties exposed cross-origin on these objects is also hard-coded.

We should probably add that URL as a comment.

> Source/WebCore/bindings/js/JSLocationCustom.cpp:-57
>      if (propertyName == state->propertyNames().toStringTagSymbol || propertyName == state->propertyNames().hasInstanceSymbol || propertyName == state->propertyNames().isConcatSpreadableSymbol) {
> -        slot.setUndefined();

Ditto here.
Also, perhaps we should extract this check into a helper function so that there's a single list instead of being duplicated in two places.
Comment 7 Chris Dumez 2017-02-06 20:46:55 PST
Created attachment 300785 [details]
Patch
Comment 8 Chris Dumez 2017-02-06 20:48:45 PST
Comment on attachment 300785 [details]
Patch

Clearing flags on attachment: 300785

Committed r211772: <http://trac.webkit.org/changeset/211772>
Comment 9 Chris Dumez 2017-02-06 20:48:51 PST
All reviewed patches have been landed.  Closing bug.