Bug 407845 - "nsCookieService::FindCookie() shouldn't return expired cookies" [p=dwitte@stanford.edu (Dan Witte [dwitte]) r+sr=biesi a1.9=damons]

netwerk/cookie/src/nsCookieService.cpp

@@ -859,21 +859,30 @@ nsCookieService::Remove(const nsACString &aHost,
   if (FindCookie(PromiseFlatCString(aHost),
                  PromiseFlatCString(aName),
                  PromiseFlatCString(aPath),
-                 matchIter)) {
+                 matchIter,
+                 PR_Now() / PR_USEC_PER_SEC)) {
     nsRefPtr<nsCookie> cookie = matchIter.current;
     RemoveCookieFromList(matchIter);
     NotifyChanged(cookie, NS_LITERAL_STRING("deleted").get());
-
-    // check if we need to add the host to the permissions blacklist.
-    if (aBlocked && mPermissionService) {
-      nsCAutoString host(NS_LITERAL_CSTRING("http://") + cookie->RawHost());
-      nsCOMPtr<nsIURI> uri;
-      NS_NewURI(getter_AddRefs(uri), host);
-
-      if (uri)
-        mPermissionService->SetAccess(uri, nsICookiePermission::ACCESS_DENY);
-    }
   }
+
+  // check if we need to add the host to the permissions blacklist.
+  if (aBlocked && mPermissionService) {
+    nsCAutoString host(NS_LITERAL_CSTRING("http://"));
+    
+    // strip off the domain dot, if necessary
+    if (!aHost.IsEmpty() && aHost.First() == '.')
+      host.Append(Substring(aHost, 1, aHost.Length() - 1));
+    else
+      host.Append(aHost);
+
+    nsCOMPtr<nsIURI> uri;
+    NS_NewURI(getter_AddRefs(uri), host);
+
+    if (uri)
+      mPermissionService->SetAccess(uri, nsICookiePermission::ACCESS_DENY);
+  }
+
   return NS_OK;
 }
 
