[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Fix cookie expiration date parsing in
From:       Benjamin Meyer <ben () meyerhome ! net>
Date:       2009-12-21 18:20:30
Message-ID: 1C22BD62-DDC2-4239-843B-B6B0C4E9ACC4 () meyerhome ! net
[Download RAW message or body]


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


[prev in list] [next in list] [prev in thread] [next in thread]