NetworkCookieJar: various small fixes and updated tests

headers/os/net/NetworkCookieJar.h

@@ -1,5 +1,5 @@
 /*
- * Copyright 2010 Haiku Inc. All rights reserved.
+ * Copyright 2010-2013 Haiku Inc. All rights reserved.
  * Distributed under the terms of the MIT License.
  */
 #ifndef _B_NETWORK_COOKIE_JAR_H_
@@ -34,10 +34,9 @@ public:
 								BNetworkCookieJar(BMessage* archive);
 	virtual						~BNetworkCookieJar();
 
-			bool				AddCookie(const BNetworkCookie& cookie);
+			status_t			AddCookie(const BNetworkCookie& cookie);
-			bool				AddCookie(BNetworkCookie* cookie);			
+			status_t			AddCookie(BNetworkCookie* cookie);
-			bool				AddCookies(
+			status_t			AddCookies(const BNetworkCookieList& cookies);
-									const BNetworkCookieList& cookies);
 
 			uint32				DeleteOutdatedCookies();
 			uint32				PurgeForExit();
@@ -117,7 +116,7 @@ private:
 								UrlIterator(const BNetworkCookieJar* map,
 									const BUrl& url);
 
-			bool				_SupDomain();
+			bool				_SuperDomain();
 			void 				_FindNext();
 			void				_FindDomain();
 			bool				_FindPath();

src/kits/network/libnetapi/NetworkCookie.cpp

@@ -100,14 +100,15 @@ BNetworkCookie::ParseCookieStringFromUrl(const BString& string,
 
 	// Parse the name and value of the cookie
 	index = _ExtractNameValuePair(string, name, value, index);
-	// The set-cookie-string is not valid
+	if (index == -1) {
-	if (index == -1)
+		// The set-cookie-string is not valid
 		return *this;
+	}
 
 	SetName(name);
 	SetValue(value);
 
-	// Parse the remaining cookie attributes
+	// Parse the remaining cookie attributes.
 	while (index < string.Length()) {
 		ASSERT(string[index] == ';');
 		index++;
@@ -119,12 +120,12 @@ BNetworkCookie::ParseCookieStringFromUrl(const BString& string,
 		else if (name.ICompare("httponly") == 0)
 			SetHttpOnly(true);
 
-		// The following attributes require a value
+		// The following attributes require a value.
 		if (value.IsEmpty())
 			continue;
 
 		if (name.ICompare("max-age") == 0) {
-			// Validate the max-age value
+			// Validate the max-age value.
 			char* end = NULL;
 			long maxAge = strtol(value.String(), &end, 10);
 			if (*end == '\0')
@@ -132,20 +133,31 @@ BNetworkCookie::ParseCookieStringFromUrl(const BString& string,
 		} else if (name.ICompare("expires") == 0) {
 			BHttpTime date(value);
 			SetExpirationDate(date.Parse());
-		} else if (name.ICompare("domain") == 0)
+		} else if (name.ICompare("domain") == 0) {
 			SetDomain(value);
-		else if (name.ICompare("path") == 0)
+		} else if (name.ICompare("path") == 0) {
 			SetPath(value);
+		}
 	}
 
-	// If no domain was specified, we set a host-only domain from the URL
+	// 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 *this;
+		}
+		// We should also reject cookies with domains that match public
+		// suffixes.
 	}
 
-	// If no path was specified we compute the default path from the URL
+	// If no path was specified or the path is invalid, we compute the default
-	if (!HasPath())
+	// path from the URL.
+	if (!HasPath() || Path()[0] != '/')
 		SetPath(_DefaultPathForUrl(url));
 
 	return *this;
