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

[Dawit A.]

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. As such we will never 
see or get the same bug reports and all the test cases in the world will not 
catch this issue in kcookiejar. The cookie will still be there until you exit 
your current session. 

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...


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
>