WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Robert Hogan
Comment 1
2010-09-09 07:46:56 PDT
Created
attachment 67029
[details]
Patch
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 6
2010-09-13 12:25:43 PDT
See:
http://bugreports.qt.nokia.com/browse/QTBUG-13601
and
http://qt.gitorious.org/qt/qt/merge_requests/2473
Robert Hogan
Comment 7
2010-09-13 12:36:49 PDT
Created
attachment 67457
[details]
Patch
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
Attachment 67457
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/3902448
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
Created
attachment 91116
[details]
Patch
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
Created
attachment 91538
[details]
Patch
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
Created
attachment 95268
[details]
Patch
Early Warning System Bot
Comment 30
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
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
Created
attachment 95463
[details]
Patch
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
Created
attachment 97701
[details]
Patch
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
Comment on
attachment 97701
[details]
Patch
Attachment 97701
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/8909446
Robert Hogan
Comment 41
2011-07-03 10:14:55 PDT
Committed
r90341
: <
http://trac.webkit.org/changeset/90341
>
Robert Hogan
Comment 42
2011-07-03 10:42:46 PDT
Committed
r90343
: <
http://trac.webkit.org/changeset/90343
>
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
Created
attachment 177251
[details]
Patch
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
Committed
r137280
: <
http://trac.webkit.org/changeset/137280
>
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.
Top of Page
Format For Printing
XML
Clone This Bug