Bug 45455 – [Qt] Support third-party cookie policy for Qt clients

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Robert Hogan]

QtWebKit does not implement a cookie security policy, this is managed by QtWebKit clients. Move related layout tests to the 'permanently skipped' category.

[Robert Hogan]

Created attachment 67029 [details]
Patch

[Antonio Gomes]

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]

(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]

(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]

(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]

See:

http://bugreports.qt.nokia.com/browse/QTBUG-13601

and

http://qt.gitorious.org/qt/qt/merge_requests/2473

[Robert Hogan]

Created attachment 67457 [details]
Patch

[Robert Hogan]

Marking for review, but it's dependent on:

http://qt.gitorious.org/qt/qt/merge_requests/2473

[Early Warning System Bot]

Attachment 67457 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3902448

[Robert Hogan]

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]

Jocelyn can you have a look at this?

[Kenneth Rohde Christiansen]

Are you going to make this compile on Qt? :)

[Robert Hogan]

(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]

(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]

(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]

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]

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]

(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]

(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]

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

(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]

Created attachment 90407 [details]
Qt Patch

[Robert Hogan]

Created attachment 90412 [details]
Qt Patch

[Robert Hogan]

Created attachment 91116 [details]
Patch

[Robert Hogan]

(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 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]

Created attachment 91538 [details]
Patch

[Robert Hogan]

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]

Created attachment 95268 [details]
Patch

[Early Warning System Bot]

Comment on attachment 95268 [details]
Patch

Attachment 95268 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8743569

[WebKit Review Bot]

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]

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]

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]

Created attachment 95463 [details]
Patch

[Peter Hartmann]

(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]

Created attachment 97701 [details]
Patch

[Benjamin Poulain]

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

(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 on attachment 97701 [details]
Patch

Attachment 97701 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8909446

[Robert Hogan]

Committed r90341: <http://trac.webkit.org/changeset/90341>

[Robert Hogan]

Committed r90343: <http://trac.webkit.org/changeset/90343>

[Csaba Osztrogonác]

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]

(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]

(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]

still valid bug

[Allan Sandfeld Jensen]

Created attachment 177251 [details]
Patch

[Csaba Osztrogonác]

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]

Committed r137280: <http://trac.webkit.org/changeset/137280>

[Csaba Osztrogonác]

(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]

I skipped it again to paint the bot green. Please unskip it with the proper fix.