[webkit-reviews] review denied: [Bug 88760] [soup] Prevent setting or editing httpOnly cookies from JavaScript : [Attachment 146809] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Christophe Dumez
<christophe.dumez at intel.com>'s request for review:
Bug 88760: [soup] Prevent setting or editing httpOnly cookies from JavaScript
https://bugs.webkit.org/show_bug.cgi?id=88760

Attachment 146809: Patch
https://bugs.webkit.org/attachment.cgi?id=146809&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.