Bug 191645 – [Curl] Implement Cookie Accept Policy.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

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.

[Takashi Komori]

Created attachment 361383 [details]
Patch

[Takashi Komori]

Implemented function of telling cookie accept policy to Curl network layer.

[Fujii Hironori]

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.

[Takashi Komori]

(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()

[Fujii Hironori]

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.

[Takashi Komori]

Created attachment 362282 [details]
Patch

[Takashi Komori]

Created attachment 362283 [details]
Patch

[Takashi Komori]

Created attachment 362288 [details]
Patch

[Takashi Komori]

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.

[Fujii Hironori]

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?

[Fujii Hironori]

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.

[Fujii Hironori]

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?

[Takashi Komori]

(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?

[Fujii Hironori]

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.

[Takashi Komori]

Created attachment 362484 [details]
Patch

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Fujii Hironori]

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".

[Basuke Suzuki]

> > 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?

[Basuke Suzuki]

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).

[Takashi Komori]

Created attachment 362682 [details]
Patch

[Takashi Komori]

Created attachment 362684 [details]
Patch

[Takashi Komori]

(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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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;

[Takashi Komori]

Created attachment 362979 [details]
Patch

[Basuke Suzuki]

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.

[Christopher Reid]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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());
}

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Christopher Reid]

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.

[Fujii Hironori]

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?

[Basuke Suzuki]

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)?

[Christopher Reid]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Michael Catanzaro]

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);

[Michael Catanzaro]

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).

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

Created attachment 363211 [details]
Patch

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

(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 {
    ....
}

[Takashi Komori]

(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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Fujii Hironori]

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".

[Christopher Reid]

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.

[Christopher Reid]

(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"

[Fujii Hironori]

Thank you for the explanation, Christopher. It makes sense.

[Takashi Komori]

Created attachment 363499 [details]
Patch

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Fujii Hironori]

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))

[Takashi Komori]

Created attachment 363580 [details]
Patch

[Takashi Komori]

(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.

[Fujii Hironori]

Comment on attachment 363580 [details]
Patch

Looks good to me.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Takashi Komori]

Created attachment 363599 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 363599 [details]
Patch

Clearing flags on attachment: 363599

Committed r242517: <https://trac.webkit.org/changeset/242517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48617899>