Closed Bug 371127 Opened 13 years ago Closed 13 years ago

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

Categories

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

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha4

People

(Reporter: xavier, Assigned: enndeakin)

References

Details

Attachments

(3 files)

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
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.
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...
Sorry, yes, you are right. However the patch in that bug still might fix this too (or not).
Yes, this seems to be commented out.

http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#2461
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
Assignee: general → enndeakin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #256197 - Flags: superreview?(dveditz)
Attachment #256197 - Flags: review?(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.
Attachment #256197 - Flags: review?(jst) → review+
Flags: blocking1.9?
Depends on: 378685
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
Attachment #256197 - Flags: superreview?(dveditz) → superreview+
Attached patch testcaseSplinter Review
not sure if this is the right way to do this chrome testcase, but if tests persistent storage functionality for chrome.
Comment on attachment 262787 [details] [diff] [review]
testcase

Robert, is this the right way to do a chrome window test?
Attachment #262787 - Flags: review?(sayrer)
checked in the patch
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
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.
Attachment #262787 - Flags: review?(sayrer) → review+
Flags: in-testsuite? → in-testsuite+
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9alpha4
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.