Changeset 246264 in webkit

Timestamp:

Jun 10, 2019, 9:16:52 AM (7 years ago)

Author:

sihui_liu@apple.com

Message:

[WKHTTPCookieStore getAllCookies:] may return duplicate cookies
​https://bugs.webkit.org/show_bug.cgi?id=198635
<rdar://problem/46010232>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Test: WebKit.WKHTTPCookieStoreWithoutProcessPoolDuplicates

• platform/Cookie.h:

(WebCore::Cookie::isKeyEqual const):
(WTF::HashTraits<WebCore::Cookie>::isEmptyValue):

Source/WebKit:

When there is no process pool, we store cookies set in memory with HashSet m_pendingCookies of WebsiteDataStore.

HashSet does not contain duplicate Cookies that are completely identical, but it may contain Cookies that have
all the other properties identical other than value. This is not correct because Cookies with same name, domain
and path should be treated as the same cookie. When a cookie is set via API, we should either insert the
cookie into m_pendingCookies if the cookie does not exist, or update the cookie value if it already exists.

Note that we still use HashSet with CookieHash for m_pendingCookies because in cookie deletion, we only delete
cookie when there is a complete match. If some cookie from m_pendingCookies has all other properties the same as
the cookie specified in the deletion function, but the value is different, it will not be removed.

• UIProcess/WebsiteData/WebsiteDataStore.cpp:

(WebKit::WebsiteDataStore::addPendingCookie):

Tools:

• TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:

(areCookiesEqual):
(TEST):

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/Cookie.h (modified) (2 diffs)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-06-10  Sihui Liu  <sihui_liu@apple.com>
+
+        [WKHTTPCookieStore getAllCookies:] may return duplicate cookies
+        https://bugs.webkit.org/show_bug.cgi?id=198635
+        <rdar://problem/46010232>
+
+        Reviewed by Ryosuke Niwa.
+
+        Test: WebKit.WKHTTPCookieStoreWithoutProcessPoolDuplicates
+
+        * platform/Cookie.h:
+        (WebCore::Cookie::isKeyEqual const):
+        (WTF::HashTraits<WebCore::Cookie>::isEmptyValue):
+
 2019-06-09  Rob Buis  <rbuis@igalia.com>
 

trunk/Source/WebCore/platform/Cookie.h

@@ -77 +77 @@
     }
     
+    bool isKeyEqual(const Cookie& otherCookie) const
+    {
+        return name == otherCookie.name
+            && domain == otherCookie.domain
+            && path == otherCookie.path;
+    }
+
     String name;
     String value;
@@ -170 +177 @@
         static void constructDeletedValue(WebCore::Cookie& slot) { slot = WebCore::Cookie(WTF::HashTableDeletedValue); }
         static bool isDeletedValue(const WebCore::Cookie& slot) { return slot.name.isHashTableDeletedValue(); }
