Synchronize cookies between QWebEngine and KCookieServer
ClosedPublic

Authored by stefanocrocco on Jul 25 2018, 4:10 PM.

Details

Summary

KHTML part and KWebKitPart both use KCookieServer to manage cookies,
while WebEnginePart doesn't. This has at least two consequences:

  • cookie settings made using the ""Cookie" page in Konqueror settings dialog or in SystemSettings aren't honoured
  • you can't use KIO to download files from many sites (in particular, sites requiring authentication) because the cookies received by the browser and those used by KIO are different.

Qt WebEngine provide a class, QWebEngineCookieStore to allow synchronizing
cookies between Qt WebEngine and other systems. Unfortunately, the API provided
by this class is quite different from the API used by KCookieServer.

I added a class, WebEnginePartCookieJar to manage the synchronization of cookies
between Qt WebEngine and KCookieServer, then added a static variable of this
class to WebEnginePart. This static variable is filled by WebEnginePart's
constructor the first time it's created.

WebEnginePartCookieJar does the following:

  • disables persistent cookies in QWebEngineProfile. This way, cookies will only be stored on disk by KCookieStore
  • on creation, loads cookies from KCookieServer and adds them to QWebEngineCookieStore
  • in response to the QWebEngineCookieStore::cookiesAdded signal, it adds the cookie to KCookieServer according to the Cookies KCM settings
  • in response to the QWebEngineCookieStore::cookiesRemoved signal, it removes the cookie from KCookieServer
  • in response to the QApplication::lastWindowClosed signal, it calls KCookieServer::deleteSessionCookies for each window (having store each window's id earlier).

Some functionality is missing, however, because of KCookieServer's API:

  • there's no way to know when a cookie is added to KCookieServer or removed from it, so (aside from loading the coockies from KCookieServer when the WebEnginePartCookieJar is created) the synchronization is only one-way: from the QWebEngineCookieStore to KCookieServer. This should not be an issue most of the times, but it also means that, if the user deletes a cookie from the Cookies KCM while Konqueror is running, this change won't be propagated to QWebEngineCookieStore until Konqueror is restarted
  • when the cookie policy is set to "Ask", KCookieServer doesn't provide a way to find out what the user chose. As a workaround, WebEnginePartCookieJar checks whether the cookie exists in KCookieStore and removes it from the QWebEngineCookieStore if it doesn't. However, there's no way to distinguish between an "Accept" and "AcceptUntilSession" answer.

Some tricks have been needed to make all this work. In particular:

  • QWebEngineCookieStore only provides the origin URL to the function set as cookie filter; however, QWebEngineCookieStore::setCookieFilter only exists since Qt 5.11, so on earlier Qt versions we can't determine whether a cookie is a cross origin cookie or not and honour the corresponding setting in the Cookie KCM
  • KCookieServer::addCookies requires an URL as parameter, but QWebEngineCookieStore::cookiesAdded doesn't provide one (I thought the URL passed to the cookie filter could be used, but there's no warranty that the order the requests are passed to the filter is the same in which cookies are added). Looking at the source code for KCookieStore and KCookieJar, however, it seems that they use this parameter mainly to determine the domain if it isn't specified in the cookie. Since the QNetworkCookie given by KCookieServer::addCookies seems to always have a non-empty domain, the URL is created using that domain as host
  • since QWebEngineCookieStore doesn't provide a way to find out which page made the request resulting in a cookie, there's no way to be sure of the window ID to pass to KCookieServer::addCookies. WebEnginePartCookieJar assumes that the window is the active one (this, of course, is only a problem when Konqueror has more than one window open)
  • there's an issue with expiration times. KCookieJar reads expiration times using QDateTime::fromString which, it seems, always interpret that time as local time even if expiration times in cookies is in GMT; this can lead to cookies to be considered expired when they should not. For example, if my local time is GMT +1 and I receive a cookie at 14:00 (local time) which expires in half an hour, it's expiration field will be something like "13:30:00 GMT"; however KCookieJar seems to interpret it as 13:30 local time and, since in local time, the time is 14:00, it considers the cookie expired. I worked around this issue by manually setting the time zone in the QNetworkCookie to GMT, but I can't understand why this happens (it also happens when calling KCookieServer::addCookie from the command line using qdbus, but it doesn't happen when using KHTMLPart).
Test Plan

Open the "Cookies" page in SystemSettings, remove all cookies, use Konqueror to navigate web pages using cookies and note which cookies have appeared in the list; repeat the operation using KHTML and check that the cookies are the same.

Diff Detail

Repository
R226 Konqueror
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
stefanocrocco requested review of this revision.Jul 25 2018, 4:10 PM
stefanocrocco created this revision.
stefanocrocco edited the test plan for this revision. (Show Details)Jul 25 2018, 4:13 PM
dfaure requested changes to this revision.Jul 25 2018, 10:25 PM

I'm not enough of a cookie expert (Dawit Alemayehu would be useful, but I haven't seen anything from him in a very long time...), so I can't really review the core logic. But go ahead with it even if nobody can really review it ;)

That timezone issue sounds weird and sounds like it need more debugging...

webenginepart/src/webenginepart.cpp
98