@@ -187,6 +199,7 @@ BNetworkCookie::SetValue(const BString& value)
 BNetworkCookie&
 BNetworkCookie::SetPath(const BString& path)
 {
+	// TODO: canonicalize the path
 	fPath = path;
 	fRawFullCookieValid = false;
 	return *this;
@@ -196,13 +209,10 @@ BNetworkCookie::SetPath(const BString& path)
 BNetworkCookie&
 BNetworkCookie::SetDomain(const BString& domain)
 {
+	// TODO: canonicalize the domain
 	fDomain = domain;
 	fHostOnly = false;
 
-	//  We always use pre-dotted domains for tail matching
-	if (fDomain.ByteAt(0) != '.')
-		fDomain.Prepend(".");
-
 	fRawFullCookieValid = false;
 	return *this;
 }
@@ -387,13 +397,10 @@ BNetworkCookie::IsValid() const
 bool
 BNetworkCookie::IsValidForUrl(const BUrl& url) const
 {
-	if (IsSecure() && url.Protocol() != "https")
+	if (Secure() && url.Protocol() != "https")
 		return false;
 
-	BString urlHost = url.Host();
+	return IsValidForDomain(url.Host()) && IsValidForPath(url.Path());
-	BString urlPath = url.Path();
-
-	return IsValidForDomain(urlHost) && IsValidForPath(urlPath);
 }
 
 

src/kits/network/libnetapi/NetworkCookieJar.cpp

@@ -1,35 +1,37 @@
 /*
- * Copyright 2010 Haiku Inc. All rights reserved.
+ * Copyright 2010-2013 Haiku Inc. All rights reserved.
  * Distributed under the terms of the MIT License.
  *
  * Authors:
  *		Christophe Huriaux, [email protected]
+ *		Hamish Morrison, [email protected]
  */
 
 
-#include <new>
-
 #include <Debug.h>
 #include <HashMap.h>
 #include <HashString.h>
 #include <Message.h>
 #include <NetworkCookieJar.h>
+
+#include <new>
+
 #include "NetworkCookieJarPrivate.h"
 
-const char* kArchivedCookieMessageName	=	"be:cookie";
+
+const char* kArchivedCookieMessageName = "be:cookie";
 
 
 BNetworkCookieJar::BNetworkCookieJar()
 	:
-	fCookieHashMap(new PrivateHashMap)
+	fCookieHashMap(new PrivateHashMap())
 {
 }
 
 
 BNetworkCookieJar::BNetworkCookieJar(const BNetworkCookieJar&)
 	:
-	BArchivable(),
+	fCookieHashMap(new PrivateHashMap())
-	fCookieHashMap(new PrivateHashMap)
 {
 	// TODO
 }
@@ -37,7 +39,7 @@ BNetworkCookieJar::BNetworkCookieJar(const BNetworkCookieJar&)
 
 BNetworkCookieJar::BNetworkCookieJar(const BNetworkCookieList& otherList)
 	:
-	fCookieHashMap(new PrivateHashMap)
+	fCookieHashMap(new PrivateHashMap())
 {
 	AddCookies(otherList);
 }
@@ -45,19 +47,22 @@ BNetworkCookieJar::BNetworkCookieJar(const BNetworkCookieList& otherList)
 
 BNetworkCookieJar::BNetworkCookieJar(BMessage* archive)
 	:
-	fCookieHashMap(new PrivateHashMap)
+	fCookieHashMap(new PrivateHashMap())
 {
 	BMessage extractedCookie;
 
-	for (int32 i = 0;
+	for (int32 i = 0; archive->FindMessage(kArchivedCookieMessageName, i,
-		archive->FindMessage(kArchivedCookieMessageName, i, &extractedCookie)
+			&extractedCookie) == B_OK; i++) {
-			== B_OK;
-		i++) {
 		BNetworkCookie* heapCookie
 			= new(std::nothrow) BNetworkCookie(&extractedCookie);
 
-		if (heapCookie == NULL || !AddCookie(heapCookie))
+		if (heapCookie == NULL)
 			break;
+
+		if (!AddCookie(heapCookie)) {
+			delete heapCookie;
+			continue;
+		}
 	}
 }
 
@@ -66,7 +71,7 @@ BNetworkCookieJar::~BNetworkCookieJar()
 {
 	BNetworkCookie* cookiePtr;
 
-	for (Iterator it(GetIterator()); (cookiePtr = it.Next()); )
+	for (Iterator it = GetIterator(); (cookiePtr = it.Next()) != NULL;)
 		delete it.Remove();
 }
 
@@ -74,49 +79,58 @@ BNetworkCookieJar::~BNetworkCookieJar()
 // #pragma mark Add cookie to cookie jar
 
 
-bool
+status_t
 BNetworkCookieJar::AddCookie(const BNetworkCookie& cookie)
 {
 	BNetworkCookie* heapCookie = new(std::nothrow) BNetworkCookie(cookie);
+	if (heapCookie == NULL)
+		return B_NO_MEMORY;
 
-	if (!AddCookie(heapCookie)) {
+	status_t result = AddCookie(heapCookie);
+	if (result != B_OK) {
 		delete heapCookie;
-		return false;
+		return result;
 	}
 
-	return true;
+	return B_OK;
 }
 
 
-bool
+status_t
 BNetworkCookieJar::AddCookie(BNetworkCookie* cookie)
 {
-	if (cookie == NULL || cookie->ShouldDeleteNow())
+	if (cookie == NULL)
-		return false;
+		return B_BAD_VALUE;
 
 	HashString key(cookie->Domain());
 
-	if (!fCookieHashMap->fHashMap.ContainsKey(key))
-		fCookieHashMap->fHashMap.Put(key, new BList);
-
 	BNetworkCookieList* list = fCookieHashMap->fHashMap.Get(key);
+	if (list == NULL) {
+		list = new(std::nothrow) BNetworkCookieList();
+		if (list == NULL || fCookieHashMap->fHashMap.Put(key, list) != B_OK)
+			return B_NO_MEMORY;
+	}
 
 	for (int32 i = 0; i < list->CountItems(); i++) {
 		BNetworkCookie* c
 			= reinterpret_cast<BNetworkCookie*>(list->ItemAt(i));
 
-		if (c->Name() == cookie->Name()) {
+		if (c->Name() == cookie->Name() && c->Path() == cookie->Path()) {
 			list->RemoveItem(i);
 			break;
 		}
 	}
 
-	list->AddItem(cookie);
+	if (cookie->ShouldDeleteNow())
-	return true;
+		delete cookie;
+	else
+		list->AddItem(cookie);
+
+	return B_OK;
 }
 
 
-bool
+status_t
 BNetworkCookieJar::AddCookies(const BNetworkCookieList& cookies)
 {
 	for (int32 i = 0; i < cookies.CountItems(); i++) {
@@ -125,11 +139,12 @@ BNetworkCookieJar::AddCookies(const BNetworkCookieList& cookies)
 
 		// Using AddCookie by reference in order to avoid multiple
 		// cookie jar share the same cookie pointers
-		if (!AddCookie(*cookiePtr))
+		status_t result = AddCookie(*cookiePtr);
-			return false;
+		if (result != B_OK)
+			return result;
 	}
 
-	return true;
+	return B_OK;
 }
 
 
@@ -437,7 +452,7 @@ BNetworkCookieJar::Iterator::NextDomain()
 		return NULL;
 	}
 
-	fList = *(fIterator->fCookieMapIterator.NextValue());
+	fList = *fIterator->fCookieMapIterator.NextValue();
 	fIndex = 0;
 	fElement = reinterpret_cast<BNetworkCookie*>(fList->ItemAt(fIndex));
 
@@ -461,8 +476,8 @@ BNetworkCookieJar::Iterator::Remove()
 		else
 			fLastList->RemoveItem(fLastList->CountItems() - 1);
 	} else {
-		fList->RemoveItem(fIndex-1);
 		fIndex--;
+		fList->RemoveItem(fIndex);
 	}
 
 	fLastElement = NULL;
@@ -474,12 +489,18 @@ BNetworkCookieJar::Iterator&
 BNetworkCookieJar::Iterator::operator=(const BNetworkCookieJar::Iterator& other)
 {
 	fCookieJar = other.fCookieJar;
-	fIterator = other.fIterator;
 	fLastList = other.fLastList;
 	fList = other.fList;
 	fElement = other.fElement;
 	fLastElement = other.fLastElement;
 	fIndex = other.fIndex;
+
+	fIterator = new(std::nothrow) PrivateIterator(*other.fIterator);
+	if (fIterator == NULL) {
+		// Make the iterator unusable.
+		fElement = NULL;
+		fLastElement = NULL;
+	}
 	return *this;
 }
 
@@ -527,25 +548,22 @@ BNetworkCookieJar::UrlIterator::UrlIterator(const BNetworkCookieJar* cookieJar,
 	fLastElement(NULL),
 	fIndex(0),
 	fLastIndex(0),
-	fUrl(const_cast<BUrl&>(url))
+	fUrl(url)
 {
-	BString domain(url.Host());
+	BString domain = url.Host();
 
 	if (!domain.Length())
 		return;
 
-	if (domain[0] != '.')
-		domain.Prepend(".");
-
-	//  Prepending another dot since _FindNext is going to
-	// call _SupDomain()
-	domain.Prepend(".");
-
 	fIterator = new(std::nothrow) PrivateIterator(
 		fCookieJar->fCookieHashMap->fHashMap.GetIterator());
-	fIterator->fKey.SetTo(domain, domain.Length());
 
-	_FindNext();
+	if (fIterator != NULL) {
+		// Prepending a dot since _FindNext is going to call _SupDomain()
+		domain.Prepend(".");
+		fIterator->fKey.SetTo(domain, domain.Length());
+		_FindNext();
+	}
 }
 
 
@@ -607,23 +625,28 @@ BNetworkCookieJar::UrlIterator::operator=(
 	fLastElement = other.fLastElement;
 	fIndex = other.fIndex;
 	fLastIndex = other.fLastIndex;
-	fUrl = other.fUrl;
+
-	fIterator = other.fIterator;
+	fIterator = new(std::nothrow) PrivateIterator(*other.fIterator);
+	if (fIterator == NULL) {
+		// Make the iterator unusable.
+		fElement = NULL;
+		fLastElement = NULL;
+	}
+
 	return *this;
 }
 
 
 bool
-BNetworkCookieJar::UrlIterator::_SupDomain()
+BNetworkCookieJar::UrlIterator::_SuperDomain()
 {
-	BString domain(fIterator->fKey.GetString());
+	const char* domain = fIterator->fKey.GetString();
-	int32 nextDot = domain.FindFirst('.', 1);
+	const char* nextDot = strchr(domain, '.');
 
-	if (nextDot == -1)
+	if (nextDot == NULL)
 		return false;
 
-	domain.Remove(0, nextDot);
+	fIterator->fKey.SetTo(nextDot + 1);
-	fIterator->fKey.SetTo(domain.String(), domain.Length());
 	return true;
 }
 
@@ -633,19 +656,16 @@ BNetworkCookieJar::UrlIterator::_FindNext()
 {
 	fLastIndex = fIndex;
 	fLastElement = fElement;
-
-	if (_FindPath())
-		return;
-
 	fLastList = fList;
-	do {
+
-		if (!_SupDomain()) {
+	while (!_FindPath()) {
+		if (!_SuperDomain()) {
 			fElement = NULL;
 			return;
 		}
 
 		_FindDomain();
-	} while (!_FindPath());
+	}
 }
 
 
@@ -665,16 +685,13 @@ bool
 BNetworkCookieJar::UrlIterator::_FindPath()
 {
 	fIndex++;
-	if (fList && fIndex < fList->CountItems()) {
+	while (fList && fIndex < fList->CountItems()) {
-		do {
+		fElement = reinterpret_cast<BNetworkCookie*>(fList->ItemAt(fIndex));
-			fElement
-				= reinterpret_cast<BNetworkCookie*>(fList->ItemAt(fIndex));
 
-			if (fElement->IsValidForPath(fUrl.Path()))
+		if (fElement->IsValidForPath(fUrl.Path()))
-				return true;
+			return true;
 
-			fIndex++;
+		fIndex++;
-		} while (fList && fIndex < fList->CountItems());
 	}
 
 	return false;

src/tests/kits/net/cookie/cookie_test.cpp

@@ -20,58 +20,81 @@ using std::cout;
 using std::endl;
 
 
-typedef struct
+typedef struct {
-{
 	const char* cookieString;
-
+	const char*	url;
 	struct
 	{
+		bool		valid;
 		const char* name;
 		const char* value;
 		const char* domain;
 		const char* path;
 		bool 		secure;
-		bool 		discard;
+		bool 		httponly;
 		bool		session;
-		int32 		maxAge;
+		BDateTime	expire;
 	} expected;
 } ExplodeTest;
 
-const ExplodeTest	kTestExplode[] =
+
-	//  Cookie string
+ExplodeTest kTestExplode[] =
-	//		    Name	 Value	  Domain     Path   Secure   Discard  Session   maxAge
+	//     Cookie string      URL
-	//       -------- --------- --------- --------- ------   -------  -------- -------
+	//     ------------- -------------
+	//		   Valid     Name     Value	       Domain         Path      Secure  HttpOnly Session  Expiration
+	//       --------- -------- --------- ----------------- ---------  -------- -------- -------  ----------
 	{
-		{ "name=value",
+		// Normal cookies
-			 { "name", "value",      "",       "",  false,  false,    true,     0 } },
+		{ "name=value", "http://www.example.com/path/path",
-		{ "name=value;secure=true",
+			{  true,    "name",  "value", "www.example.com", "/path",   false,   false,   true,   BDateTime() } },
-			 { "name", "value",      "",       "",   true,  false,    true,     0 } },
+		{ "name=value; domain=example.com; path=/; secure", "http://www.example.com/path/path",
-		{ "name=value;secure=false;maxage=5",
+			{  true,    "name",  "value",   "example.com",   "/"    ,   true,    false,   true,   BDateTime() } },
-			 { "name", "value",      "",       "",  false,  false,    false,    5 } },
+		{ "name=value; httponly; secure", "http://www.example.com/path/path",
-		{ "name=value;discard=true",
+			{  true,    "name",  "value", "www.example.com", "/path",   true,    true,    true,   BDateTime() } },
-			 { "name", "value",      "",       "",  false,  true,    true,     0 } },
+		{ "name=value; expires=Wed, 20 Feb 2013 20:00:00 UTC", "http://www.example.com/path/path",
+			{  true,    "name",  "value", "www.example.com", "/path",   false,   false,   false,
+				BDateTime(BDate(2012, 2, 20), BTime(20, 0, 0, 0)) } },
+		// Valid cookie with bad form
+		{ "name=  ;  domain   =example.com  ;path=/;  secure = yup  ; blahblah ;)", "http://www.example.com/path/path",
+			{  true,    "name",  "",   "example.com",   "/"    ,   true,    false,   true,   BDateTime() } },
+		// Invalid path, default path should be used instead
+		{ "name=value; path=invalid", "http://www.example.com/path/path",
+			{  true,    "name",  "value", "www.example.com", "/path",   false,   false,   true,   BDateTime() } },
+		// Setting for other subdomain (invalid)
+		{ "name=value; domain=subdomain.example.com", "http://www.example.com/path/path",
+			{  false,   "name",  "value", "www.example.com", "/path",   false,   false,   true,   BDateTime() } },
+		// Various invalid cookies
+		{ "name", "http://www.example.com/path/path",
+			{  false,   "name",  "value", "www.example.com", "/path",   false,   false,   true,   BDateTime() } },
+		{ "; domain=example.com", "http://www.example.com/path/path",
+			{  false,   "name",  "value", "www.example.com", "/path",   false,   false,   true,   BDateTime() } }
 	};
 
 
 void explodeImplodeTest()
 {
-	uint8 testIndex;
+	uint32 testIndex;
 	BNetworkCookie cookie;
 
 	for (testIndex = 0; testIndex < (sizeof(kTestExplode) / sizeof(ExplodeTest)); testIndex++)
 	{
-		cookie.ParseCookieString(kTestExplode[testIndex].cookieString);
+		BUrl url(kTestExplode[testIndex].url);
+		cookie.ParseCookieStringFromUrl(kTestExplode[testIndex].cookieString, url);
 
-		ASSERT(testIndex, BString(kTestExplode[testIndex].expected.name) == BString(cookie.Name()));
+		ASSERT(testIndex, kTestExplode[testIndex].expected.valid == cookie.IsValid());
-		ASSERT(testIndex, BString(kTestExplode[testIndex].expected.value) == BString(cookie.Value()));
-		ASSERT(testIndex, BString(kTestExplode[testIndex].expected.domain) == BString(cookie.Domain()));
-		ASSERT(testIndex, BString(kTestExplode[testIndex].expected.path) == BString(cookie.Path()));
-		ASSERT(testIndex, kTestExplode[testIndex].expected.secure == cookie.Secure());
-		ASSERT(testIndex, kTestExplode[testIndex].expected.discard == cookie.Discard());
-		ASSERT(testIndex, kTestExplode[testIndex].expected.session == cookie.IsSessionCookie());
 
-		if (!cookie.IsSessionCookie())
+		if (kTestExplode[testIndex].expected.valid) {
-			ASSERT(testIndex, kTestExplode[testIndex].expected.maxAge == cookie.MaxAge());
+			ASSERT(testIndex, BString(kTestExplode[testIndex].expected.name) == cookie.Name());
+			ASSERT(testIndex, BString(kTestExplode[testIndex].expected.value) == cookie.Value());
+			ASSERT(testIndex, BString(kTestExplode[testIndex].expected.domain) == cookie.Domain());
+			ASSERT(testIndex, BString(kTestExplode[testIndex].expected.path) == cookie.Path());
+			ASSERT(testIndex, kTestExplode[testIndex].expected.secure == cookie.Secure());
+			ASSERT(testIndex, kTestExplode[testIndex].expected.httponly == cookie.HttpOnly());
+			ASSERT(testIndex, kTestExplode[testIndex].expected.session == cookie.IsSessionCookie());
+
+			if (!cookie.IsSessionCookie())
+				ASSERT(testIndex, kTestExplode[testIndex].expected.expire.Time_t() == cookie.ExpirationDate());
+		}
 	}
 }