Bug 191645 - [Curl] Implement Cookie Accept Policy.
Summary: [Curl] Implement Cookie Accept Policy.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-14 12:28 PST by Basuke Suzuki
Modified: 2019-03-05 15:38 PST (History)
14 users (show)

See Also:


Attachments
Patch (10.64 KB, patch)
2019-02-07 00:29 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (30.21 KB, patch)
2019-02-18 02:45 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (30.21 KB, patch)
2019-02-18 02:46 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (30.83 KB, patch)
2019-02-18 04:11 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (31.78 KB, patch)
2019-02-19 23:51 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (33.26 KB, patch)
2019-02-21 18:54 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (33.26 KB, patch)
2019-02-21 19:21 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (35.81 KB, patch)
2019-02-26 06:30 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (34.94 KB, patch)
2019-02-28 01:15 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-highsierra (2.18 MB, application/zip)
2019-02-28 05:35 PST, Build Bot
no flags Details
Patch (34.99 KB, patch)
2019-03-04 01:04 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (34.77 KB, patch)
2019-03-04 18:38 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (3.10 MB, application/zip)
2019-03-04 20:46 PST, Build Bot
no flags Details
Patch (34.77 KB, patch)
2019-03-04 22:02 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (10.55 MB, application/zip)
2019-03-05 00:37 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2018-11-14 12:28:04 PST
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.
Comment 1 Takashi Komori 2019-02-07 00:29:03 PST
Created attachment 361383 [details]
Patch
Comment 2 Takashi Komori 2019-02-07 00:33:16 PST
Implemented function of telling cookie accept policy to Curl network layer.
Comment 3 Fujii Hironori 2019-02-07 01:44:15 PST
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.
Comment 4 Takashi Komori 2019-02-08 04:47:28 PST
(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 5 Fujii Hironori 2019-02-08 05:27:03 PST
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.
Comment 6 Takashi Komori 2019-02-18 02:45:16 PST
Created attachment 362282 [details]
Patch
Comment 7 Takashi Komori 2019-02-18 02:46:52 PST
Created attachment 362283 [details]
Patch
Comment 8 Takashi Komori 2019-02-18 04:11:35 PST
Created attachment 362288 [details]
Patch
Comment 9 Takashi Komori 2019-02-18 04:24:19 PST
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 10 Fujii Hironori 2019-02-18 18:34:26 PST
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 11 Fujii Hironori 2019-02-18 19:07:21 PST
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 12 Fujii Hironori 2019-02-18 19:18:55 PST
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?
Comment 13 Takashi Komori 2019-02-18 20:03:19 PST
(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?
Comment 14 Fujii Hironori 2019-02-18 21:20:57 PST
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.
Comment 15 Takashi Komori 2019-02-19 23:51:58 PST
Created attachment 362484 [details]
Patch
Comment 16 Takashi Komori 2019-02-19 23:53:40 PST
(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.
Comment 17 Takashi Komori 2019-02-19 23:57:15 PST
(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.
Comment 18 Takashi Komori 2019-02-19 23:59:03 PST
(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 19 Fujii Hironori 2019-02-20 02:22:19 PST
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".
Comment 20 Basuke Suzuki 2019-02-20 07:16:40 PST
> > 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 21 Basuke Suzuki 2019-02-20 07:32:41 PST
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).
Comment 22 Takashi Komori 2019-02-21 18:54:07 PST
Created attachment 362682 [details]
Patch
Comment 23 Takashi Komori 2019-02-21 19:21:31 PST
Created attachment 362684 [details]
Patch
Comment 24 Takashi Komori 2019-02-21 19:25:12 PST
(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 25 Fujii Hironori 2019-02-22 01:15:45 PST
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 26 Fujii Hironori 2019-02-22 01:22:27 PST
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 27 Fujii Hironori 2019-02-22 01:39:14 PST
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;
Comment 28 Takashi Komori 2019-02-26 06:30:18 PST
Created attachment 362979 [details]
Patch
Comment 29 Basuke Suzuki 2019-02-26 11:45:17 PST
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 30 Christopher Reid 2019-02-26 13:20:19 PST
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 31 Fujii Hironori 2019-02-26 19:04:14 PST
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 32 Fujii Hironori 2019-02-26 19:07:57 PST
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());
}
Comment 33 Takashi Komori 2019-02-26 22:29:51 PST
(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.
Comment 34 Takashi Komori 2019-02-26 22:34:13 PST
(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.
Comment 35 Takashi Komori 2019-02-26 22:36:50 PST
(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.
Comment 36 Takashi Komori 2019-02-26 22:37:52 PST
(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 37 Christopher Reid 2019-02-26 23:24:51 PST
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 38 Fujii Hironori 2019-02-27 03:03:21 PST
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 39 Basuke Suzuki 2019-02-27 14:02:32 PST
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 40 Christopher Reid 2019-02-27 14:50:02 PST
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 41 Fujii Hironori 2019-02-27 17:17:01 PST
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 42 Fujii Hironori 2019-02-27 17:18:20 PST
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 43 Michael Catanzaro 2019-02-27 19:25:03 PST
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);
Comment 44 Michael Catanzaro 2019-02-27 19:27:26 PST
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).
Comment 45 Takashi Komori 2019-02-27 22:22:57 PST
(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.
Comment 46 Takashi Komori 2019-02-27 22:26:48 PST
(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.
Comment 47 Takashi Komori 2019-02-28 01:15:31 PST
Created attachment 363211 [details]
Patch
Comment 48 Takashi Komori 2019-02-28 01:17:06 PST
(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.
Comment 49 Takashi Komori 2019-02-28 01:20:34 PST
(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.
Comment 50 Takashi Komori 2019-02-28 01:21:33 PST
(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 {
    ....
}
Comment 51 Takashi Komori 2019-02-28 01:26:33 PST
(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 52 Fujii Hironori 2019-02-28 03:12:13 PST
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 53 Fujii Hironori 2019-02-28 03:24:47 PST
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 54 Build Bot 2019-02-28 05:35:18 PST
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
Comment 55 Build Bot 2019-02-28 05:35:20 PST
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 56 Fujii Hironori 2019-02-28 05:53:36 PST
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 57 Christopher Reid 2019-02-28 13:36:24 PST
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.
Comment 58 Christopher Reid 2019-02-28 14:49:40 PST
(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"
Comment 59 Fujii Hironori 2019-02-28 22:01:59 PST
Thank you for the explanation, Christopher. It makes sense.
Comment 60 Takashi Komori 2019-03-04 01:04:58 PST
Created attachment 363499 [details]
Patch
Comment 61 Takashi Komori 2019-03-04 01:08:18 PST
(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.
Comment 62 Takashi Komori 2019-03-04 01:09:35 PST
(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 63 Fujii Hironori 2019-03-04 02:59:15 PST
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))
Comment 64 Takashi Komori 2019-03-04 18:38:09 PST
Created attachment 363580 [details]
Patch
Comment 65 Takashi Komori 2019-03-04 18:39:06 PST
(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 66 Fujii Hironori 2019-03-04 18:54:19 PST
Comment on attachment 363580 [details]
Patch

Looks good to me.
Comment 67 Build Bot 2019-03-04 20:45:56 PST
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
Comment 68 Build Bot 2019-03-04 20:46:04 PST
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
Comment 69 Takashi Komori 2019-03-04 22:02:38 PST
Created attachment 363599 [details]
Patch
Comment 70 Build Bot 2019-03-05 00:36:58 PST
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
Comment 71 Build Bot 2019-03-05 00:37:01 PST
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 72 WebKit Commit Bot 2019-03-05 15:37:02 PST
Comment on attachment 363599 [details]
Patch

Clearing flags on attachment: 363599

Committed r242517: <https://trac.webkit.org/changeset/242517>
Comment 73 WebKit Commit Bot 2019-03-05 15:37:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 74 Radar WebKit Bug Importer 2019-03-05 15:38:32 PST
<rdar://problem/48617899>