There's empty implementation on Curl network layer for this. Also the backend only has API to enable/disable cookie. Other policy should be implemented.
Created attachment 361383 [details] Patch
Implemented function of telling cookie accept policy to Curl network layer.
Comment on attachment 361383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361383&action=review > Source/WebCore/platform/network/curl/CookieJarDB.h:43 > + ExclusivelyFromMainDocumentDomain Is it possible to implement OnlyFromMainDocumentDomain and ExclusivelyFromMainDocumentDomain in CookieJarDB layer? It seems CookieJarDB doesn't know the concept of the main document.
(In reply to Fujii Hironori from comment #3) ResouceRequest object has URL for first party cookies. We can tell resource's URL for first party cookies by using ResourceRequestBase::firstPartyForCookies()
Comment on attachment 361383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361383&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:311 > + // FIXME : Support cookie policy. Umm, I don't understand your answer (comment 4). The problem of this patch is the most important part is unimplemented (this FIXME). Basically, it is a good practice to split a large patch into some patches. However, this patch seems too small to understand. I want to see the implementation.
Created attachment 362282 [details] Patch
Created attachment 362283 [details] Patch
Created attachment 362288 [details] Patch
By applying this patch some cookie policy tests passes on WebKitTestRunner. But it doesn't affect tests on DumpRenderTree because DRT doesn't have implementation of changing cookie policy.
Comment on attachment 362288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362288&action=review > Source/WebCore/ChangeLog:16 > + LayoutTests/http/tests/security/cookies/third-party-cookie-blocking.html Remove "LayoutTests/". > Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 > + if (auto cookies = cookieJarDB.searchCookies(firstParty.string())) { IIUC, this should call five arguments version of searchCookies. CookieJarDB::searchCookies(const String& firstParty, const String& requestUrl, const Optional<bool>& httpOnly, const Optional<bool>& secure, const Optional<bool>& session) > Source/WebCore/platform/network/curl/CookieJarDB.cpp:310 > +bool CookieJarDB::checkCookieAcceptPolicy(const String& firstParty, const String& url) It seems better to take the arguments as URL instead of String. How do you think? > Source/WebCore/platform/network/curl/CookieJarDB.cpp:338 > + // Set same url not to call checkCookieAcceptPolicy() infinitely. I think searchCookies should be renamed instead of having this comment. For example, searchCookiesByUrl or hasCookies? > Source/WebCore/platform/network/curl/CookieUtil.cpp:93 > +#else Why do we need the code for !ENABLE(PUBLIC_SUFFIX_LIST)? It seems that AppleWin is the only port not enabling PUBLIC_SUFFIX_LIST. I think we can assume ENABLE(PUBLIC_SUFFIX_LIST) in libcurl integration. > LayoutTests/platform/wincairo/TestExpectations:939 > +http/tests/security/cookies/third-party-cookie-blocking.html [ Pass Failure ] "[ Pass Failure ]" means these tests are flaky. Do you know the reason why these tests are failing so flaky?
Comment on attachment 362288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362288&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:327 > + auto result = searchCookies(url); If I understand Bug 193458 comment 0 correctly, this is not right. You should check cookie existence of eTLD+1. For example, http://ad.example.com is a URL we are about to request, we should check if we have cookies of example.com already.
Comment on attachment 362288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362288&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:457 > +bool CookieJarDB::canAcceptCookie(const Cookie& cookie, const String& firstParty, const String& host, CookieJarDB::Source source) It seems that canAcceptCookie should take a URL instead of host to avoid unnecessary string building. > Source/WebCore/platform/network/curl/CookieJarDB.cpp:472 > + URL cookieDomainUrl({ }, makeString("http://", cookieDomain)); checkCookieAcceptPolicy takes String argument at the moment. Why do you convert String to URL?
(In reply to Fujii Hironori from comment #10) > > > LayoutTests/platform/wincairo/TestExpectations:939 > > +http/tests/security/cookies/third-party-cookie-blocking.html [ Pass Failure ] > > "[ Pass Failure ]" means these tests are flaky. Do you know the reason why > these tests are failing so flaky? Thank you for your review. I was mistaken about TestExpectations. These tests are not flaky. On TestRunner these tests always pass and on DumpRenderTree does not. This is because DRT doesn't have the implementation of setAlwaysAcceptCookies(). This patch aims to fix these tests, but regression tests on DRT will fail when we add [Pass] lines to TestExpectations. How is it appropreate to change that?
We don't plan to maintain TestExpectations for WinCairo WK1 and WK2. And, current TestExpectations is only for WinCairo WK1. You don't need to update TestExpectations in this change. BTW, We plan to switch TestExpectations from WK1 to WK2.
Created attachment 362484 [details] Patch
(In reply to Fujii Hironori from comment #10) >> Source/WebCore/ChangeLog:16 >> + LayoutTests/http/tests/security/cookies/third-party-cookie-blocking.html > >Remove "LayoutTests/". Removed. >> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 >> + if (auto cookies = cookieJarDB.searchCookies(firstParty.string())) { > >IIUC, this should call five arguments version of searchCookies. >CookieJarDB::searchCookies(const String& firstParty, const String& requestUrl, const Optional<bool>& httpOnly, const Optional<bool>& secure, const Optional<bool>& session) Fixed. >> Source/WebCore/platform/network/curl/CookieJarDB.cpp:310 >> +bool CookieJarDB::checkCookieAcceptPolicy(const String& firstParty, const String& url) > >It seems better to take the arguments as URL instead of String. How do you think? Changed to use URL. >> Source/WebCore/platform/network/curl/CookieJarDB.cpp:338 >> + // Set same url not to call checkCookieAcceptPolicy() infinitely. > >I think searchCookies should be renamed instead of having this comment. For example, searchCookiesByUrl or hasCookies? Fixed. >> Source/WebCore/platform/network/curl/CookieUtil.cpp:93 >> +#else > >Why do we need the code for !ENABLE(PUBLIC_SUFFIX_LIST)? It seems that AppleWin is the only port not enabling PUBLIC_SUFFIX_LIST. I think we can assume ENABLE(PUBLIC_SUFFIX_LIST) in libcurl integration. Removed ENABLE(PUBLIC_SUFFIX_LIST). >> LayoutTests/platform/wincairo/TestExpectations:939 >> +http/tests/security/cookies/third-party-cookie-blocking.html [ Pass Failure ] > >"[ Pass Failure ]" means these tests are flaky. Do you know the reason why these tests are failing so flaky? Reverted.
(In reply to Fujii Hironori from comment #11) > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:327 > > + auto result = searchCookies(url); > > If I understand Bug 193458 comment 0 correctly, this is not right. You > should check cookie existence of eTLD+1. > For example, http://ad.example.com is a URL we are about to request, we > should check if we have cookies of example.com already. Fixed. hasCookies checks it.
(In reply to Fujii Hironori from comment #12) > Comment on attachment 362288 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362288&action=review > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:457 > > +bool CookieJarDB::canAcceptCookie(const Cookie& cookie, const String& firstParty, const String& host, CookieJarDB::Source source) > > It seems that canAcceptCookie should take a URL instead of host to avoid > unnecessary string building. Fixed to use URL. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:472 > > + URL cookieDomainUrl({ }, makeString("http://", cookieDomain)); > > checkCookieAcceptPolicy takes String argument at the moment. Why do you > convert String to URL? Fixed. In new patch arguments of checkCookieAcceptPolicy and searchCookies are changed to URL.
Comment on attachment 362484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362484&action=review > Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 > + if (auto cookies = cookieJarDB.searchCookies(firstParty, firstParty, WTF::nullopt, WTF::nullopt, WTF::nullopt)) { Why don't you pass the fourth argument (URL) to the second parameter of searchCookies? Is there a test case for this? > Source/WebCore/platform/network/curl/CookieJarDB.cpp:333 > + String urlHost = url.host().toString().convertToASCIILowercase(); This can be "url.host().convertToASCIILowercase()". > Source/WebCore/platform/network/curl/CookieJarDB.cpp:346 > + statement.bindText(2, String("*.") + topPrivateDomain); I think "*.example.com" doesn't match "example.com". Is this OK? > Source/WebCore/platform/network/curl/CookieJarDB.cpp:362 > + String requestHost = requestUrl.host().toString().convertToASCIILowercase(); This can be requestUrl.host().convertToASCIILowercase(). > Source/WebCore/platform/network/curl/CookieJarDB.cpp:471 > + if (!checkCookieAcceptPolicy(firstParty, cookieDomainUrl)) Do you need make a new URL? How about checkCookieAcceptPolicy(firstParty, url)? > Source/WebCore/platform/network/curl/CookieUtil.cpp:93 > + if (index != WTF::notFound && index + firstPartyDomain.length() == urlHost.length()) This doesn't seem right. "example.com" can match "xexample.com".
> > Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 > > + if (auto cookies = cookieJarDB.searchCookies(firstParty, firstParty, WTF::nullopt, WTF::nullopt, WTF::nullopt)) { > > Why don't you pass the fourth argument (URL) to the second parameter of > searchCookies? > Is there a test case for this? > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:346 > > + statement.bindText(2, String("*.") + topPrivateDomain); > > I think "*.example.com" doesn't match "example.com". Is this OK? > > > Source/WebCore/platform/network/curl/CookieUtil.cpp:93 > > + if (index != WTF::notFound && index + firstPartyDomain.length() == urlHost.length()) > > This doesn't seem right. "example.com" can match "xexample.com". I think these cases should be tested by unittest. Can you add that?
Comment on attachment 362484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362484&action=review > Tools/TestWebKitAPI/Tests/WebCore/curl/Cookies.cpp:91 > } Oh, I'm sorry, you've already have that. I don't think these refactoring is good. Unit test should be simple, easy to read even if it seems verbose. Think about this. As time goes by, the test grows. We will see the test which has different firstParty and url (of course it should). Then we have to see both test line and variable. We don't get any clue from test result output. Better way to manage the test with bunch of data set is using value parameterized tests. (https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#value-parameterized-tests).
Created attachment 362682 [details] Patch
Created attachment 362684 [details] Patch
(In reply to Fujii Hironori from comment #19) > Comment on attachment 362484 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362484&action=review > > > Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 > > + if (auto cookies = cookieJarDB.searchCookies(firstParty, firstParty, WTF::nullopt, WTF::nullopt, WTF::nullopt)) { > > Why don't you pass the fourth argument (URL) to the second parameter of > searchCookies? > Is there a test case for this? There is no test cases for searchCookies() now. I'll add test cases in another ticket. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:333 > > + String urlHost = url.host().toString().convertToASCIILowercase(); > > This can be "url.host().convertToASCIILowercase()". Fixed. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:346 > > + statement.bindText(2, String("*.") + topPrivateDomain); > > I think "*.example.com" doesn't match "example.com". Is this OK? "example.com" matches the SQL query below. >#define CHECK_EXISTS_COOKIE_SQL \ > "SELECT domain FROM Cookie WHERE ((domain = ?) OR (domain GLOB ?));" > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:362 > > + String requestHost = requestUrl.host().toString().convertToASCIILowercase(); > > This can be requestUrl.host().convertToASCIILowercase(). Fixed. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:471 > > + if (!checkCookieAcceptPolicy(firstParty, cookieDomainUrl)) > > Do you need make a new URL? How about checkCookieAcceptPolicy(firstParty, > url)? Fixed. > > Source/WebCore/platform/network/curl/CookieUtil.cpp:93 > > + if (index != WTF::notFound && index + firstPartyDomain.length() == urlHost.length()) > > This doesn't seem right. "example.com" can match "xexample.com". Fixed.
Comment on attachment 362684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362684&action=review Bug 194791 is going to change all call site of topPrivatelyControlledDomain. I think you should wait for landing the patch. > Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 > + if (auto cookies = cookieJarDB.searchCookies(firstParty, firstParty, WTF::nullopt, WTF::nullopt, WTF::nullopt)) { Why don't you pass the fourth argument (URL) to the second parameter of searchCookies? See the implementations of other ports. They are using the fourth argument.
Comment on attachment 362684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362684&action=review > Source/WebCore/platform/network/curl/CookieUtil.cpp:88 > + auto index = host.reverseFind(domain); You should use String.endsWith.
Comment on attachment 362684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362684&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:341 > + if (CookieUtil::isIPAddress(host) || !host.contains('.') || topPrivateDomain.isEmpty()) { I think you don't need to do SQL query if url doesn't have a registerable domain name. if (topPrivateDomain.isEmpty()) return false; Can Browers store cookies of domains containing no period? if (!host.contains('.')) return false;
Created attachment 362979 [details] Patch
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:316 > + if (m_acceptPolicy == CookieAcceptPolicy::OnlyFromMainDocumentDomain || m_acceptPolicy == CookieAcceptPolicy::ExclusivelyFromMainDocumentDomain) { Early return would be better.
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review Some informal review comments I had. > Source/WebCore/platform/network/curl/CookieJarDB.cpp:-37 > -#include "PublicSuffix.h" This should still include PublicSuffix.h since there's a call to isPublicSuffix. > Source/WebCore/platform/network/curl/CookieUtil.cpp:136 > + result.path = attributeValue.convertToASCIILowercase(); I don't think the path attribute value should be canonicalized to lower case. rfc6265 only says to canonicalize the domain attribute value to lower case and major browsers don't convert the path to lower case either. The original `requestUrlObj.path().convertToASCIILowercase()` conversion in searchCookies seems wrong. > Source/WebCore/platform/network/curl/PublicSuffixCurl.cpp:-29 > -#if ENABLE(PUBLIC_SUFFIX_LIST) This ENABLE(PUBLIC_SUFFIX_LIST) check should be kept. It's common in WebKit to have ENABLE checks in their respective implementaion files.
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review > Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 > + if (auto cookies = cookieJarDB.searchCookies(firstParty, url, WTF::nullopt, WTF::nullopt, WTF::nullopt)) { This is a bug fix for a separate issue. Does this bug have a actual problem in WinCairo port? >> Source/WebCore/platform/network/curl/CookieJarDB.cpp:-37 >> -#include "PublicSuffix.h" > > This should still include PublicSuffix.h since there's a call to isPublicSuffix. I got a review feedback I should remove such #include which is included other headers in the past. PublicSuffix.h is already included in RegistrableDomain.h. > Source/WebCore/platform/network/curl/CookieJarDB.cpp:395 > + pstmt->bindText(5, String("*.") + registrableDomain.string()); These code should match with the one of CookieJarDB::hasCookies. >> Source/WebCore/platform/network/curl/PublicSuffixCurl.cpp:-29 >> -#if ENABLE(PUBLIC_SUFFIX_LIST) > > This ENABLE(PUBLIC_SUFFIX_LIST) check should be kept. It's common in WebKit to have ENABLE checks in their respective implementaion files. I don't think so because this is a curl port specific file and all curl ports are defining ENABLE_PUBLIC_SUFFIX_LIST.
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:339 > + return false; Oh, this returns too early. This makes the following condition 'CookieUtil::isIPAddress(host) || !host.contains('.')' useless. My comment 27 was bad. I think you should use isPublicSuffix as well as CookieJarDB::canAcceptCookie does. if (isPublicSuffix(host)) return false; if (registrableDomain.isEmpty()) { statement.bindText(1, host); statement.bindNull(2); } else { statement.bindText(1, registrableDomain.string()); statement.bindText(2, String("*.") + registrableDomain.string()); }
(In reply to Basuke Suzuki from comment #29) > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:316 > > + if (m_acceptPolicy == CookieAcceptPolicy::OnlyFromMainDocumentDomain || m_acceptPolicy == CookieAcceptPolicy::ExclusivelyFromMainDocumentDomain) { > > Early return would be better. I'll fix it.
(In reply to Christopher Reid from comment #30) > I don't think the path attribute value should be canonicalized to lower > case. rfc6265 only says to canonicalize the domain attribute value to lower > case and major browsers don't convert the path to lower case either. The > original `requestUrlObj.path().convertToASCIILowercase()` conversion in > searchCookies seems wrong. > Thanks for comment! I'll fix it.
(In reply to Fujii Hironori from comment #31) > Comment on attachment 362979 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362979&action=review > > > Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 > > + if (auto cookies = cookieJarDB.searchCookies(firstParty, url, WTF::nullopt, WTF::nullopt, WTF::nullopt)) { > > This is a bug fix for a separate issue. Does this bug have a actual problem > in WinCairo port? > The contents of the cookie displayed in WebInspector are incorrect in WinCairo port. (See [WebInspector] -> [storage] -> [cookies]) There is no other problem.
(In reply to Fujii Hironori from comment #32) > Comment on attachment 362979 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362979&action=review > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:339 > > + return false; > > Oh, this returns too early. This makes the following condition > 'CookieUtil::isIPAddress(host) || !host.contains('.')' useless. RegistrableDomain returns a ip address or 'localhost'. So the condition is valid. See constructor of RegistrableDomain in RegistrableDomain.h and topPrivatelyControlledDomain() in PublicSuffixCurl.cpp.
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review >>> Source/WebCore/platform/network/curl/PublicSuffixCurl.cpp:-29 >>> -#if ENABLE(PUBLIC_SUFFIX_LIST) >> >> This ENABLE(PUBLIC_SUFFIX_LIST) check should be kept. It's common in WebKit to have ENABLE checks in their respective implementaion files. > > I don't think so because this is a curl port specific file and all curl ports are defining ENABLE_PUBLIC_SUFFIX_LIST. Isn't that also the case for PublicSuffixMac.mm and PublicSuffixSoup.cpp? Those files also have the enable check. I'm not sure what we gain by removing the check in this file and removing it makes disabling the flag on curl more involved.
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review >>> Source/WebCore/platform/network/curl/CookieJarDB.cpp:339 >>> + return false; >> >> Oh, this returns too early. This makes the following condition 'CookieUtil::isIPAddress(host) || !host.contains('.')' useless. >> My comment 27 was bad. I think you should use isPublicSuffix as well as CookieJarDB::canAcceptCookie does. >> >> if (isPublicSuffix(host)) >> return false; >> >> if (registrableDomain.isEmpty()) { >> statement.bindText(1, host); >> statement.bindNull(2); >> } else { >> statement.bindText(1, registrableDomain.string()); >> statement.bindText(2, String("*.") + registrableDomain.string()); >> } > > RegistrableDomain returns a ip address or 'localhost'. > So the condition is valid. > > See constructor of RegistrableDomain in RegistrableDomain.h and topPrivatelyControlledDomain() in PublicSuffixCurl.cpp. That's right. That means we can't distinguish if a RegistrableDomain is eTLD+1 or unkown TLD. I'm wondering this can be a problem... >>>> Source/WebCore/platform/network/curl/PublicSuffixCurl.cpp:-29 >>>> -#if ENABLE(PUBLIC_SUFFIX_LIST) >>> >>> This ENABLE(PUBLIC_SUFFIX_LIST) check should be kept. It's common in WebKit to have ENABLE checks in their respective implementaion files. >> >> I don't think so because this is a curl port specific file and all curl ports are defining ENABLE_PUBLIC_SUFFIX_LIST. > > Isn't that also the case for PublicSuffixMac.mm and PublicSuffixSoup.cpp? Those files also have the enable check. I'm not sure what we gain by removing the check in this file and removing it makes disabling the flag on curl more involved. I get your idea. I don't have a strong opinion. Other *Curl.cpp files are using "#if USE(CURL)". How about it?
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review >>>>> Source/WebCore/platform/network/curl/PublicSuffixCurl.cpp:-29 >>>>> -#if ENABLE(PUBLIC_SUFFIX_LIST) >>>> >>>> This ENABLE(PUBLIC_SUFFIX_LIST) check should be kept. It's common in WebKit to have ENABLE checks in their respective implementaion files. >>> >>> I don't think so because this is a curl port specific file and all curl ports are defining ENABLE_PUBLIC_SUFFIX_LIST. >> >> Isn't that also the case for PublicSuffixMac.mm and PublicSuffixSoup.cpp? Those files also have the enable check. I'm not sure what we gain by removing the check in this file and removing it makes disabling the flag on curl more involved. > > I get your idea. I don't have a strong opinion. Other *Curl.cpp files are using "#if USE(CURL)". How about it? This is the long lasting question for me. Why do Curl implementation file check the USE(CURL)?
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review >>>>>> Source/WebCore/platform/network/curl/PublicSuffixCurl.cpp:-29 >>>>>> -#if ENABLE(PUBLIC_SUFFIX_LIST) >>>>> >>>>> This ENABLE(PUBLIC_SUFFIX_LIST) check should be kept. It's common in WebKit to have ENABLE checks in their respective implementaion files. >>>> >>>> I don't think so because this is a curl port specific file and all curl ports are defining ENABLE_PUBLIC_SUFFIX_LIST. >>> >>> Isn't that also the case for PublicSuffixMac.mm and PublicSuffixSoup.cpp? Those files also have the enable check. I'm not sure what we gain by removing the check in this file and removing it makes disabling the flag on curl more involved. >> >> I get your idea. I don't have a strong opinion. Other *Curl.cpp files are using "#if USE(CURL)". How about it? > > This is the long lasting question for me. Why do Curl implementation file check the USE(CURL)? Yeah I guess cmake handles USE(CURL) for curl implementation files. I think for this case, these files don't depend on libcurl and aren't implementing curl network layer abstractions so I'm not sure if USE(CURL) applies.
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review >>>>>>> Source/WebCore/platform/network/curl/PublicSuffixCurl.cpp:-29 >>>>>>> -#if ENABLE(PUBLIC_SUFFIX_LIST) >>>>>> >>>>>> This ENABLE(PUBLIC_SUFFIX_LIST) check should be kept. It's common in WebKit to have ENABLE checks in their respective implementaion files. >>>>> >>>>> I don't think so because this is a curl port specific file and all curl ports are defining ENABLE_PUBLIC_SUFFIX_LIST. >>>> >>>> Isn't that also the case for PublicSuffixMac.mm and PublicSuffixSoup.cpp? Those files also have the enable check. I'm not sure what we gain by removing the check in this file and removing it makes disabling the flag on curl more involved. >>> >>> I get your idea. I don't have a strong opinion. Other *Curl.cpp files are using "#if USE(CURL)". How about it? >> >> This is the long lasting question for me. Why do Curl implementation file check the USE(CURL)? > > Yeah I guess cmake handles USE(CURL) for curl implementation files. I think for this case, these files don't depend on libcurl and aren't implementing curl network layer abstractions so I'm not sure if USE(CURL) applies. Good point. In my understanding, it is a practice for Xcode. We don't need such conditions in CMake-using ports.
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:337 > + RegistrableDomain registrableDomain { host }; This ctor is removed in Bug 195071.
Comment on attachment 362979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362979&action=review > Source/WebCore/platform/network/curl/CookieJarDB.h:44 > +enum class CookieAcceptPolicy { > + Always, > + Never, > + OnlyFromMainDocumentDomain, > + ExclusivelyFromMainDocumentDomain > +}; Why not just use the existing HTTPCookieAcceptPolicy enum? > Source/WebKit/NetworkProcess/Cookies/curl/WebCookieManagerCurl.cpp:56 > + m_process.forEachNetworkStorageSession([curlPolicy] (const auto& networkStorageSession) { > + networkStorageSession.cookieStorage().setCookieAcceptPolicy(networkStorageSession, curlPolicy); > + }); If my patch in bug #195140 lands first, then you'll need to change this to something like: if (auto* networkStorageSession = m_process.networkStorageSession()) networkStorageSession.cookieStorage().setCookieAcceptPolicy(networkStorageSession, curlPolicy);
Er: if (auto* networkStorageSession = m_process.networkStorageSession(sessionID)) networkStorageSession.cookieStorage().setCookieAcceptPolicy(networkStorageSession, curlPolicy); And then in WebCookieManager::platformGetHTTPCookieAcceptPolicy, instead of using m_process.defaultStorageSession, again use m_process.networkStorageSession(sessionID). Again, only if my patch in bug #195140 lands first (if yours lands first, then I'll handle it in mine).
(In reply to Michael Catanzaro from comment #43) > Comment on attachment 362979 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362979&action=review > > > Source/WebCore/platform/network/curl/CookieJarDB.h:44 > > +enum class CookieAcceptPolicy { > > + Always, > > + Never, > > + OnlyFromMainDocumentDomain, > > + ExclusivelyFromMainDocumentDomain > > +}; > > Why not just use the existing HTTPCookieAcceptPolicy enum? > Curl port is used in WebKit and WebKitLegacy.But HTTPCookieAcceptPolicy enum is defined in WebKit layer. And we shouldn't use it for WebKitLegacy. So we should define AcceptPolicy in curl port.
(In reply to Michael Catanzaro from comment #44) > if (auto* networkStorageSession = m_process.networkStorageSession(sessionID)) > > networkStorageSession.cookieStorage(). > setCookieAcceptPolicy(networkStorageSession, curlPolicy); > > And then in WebCookieManager::platformGetHTTPCookieAcceptPolicy, instead of > using m_process.defaultStorageSession, again use > m_process.networkStorageSession(sessionID). Again, only if my patch in bug > #195140 lands first (if yours lands first, then I'll handle it in mine). Thank you for your information. I understand.
Created attachment 363211 [details] Patch
(In reply to Christopher Reid from comment #30) > > Source/WebCore/platform/network/curl/PublicSuffixCurl.cpp:-29 > > -#if ENABLE(PUBLIC_SUFFIX_LIST) > > This ENABLE(PUBLIC_SUFFIX_LIST) check should be kept. It's common in WebKit > to have ENABLE checks in their respective implementaion files. Fixed it.
(In reply to Fujii Hironori from comment #31) > > Source/WebCore/platform/network/curl/CookieJarCurl.cpp:122 > > + if (auto cookies = cookieJarDB.searchCookies(firstParty, url, WTF::nullopt, WTF::nullopt, WTF::nullopt)) { > > This is a bug fix for a separate issue. Does this bug have a actual problem > in WinCairo port? > I will fix this bug as a separate issue. So in this patch takes searchCookie(firstParty, firstParty, ...) > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:395 > > + pstmt->bindText(5, String("*.") + registrableDomain.string()); > > These code should match with the one of CookieJarDB::hasCookies. > Do you mean to use registrableDoman to bindText(4) ? hasCookies() should check cookie existence of eTLD+1. But searchCookies() is not.
(In reply to Fujii Hironori from comment #32) > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:339 > > + return false; > > Oh, this returns too early. This makes the following condition > 'CookieUtil::isIPAddress(host) || !host.contains('.')' useless. > My comment 27 was bad. I think you should use isPublicSuffix as well as > CookieJarDB::canAcceptCookie does. > > if (isPublicSuffix(host)) > return false; > > if (registrableDomain.isEmpty()) { > statement.bindText(1, host); > statement.bindNull(2); > } else { > statement.bindText(1, registrableDomain.string()); > statement.bindText(2, String("*.") + registrableDomain.string()); > } RegistrableDomain returns a IP address or 'localhost'. So, we should check below. if (CookieUtil::isIPAddress(host) || !host.contains('.') || registrableDomain.isEmpty()) { .... } else { .... }
(In reply to Fujii Hironori from comment #42) > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:337 > > + RegistrableDomain registrableDomain { host }; > > This ctor is removed in Bug 195071. Fixed.
Comment on attachment 363211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363211&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:338 > + if (isPublicSuffix(url)) isPublicSuffix takes domain name, not url. > Source/WebCore/platform/network/curl/CookieJarDB.cpp:395 > + pstmt->bindText(5, String("*.") + registrableDomain.string()); Why do you need to search all cookies for the eTLD+1? For example, cookies from http://host1.example.com which doesn't specify Domain parameter shouldn't be passes to http://host2.example.com.
Comment on attachment 363211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363211&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:319 > + if (firstParty == url) Checking URLs seems useless. How about checking hosts? if (firstParty.host() == url.host()) > Source/WebCore/platform/network/curl/CookieJarDB.cpp:360 > + if (!checkCookieAcceptPolicy(firstParty, requestUrl)) I think this condition should be moved after the following condition "if (requestHost.isEmpty())" for efficiency.
Comment on attachment 363211 [details] Patch Attachment 363211 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11316555 New failing tests: storage/indexeddb/modern/blocked-open-db-requests-private.html
Created attachment 363220 [details] Archive of layout-test-results from ews113 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 363211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363211&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:390 > pstmt->bindText(4, requestHost); Is this right to specify 'requestHost'? For example, you visited "http://host1.example.com", that response set a cookie with 'Domain=example.com' parameter. Then, you visit "http://example.com". In this time you should send the cookie. However SQL query is: domain = host1.example.com OR domain GLOB "*.example.com" this query doesn't match "example.com".
Comment on attachment 363211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363211&action=review >> Source/WebCore/platform/network/curl/CookieJarDB.cpp:390 >> pstmt->bindText(4, requestHost); > > Is this right to specify 'requestHost'? > For example, you visited "http://host1.example.com", that response set a cookie with 'Domain=example.com' parameter. > Then, you visit "http://example.com". > In this time you should send the cookie. However SQL query is: > domain = host1.example.com OR domain GLOB "*.example.com" > this query doesn't match "example.com". If host1.example.com sets a cookie with "domain=example.com", the domain attribute parser enforces a leading dot and the domain value would become ".example.com". RFC6265 explains that if the domain attribute is specified, then the cookie must apply to all sub-domains and if the cookie isn't specified then it must apply only to the same origin https://tools.ietf.org/html/rfc6265#section-4.1.2.3. To support that requirement, in CookieUtil::parseCookieAttributes we enforce a leading dot on specified domain attribute values and the leading dot means the cookie will apply to all sub domains. In this case the cookie domain value would become ".example.com". That matches what other major browsers do. If there isn't a domain attribute specified, the cookie domain would be set without a leading dot i.e. "host1.example.com". >> Source/WebCore/platform/network/curl/CookieJarDB.cpp:395 >> + pstmt->bindText(5, String("*.") + registrableDomain.string()); > > Why do you need to search all cookies for the eTLD+1? > For example, cookies from http://host1.example.com which doesn't specify Domain parameter shouldn't be passes to http://host2.example.com. This query can return too many domains but there's domain matching that should filter those out later on in this function with `if (!CookieUtil::domainMatch(cookieDomain, requestHost))`. "host1.host2.example.com" needs to match cookies for host1.host2.example.com, ".host2.example.com", and ".example.com". I'm not sure if there's a good way to do that domain matching purely in SQL.
(In reply to Christopher Reid from comment #57) > "host1.host2.example.com" needs to match cookies for > host1.host2.example.com, ".host2.example.com", and ".example.com". Oh, it also has to match cookies with a domain of ".host1.host2.example.com"
Thank you for the explanation, Christopher. It makes sense.
Created attachment 363499 [details] Patch
(In reply to Fujii Hironori from comment #52) > Comment on attachment 363211 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363211&action=review > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:338 > > + if (isPublicSuffix(url)) > > isPublicSuffix takes domain name, not url. > Fixed.
(In reply to Fujii Hironori from comment #53) > Comment on attachment 363211 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363211&action=review > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:319 > > + if (firstParty == url) > > Checking URLs seems useless. How about checking hosts? > if (firstParty.host() == url.host()) Fixed. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:360 > > + if (!checkCookieAcceptPolicy(firstParty, requestUrl)) > > I think this condition should be moved after the following condition "if > (requestHost.isEmpty())" for efficiency. Fixed.
Comment on attachment 363499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363499&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:338 > + if (isPublicSuffix(url.host().toString())) Can you use 'host'? if (isPublicSuffix(host))
Created attachment 363580 [details] Patch
(In reply to Fujii Hironori from comment #63) > Comment on attachment 363499 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363499&action=review > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:338 > > + if (isPublicSuffix(url.host().toString())) > > Can you use 'host'? > if (isPublicSuffix(host)) FIXED.
Comment on attachment 363580 [details] Patch Looks good to me.
Comment on attachment 363580 [details] Patch Attachment 363580 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11371009 New failing tests: fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html editing/selection/thai-word-at-document-end.html
Created attachment 363593 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 363599 [details] Patch
Comment on attachment 363599 [details] Patch Attachment 363599 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11374068 New failing tests: fast/shadow-dom/color-input-element-shadow-manipulation.html fast/shadow-dom/click-on-slotted-anchor-with-hover.html fast/shadow-dom/click-text-inside-linked-slot.html editing/selection/thai-word-at-document-end.html
Created attachment 363614 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 363599 [details] Patch Clearing flags on attachment: 363599 Committed r242517: <https://trac.webkit.org/changeset/242517>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48617899>