Review Request: Fix cookie expiration date parsing in KCookieJar...

[Benjamin Meyer]

On Dec 21, 2009, at 11:07 AM, Dawit A. wrote:

> I looked at your implementation in qnetworkcookie.cpp and as you pointed out 
> it is much much more robust than what is accomplished with this patch that is 
> attempting to fix outstanding bugs in kcoookiejar...
> 
> There is one big difference between the way qnetworkcookie and kcookiejar 
> handle the issue of invalid cookie expiration dates however... qnetworkcookie  
> rejects all cookies whose expire date cannot be parsed properly where as 
> kcookiejar will simply treat them as a session cookies.

If this does not match the behavior of other browsers then we should change the behavior of qnetworkcookie.

> Moreover, even if I want to adapt your implementation from qnetworkcookie, as  
> a last resort parser for example, I simply cannot because of  licensing 
> incompatibilities. KCookiejar has a BSD license. And I am absolutely not 
> inclined to reinvent the wheel and write a parser from scratch to deal with 
> all the cases that violate the spec in order to be compatible with the 
> behaviour of other browsers...

While I could see about releasing my code under the bsd (it was contributed to Qt) a simple solution might be to do something like the following:

QList<QNetworkCookie> list = QCookie::parseCookies("a=b, expiration=" + expirationDateString);
expirationDate = list.value(0).expirationDate(). 

-Benjamin Meyer

> On Monday 21 December 2009 00:16:42 Benjamin Meyer wrote:
>> Looking at the current code there are plenty of real cases that are being
>> missed.
>> 
>> Earlier this year I went through and re-wrote the expiration date parser
>> for QCookieJar based upon bug reports I received in Arora.  Investigating
>> the source code in Firefox I created a new date parser which can be found
>> in the parseDateString() function in qnetworkcookie.cpp.  It is compatible
>> with the behavior of other browsers, while still being readable code.  I
>> went through a number of revisions and am proud of the final result.
>> 
>> http://qt.gitorious.org/qt/qt/blobs/HEAD/src/network/access/qnetworkcookie.
>> cpp
>> 
>> I wrote an extensive set of autotests including some that I saw in bug
>> reports
>> 
>> http://qt.gitorious.org/qt/qt/blobs/master/tests/auto/qnetworkcookie/tst_qn
>> etworkcookie.cpp
>> 
>> I would suggest running these autotests on this parser as there are many
>> sites out there that do things like "31 11 06" or "31 11 01" where you
>> need to be compatible with not the spec but the behavior of major
>> browsers.
>> -Benjamin Meyer
>> 
>> On Dec 17, 2009, at 6:37 PM, adawit at kde.org wrote:
>>> -----------------------------------------------------------
>>> This is an automatically generated e-mail. To reply, visit:
>>> http://reviewboard.kde.org/r/2404/
>>> -----------------------------------------------------------
>>> 
>>> Review request for kdelibs.
>>> 
>>> 
>>> Summary
>>> -------
>>> 
>>> This patch addresses the issue of parsing cookie expiration date
>>> correctly. Failing to parse expiration dates properly results in the
>>> cookie being treated as a session cookie which results in many unintended
>>> side effects similar to those reported in the bug reports listed above.
>>> 
>>> 
>>> This addresses bugs 19318, 145244, 176731, and 187792.
>>>   https://bugs.kde.org/show_bug.cgi?id=19318
>>>   https://bugs.kde.org/show_bug.cgi?id=145244
>>>   https://bugs.kde.org/show_bug.cgi?id=176731
>>>   https://bugs.kde.org/show_bug.cgi?id=187792
>>> 
>>> 
>>> Diffs
>>> -----
>>> 
>>> /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.cpp 1063375
>>> /trunk/KDE/kdelibs/kioslave/http/kcookiejar/tests/cookie.test 1063374
>>> 
>>> Diff: http://reviewboard.kde.org/r/2404/diff
>>> 
>>> 
>>> Testing
>>> -------
>>> 
>>> * kcookiejar unit test.
>>> * Spot check with few sites that set cookies based on a "remember me"
>>> option.
>>> 
>>> 
>>> Thanks,
>>> 
>>> adawit
>>