371127 - globalStorage not available to script running in chrome window opened from priviledged code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Xavier]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1 XPCOMViewer/0.9.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1) Gecko/20061010 Firefox/2.0

When I open a window using window.open with 'chrome=yes' from privileged code (in  a firefox extension), the script in the HTML document loaded in that window can't access globalStorage (not defined)

Opening the exact same html document with the exact same call to window.open but from unprivileged code (from the url bar for instance) works fine, globalStorage can be used.

Reproducible: Always

Steps to Reproduce:
1. Create a page containing a script that accesses globalStorage
2. open it with a window.open from privileged code and 'chrome=yes'
3. open it with the same call from unprivileged code
Actual Results:  
The first window opened will not be able to use globalStorage while the second window will be.

Expected Results:  
both windows' documents should be able to access globalStorage

Comment 1

[Martijn Wargers (dead)]

Yeah, this is a known issue, see comments in bug 357323.
I added a patch that potentially fixes this, but I'm not sure if it's a correct patch, the privilege checks are quite abundant in there.

Comment 2

[Xavier]

It seems a rather different issue to me, unless I did not get it:
Here we do not lack a domain name against which to perform the storage security checks.
Nor is it a lack of privilege restricting access: in both cases, the script that tries to access the storage is unprivileged.  But in the case where it does not succeed, the window was opened from privileged code...

Comment 3

[Martijn Wargers (dead)]

Sorry, yes, you are right. However the patch in that bug still might fix this too (or not).

Comment 4

[Neil Deakin]

Yes, this seems to be commented out.

http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#2461

Comment 5

[Neil Deakin]

Attachment: support storage from chrome

This patch:
 - enables global storage in chrome windows
 - chrome callers can access storages for all domains
 - fixes to allow jar usage (test: jar:http://xulplanet.com/ndeakin/tests/xts/storage/storage-test.jar!/global.html )
 - UniversalBrowserRead can be used to access storages of any domain
 - file:/// callers can only access storage when privileged

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 256197 [details] [diff] [review]
support storage from chrome

- In nsDOMStorage::CanUseStorage():

   if (!nsContentUtils::GetBoolPref(kStorageEnabled))
     return PR_FALSE;
 
+  // chrome can always use storage regardless of permission preferences
+  if (nsContentUtils::IsCallerChrome())
+    return PR_TRUE;

I wonder if we shouldn't move this check above the pref check? I.e. permit chrome callers to use DOM storage even if the pref is set to off? If not, and some chrome code really wants to use it they could simply flip the pref and then use it etc, and I don't think we want to see that happening in extensions or what not. Thoughts?

- In nsDOMStorage::SetDBValue():

+      nsCOMPtr<nsIURI> innerUri = NS_GetInnermostURI(uri);
+      if (innerUri) {

We should throw an error here if innerUri is null, i.e. flip the check and throw NS_ERROR_UNEXPECTED or somesuch. Same thing in nsDOMStorageList::NamedItem().

r=jst with that.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 256197 [details] [diff] [review]
support storage from chrome

>-  NS_ASSERTION(aURI && aSessionOnly, "null URI or session flag");
>+  NS_ASSERTION(aSessionOnly, "null URI or session flag");

ObNit: need to take reference to URI out of the assert message too

>   if (!nsContentUtils::GetBoolPref(kStorageEnabled))
>     return PR_FALSE;
> 
>+  // chrome can always use storage regardless of permission preferences
>+  if (nsContentUtils::IsCallerChrome())
>+    return PR_TRUE;

I think I disagree w/jst about swapping the order. Yes, chrome could always flip the pref to bypass it, but then that extension is explicitly disrespecting the user's wish. Users might have turned it off in response to a security alert, or a XULRunner app (minimo?) may have it turned off because there's no support for it.

>   PRUint32 perm;
>   permissionManager->TestPermission(aURI, kPermissionType, &perm);

TestPermission does not get the innermost URI. It probably should, but for now the cookie-permission check isn't going to work right for offline apps. Created bug 378685.

>       if (!nsDOMStorage::CanUseStorage(uri, &sessionOnly))
>         return NS_ERROR_DOM_SECURITY_ERR;
>-      
>-      rv = uri->GetAsciiHost(currentDomain);
>-      NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);
>+
>+      nsCOMPtr<nsIURI> innerUri = NS_GetInnermostURI(uri);

As a result you may want to get the innermostURI first before calling CanUseStorage. Or I guess just wait for the new bug to get fixed.

Agree w/jst about the innerUri checks.

sr=dveditz with those minor changes

Comment 8

[Neil Deakin]

Attachment: updated patch for checkin

Comment 9

[Neil Deakin]

Attachment: testcase

not sure if this is the right way to do this chrome testcase, but if tests persistent storage functionality for chrome.

Comment 10

[Neil Deakin]

Comment on attachment 262787 [details] [diff] [review]
testcase

Robert, is this the right way to do a chrome window test?

Comment 11

[Neil Deakin]

checked in the patch

Comment 12

[Robert Sayre]

Comment on attachment 262787 [details] [diff] [review]
testcase

You don't have to make a new window unless your test can't run inside the browser.