More cookie fixes

* Add some error handling in NetworkCookie and don't add broken cookies
(or should I say crumbs?) to the cookie jar
 * More control on the path and domain, as well as the expiration time

We now pass Opera cookie testsuite functionality tests, as well as some
of the negative tests (we even do better than curl). Not going further
right now as this works well enough for positive cases and most
security/privacy issues are fixed (cross domain and cross path cookie
setting or spying).

headers/os/net/NetworkCookie.h

@@ -31,8 +31,8 @@ public:
 	// Modify the cookie fields
 			BNetworkCookie&		SetName(const BString& name);
 			BNetworkCookie&		SetValue(const BString& value);
-			BNetworkCookie&		SetDomain(const BString& domain);
-			BNetworkCookie&		SetPath(const BString& path);
+			status_t			SetDomain(const BString& domain);
+			status_t			SetPath(const BString& path);
 			BNetworkCookie&		SetMaxAge(int32 maxAge);
 			BNetworkCookie&		SetExpirationDate(time_t expireDate);
 			BNetworkCookie&		SetExpirationDate(BDateTime& expireDate);
@@ -99,9 +99,9 @@ private:
 			BString				fDomain;
 			BString				fPath;
 			BDateTime			fExpiration;
+			status_t			fInitStatus;
 			bool				fSecure;
 			bool				fHttpOnly;
-
 			bool				fHostOnly;
 			bool				fSessionCookie;
 };

src/kits/network/libnetapi/HttpRequest.cpp

@@ -876,10 +876,13 @@ BHttpRequest::_AddHeaders()
 		for (BNetworkCookieJar::UrlIterator it
 				= fContext->GetCookieJar().GetUrlIterator(fUrl);
 				(cookie = it.Next()) != NULL;) {
-			cookieString << cookie->RawCookie(false);
 			cookieString << "; ";
+			cookieString << cookie->RawCookie(false);
 		}
 
+		// Remove the extra "; " we added before the first item
+		cookieString.Remove(0, 2);
+
 		if (cookieString.Length() > 0)
 			fOutputHeaders.AddHeader("Cookie", cookieString);
 	}

src/kits/network/libnetapi/HttpTime.cpp

@@ -77,6 +77,16 @@ BHttpTime::Parse()
 	
 	if (fDateString.Length() < 4)
 		return 0;
