From 64a1f5a020afbbb203b72bfb07e1b08d38431bfe Mon Sep 17 00:00:00 2001 From: Hamish Morrison Date: Thu, 7 Feb 2013 17:59:43 +0000 Subject: [PATCH] NetworkCookieJar: various small fixes and updated tests --- headers/os/net/NetworkCookieJar.h | 27 ++- src/kits/network/libnetapi/NetworkCookie.cpp | 45 ++--- .../network/libnetapi/NetworkCookieJar.cpp | 155 ++++++++++-------- src/tests/kits/net/cookie/cookie_test.cpp | 123 ++++++++------ 4 files changed, 198 insertions(+), 152 deletions(-) diff --git a/headers/os/net/NetworkCookieJar.h b/headers/os/net/NetworkCookieJar.h index 31cd196a5b..9c3e775f7d 100644 --- a/headers/os/net/NetworkCookieJar.h +++ b/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_ @@ -24,7 +24,7 @@ public: class UrlIterator; struct PrivateIterator; struct PrivateHashMap; - + public: BNetworkCookieJar(); BNetworkCookieJar( @@ -34,16 +34,15 @@ public: BNetworkCookieJar(BMessage* archive); virtual ~BNetworkCookieJar(); - bool AddCookie(const BNetworkCookie& cookie); - bool AddCookie(BNetworkCookie* cookie); - bool AddCookies( - const BNetworkCookieList& cookies); + status_t AddCookie(const BNetworkCookie& cookie); + status_t AddCookie(BNetworkCookie* cookie); + status_t AddCookies(const BNetworkCookieList& cookies); uint32 DeleteOutdatedCookies(); uint32 PurgeForExit(); // BArchivable members - virtual status_t Archive(BMessage* into, + virtual status_t Archive(BMessage* into, bool deep = true) const; static BArchivable* Instantiate(BMessage* archive); @@ -54,20 +53,20 @@ public: virtual status_t Flatten(void* buffer, ssize_t size) const; virtual bool AllowsTypeCode(type_code code) const; - virtual status_t Unflatten(type_code code, + virtual status_t Unflatten(type_code code, const void* buffer, ssize_t size); // Iterators Iterator GetIterator() const; UrlIterator GetUrlIterator(const BUrl& url) const; - + private: void _DoFlatten() const; - + private: friend class Iterator; friend class UrlIterator; - + PrivateHashMap* fCookieHashMap; mutable BString fFlattened; }; @@ -117,7 +116,7 @@ private: UrlIterator(const BNetworkCookieJar* map, const BUrl& url); - bool _SupDomain(); + bool _SuperDomain(); void _FindNext(); void _FindDomain(); bool _FindPath(); @@ -131,10 +130,10 @@ private: BNetworkCookieList* fLastList; BNetworkCookie* fElement; BNetworkCookie* fLastElement; - + int32 fIndex; int32 fLastIndex; - + BUrl fUrl; }; diff --git a/src/kits/network/libnetapi/NetworkCookie.cpp b/src/kits/network/libnetapi/NetworkCookie.cpp index e4a5311dd5..25b5e5c36d 100644 --- a/src/kits/network/libnetapi/NetworkCookie.cpp +++ b/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 (!HasPath()) + // 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 *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(); - BString urlPath = url.Path(); - - return IsValidForDomain(urlHost) && IsValidForPath(urlPath); + return IsValidForDomain(url.Host()) && IsValidForPath(url.Path()); } diff --git a/src/kits/network/libnetapi/NetworkCookieJar.cpp b/src/kits/network/libnetapi/NetworkCookieJar.cpp index 5b8a94583b..cfe3efdb6a 100644 --- a/src/kits/network/libnetapi/NetworkCookieJar.cpp +++ b/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, c.huriaux@gmail.com + * Hamish Morrison, hamishm53@gmail.com */ -#include - #include #include #include #include #include + +#include + #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; - archive->FindMessage(kArchivedCookieMessageName, i, &extractedCookie) - == B_OK; - i++) { + for (int32 i = 0; archive->FindMessage(kArchivedCookieMessageName, i, + &extractedCookie) == 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()) - return false; + if (cookie == NULL) + 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(list->ItemAt(i)); - if (c->Name() == cookie->Name()) { + if (c->Name() == cookie->Name() && c->Path() == cookie->Path()) { list->RemoveItem(i); break; } } - list->AddItem(cookie); - return true; + if (cookie->ShouldDeleteNow()) + 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)) - return false; + status_t result = AddCookie(*cookiePtr); + 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(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(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()); - int32 nextDot = domain.FindFirst('.', 1); + const char* domain = fIterator->fKey.GetString(); + const char* nextDot = strchr(domain, '.'); - if (nextDot == -1) + if (nextDot == NULL) return false; - domain.Remove(0, nextDot); - fIterator->fKey.SetTo(domain.String(), domain.Length()); + fIterator->fKey.SetTo(nextDot + 1); 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()) { - do { - fElement - = reinterpret_cast(fList->ItemAt(fIndex)); + while (fList && fIndex < fList->CountItems()) { + fElement = reinterpret_cast(fList->ItemAt(fIndex)); - if (fElement->IsValidForPath(fUrl.Path())) - return true; + if (fElement->IsValidForPath(fUrl.Path())) + return true; - fIndex++; - } while (fList && fIndex < fList->CountItems()); + fIndex++; } return false; diff --git a/src/tests/kits/net/cookie/cookie_test.cpp b/src/tests/kits/net/cookie/cookie_test.cpp index ded674ce34..8a4417eb09 100644 --- a/src/tests/kits/net/cookie/cookie_test.cpp +++ b/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 - // Name Value Domain Path Secure Discard Session maxAge - // -------- --------- --------- --------- ------ ------- -------- ------- + +ExplodeTest kTestExplode[] = + // Cookie string URL + // ------------- ------------- + // Valid Name Value Domain Path Secure HttpOnly Session Expiration + // --------- -------- --------- ----------------- --------- -------- -------- ------- ---------- { - { "name=value", - { "name", "value", "", "", false, false, true, 0 } }, - { "name=value;secure=true", - { "name", "value", "", "", true, false, true, 0 } }, - { "name=value;secure=false;maxage=5", - { "name", "value", "", "", false, false, false, 5 } }, - { "name=value;discard=true", - { "name", "value", "", "", false, true, true, 0 } }, + // Normal cookies + { "name=value", "http://www.example.com/path/path", + { true, "name", "value", "www.example.com", "/path", false, false, true, BDateTime() } }, + { "name=value; domain=example.com; path=/; secure", "http://www.example.com/path/path", + { true, "name", "value", "example.com", "/" , true, false, true, BDateTime() } }, + { "name=value; httponly; secure", "http://www.example.com/path/path", + { true, "name", "value", "www.example.com", "/path", true, true, true, BDateTime() } }, + { "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, 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()); + ASSERT(testIndex, kTestExplode[testIndex].expected.valid == cookie.IsValid()); - if (!cookie.IsSessionCookie()) - ASSERT(testIndex, kTestExplode[testIndex].expected.maxAge == cookie.MaxAge()); + if (kTestExplode[testIndex].expected.valid) { + 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()); + } } } @@ -79,27 +102,27 @@ void explodeImplodeTest() void stressTest(int32 domainNumber, int32 totalCookies, char** flat, ssize_t* size) { char **domains = new char*[domainNumber]; - + cout << "Creating random domains" << endl; srand(time(NULL)); for (int32 i = 0; i < domainNumber; i++) { int16 charNum = (rand() % 16) + 1; - + domains[i] = new char[charNum + 5]; - + // Random domain - for (int32 c = 0; c < charNum; c++) + for (int32 c = 0; c < charNum; c++) domains[i][c] = (rand() % 26) + 'a'; - + domains[i][charNum] = '.'; - + // Random tld - for (int32 c = 0; c < 3; c++) + for (int32 c = 0; c < 3; c++) domains[i][charNum+1+c] = (rand() % 26) + 'a'; - + domains[i][charNum+4] = 0; } - + BNetworkCookieJar j; BStopWatch* watch = new BStopWatch("Cookie insertion"); for (int32 i = 0; i < totalCookies; i++) { @@ -107,21 +130,21 @@ void stressTest(int32 domainNumber, int32 totalCookies, char** flat, ssize_t* si int16 domain = (rand() % domainNumber); BString name("Foo"); name << i; - + c.SetName(name); c.SetValue("Bar"); c.SetDomain(domains[domain]); c.SetPath("/"); - + j.AddCookie(c); } delete watch; - + BNetworkCookie* c; int16 domain = (rand() % domainNumber); BString host("http://"); host << domains[domain] << "/"; - + watch = new BStopWatch("Cookie filtering"); BUrl url(host); int32 count = 0; @@ -131,17 +154,17 @@ void stressTest(int32 domainNumber, int32 totalCookies, char** flat, ssize_t* si } delete watch; cout << "Count for " << host << ": " << count << endl; - - + + cout << "Flat view of cookie jar is " << j.FlattenedSize() << " bytes large." << endl; *flat = new char[j.FlattenedSize()]; *size = j.FlattenedSize(); - + if (j.Flatten(*flat, j.FlattenedSize()) == B_OK) cout << "Flatten() success!" << endl; else cout << "Flatten() error!" << endl; - + delete[] domains; } @@ -152,22 +175,22 @@ main(int, char**) cout << "Running explodeImplodeTest:" << endl; explodeImplodeTest(); cout << endl << endl; - + cout << "Running stressTest:" << endl; char* flatJar; ssize_t size; stressTest(10000, 40000, &flatJar, &size); - + BNetworkCookieJar j; j.Unflatten(B_ANY_TYPE, flatJar, size); - + int32 count = 0; BNetworkCookie* c; for (BNetworkCookieJar::Iterator it(j.GetIterator()); (c = it.Next()); ) count++; cout << "Count : " << count << endl; - + delete[] flatJar; - + return EXIT_SUCCESS; }