Bug 45455 - [Qt] Support third-party cookie policy for Qt clients
Summary: [Qt] Support third-party cookie policy for Qt clients
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 62929 79666
  Show dependency treegraph
 
Reported: 2010-09-09 07:42 PDT by Robert Hogan
Modified: 2014-01-29 09:13 PST (History)
13 users (show)

See Also:


Attachments
Patch (6.44 KB, patch)
2010-09-09 07:46 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.28 KB, patch)
2010-09-13 12:36 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Qt Patch (192.81 KB, patch)
2011-04-20 14:07 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Qt Patch (207.27 KB, patch)
2011-04-20 14:22 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (35.02 KB, patch)
2011-04-26 09:15 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (34.83 KB, patch)
2011-04-28 12:42 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (35.01 KB, patch)
2011-05-28 13:09 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
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 Details
Patch (34.97 KB, patch)
2011-05-31 12:40 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (37.52 KB, patch)
2011-06-18 06:24 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (1.43 KB, patch)
2012-12-03 07:29 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 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.
Comment 1 Robert Hogan 2010-09-09 07:46:56 PDT
Created attachment 67029 [details]
Patch
Comment 2 Antonio Gomes 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?
Comment 3 Robert Hogan 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?
Comment 4 Andreas Kling 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.
Comment 5 Robert Hogan 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.
Comment 7 Robert Hogan 2010-09-13 12:36:49 PDT
Created attachment 67457 [details]
Patch
Comment 8 Robert Hogan 2010-09-13 12:37:38 PDT
Marking for review, but it's dependent on:

http://qt.gitorious.org/qt/qt/merge_requests/2473
Comment 9 Early Warning System Bot 2010-09-13 13:20:44 PDT
Attachment 67457 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3902448
Comment 10 Robert Hogan 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?
Comment 11 Kenneth Rohde Christiansen 2010-12-24 02:18:47 PST
Jocelyn can you have a look at this?
Comment 12 Kenneth Rohde Christiansen 2011-01-09 05:56:17 PST
Are you going to make this compile on Qt? :)
Comment 13 Robert Hogan 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
Comment 14 Jocelyn Turcotte 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.
Comment 15 Robert Hogan 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.
Comment 16 Robert Hogan 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!
Comment 17 Markus Goetz 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..
Comment 18 Jocelyn Turcotte 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.
Comment 19 Robert Hogan 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.
Comment 20 Peter Hartmann 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.
Comment 21 Robert Hogan 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.
Comment 22 Robert Hogan 2011-04-20 14:07:04 PDT
Created attachment 90407 [details]
Qt Patch
Comment 23 Robert Hogan 2011-04-20 14:22:16 PDT
Created attachment 90412 [details]
Qt Patch
Comment 24 Robert Hogan 2011-04-26 09:15:46 PDT
Created attachment 91116 [details]
Patch
Comment 25 Robert Hogan 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
Comment 26 Tor Arne Vestbø 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.
Comment 27 Robert Hogan 2011-04-28 12:42:05 PDT
Created attachment 91538 [details]
Patch
Comment 28 Robert Hogan 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?
Comment 29 Robert Hogan 2011-05-28 13:09:20 PDT
Created attachment 95268 [details]
Patch
Comment 30 Early Warning System Bot 2011-05-28 13:24:14 PDT
Comment on attachment 95268 [details]
Patch

Attachment 95268 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8743569
Comment 31 WebKit Review Bot 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
Comment 32 WebKit Review Bot 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
Comment 33 Robert Hogan 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 ?
Comment 34 Robert Hogan 2011-05-31 12:40:13 PDT
Created attachment 95463 [details]
Patch
Comment 35 Peter Hartmann 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...
Comment 36 Robert Hogan 2011-06-18 06:24:12 PDT
Created attachment 97701 [details]
Patch
Comment 37 Benjamin Poulain 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)?
Comment 38 Benjamin Poulain 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.
Comment 39 Robert Hogan 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.
Comment 40 Early Warning System Bot 2011-06-20 01:40:04 PDT
Comment on attachment 97701 [details]
Patch

Attachment 97701 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8909446
Comment 41 Robert Hogan 2011-07-03 10:14:55 PDT
Committed r90341: <http://trac.webkit.org/changeset/90341>
Comment 42 Robert Hogan 2011-07-03 10:42:46 PDT
Committed r90343: <http://trac.webkit.org/changeset/90343>
Comment 43 Csaba Osztrogonác 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')
Comment 44 Robert Hogan 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.
Comment 45 Csaba Osztrogonác 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.
Comment 46 Csaba Osztrogonác 2012-11-21 03:16:32 PST
still valid bug
Comment 47 Allan Sandfeld Jensen 2012-12-03 07:29:10 PST
Created attachment 177251 [details]
Patch
Comment 48 Csaba Osztrogonác 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.
Comment 49 Allan Sandfeld Jensen 2012-12-11 03:32:08 PST
Committed r137280: <http://trac.webkit.org/changeset/137280>
Comment 50 Csaba Osztrogonác 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?
Comment 51 Csaba Osztrogonác 2012-12-11 04:29:25 PST
I skipped it again to paint the bot green. Please unskip it with the proper fix.