Bug 191406 – [Curl] Reject entire cookie if the domain fails a tailmatch.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

Currently we don't put domain attribute of cookie when it fails a tailmatch. As Firefox and Chrome do, we are going to reject the entire cookie if the domain fails a tailmatch instead.

[Basuke Suzuki]

The entire cookie needs to be rejected if a site tries to set a cookie that fails these conditions.
https://tools.ietf.org/html/rfc6265#section-5.1.3.

Basically the domain attribute of a cookie must either be an exact match, or a suffix starting at the start of a %2e (".") separated domain part that is not an IP address.

[Basuke Suzuki]

Created attachment 354291 [details]
PATCH

[EWS Watchlist]

Attachment 354291 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/FileSystemWin.cpp:574:  file_op is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/FileSystemWin.cpp:575:  Use nullptr instead of NULL.  [readability/null] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Comment on attachment 354291 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=354291&action=review

Overall looks good minus a couple nits.

> Source/WebCore/platform/FileSystem.h:193
> +#if USE(CURL)
> +WEBCORE_EXPORT String createTemporaryDirectory(String directoryPrefix);
> +WEBCORE_EXPORT bool deleteNonEmptyDirectory(const String&);
> +#endif

Having something be USE(CURL) within FileSystem.h is a little odd. I would say make it based on Windows since you have that implementation.

