Fixing bug 495112. Partially revert the fix for bug 455070 to fix session storage regressions. r+sr=jst@mozilla.org
authorhonzab.moz@firemni.cz
Thu, 28 May 2009 16:15:26 -0700
changeset 28820 363750f510ec5a5e3975391591f1c60f939f1a10
parent 28819 8bbbfa16fe349d87077506715223f7bbb0a23b5c
child 28821 6b5508397a29a68a8563ad5c514dbbd122420a1b
push id7275
push userjst@mozilla.com
push dateThu, 28 May 2009 23:15:42 +0000
treeherdermozilla-central@363750f510ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs495112, 455070
milestone1.9.2a1pre
Fixing bug 495112. Partially revert the fix for bug 455070 to fix session storage regressions. r+sr=jst@mozilla.org
docshell/base/nsDocShell.cpp
dom/src/storage/nsDOMStorage.cpp
dom/src/storage/nsDOMStorage.h
dom/tests/mochitest/sessionstorage/Makefile.in
dom/tests/mochitest/sessionstorage/file_http.html
dom/tests/mochitest/sessionstorage/file_https.html
dom/tests/mochitest/sessionstorage/test_sessionStorageFromChrome.xhtml
dom/tests/mochitest/sessionstorage/test_sessionStorageHttpHttps.html
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -2135,16 +2135,44 @@ nsDocShell::HistoryPurged(PRInt32 aNumEn
         if (shell) {
             shell->HistoryPurged(aNumEntries);
         }
     }
 
     return NS_OK;
 }
 