Leaked (never deleted).
No effect on the user (lifetime = application) but would show up in memory leak checkers.

Use a local static variable here (no pointer)? Will get created the first time we're here.

webenginepart/src/webenginepartcookiejar.h
282

CookieIdentifier is bigger than a pointer, so a QVector would be much more efficient.

webenginepart/src/webenginepartfactory.cpp
26

the changes in this file could be reverted (only new includes)

webenginepart/src/webenginepartfactory.h
46 ↗(On Diff #38441)

unused member

This revision now requires changes to proceed.Jul 25 2018, 10:25 PM

Made the requested changes

Check whether active window exists before calling winId

That timezone issue sounds weird and sounds like it need more debugging...

The same issue happens if I add a cookie using the command line with qdbus. For example, issuing the commandd
qdbus org.kde.kcookiejar5 /modules/kcookiejar addCookies http://www.xyz.com 'Set-Cookie: test=xxx; expires=02-Aug-2018 12:00:00 GMT' 0
and looking at the cookie in SystemSettings, I see that the expiration date is 12:00 while I believe it should be 14:00, as local time is GMT +2. If I try adding a cookie which should expire in 1 hour (10:30 CEST, 8:30 GMT) with the command:
qdbus org.kde.kcookiejar5 /modules/kcookiejar addCookies http://www.xyz.com 'Set-Cookie: test=xxx; expires=02-Aug-2018 08:30:00 GMT' 0
the cookie isn't added to the list, most likely because the cookie is considered expired, which can only happen if the expiration time is interpreted as local time (CEST) rather than as GMT time as it should. What I still can't understand is why this doesn't happen with cookies added by the http ioslave (I tried looking at its code, but nothing related to this caught my eye).

How do you think I should proceed investigating this issue? Should I open a bug report for KCookieServer? Is there someone I can contact to discuss this issue?

stefanocrocco updated this revision to Diff 38936.EditedAug 2 2018, 9:56 AM

Improve choice of window ID

Simply using QApplication::activeWindow didn't work if, for example, a page took a long time loading and the user switched to another application in the meanwhile. In this case, the only alternative was to pass 0 to KCookieServer::addCookies.

Now, if QApplication::activeWindow is nullptr, the window to use is chosen as the first among QApplication::topLevelWidgets which is a window (according to QWidget::isWindow) and doesn't have a parent widget. If no such widget is found, 0 is used as window ID

dfaure added a comment.EditedAug 5 2018, 3:48 PM

How do you think I should proceed investigating this issue?

Debugging kcookiejar ;)

Using gdb on kded5, and then calling qdbus org.kde.kcookiejar5 /modules/kcookiejar addCookies http://www.xyz.com 'Set-Cookie: test=xxx; expires=05-Aug-2018 17:00:00 GMT' 0, I can see that parseDate(_value="05-Aug-2018 17:00:00 GMT") is called, it succeeds in parsing this with one of the GMT formats, then it calls toUtc() and returns:

(gdb) p dt.toString(Qt::ISODate)
$1 = "2018-08-05T15:00:00Z"

Hmm, that means it saw the string "GMT" but didn't treat it like a UTC timezone.

OK I have a fix. Now it returns "2018-08-05T17:00:00Z". https://phabricator.kde.org/D14627

dfaure requested changes to this revision.Aug 6 2018, 8:23 AM
dfaure added inline comments.
webenginepart/src/webenginepartcookiejar.cpp
118

Hmm, if this can be triggered async, the active window could be anything at the time this gets called. Including a dialog box... no way to pass along some proper view identifier?

151

Please update the comment now that I fixed the bug.

The fix will only be in 5.50 though, so you can keep a workaround with a KIO version ifdef, if you want.

183

I find 10 * 60 * 1000 more readable, but OK, I'm nitpicking ;)

207

join with previous line

230

This could go out of bounds if cookies doesn't have a multiple-of-3 number of items. Safer to write i < cookies.length() - 2, right?

webenginepart/src/webenginepartcookiejar.h
234

5 spaces? strange indentation

This revision now requires changes to proceed.Aug 6 2018, 8:23 AM
  • Made the requested changes

Fix indentation

stefanocrocco added inline comments.Aug 6 2018, 9:26 AM
webenginepart/src/webenginepartcookiejar.cpp
118

I can't think of a way to pass a view identifier. To avoid the possibility of a dialog box, I checked for the absence of the Qt::Dialog window flag.

230

It should never happen, since KCookieServer::findCookies returns 3 fields (the number of elements in the fields variable) for each cookie. However, as you say, better be on the safe side.

dfaure requested changes to this revision.Aug 6 2018, 10:39 AM
dfaure added inline comments.
webenginepart/src/webenginepartcookiejar.cpp
118

OK, that's already something.

It leaves the risk that the user had time to switch to another browser window meanwhile, but yeah, not sure what we can do about it.

124

Shouldn't this test for dialogs too?

This revision now requires changes to proceed.Aug 6 2018, 10:39 AM

Exclude dialogs when looking for a toplevel window

dfaure accepted this revision.Aug 6 2018, 9:29 PM
This revision is now accepted and ready to land.Aug 6 2018, 9:29 PM
This revision was automatically updated to reflect the committed changes.