+
+	expireTime.tm_sec = 0;
+	expireTime.tm_min = 0;
+	expireTime.tm_hour = 0;
+	expireTime.tm_mday = 0;
+	expireTime.tm_mon = 0;
+	expireTime.tm_year = 0;
+	expireTime.tm_wday = 0;
+	expireTime.tm_yday = 0;
+	expireTime.tm_isdst = 0;
 		
 	if (fDateString[3] == ',') {
 		if (strptime(fDateString.String(), kRfc1123Format, &expireTime)

src/kits/network/libnetapi/NetworkCookie.cpp

@@ -45,7 +45,7 @@ BNetworkCookie::BNetworkCookie(const char* name, const char* value,
 BNetworkCookie::BNetworkCookie(const BString& cookieString, const BUrl& url)
 {
 	_Reset();
-	ParseCookieString(cookieString, url);
+	fInitStatus = ParseCookieString(cookieString, url);
 }
 
 
@@ -89,6 +89,11 @@ BNetworkCookie::ParseCookieString(const BString& string, const BUrl& url)
 {
 	_Reset();
 
+	// Set default values (these can be overriden later on)
+	SetPath(_DefaultPathForUrl(url));
+	SetDomain(url.Host());
+	fHostOnly = true;
+
 	BString name;
 	BString value;
 	int32 index = 0;
@@ -103,6 +108,10 @@ BNetworkCookie::ParseCookieString(const BString& string, const BUrl& url)
 	SetName(name);
 	SetValue(value);
 
+	// Note on error handling: even if there are parse errors, we will continue
+	// and try to parse as much from the cookie as we can.
+	status_t result = B_OK;
+
 	// Parse the remaining cookie attributes.
 	while (index < string.Length()) {
 		ASSERT(string[index] == ';');
@@ -116,44 +125,55 @@ BNetworkCookie::ParseCookieString(const BString& string, const BUrl& url)
 			SetHttpOnly(true);
 
 		// The following attributes require a value.
-		if (value.IsEmpty())
-			continue;
 
 		if (name.ICompare("max-age") == 0) {
+			if (value.IsEmpty()) {
+				result = B_BAD_VALUE;
+				continue;
+			}
 			// Validate the max-age value.
 			char* end = NULL;
 			long maxAge = strtol(value.String(), &end, 10);
 			if (*end == '\0')
 				SetMaxAge((int)maxAge);
+			else
+				SetMaxAge(-1); // cookie will expire immediately
 		} else if (name.ICompare("expires") == 0) {
+			if (value.IsEmpty()) {
+				result = B_BAD_VALUE;
+				continue;
+			}
 			BHttpTime date(value);
 			SetExpirationDate(date.Parse());
 		} else if (name.ICompare("domain") == 0) {
-			SetDomain(value);
+			if (value.IsEmpty()) {
+				result = B_BAD_VALUE;
+				continue;
+			}
+
+			status_t domainResult = SetDomain(value);
+			// Do not reset the result to B_OK if something else already failed
+			if (result == B_OK)
+				result = domainResult;
 		} else if (name.ICompare("path") == 0) {
-			SetPath(value);
+			if (value.IsEmpty()) {
+				result = B_BAD_VALUE;
+				continue;
+			}
+			status_t pathResult = SetPath(value);
+			if (result == B_OK)
+				result = pathResult;
 		}
 	}
 
-	// If no domain was specified, we set a host-only domain from the URL.
-	if (!HasDomain()) {
-		SetDomain(url.Host());
-		fHostOnly = true;
-	} else {
-		// Otherwise the setting URL must domain-match the domain it set.
-		if (!IsValidForDomain(url.Host())) {
-			// Invalidate the cookie.
-			_Reset();
-			return B_NOT_ALLOWED;
-		}
+	if (!IsValidForDomain(url.Host())) {
+		// Invalidate the cookie.
+		_Reset();
+		return B_NOT_ALLOWED;
 	}
 
-	// If no path was specified or the path is invalid, we compute the default
-	// path from the URL.
-	if (!HasPath() || Path()[0] != '/')
-		SetPath(_DefaultPathForUrl(url));
 
-	return B_OK;
+	return result;
 }
 
 
@@ -180,21 +200,24 @@ BNetworkCookie::SetValue(const BString& value)
 }
 
 
-BNetworkCookie&
+status_t
 BNetworkCookie::SetPath(const BString& path)
 {
+	if(path[0] != '/')
+		return B_BAD_DATA;
+
 	// TODO: canonicalize the path
 	fPath = path;
 	fRawFullCookieValid = false;
-	return *this;
+	return B_OK;
 }
 
 
-BNetworkCookie&
+status_t
 BNetworkCookie::SetDomain(const BString& domain)
 {
 	// TODO: canonicalize the domain
-	fDomain = domain;
+	BString newDomain = domain;
 
 	// RFC 2109 (legacy) support: domain string may start with a dot,
 	// meant to indicate the cookie should also be used for subdomains.
@@ -202,12 +225,19 @@ BNetworkCookie::SetDomain(const BString& domain)
 	// not specified at all (in this case it has to exactly match the Url of
 	// the page that set the cookie). In any case, we don't need to handle
 	// dot-cookies specifically anymore, so just remove the extra dot.
-	if(fDomain[0] == '.')
-		fDomain.Remove(0, 1);
+	if(newDomain[0] == '.')
+		newDomain.Remove(0, 1);
+
+	// check we're not trying to set a cookie on a TLD or empty domain
+	if(newDomain.FindLast('.') <= 0)
+		return B_BAD_DATA;
+
+	fDomain = newDomain.ToLower();
+
 	fHostOnly = false;
 
 	fRawFullCookieValid = false;
-	return *this;
+	return B_OK;
 }
 
 
@@ -383,7 +413,7 @@ BNetworkCookie::IsSessionCookie() const
 bool
 BNetworkCookie::IsValid() const
 {
-	return HasName() && HasDomain() && HasPath();
+	return fInitStatus == B_OK && HasName() && HasDomain() && HasPath();
 }
 
 
@@ -410,9 +440,8 @@ BNetworkCookie::IsValidForDomain(const BString& domain) const
 		return false;
 
 	// If the cookie is host-only the domains must match exactly.
-	if (IsHostOnly()) {
+	if (IsHostOnly())
 		return domain == cookieDomain;
-	}
 
 	// FIXME prevent supercookies with a domain of ".com" or similar
 	// This is NOT as straightforward as relying on the last dot in the domain.
@@ -436,24 +465,17 @@ bool
 BNetworkCookie::IsValidForPath(const BString& path) const
 {
 	const BString& cookiePath = Path();
+	BString normalizedPath = path;
 
-	if (path.Length() < cookiePath.Length())
+	int slashPos = normalizedPath.FindLast('/');
+	if(slashPos != normalizedPath.Length() - 1)
+		normalizedPath.Truncate(slashPos + 1);
+
+	if (normalizedPath.Length() < cookiePath.Length())
 		return false;
 
 	// The cookie path must be a prefix of the path string
-	if (path.Compare(cookiePath, cookiePath.Length()) != 0)
-		return false;
-
-	// The paths match if they are identical, or if the last
-	// character of the prefix is a slash, or if the character
-	// after the prefix is a slash.
-	if (path.Length() == cookiePath.Length()
-			|| cookiePath[cookiePath.Length() - 1] == '/'
-			|| path[cookiePath.Length()] == '/') {
-		return true;
-	}
-
-	return false;
+	return normalizedPath.Compare(cookiePath, cookiePath.Length()) == 0;
 }
 
 
@@ -720,6 +742,13 @@ BNetworkCookie::_ExtractAttributeValuePair(const BString& cookieString,
 	else
 		value.SetTo("");
 
+	// values may (or may not) have quotes around them.
+	if(value[0] == '"' && value[value.Length() - 1] == '"')
+	{
+		value.Remove(0, 1);
+		value.Remove(value.Length() - 1, 1);
+	}
+
 	return cookieAVEnd;
 }
 

src/kits/network/libnetapi/NetworkCookieJar.cpp

@@ -117,7 +117,7 @@ BNetworkCookieJar::AddCookie(const BString& cookie, const BUrl& referrer)
 status_t
 BNetworkCookieJar::AddCookie(BNetworkCookie* cookie)
 {
-	if (cookie == NULL)
+	if (cookie == NULL || !cookie->IsValid())
 		return B_BAD_VALUE;
 
 	HashString key(cookie->Domain());
@@ -140,8 +140,9 @@ BNetworkCookieJar::AddCookie(BNetworkCookie* cookie)
 
 	if (cookie->ShouldDeleteNow())
 		delete cookie;
-	else
+	else {
 		list->AddItem(cookie);
+	}
 
 	return B_OK;
 }