+
+        static const bool hasIsEmptyValueFunction = true;
+        static bool isEmptyValue(const WebCore::Cookie& slot) { return slot.isNull(); }
     };
     template<> struct EnumTraits<WebCore::Cookie::SameSitePolicy> {

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-06-10  Sihui Liu  <sihui_liu@apple.com>
+
+        [WKHTTPCookieStore getAllCookies:] may return duplicate cookies
+        https://bugs.webkit.org/show_bug.cgi?id=198635
+        <rdar://problem/46010232>
+
+        Reviewed by Ryosuke Niwa.
+
+        When there is no process pool, we store cookies set in memory with HashSet m_pendingCookies of WebsiteDataStore.
+
+        HashSet does not contain duplicate Cookies that are completely identical, but it may contain Cookies that have 
+        all the other properties identical other than value. This is not correct because Cookies with same name, domain
+        and path should be treated as the same cookie. When a cookie is set via API, we should either insert the 
+        cookie into m_pendingCookies if the cookie does not exist, or update the cookie value if it already exists.
+
+        Note that we still use HashSet with CookieHash for m_pendingCookies because in cookie deletion, we only delete 
+        cookie when there is a complete match. If some cookie from m_pendingCookies has all other properties the same as
+        the cookie specified in the deletion function, but the value is different, it will not be removed.
+
+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
+        (WebKit::WebsiteDataStore::addPendingCookie):
+
 2019-06-10  Philippe Normand  <pnormand@igalia.com>
 

trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp

@@ -1942 +1942 @@
 void WebsiteDataStore::addPendingCookie(const WebCore::Cookie& cookie)
 {
+    m_pendingCookies.removeIf([&cookie](auto& pendingCookie) {
+        return pendingCookie.isKeyEqual(cookie);
+    });
     m_pendingCookies.add(cookie);
 }

trunk/Tools/ChangeLog

@@ +1 @@
+2019-06-10  Sihui Liu  <sihui_liu@apple.com>
+
+        [WKHTTPCookieStore getAllCookies:] may return duplicate cookies
+        https://bugs.webkit.org/show_bug.cgi?id=198635
+        <rdar://problem/46010232>
+
+        Reviewed by Ryosuke Niwa.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:
+        (areCookiesEqual):
+        (TEST):
+
 2019-06-10  Adrian Perez de Castro  <aperez@igalia.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm

@@ -628 +628 @@
     TestWebKitAPI::Util::run(&finished);
 }
+
+static bool areCookiesEqual(NSHTTPCookie *first, NSHTTPCookie *second)
+{
+    return [first.name isEqual:second.name] && [first.domain isEqual:second.domain] && [first.path isEqual:second.path] && [first.value isEqual:second.value];
+}
+
+TEST(WebKit, WKHTTPCookieStoreWithoutProcessPoolDuplicates)
+{
+    RetainPtr<WKHTTPCookieStore> httpCookieStore = [WKWebsiteDataStore defaultDataStore].httpCookieStore;
+    RetainPtr<NSHTTPCookie> sessionCookie = [NSHTTPCookie cookieWithProperties:@{
+        NSHTTPCookiePath: @"/",
+        NSHTTPCookieName: @"SessionCookieName",
+        NSHTTPCookieValue: @"CookieValue",
+        NSHTTPCookieDomain: @"127.0.0.1",
+    }];
+
+    auto properties = adoptNS([sessionCookie.get().properties mutableCopy]);
+    properties.get()[NSHTTPCookieDomain] = @"localhost";
+    RetainPtr<NSHTTPCookie> sessionCookieDifferentDomain = [NSHTTPCookie cookieWithProperties:properties.get()];
+    properties.get()[NSHTTPCookieValue] = @"OtherCookieValue";
+    RetainPtr<NSHTTPCookie> sessionCookieDifferentValue = [NSHTTPCookie cookieWithProperties:properties.get()];
+    finished = false;
+    
+    [httpCookieStore.get() setCookie:sessionCookie.get() completionHandler:^{
+        finished = true;
+    }];
+    TestWebKitAPI::Util::run(&finished);
+    finished = false;
+
+    [httpCookieStore.get() setCookie:sessionCookieDifferentDomain.get() completionHandler:^{
+        finished = true;
+    }];
+    TestWebKitAPI::Util::run(&finished);
+    finished = false;
+
+    [httpCookieStore.get() setCookie:sessionCookieDifferentValue.get() completionHandler:^{
+        finished = true;
+    }];
+    TestWebKitAPI::Util::run(&finished);
+    finished = false;
+
+    [httpCookieStore.get() getAllCookies:^(NSArray<NSHTTPCookie *> *cookies) {
+        EXPECT_EQ(2u, cookies.count);
+        bool sessionCookieExists = false, otherSessionCookieExists = false;
+        for (NSHTTPCookie* cookie in cookies) {
+            if (areCookiesEqual(cookie, sessionCookie.get()))
+                sessionCookieExists = true;
+            else if (areCookiesEqual(cookie, sessionCookieDifferentValue.get()))
+                otherSessionCookieExists = true;
+        }
+        EXPECT_TRUE(sessionCookieExists && otherSessionCookieExists);
+        finished = true;
+    }];
+    TestWebKitAPI::Util::run(&finished);
+}