980023 - content scripts have no access to localStorage if cross-domain-content is enabled

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Is this working as intended? The documentation implies that only unsafeWindow is unavailable.

https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts/Cross_Domain_Content_Scripts#Content_Permissions_and_unsafeWindow

Sample code: https://github.com/monicachew/addon-test/commit/70428d7103be1dc013e9a1c37258ad9351c911e0

Sample trace: https://gist.github.com/monicachew/9374576

Comment 1

[Tomislav Jovanovic :zombie]

Gabor, hate to do this to you twice in two days, but..

Comment 2

[Gabor Krizsanits (INACTIVE)]

Yes I've just run into the same problem a few days ago, so I know what is going on. Yes it's a bug. https://bugzilla.mozilla.org/show_bug.cgi?id=946815#c54

I will write a quick patch for this soon.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: Localstorage access with nsEp. v1

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

Comment 4

[Gabor Krizsanits (INACTIVE)]

wrong try flags previously...
https://tbpl.mozilla.org/?tree=Try&rev=15e43690ece0

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8388357 [details] [diff] [review]
Localstorage access with nsEp. v1

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

r=bholley with those things fixed.

::: js/xpconnect/tests/chrome/test_localstorage_with_nsEp.xul
@@ +1,5 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
> +<?xml-stylesheet type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"?>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=980023 

whitespace.

@@ +20,5 @@
> +
> +  /** Test for localstorage access with expanded principal. **/
> +  SimpleTest.waitForExplicitFinish();
> +  const Cu = Components.utils;
> +  let global = Cu.getGlobalForObject.bind(Cu);

I don't think you need this.

@@ +33,5 @@
> +  }
> +
> +  ]]>
> +  </script>
> +  <iframe id="ifr" onload="go();" src="http://example.org/tests/js/xpconnect/tests/mochitest/file_empty.html" />

Why do you need the cross-domain iframe here? Seems like the sandboxPrototype should just use |window|. That way, you could verify the result of the localStorage write from the page context as well as from the Sandbox.

Given that localStorage is persistent across navigations (and across sessions) I think we need:
* Better namespacing than "test". how about test_localstorage_with_nsEp?
* To delete the value before calling SimpleTest.finish().

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #5)

Sorry, seems like I was a bit hasty again with this patch...

> > +  <iframe id="ifr" onload="go();" src="http://example.org/tests/js/xpconnect/tests/mochitest/file_empty.html" />
> 
> Why do you need the cross-domain iframe here? Seems like the
> sandboxPrototype should just use |window|. That way, you could verify the
> result of the localStorage write from the page context as well as from the
> Sandbox.

I don't understand. This is a chrome test, so the |window| is chrome right?
For the test I need a content window, and an nsEP sandbox that's why
I use the iframe.
 
> Given that localStorage is persistent across navigations (and across
> sessions) I think we need:
> * Better namespacing than "test". how about test_localstorage_with_nsEp?
> * To delete the value before calling SimpleTest.finish().

Thanks, this makes sense.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> (In reply to Bobby Holley (:bholley) from comment #5)
> 
> Sorry, seems like I was a bit hasty again with this patch...
> 
> > > +  <iframe id="ifr" onload="go();" src="http://example.org/tests/js/xpconnect/tests/mochitest/file_empty.html" />
> > 
> > Why do you need the cross-domain iframe here? Seems like the
> > sandboxPrototype should just use |window|. That way, you could verify the
> > result of the localStorage write from the page context as well as from the
> > Sandbox.
> 
> I don't understand. This is a chrome test, so the |window| is chrome right?
> For the test I need a content window, and an nsEP sandbox that's why
> I use the iframe.

Oh right. Sorry.

We should still also test localStorage from the chrome scope (via privileged Xrays) and the content scope (via eval) to make sure everything agrees.

Comment 8

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14e185bc3ef8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14e185bc3ef8