RESOLVED INVALID 45455
[Qt] Support third-party cookie policy for Qt clients
https://bugs.webkit.org/show_bug.cgi?id=45455
Summary [Qt] Support third-party cookie policy for Qt clients
Robert Hogan
Reported 2010-09-09 07:42:31 PDT
QtWebKit does not implement a cookie security policy, this is managed by QtWebKit clients. Move related layout tests to the 'permanently skipped' category.
Attachments
Patch (6.44 KB, patch)
2010-09-09 07:46 PDT, Robert Hogan
no flags
Patch (13.28 KB, patch)
2010-09-13 12:36 PDT, Robert Hogan
no flags
Qt Patch (192.81 KB, patch)
2011-04-20 14:07 PDT, Robert Hogan
no flags
Qt Patch (207.27 KB, patch)
2011-04-20 14:22 PDT, Robert Hogan
no flags
Patch (35.02 KB, patch)
2011-04-26 09:15 PDT, Robert Hogan
no flags
Patch (34.83 KB, patch)
2011-04-28 12:42 PDT, Robert Hogan
no flags
Patch (35.01 KB, patch)
2011-05-28 13:09 PDT, Robert Hogan
no flags
Archive of layout-test-results from ec2-cr-linux-01 (1.25 MB, application/zip)
2011-05-28 13:54 PDT, WebKit Review Bot
no flags
Patch (34.97 KB, patch)
2011-05-31 12:40 PDT, Robert Hogan
no flags
Patch (37.52 KB, patch)
2011-06-18 06:24 PDT, Robert Hogan
no flags
Patch (1.43 KB, patch)
2012-12-03 07:29 PST, Allan Sandfeld Jensen
no flags
Robert Hogan
Comment 1 2010-09-09 07:46:56 PDT
Antonio Gomes
Comment 2 2010-09-09 07:54:32 PDT
Robert, I would likely sr=me this patch, but I always think that making DRT working as close as possible to real world WebKit clients (e.g. a browser) is a good idea. Are you planning on implementing cache policies for DRT at all? It would be a good idea in my option. (Well, since you are moving the tests to the "permanently skipped" section, I would say you are not, but let me ask anyways.) Maybe we should discuss that on webkit-qt mailing lisT?
Robert Hogan
Comment 3 2010-09-09 08:23:20 PDT
(In reply to comment #2) > Robert, I would likely sr=me this patch, but I always think that making DRT working as close as possible to real world WebKit clients (e.g. a browser) is a good idea. > > Are you planning on implementing cache policies for DRT at all? It would be a good idea in my option. (Well, since you are moving the tests to the "permanently skipped" section, I would say you are not, but let me ask anyways.) I was going to do exactly that but then I realized that the only thing the layout tests would be validating is my re-implementation of setCookieFromUrl() in DRT. What do you think? Is there a real benefit? Or is it worth it just to get them out of the skipped list?
Andreas Kling
Comment 4 2010-09-09 09:19:03 PDT
(In reply to comment #3) > What do you think? Is there a real benefit? Or is it worth it just to get them out of the skipped list? We'd still be testing the code that lets you hook up a cookie policy, no? Unless no, I think we should do it. More coverage + fewer skipped tests: good stuff.
Robert Hogan
Comment 5 2010-09-09 10:06:40 PDT
(In reply to comment #4) > (In reply to comment #3) > > What do you think? Is there a real benefit? Or is it worth it just to get them out of the skipped list? > > We'd still be testing the code that lets you hook up a cookie policy, no? > > Unless no, I think we should do it. More coverage + fewer skipped tests: good stuff. Turns out we need to add a QNetworkRequest::FirstPartyForCookies attribute to Qt if clients are to have any hope of blocking third-party cookies without reinventing the world. There's a QtWebKit and a Qt part to that so will open separate bugs for each and make the webkit one block this.
Robert Hogan
Comment 7 2010-09-13 12:36:49 PDT
Robert Hogan
Comment 8 2010-09-13 12:37:38 PDT
Marking for review, but it's dependent on: http://qt.gitorious.org/qt/qt/merge_requests/2473
Early Warning System Bot
Comment 9 2010-09-13 13:20:44 PDT
Robert Hogan
Comment 10 2010-09-15 14:26:08 PDT
There is some discussion at http://qt.gitorious.org/qt/qt/merge_requests/2473. The issue is preserving binary compatibility. I need suggestions on alternatives to amending the current API for setCookiesFromUrl() - which is a non-runner. On way is for clients to ensure, in their implementation, that they can access the QNetworkRequest responsible for the call to setCookiesFromUrl() so that they can read the new attribute – that wouldn’t be trivial. Alternatively I could hack QNetworkCookie to make the firstPartyUrl available. Or lastly, introduce a new virtual function that ultimately replaces it. From the comments, that isn’t permissible either. Am I missing any other alternatives?
Kenneth Rohde Christiansen
Comment 11 2010-12-24 02:18:47 PST
Jocelyn can you have a look at this?
Kenneth Rohde Christiansen
Comment 12 2011-01-09 05:56:17 PST
Are you going to make this compile on Qt? :)
Robert Hogan
Comment 13 2011-01-09 06:12:03 PST
(In reply to comment #12) > Are you going to make this compile on Qt? :) It depends on: http://bugreports.qt.nokia.com/browse/QTBUG-13601 and http://qt.gitorious.org/qt/qt/merge_requests/2473
Jocelyn Turcotte
Comment 14 2011-01-10 03:35:54 PST
(In reply to comment #10) Since this whole first-party URL matching logic is pretty much clear and won't be changed by most users, we could also introduce QWebSettings::ThirdPartyCookiesAllowed which would, if false, prevent the call to setCookiesFromUrl in the first place. I would prefer it over adding a separate QMap for first-party URLs in QNetworkCookieJar. The ideal solution would be to pass it in setCookiesFromUrl like you wanted, but since binary compatibility isn't allowing it I think that we need to weight the gain that a complex solution would grant to the user. If we chose this way, it would be nice to know some use cases where the user would need to handle the decision in his subclassed QNetworkCookieJar instead of letting QtWebKit handle the decision.
Robert Hogan
Comment 15 2011-02-08 12:23:36 PST
(In reply to comment #14) > (In reply to comment #10) > > Since this whole first-party URL matching logic is pretty much clear and won't be changed by most users, we could also introduce QWebSettings::ThirdPartyCookiesAllowed which would, if false, prevent the call to setCookiesFromUrl in the first place. > > I would prefer it over adding a separate QMap for first-party URLs in QNetworkCookieJar. The ideal solution would be to pass it in setCookiesFromUrl like you wanted, but since binary compatibility isn't allowing it I think that we need to weight the gain that a complex solution would grant to the user. > If we chose this way, it would be nice to know some use cases where the user would need to handle the decision in his subclassed QNetworkCookieJar instead of letting QtWebKit handle the decision. Thanks Jocelyn - I'll go with that.
Robert Hogan
Comment 16 2011-04-04 11:31:54 PDT
Yesterday I looked into this again with a view to making it a QWebSetting and discovered that the major blocker is distinguishing the top-level domain in a url. If you can't do this, you end up blocking cookies from different hosts rather than different registry-controlled domains. Fortunately I discovered Peter Hartmann already done all the hard work by adding a hash table based on the public suffix list to QtNetwork, used privately by QNetworkCookieJar. Since it seems best to keep the policy logic outside QtNetwork (otherwise you end up having to communicate both the policy and the first-party url from QtWebKit via the QNetworkRequest) the most effective way of making use of Peter's TLD table seems to me to be through something like QUrl::topLevelDomain(). This would allow QtWebKit make a proper comparison between urls to establish if they are from the same registry-controlled domain. It would also be a natural fit with QUrl's current API which is able to dissect every other component of the url. The downside is that the hash table has to get included through a wrapper into a QtCore class (ie QUrl) and as a consequence even ends up in the qmake binary, growing it by 100K. This is my first hack at creating the required QUrl::topLevelDomain() and sharing the table between QtNetwork and QtCore: https://gist.github.com/900765 (I've snipped the move of the hash table to a qurltlds_p.h file in that paste so that it isn't thousands of lines long). I've pm'd Peter for his thoughts. If anyone thinks I'm going about it all wrong please shout!
Markus Goetz
Comment 17 2011-04-14 00:52:13 PDT
We could have QtCore exporting a function or data structure pointer. QtNetwork would fill it in with the TLD data. That way we don't bloat QtCore. Just an idea..
Jocelyn Turcotte
Comment 18 2011-04-14 03:12:59 PDT
(In reply to comment #17) > We could have QtCore exporting a function or data structure pointer. QtNetwork would fill it in with the TLD data. > That way we don't bloat QtCore. > Just an idea.. That's a good point, especially if we wan't to continue increasing the isolation of QtNetwork from QtCore. What we need is just a function that returns the TLD from a host/URL if I'm right.
Robert Hogan
Comment 19 2011-04-14 11:24:31 PDT
(In reply to comment #17) > We could have QtCore exporting a function or data structure pointer. QtNetwork would fill it in with the TLD data. > That way we don't bloat QtCore. > Just an idea.. OK, so to be sure I'm clear: you're suggesting an exported public API such as QTopLevelDomainDB that contains methods for passing urls and strings and getting back the TLDs or whatever other info might be needed from the public suffix database? Please let me know if I've misunderstood, as I'm not sure I've grokked what you're suggesting by 'QtCore exporting a function or data structure pointer'. I can't see how QtCore could export a function that QtNetwork could populate without making QtCore depend on QtNetwork. Which I thought would be a bad thing.
Peter Hartmann
Comment 20 2011-04-20 05:01:27 PDT
> OK, so to be sure I'm clear: you're suggesting an exported public API such as QTopLevelDomainDB that contains methods for passing urls and strings and getting back the TLDs or whatever other info might be needed from the public suffix database? > > Please let me know if I've misunderstood, as I'm not sure I've grokked what you're suggesting by 'QtCore exporting a function or data structure pointer'. I can't see how QtCore could export a function that QtNetwork could populate without making QtCore depend on QtNetwork. Which I thought would be a bad thing. I think we should just move the TLD code over to Qt Core (which will grow it 100K), and #ifdef it out for the bootstrap mode so it does not end up in qmake, then we can add the functionality in QUrl, as Robert suggested.
Robert Hogan
Comment 21 2011-04-20 14:06:01 PDT
(In reply to comment #20) > > OK, so to be sure I'm clear: you're suggesting an exported public API such as QTopLevelDomainDB that contains methods for passing urls and strings and getting back the TLDs or whatever other info might be needed from the public suffix database? > > > > Please let me know if I've misunderstood, as I'm not sure I've grokked what you're suggesting by 'QtCore exporting a function or data structure pointer'. I can't see how QtCore could export a function that QtNetwork could populate without making QtCore depend on QtNetwork. Which I thought would be a bad thing. > > I think we should just move the TLD code over to Qt Core (which will grow it 100K), and #ifdef it out for the bootstrap mode so it does not end up in qmake, then we can add the functionality in QUrl, as Robert suggested. Thanks Peter - I have a patch for Qt, but gitorious is erroring out when I create merge requests. I'll post it here for now. Might be easier to review/comment line by line that way anyway.
Robert Hogan
Comment 22 2011-04-20 14:07:04 PDT
Created attachment 90407 [details] Qt Patch
Robert Hogan
Comment 23 2011-04-20 14:22:16 PDT
Created attachment 90412 [details] Qt Patch
Robert Hogan
Comment 24 2011-04-26 09:15:46 PDT
Robert Hogan
Comment 25 2011-04-26 09:16:32 PDT
(In reply to comment #21) > > Thanks Peter - I have a patch for Qt, but gitorious is erroring out when I create merge requests. I'll post it here for now. Might be easier to review/comment line by line that way anyway. Merge request for Qt at: https://qt.gitorious.org/qt/qt/merge_requests/1205
Tor Arne Vestbø
Comment 26 2011-04-26 16:44:37 PDT
Comment on attachment 91116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91116&action=review > Source/WebCore/platform/qt/ThirdPartyCookiesQt.cpp:37 > +static QNetworkCookieJar *cookieJar(QObject *originatingFrame) Style, * on the left, there's a few of these.
Robert Hogan
Comment 27 2011-04-28 12:42:05 PDT
Robert Hogan
Comment 28 2011-05-24 12:14:00 PDT
The Qt part of this is in master now (thanks Peter!): http://bugreports.qt.nokia.com/browse/QTBUG-13601 http://gitorious.org/qt/qt/merge_requests/1205 Would someone care to review before I rebase?
Robert Hogan
Comment 29 2011-05-28 13:09:20 PDT
Early Warning System Bot
Comment 30 2011-05-28 13:24:14 PDT
WebKit Review Bot
Comment 31 2011-05-28 13:53:57 PDT
Comment on attachment 95268 [details] Patch Attachment 95268 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8743574 New failing tests: http/tests/cookies/strict-third-party-cookie-blocking.html
WebKit Review Bot
Comment 32 2011-05-28 13:54:03 PDT
Created attachment 95273 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Robert Hogan
Comment 33 2011-05-29 07:56:57 PDT
The Qt part of this patch is in the 4.8 branch. Peter, which version of Qt should I compile-guard against? 4.8.0 ? 4.8.X ?
Robert Hogan
Comment 34 2011-05-31 12:40:13 PDT
Peter Hartmann
Comment 35 2011-06-01 09:35:36 PDT
(In reply to comment #33) > The Qt part of this patch is in the 4.8 branch. > > Peter, which version of Qt should I compile-guard against? 4.8.0 ? 4.8.X ? I would just use the 4.8 branch from http://qt.gitorious.org/qt/qt/commits/4.8 . I have no idea what the Webkit bot uses, though...
Robert Hogan
Comment 36 2011-06-18 06:24:12 PDT
Benjamin Poulain
Comment 37 2011-06-18 07:42:51 PDT
Comment on attachment 97701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97701&action=review Should we change the default to AllowThirdPartyWithExistingCookies and document it? Do you know if there is a security reason justifying the behavior of Safari? > Source/WebCore/platform/qt/ThirdPartyCookiesQt.cpp:61 > + firstPartyDomain.remove(firstPartyDomain.length() - firstPartyTLD.length(), firstPartyTLD.length()); > + firstPartyDomain.prepend(QLatin1Char('.')); It would be nice to have an inline function to do this. That would make the code naturally self documented. > Source/WebCore/platform/qt/ThirdPartyCookiesQt.h:24 > +#include <QNetworkCookieJar> > +#include <QUrl> You don't need those includes. > Source/WebCore/platform/qt/ThirdPartyCookiesQt.h:29 > +bool thirdPartyCookiePolicyPermits(QNetworkCookieJar* , const QUrl&, const QUrl& firstPartyUrl); > +bool thirdPartyCookiePolicyPermits(QObject* originatingFrame, const QUrl&, const QUrl& firstPartyUrl); QNetworkCookieJar is also a QObject. This is ok but I think that is slightly more confusing than it should be. I think -thirdPartyCookiePolicyPermits should take a QWebFrame, and the cast could be done before -alternatively, thirdPartyCookiePolicyPermits could be renamed to thirdPartyCookiePolicyPermitsForFrame() Just to make sure they are never mixed by accident. > Source/WebKit/qt/Api/qwebsettings.cpp:395 > + \since 4.8 Probably not. Do we do since QtWebKit 2.3? I guess we can add \since 4.8 if the patch is integrated. > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1069 > +bool DumpRenderTreeSupportQt::thirdPartyCookiePolicy(QNetworkCookieJar* jar, const QUrl& url, const QUrl& firstPartyUrl) I don't like the name of the function thirdPartyCookiePolicy(), it returns a bool, the name suggest it return an enum with the policy. > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:92 > +#if QT_VERSION >= 0x040800 Why not >= QT_VERSION_CHECK(4, 8, 0)? > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2936 > +#if QT_VERSION >= 0x040800 Why not >= QT_VERSION_CHECK(4, 8, 0)?
Benjamin Poulain
Comment 38 2011-06-18 07:48:27 PDT
Comment on attachment 97701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97701&action=review > Source/WebCore/platform/qt/ThirdPartyCookiesQt.cpp:42 > + QWebFrame* frame = qobject_cast<QWebFrame*>(originatingFrame); > + if (!frame) If frame is null here, it is a programming mistake somewhere before this call. I don't mind the check to avoid a crash, but I think an ASSERT(frame) would be nice as well.
Robert Hogan
Comment 39 2011-06-19 04:06:37 PDT
(In reply to comment #37) > (From update of attachment 97701 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97701&action=review > > Should we change the default to AllowThirdPartyWithExistingCookies and document it? Do you know if there is a security reason justifying the behavior of Safari? You mean to justify selecting this as their default mode for blocking cookies? It's more a usability reason: https://bugs.webkit.org/show_bug.cgi?id=35824#c33 I think the QtWebKit default should be to allow them, and let clients decide on their own default. > > Source/WebKit/qt/Api/qwebsettings.cpp:395 > > + \since 4.8 > > Probably not. > Do we do since QtWebKit 2.3? > I guess we can add \since 4.8 if the patch is integrated. Updated it to QtWebKit 2.3 > > > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:92 > > +#if QT_VERSION >= 0x040800 > > Why not >= QT_VERSION_CHECK(4, 8, 0)? > For some reason the unit test doesn't understand QT_VERSION_CHECK. Couldn't figure out why.
Early Warning System Bot
Comment 40 2011-06-20 01:40:04 PDT
Robert Hogan
Comment 41 2011-07-03 10:14:55 PDT
Robert Hogan
Comment 42 2011-07-03 10:42:46 PDT
Csaba Osztrogonác
Comment 43 2012-01-24 07:29:34 PST
Reopen, because platform/qt/http/tests/cookies/strict-third-party-cookie-blocking.html still fails with Qt 4.8 and Qt5 too (and it is in platform/qt/Skipped list): +CONSOLE MESSAGE: line 5: ReferenceError: Can't find variable: resetCookies +CONSOLE MESSAGE: line 79: TypeError: 'undefined' is not an object (evaluating 'functions.length')
Robert Hogan
Comment 44 2012-01-24 13:01:35 PST
(In reply to comment #43) > Reopen, because platform/qt/http/tests/cookies/strict-third-party-cookie-blocking.html still fails with Qt 4.8 and Qt5 too (and it is in platform/qt/Skipped list): > > +CONSOLE MESSAGE: line 5: ReferenceError: Can't find variable: resetCookies > +CONSOLE MESSAGE: line 79: TypeError: 'undefined' is not an object (evaluating 'functions.length') It looks like platform/ tests are no longer run from the non-platform-specific directories. I'll update the test.
Csaba Osztrogonác
Comment 45 2012-05-18 06:32:14 PDT
(In reply to comment #44) > (In reply to comment #43) > > Reopen, because platform/qt/http/tests/cookies/strict-third-party-cookie-blocking.html still fails with Qt 4.8 and Qt5 too (and it is in platform/qt/Skipped list): > > > > +CONSOLE MESSAGE: line 5: ReferenceError: Can't find variable: resetCookies > > +CONSOLE MESSAGE: line 79: TypeError: 'undefined' is not an object (evaluating 'functions.length') > > It looks like platform/ tests are no longer run from the non-platform-specific directories. I'll update the test. The bug is still valid.
Csaba Osztrogonác
Comment 46 2012-11-21 03:16:32 PST
still valid bug
Allan Sandfeld Jensen
Comment 47 2012-12-03 07:29:10 PST
Csaba Osztrogonác
Comment 48 2012-12-11 03:24:23 PST
Comment on attachment 177251 [details] Patch LGTM, r=me. Please remove qt/http/tests/cookies/strict-third-party-cookie-blocking.html from qt/TestExpectations when you lands the fix.
Allan Sandfeld Jensen
Comment 49 2012-12-11 03:32:08 PST
Csaba Osztrogonác
Comment 50 2012-12-11 04:20:53 PST
(In reply to comment #49) > Committed r137280: <http://trac.webkit.org/changeset/137280> The test still fails with the following error message: --- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/platform/qt/http/tests/cookies/strict-third-party-cookie-blocking-expected.txt +++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/platform/qt/http/tests/cookies/strict-third-party-cookie-blocking-actual.txt @@ -1,3 +1,4 @@ +ALERT: Attempt to clear http://:8000/cookies/resources/cookie-utility.php?queryfunction=deleteCookies cookies might have failed. Test results might be off from here on out. (Error: NETWORK_ERR: XMLHttpRequest Exception 101) ALERT: ALERT: Allowing all cookies @@ -44,6 +45,7 @@ ALERT: http://localhost:8000/cookies/resources/cookie-utility.php?queryfunction=setFooAndBarCookie ALERT: XHR response - Set the foo and bar cookies ALERT: Test stage 10 document.cookie is: +ALERT: Attempt to clear http://:8000/cookies/resources/cookie-utility.php?queryfunction=deleteCookies cookies might have failed. Test results might be off from here on out. (Error: NETWORK_ERR: XMLHttpRequest Exception 101) Be default, Safari only blocks third-party cookies if the third-party has no cookies already set. This test is a stricter version of third-party-cookie-relaxing.html that expects third-party cookies to get blocked all the time. Could you check and fix it please?
Csaba Osztrogonác
Comment 51 2012-12-11 04:29:25 PST
I skipped it again to paint the bot green. Please unskip it with the proper fix.
Note You need to log in before you can comment on or make changes to this bug.