965898 - Align Gecko with the new spec for cross-origin objects

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

See https://www.w3.org/Bugs/Public/show_bug.cgi?id=20701. This is a moving target at the moment, but I want to have a bug on file for my tentative efforts to implement the new behavior.

Comment 1

[David Bruant]

For those following and being lazy about reading all https://www.w3.org/Bugs/Public/show_bug.cgi?id=20701 , the most recent state of affairs is this etherpad with prose:
https://etherpad.mozilla.org/html5-cross-origin-objects
Tentative test suite (as a patch to W3C's web-platform-tests)
https://www.w3.org/Bugs/Public/attachment.cgi?id=1433

Changing URL to start at comment 128 as it's probably more relevant (and maybe less scary)

Comment 2

[Bobby Holley (:bholley)]

Here's my WIP: https://github.com/bholley/gecko-dev/tree/xospecalign

I've fixed all the stuff I can that doesn't require moving these objects to WebIDL. Currently at 11 passes and 4 failures.

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=90f018c14a97

Comment 4

[Bobby Holley (:bholley)]

We're in good shape here, but need WebIDL Window and Location to fully pass the test suite. Blocking on those.

Comment 5

[Bobby Holley (:bholley)]

Looks like WebIDL Window has landed. Now we're just waiting on WebIDL Location.

Comment 6

[Bobby Holley (:bholley)]

The deps here are now done. I'm working on this now.

Comment 8

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=11b6da7d94a2

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 1 - Drop support for XPCWN XOWs. v1

OWs only allow access to Window and Location, both of which are on WebIDL now.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement an Xray subclass that we can use to implement HTML5 cross-origin semantics. v2

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - All properties on cross-origin DOM objects should be |own|. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 4 - All properties from cross-origin objects are "configurable", non-enumerable, and non-writable. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 5 - Cross-origin objects should have null prototypes. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 6 - Implement proper behavior for [[Enumerate]] And [[OwnPropertyKeys]]. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 7 - Throw for [[Delete]] and [[DefineOwnProperty]]. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 8 - Don't use a FilteringWrapper to get an unfiltered view in ChromeObjectWrapper. v1

This is necessary because subsequent patches cause us to assert when invoking
getPropertyDescriptor on a FilteringWrapper for which |Policy| denies both GET
and SET.

This stuff is really a mess. I'm looking forward to it going away.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 9 - Don't reuse the JSPropertyDescriptor in BaseProxyHandler::keys. v1

This causes garbage from a previous lookup to propagate into subsequent lookups,
and creates confusing situations (like having both a value and a getter).

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 10 - Generalize FilterSetter to work on all the relevant fields of JSPropertyDescriptor. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 11 - Switch policies for get{,Own}PropertyDescriptor. v1

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 12 - Squelch exceptions during the shadowing detection in isCrossOriginAccessPermitted. v1

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 13 - Allow both gettable and settable attributes in Filter(). v1

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 14 - Add subdomains for mochi.test. v1

This allows us to test document.domain without hoisting the whole test into
an example.com iframe.

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 15 - Import web-platform-test into the tree. v1

Just flagging Ms2ger to make sure I did the test import right. The full review
for these tests is happening on github/critic for web-platform-tests.

Comment 24

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=c9393aede029

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8462278 [details] [diff] [review]
Part 7 - Throw for [[Delete]] and [[DefineOwnProperty]]. v1

Review of attachment 8462278 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +238,5 @@
> +    // through here.
> +    if (XrayUtils::IsXrayResolving(cx, wrapper, id))
> +        return SecurityXrayDOM::defineProperty(cx, wrapper, id, desc);
> +
> +    JS_ReportError(cx, "Permission denied to define property on cross-origin object");

I would expect a SecurityError DOMException

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 8462286 [details] [diff] [review]
Part 14 - Add subdomains for mochi.test. v1

Review of attachment 8462286 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/pgo/server-locations.txt
@@ +60,5 @@
>  http://127.0.0.1:8888             privileged
>  http://test:80                    privileged
>  http://mochi.test:8888            privileged
> +http://test1.mochi.test:8888      privileged
> +http://test2.mochi.test:8888      privileged

I'm pretty sure the 'privileged' option hasn't done anything since you ripped out per-site caps. Might as well leave it out.

Comment 27

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #25)
> I would expect a SecurityError DOMException

Filed bug 1044083.

Comment 28

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8462274 [details] [diff] [review]
Part 3 - All properties on cross-origin DOM objects should be |own|. v1

Review of attachment 8462274 [details] [diff] [review]:
-----------------------------------------------------------------

Can you please add to the etherpad that all white-listed properties should be own props?

Comment 29

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8462275 [details] [diff] [review]
Part 4 - All properties from cross-origin objects are "configurable", non-enumerable, and non-writable. v1

Review of attachment 8462275 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +190,5 @@
>          // All properties on cross-origin DOM objects are |own|.
>          desc.object().set(wrapper);
> +
> +        // All properties on cross-origin DOM objects are non-enumerable and
> +        // "configurable". Any value attributes are read-only.

I don't see how can these properties be configurable when we throw on delete attempts. Nor do I think we handle it here... Am I missing something?

Comment 30

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8462277 [details] [diff] [review]
Part 6 - Implement proper behavior for [[Enumerate]] And [[OwnPropertyKeys]]. v1

Review of attachment 8462277 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +225,5 @@
> +{
> +    // All properties on cross-origin objects are supposed |own|, despite what
> +    // the underlying native object may report. Override the inherited trap to
> +    // avoid passing JSITER_OWNONLY as a flag.
> +    return SecurityXrayDOM::enumerate(cx, wrapper, JSITER_HIDDEN, props);

Do we really need hidden here? A simple JSITER_ENUMERATE would not cover all what we need?

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1912,5 @@
>              return false;
>      }
>  
> +    // Go through the properties we found on the underlying object and see if
> +    // they appear on the XrayWrapper. If if throws (which may happen if the

Nit: "If if"

Comment 31

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8462279 [details] [diff] [review]
Part 8 - Don't use a FilteringWrapper to get an unfiltered view in ChromeObjectWrapper. v1

Review of attachment 8462279 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
@@ +59,5 @@
>      MOZ_ASSERT(js::Wrapper::wrapperHandler(wrapper) ==
>                 &ChromeObjectWrapper::singleton);
>      Rooted<JSPropertyDescriptor> desc(cx);
>      const ChromeObjectWrapper *handler = &ChromeObjectWrapper::singleton;
> +    if (!handler->CrossCompartmentSecurityWrapper::getPropertyDescriptor(cx, wrapper, id,

You might want to update the comment as well above this function.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> I don't see how can these properties be configurable when we throw on delete attempts. 

There is no spec requirement that configurable properties be deletable.  The only requirement is that non-configurable properties not be deletable.

Comment 33

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #32)
> There is no spec requirement that configurable properties be deletable.  The
> only requirement is that non-configurable properties not be deletable.

Right, I should have rely on the spec instead of our docs it seems...

Comment 34

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8462275 [details] [diff] [review]
Part 4 - All properties from cross-origin objects are "configurable", non-enumerable, and non-writable. v1

Review of attachment 8462275 [details] [diff] [review]:
-----------------------------------------------------------------

Ignore my previous comment on this patch.

Comment 35

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8462282 [details] [diff] [review]
Part 11 - Switch policies for get{,Own}PropertyDescriptor. v1

Review of attachment 8462282 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/AccessCheck.h
@@ +78,5 @@
>  struct ExposedPropertiesOnly : public Policy {
>      static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act);
>  
>      static bool deny(js::Wrapper::Action act, JS::HandleId id) {
> +        // Fail silently for GET ENUMERATE, and GET_PROPERTY_DESCRIPTOR

Nit: missing .

Comment 36

[Bobby Holley (:bholley)]

Thanks for doing this marathon of reviews so quickly! Especially since it involved a fair amount of background reading on this spec issue that's been dragging on for over a year. ;-)

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #28)
> Can you please add to the etherpad that all white-listed properties should
> be own props?

Wow, good catch. I guess the prose there is pretty vague at present. Fixed.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #30)
> Comment on attachment 8462277 [details] [diff] [review]
> Part 6 - Implement proper behavior for [[Enumerate]] And
> ::: js/xpconnect/wrappers/FilteringWrapper.cpp
> 
> Do we really need hidden here? A simple JSITER_ENUMERATE would not cover all
> what we need?

I seem to remember needing it when I wrote this patch, though it might have been back in January when this stuff was still on XPCWNs. Regardless though, it seems better to avoid depending on enumerability (given that it's a kinda-sorta dead concept in ES), and just enumerate all the properties (the way Object.getOwnPropertyNames would), since we subsequently filter the properties and override the attributes.

Comment 37

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c4c7ab1a2f70

Comment 38

[Bobby Holley (:bholley)]

Hm, I could have sworn I had a green all-platform try push for this, but all I can find is the (green) linux64 one in comment 24. This is a rather large change, so hopefully nothing breaks on another platform.

Comment 39

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4abde3579f

Comment 40

[Bobby Holley (:bholley)]

Looks like there were rooting hazards. I'll back out and fix in the morning:

https://hg.mozilla.org/integration/mozilla-inbound/rev/042fa33c3f5c

Comment 41

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #36)
> Thanks for doing this marathon of reviews so quickly! Especially since it
> involved a fair amount of background reading on this spec issue that's been
> dragging on for over a year. ;-)
> 

Well... the real hard part of the job is to validate if the _what_ to implement
part is all correct. But luckily that part was sort of already done by lot more
knowledgeable people than me :)

Comment 42

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1f84691d824f

Comment 43

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=22e1b7b69877

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e76ece9c756b
https://hg.mozilla.org/mozilla-central/rev/88a562ab485b
https://hg.mozilla.org/mozilla-central/rev/9e85835de239
https://hg.mozilla.org/mozilla-central/rev/f440504714b9
https://hg.mozilla.org/mozilla-central/rev/103615c82485
https://hg.mozilla.org/mozilla-central/rev/cf2bc60412d6
https://hg.mozilla.org/mozilla-central/rev/4209175a1f0a
https://hg.mozilla.org/mozilla-central/rev/92d1c61c3cdf
https://hg.mozilla.org/mozilla-central/rev/2a6260b2ae9c
https://hg.mozilla.org/mozilla-central/rev/60dcc2593586
https://hg.mozilla.org/mozilla-central/rev/a5b95c1ec252
https://hg.mozilla.org/mozilla-central/rev/d099909ba007
https://hg.mozilla.org/mozilla-central/rev/7185b7e3b96b
https://hg.mozilla.org/mozilla-central/rev/00cb0d79e405
https://hg.mozilla.org/mozilla-central/rev/79ac7fa6e649
https://hg.mozilla.org/mozilla-central/rev/22e1b7b69877

Comment 45

[Bobby Holley (:bholley)]

Jean-Yves, can you explain why this needs dev-doc?

Comment 46

[Jean-Yves Perrier [:teoli]]

Hi. I think we should explain what cross-origin objects are and some of their behavior. At first sight, I would say it may be worth to document the cross-origin behaviors of Window and Location. Does it make sense?

Comment 47

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Perrier [:teoli] from comment #46)
> Hi. I think we should explain what cross-origin objects are and some of
> their behavior. At first sight, I would say it may be worth to document the
> cross-origin behaviors of Window and Location. Does it make sense?

Updating the documentation on the same-origin policy [1] definitely makes sense. In particular, it would be really helpful to include a list of the properties on Window and Location that are actually available cross-origin.

The changes in this patch specifically are probably too subtle and technical to be worth documenting outside of the spec.

[1] https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy

Comment 48

[Will Bamberg [:wbamberg]]

OK, I've had a go at this: https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy#Cross-origin_script_API_access. The only difference I could see between Firefox 36 and the spec (http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#security-window) is that Firefox 36 allows access to window.length.

Comment 49

[Bobby Holley (:bholley)]

(In reply to Will Bamberg [:wbamberg] from comment #48)
> OK, I've had a go at this:
> https://developer.mozilla.org/en-US/docs/Web/Security/Same-
> origin_policy#Cross-origin_script_API_access. The only difference I could
> see between Firefox 36 and the spec
> (http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.
> html#security-window) is that Firefox 36 allows access to window.length.

window.length is a "dynamic nested browsing context properties", which is accessible per-spec.

Looks great!