+static
+nsresult
+GetPrincipalDomain(nsIPrincipal* aPrincipal, nsACString& aDomain)
+{
+  aDomain.Truncate();
+
+  nsCOMPtr<nsIURI> codebaseURI;
+  nsresult rv = aPrincipal->GetDomain(getter_AddRefs(codebaseURI));
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (!codebaseURI) {
+     rv = aPrincipal->GetURI(getter_AddRefs(codebaseURI));
+     NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  if (!codebaseURI)
+     return NS_OK;
+
+  nsCOMPtr<nsIURI> innerURI = NS_GetInnermostURI(codebaseURI);
+  NS_ASSERTION(innerURI, "Failed to get innermost URI");
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  rv = innerURI->GetAsciiHost(aDomain);
+  if (NS_FAILED(rv))
+      return rv;
+
+  return NS_OK;
+}
+
 NS_IMETHODIMP
 nsDocShell::GetSessionStorageForPrincipal(nsIPrincipal* aPrincipal,
                                           PRBool aCreate,
                                           nsIDOMStorage** aStorage)
 {
     NS_ENSURE_ARG_POINTER(aStorage);
     *aStorage = nsnull;
 
@@ -2161,36 +2189,38 @@ nsDocShell::GetSessionStorageForPrincipa
     if (!topItem)
         return NS_ERROR_FAILURE;
 
     nsDocShell* topDocShell = static_cast<nsDocShell*>(topItem.get());
     if (topDocShell != this)
         return topDocShell->GetSessionStorageForPrincipal(aPrincipal, aCreate,
                                                           aStorage);
 
-    nsXPIDLCString origin;
-    rv = aPrincipal->GetOrigin(getter_Copies(origin));
+    nsCAutoString currentDomain;
+    rv = GetPrincipalDomain(aPrincipal, currentDomain);
     if (NS_FAILED(rv))
         return rv;
 
-    if (origin.IsEmpty())
-        return NS_ERROR_FAILURE;
-
-    if (!mStorages.Get(origin, aStorage) && aCreate) {
+    if (currentDomain.IsEmpty())
+        return NS_OK;
+
+    if (!mStorages.Get(currentDomain, aStorage) && aCreate) {
         nsCOMPtr<nsIDOMStorage> newstorage =
             do_CreateInstance("@mozilla.org/dom/storage;2");
         if (!newstorage)
             return NS_ERROR_OUT_OF_MEMORY;
 
         nsCOMPtr<nsPIDOMStorage> pistorage = do_QueryInterface(newstorage);
         if (!pistorage)
             return NS_ERROR_FAILURE;
-        pistorage->InitAsSessionStorage(aPrincipal);
-
-        if (!mStorages.Put(origin, newstorage))
+        rv = pistorage->InitAsSessionStorage(aPrincipal);
+        if (NS_FAILED(rv))
+            return rv;
+
+        if (!mStorages.Put(currentDomain, newstorage))
             return NS_ERROR_OUT_OF_MEMORY;
 
         newstorage.swap(*aStorage);
         return NS_OK;
     }
 
     nsCOMPtr<nsPIDOMStorage> piStorage = do_QueryInterface(*aStorage);
     if (piStorage) {
@@ -2251,29 +2281,29 @@ nsDocShell::AddSessionStorage(nsIPrincip
     nsCOMPtr<nsIDocShellTreeItem> topItem;
     nsresult rv = GetSameTypeRootTreeItem(getter_AddRefs(topItem));
     if (NS_FAILED(rv))
         return rv;
 
     if (topItem) {
         nsCOMPtr<nsIDocShell> topDocShell = do_QueryInterface(topItem);
         if (topDocShell == this) {
-            nsXPIDLCString origin;
-            rv = aPrincipal->GetOrigin(getter_Copies(origin));
+            nsCAutoString currentDomain;
+            rv = GetPrincipalDomain(aPrincipal, currentDomain);
             if (NS_FAILED(rv))
                 return rv;
 
-            if (origin.IsEmpty())
+            if (currentDomain.IsEmpty())
                 return NS_ERROR_FAILURE;
 
             // Do not replace an existing session storage.
-            if (mStorages.GetWeak(origin))
+            if (mStorages.GetWeak(currentDomain))
                 return NS_ERROR_NOT_AVAILABLE;
 
-            if (!mStorages.Put(origin, aStorage))
+            if (!mStorages.Put(currentDomain, aStorage))
                 return NS_ERROR_OUT_OF_MEMORY;
         }
         else {
             return topDocShell->AddSessionStorage(aPrincipal, aStorage);
         }
     }
 
     return NS_OK;
--- a/dom/src/storage/nsDOMStorage.cpp
+++ b/dom/src/storage/nsDOMStorage.cpp
@@ -530,65 +530,60 @@ NS_NewDOMStorage2(nsISupports* aOuter, R
 }
 
 nsDOMStorage::nsDOMStorage()
   : mUseDB(PR_FALSE)
   , mSessionOnly(PR_TRUE)
   , mLocalStorage(PR_FALSE)
   , mItemsCached(PR_FALSE)
 {
+  mSecurityChecker = this;
   mItems.Init(8);
   if (nsDOMStorageManager::gStorageManager)
     nsDOMStorageManager::gStorageManager->AddToStoragesHash(this);
 }
 
-static PLDHashOperator
-CopyStorageItems(nsSessionStorageEntry* aEntry, void* userArg)
-{
-  nsDOMStorage* newstorage = static_cast<nsDOMStorage*>(userArg);
-
-  newstorage->SetItem(aEntry->GetKey(), aEntry->mItem->GetValueInternal());
-
-  if (aEntry->mItem->IsSecure()) {
-    newstorage->SetSecure(aEntry->GetKey(), PR_TRUE);
-  }
-
-  return PL_DHASH_NEXT;
-}
-
 nsDOMStorage::nsDOMStorage(nsDOMStorage& aThat)
   : mUseDB(PR_FALSE) // Any clone is not using the database
   , mSessionOnly(PR_TRUE)
   , mLocalStorage(PR_FALSE) // Any clone is not a localStorage
   , mItemsCached(PR_FALSE)
   , mDomain(aThat.mDomain)
 #ifdef MOZ_STORAGE
   , mScopeDBKey(aThat.mScopeDBKey)
 #endif
 {
+  mSecurityChecker = this;
   mItems.Init(8);
-  aThat.mItems.EnumerateEntries(CopyStorageItems, this);
 
   if (nsDOMStorageManager::gStorageManager)
     nsDOMStorageManager::gStorageManager->AddToStoragesHash(this);
 }
 
 nsDOMStorage::~nsDOMStorage()
 {
   if (nsDOMStorageManager::gStorageManager)
     nsDOMStorageManager::gStorageManager->RemoveFromStoragesHash(this);
 }
 
 static
 nsresult
-GetDomainURI(nsIPrincipal *aPrincipal, nsIURI **_domain)
+GetDomainURI(nsIPrincipal *aPrincipal, PRBool aIncludeDomain, nsIURI **_domain)
 {
   nsCOMPtr<nsIURI> uri;
-  nsresult rv = aPrincipal->GetURI(getter_AddRefs(uri));
-  NS_ENSURE_SUCCESS(rv, rv);
+
+  if (aIncludeDomain) {
+    nsresult rv = aPrincipal->GetDomain(getter_AddRefs(uri));
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  if (!uri) {
+    nsresult rv = aPrincipal->GetURI(getter_AddRefs(uri));
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
 
   // Check if we really got any URI. System principal doesn't return a URI
   // instance and we would crash in NS_GetInnermostURI below.
   if (!uri)
     return NS_ERROR_NOT_AVAILABLE;
 
   nsCOMPtr<nsIURI> innerURI = NS_GetInnermostURI(uri);
   if (!innerURI)
@@ -597,17 +592,17 @@ GetDomainURI(nsIPrincipal *aPrincipal, n
 
   return NS_OK;
 }
 
 nsresult
 nsDOMStorage::InitAsSessionStorage(nsIPrincipal *aPrincipal)
 {
   nsCOMPtr<nsIURI> domainURI;
-  nsresult rv = GetDomainURI(aPrincipal, getter_AddRefs(domainURI));
+  nsresult rv = GetDomainURI(aPrincipal, PR_TRUE, getter_AddRefs(domainURI));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // No need to check for a return value. If this would fail we would not get
   // here as we call GetPrincipalURIAndHost (nsDOMStorage.cpp:88) from
   // nsDOMStorage::CanUseStorage before we query the storage manager for a new
   // sessionStorage. It calls GetAsciiHost on innermost URI. If it fails, we
   // won't get to InitAsSessionStorage.
   domainURI->GetAsciiHost(mDomain);
@@ -619,17 +614,17 @@ nsDOMStorage::InitAsSessionStorage(nsIPr
 #endif
   return NS_OK;
 }
 
 nsresult
 nsDOMStorage::InitAsLocalStorage(nsIPrincipal *aPrincipal)
 {
   nsCOMPtr<nsIURI> domainURI;
-  nsresult rv = GetDomainURI(aPrincipal, getter_AddRefs(domainURI));
+  nsresult rv = GetDomainURI(aPrincipal, PR_FALSE, getter_AddRefs(domainURI));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // No need to check for a return value. If this would fail we would not get
   // here as we call GetPrincipalURIAndHost (nsDOMStorage.cpp:88) from
   // nsDOMStorage::CanUseStorage before we query the storage manager for a new
   // localStorage. It calls GetAsciiHost on innermost URI. If it fails, we won't
   // get to InitAsLocalStorage. Actually, mDomain will get replaced with
   // mPrincipal in bug 455070. It is not even used for localStorage.
@@ -665,16 +660,37 @@ nsDOMStorage::InitAsGlobalStorage(const 
   if (!(mUseDB = !mScopeDBKey.IsEmpty()))
     mScopeDBKey.AppendLiteral(":");
 
   nsDOMStorageDBWrapper::CreateQuotaDomainDBKey(aDomainDemanded, PR_TRUE, mQuotaDomainDBKey);
 #endif
   return NS_OK;
 }
 
+static PLDHashOperator
+CopyStorageItems(nsSessionStorageEntry* aEntry, void* userArg)
+{
+  nsDOMStorage* newstorage = static_cast<nsDOMStorage*>(userArg);
+
+  newstorage->SetItem(aEntry->GetKey(), aEntry->mItem->GetValueInternal());
+
+  if (aEntry->mItem->IsSecure()) {
+    newstorage->SetSecure(aEntry->GetKey(), PR_TRUE);
+  }
+
+  return PL_DHASH_NEXT;
+}
+
+nsresult
+nsDOMStorage::CloneFrom(nsDOMStorage* aThat)
+{
+  aThat->mItems.EnumerateEntries(CopyStorageItems, this);
+  return NS_OK;
+}
+
 //static
 PRBool
 nsDOMStorage::CanUseStorage(PRPackedBool* aSessionOnly)
 {
   // check if the calling domain can use storage. Downgrade to session
   // only if only session storage may be used.
   NS_ASSERTION(aSessionOnly, "null session flag");
   *aSessionOnly = PR_FALSE;
@@ -745,17 +761,18 @@ nsDOMStorage::CacheStoragePermissions()
 
   nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
   if (!ssm)
     return PR_FALSE;
 
   nsCOMPtr<nsIPrincipal> subjectPrincipal;
   ssm->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
 
-  return CanAccess(subjectPrincipal);
+  NS_ASSERTION(mSecurityChecker, "Has non-null mSecurityChecker");
+  return mSecurityChecker->CanAccess(subjectPrincipal);
 }
 
 
 class ItemCounterState
 {
 public:
   ItemCounterState(PRBool aIsCallerSecure)
     : mIsCallerSecure(aIsCallerSecure), mCount(0)
@@ -1399,37 +1416,41 @@ NS_INTERFACE_MAP_END
 
 nsDOMStorage2::nsDOMStorage2()
 {
 }
 
 nsDOMStorage2::nsDOMStorage2(nsDOMStorage2& aThat)
 {
   mStorage = new nsDOMStorage(*aThat.mStorage.get());
+  mStorage->mSecurityChecker = mStorage;
   mPrincipal = aThat.mPrincipal;
 }
 
 nsresult
 nsDOMStorage2::InitAsSessionStorage(nsIPrincipal *aPrincipal)
 {
   mStorage = new nsDOMStorage();
   if (!mStorage)
     return NS_ERROR_OUT_OF_MEMORY;
 
+  // Leave security checks only for domain (nsDOMStorage implementation)
+  mStorage->mSecurityChecker = mStorage;
   mPrincipal = aPrincipal;
   return mStorage->InitAsSessionStorage(aPrincipal);
 }
 
 nsresult
 nsDOMStorage2::InitAsLocalStorage(nsIPrincipal *aPrincipal)
 {
   mStorage = new nsDOMStorage();
   if (!mStorage)
     return NS_ERROR_OUT_OF_MEMORY;
 
+  mStorage->mSecurityChecker = this;
   mPrincipal = aPrincipal;
   return mStorage->InitAsLocalStorage(aPrincipal);
 }
 
 nsresult
 nsDOMStorage2::InitAsGlobalStorage(const nsACString &aDomainDemanded)
 {
   NS_ASSERTION(PR_FALSE, "Should not initialize nsDOMStorage2 as global storage.");
@@ -1438,16 +1459,17 @@ nsDOMStorage2::InitAsGlobalStorage(const
 
 already_AddRefed<nsIDOMStorage>
 nsDOMStorage2::Clone()
 {
   nsDOMStorage2* storage = new nsDOMStorage2(*this);
   if (!storage)
     return nsnull;
 
+  storage->mStorage->CloneFrom(mStorage);
   NS_ADDREF(storage);
 
   return storage;
 }
 
 nsTArray<nsString> *
 nsDOMStorage2::GetKeys()
 {
@@ -1458,16 +1480,19 @@ nsIPrincipal*
 nsDOMStorage2::Principal()
 {
   return mPrincipal;
 }
 
 PRBool
 nsDOMStorage2::CanAccess(nsIPrincipal *aPrincipal)
 {
+  if (mStorage->mSecurityChecker != this)
+    return mStorage->mSecurityChecker->CanAccess(aPrincipal);
+
   // Allow C++ callers to access the storage
   if (!aPrincipal)
     return PR_TRUE;
 
   // Allow more powerful principals (e.g. system) to access the storage
   PRBool subsumes;
   nsresult rv = aPrincipal->Subsumes(mPrincipal, &subsumes);
   if (NS_FAILED(rv))
--- a/dom/src/storage/nsDOMStorage.h
+++ b/dom/src/storage/nsDOMStorage.h
@@ -188,16 +188,19 @@ public:
 
   // set the value corresponding to a key as secure.
   nsresult
   SetSecure(const nsAString& aKey, PRBool aSecure);
 
   // clear all values from the store
   void ClearAll();
 
+  nsresult
+  CloneFrom(nsDOMStorage* aThat);
+
   nsIDOMStorageItem* GetNamedItem(const nsAString& aKey, nsresult* aResult);
 
   static nsDOMStorage* FromSupports(nsISupports* aSupports)
   {
     return static_cast<nsDOMStorage*>(static_cast<nsIDOMStorageObsolete*>(aSupports));
   }
 
 protected:
@@ -239,16 +242,19 @@ protected:
   // the key->value item pairs
   nsTHashtable<nsSessionStorageEntry> mItems;
 
   // keys are used for database queries.
   // see comments of the getters bellow.
   nsCString mScopeDBKey;
   nsCString mQuotaDomainDBKey;
 
+  friend class nsIDOMStorage2;
+  nsPIDOMStorage* mSecurityChecker;
+
 public:
   // e.g. "moc.rab.oof.:" or "moc.rab.oof.:http:80" depending
   // on association with a domain (globalStorage) or
   // an origin (localStorage).
   nsCString& GetScopeDBKey() {return mScopeDBKey;}
 
   // e.g. "moc.rab.%" - reversed eTLD+1 subpart of the domain or
   // (in future) reversed offline application allowed domain.
--- a/dom/tests/mochitest/sessionstorage/Makefile.in
+++ b/dom/tests/mochitest/sessionstorage/Makefile.in
@@ -44,19 +44,22 @@ relativesrcdir	= dom/tests/mochitest/ses
 include $(DEPTH)/config/autoconf.mk
 
 include $(topsrcdir)/config/rules.mk
 
 _TEST_FILES	= \
     frameReplace.html \
     frameEqual.html \
     frameNotEqual.html \
+    file_http.html \
+    file_https.html \
     test_sessionStorageBase.html \
     test_sessionStorageClone.html \
     test_sessionStorageReplace.html \
+    test_sessionStorageHttpHttps.html \
     interOriginSlave.js \
     interOriginTest.js \
     $(NULL)
 
 _CHROME_FILES = \
     test_sessionStorageFromChrome.xhtml \
     $(NULL)
 
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/sessionstorage/file_http.html
@@ -0,0 +1,40 @@
+<html>
+<head>
+<script>
+
+window.addEventListener("message", onMessageReceived, false);
+
+function postMsg(msg)
+{
+  parent.postMessage(msg, "http://localhost:8888");
+}
+
+function onMessageReceived(event)
+{
+  if (event.data == "check") {
+    postMsg(sessionStorage.getItem("foo"));
+
+    var gotValue = "threw";
+    try {
+      gotValue = sessionStorage.getItem("foo-https");
+    } catch (e) {
+    }
+
+    postMsg(gotValue);
+
+    postMsg("the end");
+  }
+}
+
+function start()
+{
+  sessionStorage.setItem("foo", "insecure");
+  postMsg(sessionStorage.getItem("foo"));
+}
+
+</script>
+</head>
+<body onload="start();">
+insecure
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/sessionstorage/file_https.html
@@ -0,0 +1,15 @@
+<html>
+<head>
+<script>
+function start()
+{
+  sessionStorage.setItem("foo-https", "secure");
+  parent.postMessage(sessionStorage.getItem("foo-https"),
+                     "http://localhost:8888");
+}
+</script>
+</head>
+<body onload="start();">
+secure
+</body>
+</html>
--- a/dom/tests/mochitest/sessionstorage/test_sessionStorageFromChrome.xhtml
+++ b/dom/tests/mochitest/sessionstorage/test_sessionStorageFromChrome.xhtml
@@ -12,21 +12,21 @@ function startTest()
 {
   // Check that we do not crash when we access the sessionStorage object from
   // chrome and that we throw.  See bug 404453.
   var exceptionCaught = false;
   try {
     sessionStorage;
   }
   catch (e) {
-    is(e.result, Components.results.NS_ERROR_NOT_AVAILABLE,
+    is(e.result, 2152923145,
        "Testing that we get the expected exception.");
     exceptionCaught = true;
   }
-  todo_is(exceptionCaught, true, "Testing that an exception was thrown.");
+  is(exceptionCaught, true, "Testing that an exception was thrown.");
 
   SimpleTest.finish();
 }
 
 </script>
 
 </head>
 
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/sessionstorage/test_sessionStorageHttpHttps.html
@@ -0,0 +1,60 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head>
+<title>sessionStorage replace test</title>
+
+<!--
+  This test checks that sessionStorage values set in an https page
+  are not readable from a non-https page from the same domain.
+-->
+
+<script type="text/javascript" src="/MochiKit/packed.js"></script>
+<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+
+<script type="text/javascript">
+
+window.addEventListener("message", onMessageReceived, false);
+
+var messages = [];
+
+function onMessageReceived(event)
+{
+  if (event.data == "the end") {
+    is(messages.length, 4, "Wrong number of messages.");
+    is(messages[0], "insecure", "Wrong message from insecure page");
+    is(messages[1], "secure", "Wrong message from secure page");
+    is(messages[2], "insecure", "Wrong second message from insecure page");
+    is(messages[3], null, "Insecure page got secure message?");
+
+    SimpleTest.finish();
+
+    return;
+  }
+
+  messages.push(event.data);
+
+  if (event.data == "insecure" && messages.length == 1) {
+    window.httpsframe.location = "https://test1.example.com/tests/dom/tests/mochitest/sessionstorage/file_https.html";
+  }
+
+  if (event.data == "secure") {
+    window.httpframe.postMessage("check", "http://test1.example.com");
+  }
+}
+
+function startTest()
+{
+  window.httpframe.location = "http://test1.example.com/tests/dom/tests/mochitest/sessionstorage/file_http.html";
+}
+
+SimpleTest.waitForExplicitFinish();
+
+</script>
+
+</head>
+
+<body onload="startTest();">
+  <iframe src="" name="httpframe"></iframe>
+  <iframe src="" name="httpsframe"></iframe>
+</body>
+</html>