@@ -1376,8 +1385,8 @@ nsCookieService::AddInternal(nsCookie   *aCookie,
   mozStorageTransaction transaction(mDBConn, PR_TRUE);
 
   nsListIter matchIter;
-  const PRBool foundCookie =
+  PRBool foundCookie = FindCookie(aCookie->Host(), aCookie->Name(), aCookie->Path(),
-    FindCookie(aCookie->Host(), aCookie->Name(), aCookie->Path(), matchIter);
+                                  matchIter, aCurrentTime);
 
   nsRefPtr<nsCookie> oldCookie;
   if (foundCookie) {
@@ -2024,10 +2033,11 @@ nsCookieService::CookieExists(nsICookie2 *aCookie,
   NS_ENSURE_ARG_POINTER(aCookie);
 
   // just a placeholder
-  nsEnumerationData data(PR_Now() / PR_USEC_PER_SEC, LL_MININT);
+  nsListIter iter;
   nsCookie *cookie = static_cast<nsCookie*>(aCookie);
 
-  *aFoundCookie = FindCookie(cookie->Host(), cookie->Name(), cookie->Path(), data.iter);
+  *aFoundCookie = FindCookie(cookie->Host(), cookie->Name(), cookie->Path(),
+                             iter, PR_Now() / PR_USEC_PER_SEC);
   return NS_OK;
 }
 
@@ -2080,16 +2090,18 @@ nsCookieService::CountCookiesFromHost(const nsACString &aHost,
   return NS_OK;
 }
 
-// find an exact previous match.
+// find an exact cookie specified by host, name, and path that hasn't expired.
 PRBool
 nsCookieService::FindCookie(const nsAFlatCString &aHost,
                             const nsAFlatCString &aName,
                             const nsAFlatCString &aPath,
-                            nsListIter           &aIter)
+                            nsListIter           &aIter,
+                            PRInt64               aCurrentTime)
 {
   nsCookieEntry *entry = mHostTable.GetEntry(aHost.get());
   for (aIter = nsListIter(entry); aIter.current; ++aIter) {
-    if (aPath.Equals(aIter.current->Path()) &&
+    if (aIter.current->Expiry() > aCurrentTime &&
+        aPath.Equals(aIter.current->Path()) &&
         aName.Equals(aIter.current->Name())) {
       return PR_TRUE;
     }

netwerk/cookie/src/nsCookieService.h

@@ -181,7 +181,7 @@ class nsCookieService : public nsICookieService
     static PRBool                 GetExpiry(nsCookieAttributes &aCookie, PRInt64 aServerTime, PRInt64 aCurrentTime);
     void                          RemoveAllFromMemory();
     void                          RemoveExpiredCookies(PRInt64 aCurrentTime);
-    PRBool                        FindCookie(const nsAFlatCString &aHost, const nsAFlatCString &aName, const nsAFlatCString &aPath, nsListIter &aIter);
+    PRBool                        FindCookie(const nsAFlatCString &aHost, const nsAFlatCString &aName, const nsAFlatCString &aPath, nsListIter &aIter, PRInt64 aCurrentTime);
     void                          FindOldestCookie(nsEnumerationData &aData);
     PRUint32                      CountCookiesFromHostInternal(const nsACString &aHost, nsEnumerationData &aData);
     void                          NotifyRejected(nsIURI *aHostURI);

netwerk/test/TestCookie.cpp

@@ -1,4 +1,4 @@
-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
@@ -659,7 +659,7 @@ main(PRInt32 argc, char *argv[])
                                            PR_FALSE,                             // is secure
                                            PR_TRUE,                              // is httponly
                                            PR_TRUE,                              // is session
-                                           LL_MAXINT));                          // expiry time
+                                           PR_Now() / PR_USEC_PER_SEC + 2));     // expiry time
       rv[3] = NS_SUCCEEDED(cookieMgr2->Add(NS_LITERAL_CSTRING("new.domain"),     // domain
                                            NS_LITERAL_CSTRING("/rabbit"),        // path
                                            NS_LITERAL_CSTRING("test3"),          // name
@@ -673,18 +673,20 @@ main(PRInt32 argc, char *argv[])
       rv[4] = NS_SUCCEEDED(cookieMgr->GetEnumerator(getter_AddRefs(enumerator)));
       PRInt32 i = 0;
       PRBool more;
-      nsCOMPtr<nsICookie2> newDomainCookie;
+      nsCOMPtr<nsICookie2> expiredCookie, newDomainCookie;
       while (NS_SUCCEEDED(enumerator->HasMoreElements(&more)) && more) {
         nsCOMPtr<nsISupports> cookie;
         if (NS_FAILED(enumerator->GetNext(getter_AddRefs(cookie)))) break;
         ++i;
         
-        // keep tabs on the third cookie, so we can check it later
+        // keep tabs on the second and third cookies, so we can check them later
         nsCOMPtr<nsICookie2> cookie2(do_QueryInterface(cookie));
         if (!cookie2) break;
-        nsCAutoString domain;
+        nsCAutoString name;
-        cookie2->GetRawHost(domain);
+        cookie2->GetName(name);
-        if (domain == NS_LITERAL_CSTRING("new.domain"))
+        if (name == NS_LITERAL_CSTRING("test2"))
+          expiredCookie = cookie2;
+        else if (name == NS_LITERAL_CSTRING("test3"))
           newDomainCookie = cookie2;
       }
       rv[5] = i == 3;
@@ -715,13 +717,19 @@ main(PRInt32 argc, char *argv[])
                                             PR_TRUE,                              // is session
                                             LL_MININT));                          // expiry time
       rv[13] = NS_SUCCEEDED(cookieMgr2->CookieExists(newDomainCookie, &found)) && !found;
+      // sleep four seconds, to make sure the second cookie has expired
+      PR_Sleep(4 * PR_TicksPerSecond());
+      // check CountCookiesFromHost() and CookieExists() don't count the expired cookie
+      rv[14] = NS_SUCCEEDED(cookieMgr2->CountCookiesFromHost(NS_LITERAL_CSTRING("cookiemgr.test"), &hostCookies)) &&
+              hostCookies == 1;
+      rv[15] = NS_SUCCEEDED(cookieMgr2->CookieExists(expiredCookie, &found)) && !found;
       // double-check RemoveAll() using the enumerator
-      rv[14] = NS_SUCCEEDED(cookieMgr->RemoveAll());
+      rv[16] = NS_SUCCEEDED(cookieMgr->RemoveAll());
-      rv[15] = NS_SUCCEEDED(cookieMgr->GetEnumerator(getter_AddRefs(enumerator))) &&
+      rv[17] = NS_SUCCEEDED(cookieMgr->GetEnumerator(getter_AddRefs(enumerator))) &&
                NS_SUCCEEDED(enumerator->HasMoreElements(&more)) &&
                !more;
 
-      allTestsPassed = PrintResult(rv, 16) && allTestsPassed;
+      allTestsPassed = PrintResult(rv, 18) && allTestsPassed;
 
 
       // *** eviction and creation ordering tests