Bug 1302824 - Avoid duplicating domain-matching and path-matching logic in cookie service. r=ehsan
authorJosh Matthews <josh@joshmatthews.net>
Mon, 19 Sep 2016 15:40:12 -0400
changeset 314561 5f261677780f
parent 314560 9a370db31b85
child 314562 39b278f56017
push id81931
push userryanvm@gmail.com
push date2016-09-21 01:49 +0000
treeherdermozilla-inbound@6fb622c938de [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1302824
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1302824 - Avoid duplicating domain-matching and path-matching logic in cookie service. r=ehsan
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.
 }