Align Gecko with the new spec for cross-origin objects

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla34
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(15 attachments)

3.41 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
3.48 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
3.20 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.45 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
2.49 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
8.64 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
3.45 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.51 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.37 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
3.28 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
14.28 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.92 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
988 bytes, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.22 KB, patch
ted
: review+
Details | Diff | Splinter Review
21.51 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
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.
Depends on: 965901
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)
Depends on: 965981
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.
We're in good shape here, but need WebIDL Window and Location to fully pass the test suite. Blocking on those.
Depends on: 789261, 832014
Looks like WebIDL Window has landed. Now we're just waiting on WebIDL Location.
The deps here are now done. I'm working on this now.
Depends on: 1040579
Depends on: 1041626
No longer depends on: 1040579
Duplicate of this bug: 1040579
Blocks: 787070
OWs only allow access to Window and Location, both of which are on WebIDL now.
Attachment #8462272 - Flags: review?(gkrizsanits)
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.
Attachment #8462279 - Flags: review?(gkrizsanits)
This causes garbage from a previous lookup to propagate into subsequent lookups,
and creates confusing situations (like having both a value and a getter).
Attachment #8462280 - Flags: review?(gkrizsanits)
This allows us to test document.domain without hoisting the whole test into
an example.com iframe.
Attachment #8462286 - Flags: review?(ted)
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.
Attachment #8462287 - Flags: review?(Ms2ger)
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
Attachment #8462287 - Flags: review?(Ms2ger) → review+
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.
Attachment #8462286 - Flags: review?(ted) → review+
Blocks: 1044083
(In reply to :Ms2ger from comment #25)
> I would expect a SecurityError DOMException

Filed bug 1044083.
Attachment #8462272 - Flags: review?(gkrizsanits) → review+
Attachment #8462273 - Flags: review?(gkrizsanits) → review+
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?
Attachment #8462274 - Flags: review?(gkrizsanits) → review+
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?
Attachment #8462276 - Flags: review?(gkrizsanits) → review+
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"
Attachment #8462277 - Flags: review?(gkrizsanits) → review+
Attachment #8462278 - Flags: review?(gkrizsanits) → review+
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.
Attachment #8462279 - Flags: review?(gkrizsanits) → review+
Attachment #8462280 - Flags: review?(gkrizsanits) → review+
> 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.
(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 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.
Attachment #8462275 - Flags: review?(gkrizsanits) → review+
Attachment #8462281 - Flags: review?(gkrizsanits) → review+
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 .
Attachment #8462282 - Flags: review?(gkrizsanits) → review+
Attachment #8462283 - Flags: review?(gkrizsanits) → review+
Attachment #8462284 - Flags: review?(gkrizsanits) → review+
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.
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.
Looks like there were rooting hazards. I'll back out and fix in the morning:

https://hg.mozilla.org/integration/mozilla-inbound/rev/042fa33c3f5c
Flags: needinfo?(bobbyholley)
(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 :)
Jean-Yves, can you explain why this needs dev-doc?
Flags: needinfo?(jypenator)
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?
Flags: needinfo?(bobbyholley)
(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
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(jypenator) → needinfo?(bobbyholley)
(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!
Flags: needinfo?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.