hrev46227 adds 1 changeset to branch 'master' old head: ae527df336f44a3a7f12353def921078e2bfabf0 new head: c9d31eeed6ffa94ae37ce66959242a9a9ed40a60 overview: http://cgit.haiku-os.org/haiku/log/?qt=range&q=c9d31ee+%5Eae527df ---------------------------------------------------------------------------- c9d31ee: 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). [ Adrien Destugues <pulkomandy@xxxxxxxxxxxxx> ] ---------------------------------------------------------------------------- Revision: hrev46227 Commit: c9d31eeed6ffa94ae37ce66959242a9a9ed40a60 URL: http://cgit.haiku-os.org/haiku/commit/?id=c9d31ee Author: Adrien Destugues <pulkomandy@xxxxxxxxxxxxx> Date: Mon Oct 14 13:16:37 2013 UTC ---------------------------------------------------------------------------- 5 files changed, 94 insertions(+), 51 deletions(-) headers/os/net/NetworkCookie.h | 6 +- src/kits/network/libnetapi/HttpRequest.cpp | 5 +- src/kits/network/libnetapi/HttpTime.cpp | 10 ++ src/kits/network/libnetapi/NetworkCookie.cpp | 119 ++++++++++++-------- src/kits/network/libnetapi/NetworkCookieJar.cpp | 5 +- ---------------------------------------------------------------------------- diff --git a/headers/os/net/NetworkCookie.h b/headers/os/net/NetworkCookie.h index 9c900e4..56512d5 100644 --- a/headers/os/net/NetworkCookie.h +++ b/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; }; diff --git a/src/kits/network/libnetapi/HttpRequest.cpp b/src/kits/network/libnetapi/HttpRequest.cpp index 21e11fb..796764d 100644 --- a/src/kits/network/libnetapi/HttpRequest.cpp +++ b/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); } diff --git a/src/kits/network/libnetapi/HttpTime.cpp b/src/kits/network/libnetapi/HttpTime.cpp index 067a1f7..b963f95 100644 --- a/src/kits/network/libnetapi/HttpTime.cpp +++ b/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) diff --git a/src/kits/network/libnetapi/NetworkCookie.cpp b/src/kits/network/libnetapi/NetworkCookie.cpp index 4b5c4fb..615e218 100644 --- a/src/kits/network/libnetapi/NetworkCookie.cpp +++ b/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()) - return false; + int slashPos = normalizedPath.FindLast('/'); + if(slashPos != normalizedPath.Length() - 1) + normalizedPath.Truncate(slashPos + 1); - // The cookie path must be a prefix of the path string - if (path.Compare(cookiePath, cookiePath.Length()) != 0) + if (normalizedPath.Length() < cookiePath.Length()) 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; + // The cookie path must be a prefix of the path string + 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; } diff --git a/src/kits/network/libnetapi/NetworkCookieJar.cpp b/src/kits/network/libnetapi/NetworkCookieJar.cpp index 4383b1e..2a99a0f 100644 --- a/src/kits/network/libnetapi/NetworkCookieJar.cpp +++ b/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; }