Bug 88760

Created attachment 146809 [details] Patch

*** Bug 87247 has been marked as a duplicate of this bug. ***

Comment on attachment 146809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83 > - soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data()); any reason for this change? It seems unrelated to the CL description

Comment on attachment 146809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83 >> - soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data()); > > any reason for this change? It seems unrelated to the CL description It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow.

(In reply to comment #4) > (From update of attachment 146809 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review > > >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83 > >> - soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data()); > > > > any reason for this change? It seems unrelated to the CL description > > It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow. Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally: 1. soup_cookie_parse(); // Useless since we already did it 2. policy check; 3. soup_cookie_jar_add_cookie(); I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1. Hopefully, this is clearer.

(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 146809 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review > > > > >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83 > > >> - soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data()); > > > > > > any reason for this change? It seems unrelated to the CL description > > > > It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow. > > Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally: > 1. soup_cookie_parse(); // Useless since we already did it > 2. policy check; > 3. soup_cookie_jar_add_cookie(); > > I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1. > > Hopefully, this is clearer. It seems unfortunate to duplicate the policy check code. I understand that you want to avoid parsing the cookie twice. But how can we make sure that the policy check here and in soup_cookie_jar_set_cookie_with_first_party do not diverge over time?

(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (From update of attachment 146809 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review > > > > > > >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83 > > > >> - soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data()); > > > > > > > > any reason for this change? It seems unrelated to the CL description > > > > > > It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow. > > > > Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally: > > 1. soup_cookie_parse(); // Useless since we already did it > > 2. policy check; > > 3. soup_cookie_jar_add_cookie(); > > > > I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1. > > > > Hopefully, this is clearer. > > It seems unfortunate to duplicate the policy check code. I understand that you want to avoid parsing the cookie twice. But how can we make sure that the policy check here and in soup_cookie_jar_set_cookie_with_first_party do not diverge over time? kov/mrobinson: What's your preference? I don't really mind either way, it just seemed unfortunate to parse the Cookie twice.

Comment on attachment 146809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:82 > + GOwnPtr<SoupCookie> newCookie(soup_cookie_parse(value.utf8().data(), origin.get())); I think we may have a problem with API mismatch here. setCookies() implies value may contain more than one cookie, but all soup API we're using here only deals with the first cookie if there are many. We probably need a soup_cookie*s*_parse, and a soup_cookie_jar_set_cookie*s*_with_first_party(). > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:87 > + // set from JavaScript (Bug 86067). I don't see value in mentioning this bug number in the code comments, the ChangeLog is enough. > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:92 > + GOwnPtr<GSList> existingCookies(soup_cookie_jar_all_cookies(jar)); I think this is very inefficient, we should not be listing all cookies, how about using soup_cookie_jar_get_cookies(), so you can give it the URI? >>>>>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83 >>>>>> - soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data()); >>>>> >>>>> any reason for this change? It seems unrelated to the CL description >>>> >>>> It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow. >>> >>> Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally: >>> 1. soup_cookie_parse(); // Useless since we already did it >>> 2. policy check; >>> 3. soup_cookie_jar_add_cookie(); >>> >>> I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1. >>> >>> Hopefully, this is clearer. >> >> It seems unfortunate to duplicate the policy check code. I understand that you want to avoid parsing the cookie twice. But how can we make sure that the policy check here and in soup_cookie_jar_set_cookie_with_first_party do not diverge over time? > > kov/mrobinson: What's your preference? I don't really mind either way, it just seemed unfortunate to parse the Cookie twice. I prefer reliability and readability in this case. Unless there's a performance issue we need to fix I don't think the loss in readability and the risk of diverging is paid by the gain in not parsing the cookie twice. If you want to make this more efficient I'd suggest proposing new API for soup to set a cookie from an existing SoupCookie object.

Comment on attachment 146809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review By the way, the "1 vs several" cookie parsing is a separate issue (which was already there). Should I open another bug for it and use the current bug report only for prevent httpOnly cookie setting from JS? >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:82 >> + GOwnPtr<SoupCookie> newCookie(soup_cookie_parse(value.utf8().data(), origin.get())); > > I think we may have a problem with API mismatch here. setCookies() implies value may contain more than one cookie, but all soup API we're using here only deals with the first cookie if there are many. We probably need a soup_cookie*s*_parse, and a soup_cookie_jar_set_cookie*s*_with_first_party(). Agreed. Unfortunately, those APIs do not exist in libsoup yet. >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:92 >> + GOwnPtr<GSList> existingCookies(soup_cookie_jar_all_cookies(jar)); > > I think this is very inefficient, we should not be listing all cookies, how about using soup_cookie_jar_get_cookies(), so you can give it the URI? Well, I considered that first but the issue is that soup_cookie_jar_get_cookies() returns the cookies in a single gchar*. Then, I have currently no way of parsing more than one cookie from a gchar*. >>>>>>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:-83 >>>>>>> - soup_cookie_jar_set_cookie_with_first_party(jar, origin.get(), firstParty.get(), value.utf8().data()); >>>>>> >>>>>> any reason for this change? It seems unrelated to the CL description >>>>> >>>>> It has been replaced by soup_cookie_jar_add_cookie(jar, newCookie.release()) a few lines bellow since we already have the Cookie parsed. The first_party check is also done bellow. >>>> >>>> Otherwise soup_cookie_jar_set_cookie_with_first_party() will call internally: >>>> 1. soup_cookie_parse(); // Useless since we already did it >>>> 2. policy check; >>>> 3. soup_cookie_jar_add_cookie(); >>>> >>>> I'm now doing step 2 and 3 manually instead of calling soup_cookie_jar_set_cookie_with_first_party(), in order to an extra step 1. >>>> >>>> Hopefully, this is clearer. >>> >>> It seems unfortunate to duplicate the policy check code. I understand that you want to avoid parsing the cookie twice. But how can we make sure that the policy check here and in soup_cookie_jar_set_cookie_with_first_party do not diverge over time? >> >> kov/mrobinson: What's your preference? I don't really mind either way, it just seemed unfortunate to parse the Cookie twice. > > I prefer reliability and readability in this case. Unless there's a performance issue we need to fix I don't think the loss in readability and the risk of diverging is paid by the gain in not parsing the cookie twice. If you want to make this more efficient I'd suggest proposing new API for soup to set a cookie from an existing SoupCookie object. Ok, I understand. I'll do that.

Comment on attachment 146809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146809&action=review >>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:92 >>> + GOwnPtr<GSList> existingCookies(soup_cookie_jar_all_cookies(jar)); >> >> I think this is very inefficient, we should not be listing all cookies, how about using soup_cookie_jar_get_cookies(), so you can give it the URI? > > Well, I considered that first but the issue is that soup_cookie_jar_get_cookies() returns the cookies in a single gchar*. Then, I have currently no way of parsing more than one cookie from a gchar*. One other issue with soup_cookie_jar_get_cookies() is that it calls soup_cookie_to_cookie_header() for each cookie internally and not soup_cookie_to_set_cookie_header(). As a consequence, the returned cookies do not have a PATH set (only "NAME=VALUE"). However, I need the PATH (in addition to the NAME) in order to do an "overwrite" check.

Created attachment 146878 [details] Patch Based on the feedback, I'm now proposing this patch. The patch now fixes setCookies() so that it processes all the cookies and not just the first one. I have clearly documented with FIXME comments the methods that should be removed later and provided by libsoup instead (optimally). However, I hope we can land this patch like this and improve the code later since this fixes a valid issue and getting a new and improved libsoup release may take time. I don't mind writing the patches for libsoup though if it helps.

Comment on attachment 146878 [details] Patch Attachment 146878 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12935768

(In reply to comment #9) > By the way, the "1 vs several" cookie parsing is a separate issue (which was already there). Should I open another bug for it and use the current bug report only for prevent httpOnly cookie setting from JS? Yes, please, I just thought I'd mention it so we can all be aware of the issue. > However, I hope we can land this patch like this and improve the code later since this fixes a valid issue and getting a new and improved libsoup release may take time. > I don't mind writing the patches for libsoup though if it helps. Like I said on IRC, I don't think there's a particular rush: we don't have any deadlines pressing us, and we can use our jhbuild infrastructure to bump the libsoup version as soon as new APIs are pushed. That said, if you want to go ahead I think your first patch is better than creating all those helper functions, modulo copying the policy check logic instead of calling soup's API we discussed.

Comment on attachment 146878 [details] Patch Clearing flags. Will propose a new patch once libsoup is updated.

Proposed the following libsoup patches to add the few API functions we need: https://bugzilla.gnome.org/show_bug.cgi?id=677922 https://bugzilla.gnome.org/show_bug.cgi?id=677923 I don't really think we need a new soup_cookies_parse() function in libsoup since we can split on newlines and call soup_cookie_parse() for each cookie. From libsoup point of view, such function makes little sense IMHO since it is not a header format (No "Set-Cookie:" or "Cookie:" at the beginning of the lines).

Skipping http/tests/cookies/js-get-and-set-http-only-cookie.html in the GTK port: http://trac.webkit.org/changeset/120052 Please unskip when fixed. Thanks!

Created attachment 147124 [details] Patch Updated patch now that the libsoup patches landed.

Comment on attachment 147124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147124&action=review Since I have a small suggestion I'm not setting cq+, thanks, great work! > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:75 > +static inline SoupCookie* findCookieInList(const GSList* cookies, const gchar* name, const gchar* path) I think you could make this helper contain the full logic, so instead of finding the cookie in the list and then verifying it's http-only at the caller you can instead have httpOnlyCookiExists(...). > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:113 > + // Make sure we do not overwrite httpOnly cookies from JavaScript. > + SoupCookie* existingCookie = findCookieInList(existingCookies, soup_cookie_get_name(cookie.get()), soup_cookie_get_path(cookie.get())); > + if (existingCookie && soup_cookie_get_http_only(existingCookie)) > + continue; Would make this more readable =).

Created attachment 147133 [details] Patch Take feedback into consideration. Could someone please cq+ ?

Comment on attachment 147133 [details] Patch Attachment 147133 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12940720

Comment on attachment 147133 [details] Patch The ews efl did not take into consideration the libsoup update. False alarm. Marking as cq? again.

Comment on attachment 147133 [details] Patch Clearing flags on attachment: 147133 Committed r120145: <http://trac.webkit.org/changeset/120145>

All reviewed patches have been landed. Closing bug.

http/tests/misc/redirect-to-about-blank.html has started to fail after committing this.

(In reply to comment #24) > http/tests/misc/redirect-to-about-blank.html has started to fail after committing this. Yes, I skipped the test for both GTK and EFL and I opened Bug 88961 to track the issue. This is caused by a regression in the new version of libsoup. It is actually unrelated to this fix but a side effect of the libsoup version bump in jhbuild.

I am using a non jhbuild on ubuntu 12.04 to build webkit-gtk with options to autogen --enable-webkit2 && --with-gtk=3.0 and I get the following build error on svn rev# 120311 ../Source/WebCore/platform/network/soup/CookieJarSoup.cpp: In function ‘void WebCore::setCookies(WebCore::Document*, const WebCore::KURL&, const WTF::String&)’: ../Source/WebCore/platform/network/soup/CookieJarSoup.cpp:99:86: error: ‘soup_cookie_jar_get_cookie_list’ was not declared in this scope ../Source/WebCore/platform/network/soup/CookieJarSoup.cpp:117:92: error: ‘soup_cookie_jar_add_cookie_with_first_party’ was not declared in this scope I tried installing latest libsoup-2.39.2 but of no use, I could not find these APIs defined soup_cookie_jar_get_cookie_list & soup_cookie_jar_add_cookie_with_first_party in libsoup-2.39.2 Is something getting missed here?

We're now using API from an unreleased version of soup. You need to build libsoup from git or wait for the next release.