> Source/WebCore/platform/win/FileSystemWin.cpp:574
> +    SHFILEOPSTRUCT file_op = {

Just rename to make the style checker happy.

> Tools/TestWebKitAPI/Tests/WebCore/curl/Cookies.cpp:73
> +TEST_F(CurlCookies, RejectTailmatchFailureDomain)
> +{
> +    // success: domain match
> +    EXPECT_EQ(0, m_cookieJar->setCookie("http://example.com", "foo=bar; Domain=example.com", false));
> +    // success: wildcard of domains
> +    EXPECT_EQ(0, m_cookieJar->setCookie("http://example.com", "foo=bar; Domain=.example.com", false));
> +    // failure: specific sub domain
> +    EXPECT_EQ(-1, m_cookieJar->setCookie("http://example.com", "foo=bar; Domain=www.example.com", false));
> +    // failure: different domain
> +    EXPECT_EQ(-1, m_cookieJar->setCookie("http://example.com", "foo=bar; Domain=sample.com", false));
> +}

:+1: for tests!

[Basuke Suzuki]

Comment on attachment 354291 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=354291&action=review

Thanks for your comments.

>> Source/WebCore/platform/FileSystem.h:193
>> +#endif
> 
> Having something be USE(CURL) within FileSystem.h is a little odd. I would say make it based on Windows since you have that implementation.

Oh, that's right. I put the code on FileSystemWin.cpp so that it should use PLATFORM(WINCAIRO). Thanks

>> Source/WebCore/platform/win/FileSystemWin.cpp:574
>> +    SHFILEOPSTRUCT file_op = {
> 
> Just rename to make the style checker happy.

Sure. Copy and pasted from MS homepage. My bad.

>> Tools/TestWebKitAPI/Tests/WebCore/curl/Cookies.cpp:73
>> +}
> 
> :+1: for tests!

My first unittest on WebCore!

[Basuke Suzuki]

Created attachment 354369 [details]
PATCH

[youenn fablet]

Comment on attachment 354369 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=354369&action=review

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:460
>  int CookieJarDB::setCookie(const String& url, const String& cookie, bool fromJavaScript)

Could it return a bool instead?
Or are we passing some meaningful error codes?

> Source/WebCore/platform/network/curl/CookieUtil.cpp:140
> +bool parseCookieHeader(const String& cookieLine, Cookie& result)

Might be a follow-up patch but we now like to do things like:
std::optional<Cookie> parseCookieHeader(const String& cookieLine);
or ExceptionOr<Cookie> if there is a need to provide more accurate error handling.

> Source/WebCore/platform/network/curl/CookieUtil.h:37
> +bool parseCookieHeader(const String& cookieLine, Cookie& result);

s/ result//

> Source/WebCore/platform/win/FileSystemWin.cpp:379
> +static String generateTemporaryPath(Function<bool(const String&)> action)

Can action be a const Function&?

> Source/WebCore/platform/win/FileSystemWin.cpp:412
> +    String proposedPath = generateTemporaryPath([&handle](const String proposedPath) {

s/const String/const String&/

> Source/WebCore/platform/win/FileSystemWin.cpp:563
> +String createTemporaryDirectory(String directoryPrefix)

directoryPrefix does not seem to be used.
Also it is a String, should it be a const String&.

> Source/WebCore/platform/win/FileSystemWin.cpp:565
> +    String proposedPath = generateTemporaryPath([](const String proposedPath) {

s/const String/const String&/

> Source/WebCore/platform/win/FileSystemWin.cpp:569
> +    return proposedPath;

Maybe we could remove this temporary variable and directly write 'return generate...'
This would remove the use of two different 'proposedPath' variables in the same function.

> Tools/TestWebKitAPI/Tests/WebCore/curl/Cookies.cpp:66
> +    EXPECT_EQ(0, m_cookieJar->setCookie("http://example.com", "foo=bar; Domain=example.com", false));

If we return a boolean, we would write it:
EXPECT_TRUE(m_cookieJar->setCookie("http://example.com", "foo=bar; Domain=example.com", false));
which seems a bit more readable.

Also false is not super readable.
We could write it as:
bool fromJavaScript = false here or use an enumeration instead of a bool.

There are no tests for setCookie with fromJavaScript = true.
Should it be tested as well?

[youenn fablet]

Comment on attachment 354369 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=354369&action=review

> Tools/ChangeLog:1
> [phone]  Christopher Reid  <[e-mail]>

Is it a patch from Christopher, from Basuke or a joint work?

[Basuke Suzuki]

Comment on attachment 354369 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=354369&action=review

Separate the refactoring and initial preparation of the unit test file to https://bugs.webkit.org/show_bug.cgi?id=191493

>> Source/WebCore/platform/network/curl/CookieJarDB.cpp:460
>>  int CookieJarDB::setCookie(const String& url, const String& cookie, bool fromJavaScript)
> 
> Could it return a bool instead?
> Or are we passing some meaningful error codes?

bool must be fine.

>> Source/WebCore/platform/network/curl/CookieUtil.cpp:140
>> +bool parseCookieHeader(const String& cookieLine, Cookie& result)
> 
> Might be a follow-up patch but we now like to do things like:
> std::optional<Cookie> parseCookieHeader(const String& cookieLine);
> or ExceptionOr<Cookie> if there is a need to provide more accurate error handling.

Good to catch this improvement on this patch. Thanks

>> Source/WebCore/platform/network/curl/CookieUtil.h:37
>> +bool parseCookieHeader(const String& cookieLine, Cookie& result);
> 
> s/ result//

Might change using optional as above.

>> Source/WebCore/platform/win/FileSystemWin.cpp:379
>> +static String generateTemporaryPath(Function<bool(const String&)> action)
> 
> Can action be a const Function&?

I try. Let me see.

>> Source/WebCore/platform/win/FileSystemWin.cpp:412
>> +    String proposedPath = generateTemporaryPath([&handle](const String proposedPath) {
> 
> s/const String/const String&/

Right.

>> Source/WebCore/platform/win/FileSystemWin.cpp:563
>> +String createTemporaryDirectory(String directoryPrefix)
> 
> directoryPrefix does not seem to be used.
> Also it is a String, should it be a const String&.

Right.

>> Source/WebCore/platform/win/FileSystemWin.cpp:569
>> +    return proposedPath;
> 
> Maybe we could remove this temporary variable and directly write 'return generate...'
> This would remove the use of two different 'proposedPath' variables in the same function.

Right.

>> Tools/ChangeLog:1
>> [phone]  Christopher Reid  <[e-mail]>
> 
> Is it a patch from Christopher, from Basuke or a joint work?

This is Chris's work and tested by me.

>> Tools/TestWebKitAPI/Tests/WebCore/curl/Cookies.cpp:66
>> +    EXPECT_EQ(0, m_cookieJar->setCookie("http://example.com", "foo=bar; Domain=example.com", false));
> 
> If we return a boolean, we would write it:
> EXPECT_TRUE(m_cookieJar->setCookie("http://example.com", "foo=bar; Domain=example.com", false));
> which seems a bit more readable.
> 
> Also false is not super readable.
> We could write it as:
> bool fromJavaScript = false here or use an enumeration instead of a bool.
> 
> There are no tests for setCookie with fromJavaScript = true.
> Should it be tested as well?

It should be. I'll change the code using Enum.

[Basuke Suzuki]

Created attachment 354600 [details]
PATCH

After bug 191493 was landed, this patch just is patch for the tailmatch and add test for that.

[youenn fablet]

Comment on attachment 354600 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=354600&action=review

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:478
> +        || !CookieUtil::domainMatch(cookie->domain, host))

maybe rewrite it as 
if (fromJavaScript && (cookie->httpOnly || hasHttpOnlyCookie(cookie->name, cookie->domain, cookie->path))
    return false;
if (!CookieUtil::domainMatch(cookie->domain, host))
    return false;

[Basuke Suzuki]

Created attachment 354653 [details]
PATCH

Thanks. Fixed.

[WebKit Commit Bot]

Comment on attachment 354653 [details]
PATCH

Rejecting attachment 354653 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 354653, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/9969355

[Basuke Suzuki]

Created attachment 354654 [details]
PATCH

[WebKit Commit Bot]

Comment on attachment 354654 [details]
PATCH

Clearing flags on attachment: 354654

Committed r238123: <https://trac.webkit.org/changeset/238123>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46019326>