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.
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.
Created attachment 354291 [details] PATCH
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.
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!
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!
Created attachment 354369 [details] PATCH
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?
Comment on attachment 354369 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=354369&action=review > Tools/ChangeLog:1 > +2018-11-08 Christopher Reid <chris.reid@sony.com> Is it a patch from Christopher, from Basuke or a joint work?
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 >> +2018-11-08 Christopher Reid <chris.reid@sony.com> > > 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.
Created attachment 354600 [details] PATCH After bug 191493 was landed, this patch just is patch for the tailmatch and add test for that.
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;
Created attachment 354653 [details] PATCH Thanks. Fixed.
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
Created attachment 354654 [details] PATCH
Comment on attachment 354654 [details] PATCH Clearing flags on attachment: 354654 Committed r238123: <https://trac.webkit.org/changeset/238123>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46019326>