Bug 1302824 - Avoid duplicating domain-matching and path-matching logic in cookie service. r=ehsan, a=ritu
authorJosh Matthews <josh@joshmatthews.net>
Mon, 19 Sep 2016 15:40:12 -0400
changeset 340150 c4b72a980594
parent 340149 f2f356aa4228
child 340151 e77a60c374ed
push id10049
push userryanvm@gmail.com
push date2016-09-22 01:56 +0000
treeherdermozilla-aurora@e77a60c374ed [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan, ritu
bugs1302824
milestone51.0a2
Bug 1302824 - Avoid duplicating domain-matching and path-matching logic in cookie service. r=ehsan, a=ritu
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/test/unit/test_eviction.js
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -3034,16 +3034,57 @@ public:
     // when path lengths match, older cookies should be listed first.  this is
     // required for backwards compatibility since some websites erroneously
     // depend on receiving cookies in the order in which they were sent to the
     // browser!  see bug 236772.
     return aCookie1->CreationTime() < aCookie2->CreationTime();
   }
 };
 
+static bool
+DomainMatches(nsCookie* aCookie, const nsACString& aHost) {
+  // first, check for an exact host or domain cookie match, e.g. "google.com"
+  // or ".google.com"; second a subdomain match, e.g.
+  // host = "mail.google.com", cookie domain = ".google.com".
+  return aCookie->RawHost() == aHost ||
+      (aCookie->IsDomain() && StringEndsWith(aHost, aCookie->Host()));
+}
+
+static bool
+PathMatches(nsCookie* aCookie, const nsACString& aPath) {
+  // calculate cookie path length, excluding trailing '/'
+  uint32_t cookiePathLen = aCookie->Path().Length();
+  if (cookiePathLen > 0 && aCookie->Path().Last() == '/')
+    --cookiePathLen;
+
+  // if the given path is shorter than the cookie path, it doesn't match
+  // if the given path doesn't start with the cookie path, it doesn't match.
+  if (!StringBeginsWith(aPath, Substring(aCookie->Path(), 0, cookiePathLen)))
+    return false;
+
+  // if the given path is longer than the cookie path, and the first char after
+  // the cookie path is not a path delimiter, it doesn't match.
+  if (aPath.Length() > cookiePathLen &&
+      !ispathdelimiter(aPath.CharAt(cookiePathLen))) {
+    /*
+     * |ispathdelimiter| tests four cases: '/', '?', '#', and ';'.
+     * '/' is the "standard" case; the '?' test allows a site at host/abc?def
+     * to receive a cookie that has a path attribute of abc.  this seems
+     * strange but at least one major site (citibank, bug 156725) depends
+     * on it.  The test for # and ; are put in to proactively avoid problems
+     * with other sites - these are the only other chars allowed in the path.
+     */
+    return false;
+  }
+
+  // either the paths match exactly, or the cookie path is a prefix of
+  // the given path.
+  return true;
+}
+
 void
 nsCookieService::GetCookieStringInternal(nsIURI *aHostURI,
                                          bool aIsForeign,
                                          bool aHttpBound,
                                          const NeckoOriginAttributes aOriginAttrs,
                                          bool aIsPrivate,
                                          nsCString &aCookieString)
 {
@@ -3114,54 +3155,32 @@ nsCookieService::GetCookieStringInternal
     return;
 
   // iterate the cookies!
   const nsCookieEntry::ArrayType &cookies = entry->GetCookies();
   for (nsCookieEntry::IndexType i = 0; i < cookies.Length(); ++i) {
     cookie = cookies[i];
 
     // check the host, since the base domain lookup is conservative.
-    // first, check for an exact host or domain cookie match, e.g. "google.com"
-    // or ".google.com"; second a subdomain match, e.g.
-    // host = "mail.google.com", cookie domain = ".google.com".
-    if (cookie->RawHost() != hostFromURI &&
-        !(cookie->IsDomain() && StringEndsWith(hostFromURI, cookie->Host())))
+    if (!DomainMatches(cookie, hostFromURI))
       continue;
 
     // if the cookie is secure and the host scheme isn't, we can't send it
     if (cookie->IsSecure() && !isSecure)
       continue;
 
     // if the cookie is httpOnly and it's not going directly to the HTTP
     // connection, don't send it
     if (cookie->IsHttpOnly() && !aHttpBound)
       continue;
 
-    // calculate cookie path length, excluding trailing '/'
-    uint32_t cookiePathLen = cookie->Path().Length();
-    if (cookiePathLen > 0 && cookie->Path().Last() == '/')
-      --cookiePathLen;
-
-    // if the nsIURI path is shorter than the cookie path, don't send it back
-    if (!StringBeginsWith(pathFromURI, Substring(cookie->Path(), 0, cookiePathLen)))
+    // if the nsIURI path doesn't match the cookie path, don't send it back
+    if (!PathMatches(cookie, pathFromURI))
       continue;
 
-    if (pathFromURI.Length() > cookiePathLen &&
-        !ispathdelimiter(pathFromURI.CharAt(cookiePathLen))) {
-      /*
-       * |ispathdelimiter| tests four cases: '/', '?', '#', and ';'.
-       * '/' is the "standard" case; the '?' test allows a site at host/abc?def
-       * to receive a cookie that has a path attribute of abc.  this seems
-       * strange but at least one major site (citibank, bug 156725) depends
-       * on it.  The test for # and ; are put in to proactively avoid problems
-       * with other sites - these are the only other chars allowed in the path.
-       */
-      continue;
-    }
-
     // check if the cookie has expired
     if (cookie->Expiry() <= currentTime) {
       continue;
     }
 
     // all checks passed - add to list and check if lastAccessed stamp needs updating
     foundCookieList.AppendElement(cookie);
     if (cookie->IsStale()) {
@@ -4009,36 +4028,44 @@ nsCookieService::CheckDomain(nsCookieAtt
     return false;
   }
 
   // no domain specified, use hostFromURI
   aCookieAttributes.host = hostFromURI;
   return true;
 }
 
+nsCString
+GetPathFromURI(nsIURI* aHostURI)
+{
+  // strip down everything after the last slash to get the path,
+  // ignoring slashes in the query string part.
+  // if we can QI to nsIURL, that'll take care of the query string portion.
+  // otherwise, it's not an nsIURL and can't have a query string, so just find the last slash.
+  nsAutoCString path;
+  nsCOMPtr<nsIURL> hostURL = do_QueryInterface(aHostURI);
+  if (hostURL) {
+    hostURL->GetDirectory(path);
+  } else {
+    aHostURI->GetPath(path);
+    int32_t slash = path.RFindChar('/');
+    if (slash != kNotFound) {
+      path.Truncate(slash + 1);
+    }
+  }
+  return path;
+}
+
 bool
 nsCookieService::CheckPath(nsCookieAttributes &aCookieAttributes,
                            nsIURI             *aHostURI)
 {
   // if a path is given, check the host has permission
   if (aCookieAttributes.path.IsEmpty() || aCookieAttributes.path.First() != '/') {
-    // strip down everything after the last slash to get the path,
-    // ignoring slashes in the query string part.
-    // if we can QI to nsIURL, that'll take care of the query string portion.
-    // otherwise, it's not an nsIURL and can't have a query string, so just find the last slash.
-    nsCOMPtr<nsIURL> hostURL = do_QueryInterface(aHostURI);
-    if (hostURL) {
-      hostURL->GetDirectory(aCookieAttributes.path);
-    } else {
-      aHostURI->GetPath(aCookieAttributes.path);
-      int32_t slash = aCookieAttributes.path.RFindChar('/');
-      if (slash != kNotFound) {
-        aCookieAttributes.path.Truncate(slash + 1);
-      }
-    }
+    aCookieAttributes.path = GetPathFromURI(aHostURI);
 
 #if 0
   } else {
     /**
      * The following test is part of the RFC2109 spec.  Loosely speaking, it says that a site
      * cannot set a cookie for a path that it is not on.  See bug 155083.  However this patch
      * broke several sites -- nordea (bug 155768) and citibank (bug 156725).  So this test has
      * been disabled, unless we can evangelize these sites.
@@ -4364,17 +4391,17 @@ nsCookieService::FindStaleCookie(nsCooki
                                  nsIURI* aSource,
                                  nsListIter &aIter)
 {
   bool requireHostMatch = true;
   nsAutoCString baseDomain, sourceHost, sourcePath;
   if (aSource) {
     GetBaseDomain(aSource, baseDomain, requireHostMatch);
     aSource->GetAsciiHost(sourceHost);
-    aSource->GetPath(sourcePath);
+    sourcePath = GetPathFromURI(aSource);
   }
 
   const nsCookieEntry::ArrayType &cookies = aEntry->GetCookies();
 
   int64_t oldestNonMatchingSessionCookieTime = 0;
   nsListIter oldestNonMatchingSessionCookie;
   oldestNonMatchingSessionCookie.entry = nullptr;
 
@@ -4400,29 +4427,22 @@ nsCookieService::FindStaleCookie(nsCooki
       return;
     }
 
     // Update our various records of oldest cookies fitting several restrictions:
     // * session cookies
     // * non-session cookies
     // * cookies with paths and domains that don't match the cookie triggering this purge
 
-    uint32_t cookiePathLen = cookie->Path().Length();
-    if (cookiePathLen > 0 && cookie->Path().Last() == '/')
-      --cookiePathLen;
-
     // This cookie is a candidate for eviction if we have no information about
     // the source request, or if it is not a path or domain match against the
     // source request.
     bool isPrimaryEvictionCandidate = true;
     if (aSource) {
-      bool pathMatches = StringBeginsWith(sourcePath, Substring(cookie->Path(), 0, cookiePathLen));
-      bool domainMatches = cookie->RawHost() == sourceHost ||
-          (cookie->IsDomain() && StringEndsWith(sourceHost, cookie->Host()));
-      isPrimaryEvictionCandidate = !pathMatches || !domainMatches;
+      isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost);
     }
 
     int64_t lastAccessed = cookie->LastAccessed();
     if (cookie->IsSession()) {
       if (!oldestSessionCookie.entry || oldestSessionCookieTime > lastAccessed) {
         oldestSessionCookieTime = lastAccessed;
         oldestSessionCookie.entry = aEntry;
         oldestSessionCookie.index = i;
--- a/netwerk/cookie/test/unit/test_eviction.js
+++ b/netwerk/cookie/test/unit/test_eviction.js
@@ -28,21 +28,43 @@ function run_test() {
             var t = tests.splice(0, 1)[0];
             do_print('testing with host ' + t[0]);
             yield t[1]();
             cm.removeAll();
         });
     }
     add_task(function*() {
         yield test_localdomain();
+        cm.removeAll();
+    });
+
+    add_task(function*() {
+        yield test_path_prefix();
     });
 
     run_next_test();
 }
 
+// Verify that cookies that share a path prefix with the URI path are still considered
+// candidates for eviction, since the paths do not actually match.
+function* test_path_prefix() {
+    Services.prefs.setIntPref("network.cookie.maxPerHost", 2);
+
+    const BASE_URI = Services.io.newURI("http://example.org/", null, null);
+    const BASE_BAR = Services.io.newURI("http://example.org/bar/", null, null);
+    const BASE_BARBAR = Services.io.newURI("http://example.org/barbar/", null, null);
+
+    yield setCookie("session_first", null, null, null, BASE_URI);
+    yield setCookie("session_second", null, "/bar", null, BASE_BAR);
+    verifyCookies(['session_first', 'session_second'], BASE_URI);
+
+    yield setCookie("session_third", null, "/barbar", null, BASE_BARBAR);
+    verifyCookies(['session_first', 'session_third'], BASE_URI);
+}
+
 // Verify that subdomains of localhost are treated as separate hosts and aren't considered
 // candidates for eviction.
 function* test_localdomain() {
     Services.prefs.setIntPref("network.cookie.maxPerHost", 2);
 
     const BASE_URI = Services.io.newURI("http://localhost", null, null);
     const BASE_BAR = Services.io.newURI("http://localhost/bar", null, null);
     const OTHER_URI = Services.io.newURI("http://other.localhost", null, null);
@@ -72,21 +94,21 @@ function* test_localdomain() {
 // or path matches, but not both.
 function* test_domain_or_path_matches_not_both(base_host,
                                                subdomain_host,
                                                other_subdomain_host,
                                                another_subdomain_host) {
     Services.prefs.setIntPref("network.cookie.maxPerHost", 2);
 
     const BASE_URI = Services.io.newURI("http://" + base_host, null, null);
-    const PUB_FOO_PATH = Services.io.newURI("http://" + subdomain_host + "/foo", null, null);
-    const WWW_BAR_PATH = Services.io.newURI("http://" + other_subdomain_host + "/bar", null, null);
-    const OTHER_BAR_PATH = Services.io.newURI("http://" + another_subdomain_host + "/bar", null, null);
-    const PUB_BAR_PATH = Services.io.newURI("http://" + subdomain_host + "/bar", null, null);
-    const WWW_FOO_PATH = Services.io.newURI("http://" + other_subdomain_host + "/foo", null, null);
+    const PUB_FOO_PATH = Services.io.newURI("http://" + subdomain_host + "/foo/", null, null);
+    const WWW_BAR_PATH = Services.io.newURI("http://" + other_subdomain_host + "/bar/", null, null);
+    const OTHER_BAR_PATH = Services.io.newURI("http://" + another_subdomain_host + "/bar/", null, null);
+    const PUB_BAR_PATH = Services.io.newURI("http://" + subdomain_host + "/bar/", null, null);
+    const WWW_FOO_PATH = Services.io.newURI("http://" + other_subdomain_host + "/foo/", null, null);
 
     yield setCookie("session_pub_with_foo_path", subdomain_host, "/foo", null, PUB_FOO_PATH);
     yield setCookie("session_www_with_bar_path", other_subdomain_host, "/bar", null, WWW_BAR_PATH);
     verifyCookies(['session_pub_with_foo_path',
                    'session_www_with_bar_path'], BASE_URI);
 
     yield setCookie("session_pub_with_bar_path", subdomain_host, "/bar", null, PUB_BAR_PATH);
     verifyCookies(['session_www_with_bar_path',
@@ -98,18 +120,18 @@ function* test_domain_or_path_matches_no
 }
 
 function* test_basic_eviction(base_host, subdomain_host, other_subdomain_host) {
     Services.prefs.setIntPref("network.cookie.maxPerHost", 5);
 
     const BASE_URI = Services.io.newURI("http://" + base_host, null, null);
     const SUBDOMAIN_URI = Services.io.newURI("http://" + subdomain_host, null, null);
     const OTHER_SUBDOMAIN_URI = Services.io.newURI("http://" + other_subdomain_host, null, null);
-    const FOO_PATH = Services.io.newURI("http://" + base_host + "/foo", null, null);
-    const BAR_PATH = Services.io.newURI("http://" + base_host + "/bar", null, null);
+    const FOO_PATH = Services.io.newURI("http://" + base_host + "/foo/", null, null);
+    const BAR_PATH = Services.io.newURI("http://" + base_host + "/bar/", null, null);
     const ALL_SUBDOMAINS = '.' + base_host;
     const OTHER_SUBDOMAIN = other_subdomain_host;
 
     // Initialize the set of cookies with a mix of non-session cookies with no path,
     // and session cookies with explicit paths. Any subsequent cookies added will cause
     // existing cookies to be evicted.
     yield setCookie("non_session_non_path_non_domain", null, null, 100000, BASE_URI);
     yield setCookie("non_session_non_path_subdomain", ALL_SUBDOMAINS, null, 100000, SUBDOMAIN_URI);
@@ -158,46 +180,48 @@ function* test_basic_eviction(base_host,
     yield setCookie("session_non_path_non_domain_3", null, null, null, BASE_URI);
     verifyCookies(['non_session_non_path_non_domain',
                    'non_session_non_path_subdomain',
                    'session_bar_path_2',
                    'non_session_non_path_non_domain_2',
                    'session_non_path_non_domain_3'], BASE_URI);
 
     // Evict oldest session cookie; all such cookies match example.org/bar (session_bar_path_2)
-    yield setCookie("non_session_non_path_non_domain_3", null, null, 100000, BAR_PATH);
+    // note: this new cookie doesn't have an explicit path, but empty paths inherit the
+    // request's path
+    yield setCookie("non_session_bar_path_non_domain", null, null, 100000, BAR_PATH);
     verifyCookies(['non_session_non_path_non_domain',
                    'non_session_non_path_subdomain',
                    'non_session_non_path_non_domain_2',
                    'session_non_path_non_domain_3',
-                   'non_session_non_path_non_domain_3'], BASE_URI);
+                   'non_session_bar_path_non_domain'], BASE_URI);
 
     // Evict oldest session cookie, even though it matches pub.example.org (session_non_path_non_domain_3)
     yield setCookie("non_session_non_path_pub_domain", null, null, 100000, OTHER_SUBDOMAIN_URI);
     verifyCookies(['non_session_non_path_non_domain',
                    'non_session_non_path_subdomain',
                    'non_session_non_path_non_domain_2',
-                   'non_session_non_path_non_domain_3',
+                   'non_session_bar_path_non_domain',
                    'non_session_non_path_pub_domain'], BASE_URI);
 
     // All session cookies have been evicted.
     // Evict oldest non-session non-domain-matching cookie (non_session_non_path_pub_domain)
-    yield setCookie("non_session_bar_path_non_domain", null, '/bar', 100000, BAR_PATH);
+    yield setCookie("non_session_bar_path_non_domain_2", null, '/bar', 100000, BAR_PATH);
     verifyCookies(['non_session_non_path_non_domain',
                    'non_session_non_path_subdomain',
                    'non_session_non_path_non_domain_2',
-                   'non_session_non_path_non_domain_3',
-                   'non_session_bar_path_non_domain'], BASE_URI);
+                   'non_session_bar_path_non_domain',
+                   'non_session_bar_path_non_domain_2'], BASE_URI);
 
     // Evict oldest non-session non-path-matching cookie (non_session_bar_path_non_domain)
     yield setCookie("non_session_non_path_non_domain_4", null, null, 100000, BASE_URI);
     verifyCookies(['non_session_non_path_non_domain',
                    'non_session_non_path_subdomain',
                    'non_session_non_path_non_domain_2',
-                   'non_session_non_path_non_domain_3',
+                   'non_session_bar_path_non_domain_2',
                    'non_session_non_path_non_domain_4'], BASE_URI);
 
     // At this point all remaining cookies are non-session cookies, have a path of /,
     // and either don't have a domain or have one that matches subdomains.
     // They will therefore be evicted from oldest to newest if all new cookies added share
     // similar characteristics.
 }