Bug 213502 - [GTK][WPE] Change the cookies accept policy when ITP is enabled
Summary: [GTK][WPE] Change the cookies accept policy when ITP is enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: Gtk
Depends on:
Blocks: 210184
  Show dependency treegraph
 
Reported: 2020-06-23 00:10 PDT by Carlos Garcia Campos
Modified: 2020-07-06 01:49 PDT (History)
8 users (show)

See Also:


Attachments
Patch (15.24 KB, patch)
2020-06-29 01:56 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (15.25 KB, patch)
2020-06-29 05:44 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Different approach (16.52 KB, patch)
2020-07-02 04:42 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch for landing (16.56 KB, patch)
2020-07-02 07:13 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (3.80 KB, patch)
2020-07-03 14:06 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-06-23 00:10:01 PDT
As Michael pointed out in https://gitlab.gnome.org/GNOME/epiphany/-/merge_requests/713#note_844468 there are combinations of ITP enabled/disabled with cookies accept policy that don't make sense. I'm not sure if we should just set accept policy to always internally when ITP is enabled and document that the cookies setting is ignored in that case, or if it's something expected to be handled by the browser.
Comment 1 Carlos Garcia Campos 2020-06-29 01:56:18 PDT
Created attachment 403038 [details]
Patch
Comment 2 EWS Watchlist 2020-06-29 01:56:59 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Carlos Garcia Campos 2020-06-29 05:44:12 PDT
Created attachment 403051 [details]
Patch
Comment 4 Michael Catanzaro 2020-06-29 08:11:32 PDT
Comment on attachment 403051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403051&action=review

OK, I'm pretty sure this is a sensible approach, but I'd still like to hear from Apple what their recommendation is. Seems like their API has exactly the same problem.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:207
> + * Note that the returned policy will always be %WEBKIT_COOKIE_POLICY_ACCEPT_ALWAYS while

while -> if
Comment 5 Michael Catanzaro 2020-06-29 09:11:25 PDT
(In reply to Michael Catanzaro from comment #4)
> OK, I'm pretty sure this is a sensible approach, but I'd still like to hear
> from Apple what their recommendation is. Seems like their API has exactly
> the same problem.

Alternative: we could leave things as-is, but document that applications enabling ITP likely want to use ACCEPT_ALWAYS cookie policy, because all the restrictions of both ITP and the cookie policy will be applied.

In theory, a really restrictive application (something much stricter than Epiphany) might want to block all cookies but still enable the other features of ITP, which this patch will prohibit.
Comment 6 Carlos Garcia Campos 2020-07-02 02:02:20 PDT
(In reply to Michael Catanzaro from comment #5)
> (In reply to Michael Catanzaro from comment #4)
> > OK, I'm pretty sure this is a sensible approach, but I'd still like to hear
> > from Apple what their recommendation is. Seems like their API has exactly
> > the same problem.
> 
> Alternative: we could leave things as-is, but document that applications
> enabling ITP likely want to use ACCEPT_ALWAYS cookie policy, because all the
> restrictions of both ITP and the cookie policy will be applied.

I think it's better if apps just need to enable itp.

> In theory, a really restrictive application (something much stricter than
> Epiphany) might want to block all cookies but still enable the other
> features of ITP, which this patch will prohibit.

I thought about that, maybe we can only change the policy if it's no-third-party.
Comment 7 Carlos Garcia Campos 2020-07-02 04:42:02 PDT
Created attachment 403362 [details]
Different approach

This only sets always when no-third-party is set, so you can still disable cookies even with ITP enabled.
Comment 8 Diego Pino 2020-07-02 05:38:35 PDT
According to the EWS GTK-WK2 bot this patch will cause 8 tests failures when it lands (https://ews-build.webkit-uat.org/#/builders/34/builds/1005/steps/16/logs/stdio):

Regressions: Unexpected text-only failures (8)
  http/tests/plugins/third-party-cookie-accept-policy.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html [ Failure ]
  http/wpt/service-workers/third-party-cookie.html [ Failure ]
  imported/w3c/web-platform-tests/cors/credentials-flag.htm [ Failure ]
  imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies.any.worker.html [ Failure ]
  imported/w3c/web-platform-tests/xhr/access-control-preflight-request-must-not-contain-cookie.htm [ Failure ]

Try running these tests locally to confirm.
Comment 9 Carlos Garcia Campos 2020-07-02 05:46:52 PDT
(In reply to Diego Pino from comment #8)
> According to the EWS GTK-WK2 bot this patch will cause 8 tests failures when
> it lands
> (https://ews-build.webkit-uat.org/#/builders/34/builds/1005/steps/16/logs/
> stdio):
> 
> Regressions: Unexpected text-only failures (8)
>   http/tests/plugins/third-party-cookie-accept-policy.html [ Failure ]
>  
> http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-
> party.html [ Failure ]
>  
> http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-as-
> third-party.html [ Failure ]
>   http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html [
> Failure ]
>   http/wpt/service-workers/third-party-cookie.html [ Failure ]
>   imported/w3c/web-platform-tests/cors/credentials-flag.htm [ Failure ]
>  
> imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies.any.worker.html
> [ Failure ]
>  
> imported/w3c/web-platform-tests/xhr/access-control-preflight-request-must-
> not-contain-cookie.htm [ Failure ]
> 
> Try running these tests locally to confirm.

This is with the first patch, right?
Comment 10 Michael Catanzaro 2020-07-02 06:22:57 PDT
Why do we have an EWS that doesn't appear on Bugzilla...?
Comment 11 Michael Catanzaro 2020-07-02 06:25:00 PDT
Comment on attachment 403362 [details]
Different approach

This looks good to me. Something must be wrong if it's really affecting non-ITP tests, but I'm sure you'll figure that out before landing.
Comment 12 Carlos Garcia Campos 2020-07-02 06:42:29 PDT
Yes, it happens with the second patch too. The WPT tests that are failing are actually progressions, the others are actual regressions. It affects non-ITP tests, because WTR always enables ITP. So, for these tests that are failing now ITP should be blocking the third-party cookies, but it isn't because WKWebsiteDataStoreStatisticsResetToConsistentState() sets the ITP third-party blocking mode to ThirdPartyCookieBlockingMode::OnlyAccordingToPerDomainPolicy. So, I wonder if we should only change the accept policy when the thisrd-party blocking mode is set to ThirdPartyCookieBlockingMode::All
Comment 13 Michael Catanzaro 2020-07-02 07:02:02 PDT
(In reply to Carlos Garcia Campos from comment #12)
> So, I wonder
> if we should only change the accept policy when the thisrd-party blocking
> mode is set to ThirdPartyCookieBlockingMode::All

That sounds probably right. I didn't know about this ThirdPartyCookieBlockingMode.

That only affects our tests though, since API users cannot change the mode away from OnlyAccordingToPerDomainPolicy.
Comment 14 Carlos Garcia Campos 2020-07-02 07:13:46 PDT
Created attachment 403364 [details]
Patch for landing

This should fix the tests failing.
Comment 15 Carlos Garcia Campos 2020-07-02 07:14:41 PDT
(In reply to Michael Catanzaro from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > So, I wonder
> > if we should only change the accept policy when the thisrd-party blocking
> > mode is set to ThirdPartyCookieBlockingMode::All
> 
> That sounds probably right. I didn't know about this
> ThirdPartyCookieBlockingMode.
> 
> That only affects our tests though, since API users cannot change the mode
> away from OnlyAccordingToPerDomainPolicy.

Exactly, from the API point of view All is always used.
Comment 16 Carlos Garcia Campos 2020-07-03 06:12:05 PDT
This patch would break several storageAccess tests. I don't understand how this can work in apple ports.

Regressions: Unexpected text-only failures (4)
  http/tests/storageAccess/request-and-grant-access-then-navigate-cross-site-should-not-have-access-database.html [ Failure ]
  http/tests/storageAccess/request-and-grant-access-then-navigate-cross-site-should-not-have-access.html [ Failure ]
  http/tests/storageAccess/request-and-grant-access-then-navigate-same-site-should-have-access-database.html [ Failure ]
  http/tests/storageAccess/request-and-grant-access-then-navigate-same-site-should-have-access.html [ Failure ]

Those are failing because the cookie is not set after being granted due to the no-third-party accept policy (which is the default in tests for all ports). Those tests don't change the ThirdPartyCookieBlockingMode, so OnlyAccordingToPerDomainPolicy is used and we don't change the policy in that case. I don't know why we are enabling ITP in all tests either.
Comment 17 Michael Catanzaro 2020-07-03 07:15:14 PDT
(In reply to Carlos Garcia Campos from comment #16)
> Those are failing because the cookie is not set after being granted due to
> the no-third-party accept policy (which is the default in tests for all
> ports). Those tests don't change the ThirdPartyCookieBlockingMode, so
> OnlyAccordingToPerDomainPolicy is used and we don't change the policy in
> that case. 

I think we could still make this work by having the GTK API layer fudge the cookie policy (since the test controller will not run that code), rather than doing it in the network layer.

But maybe it's better to just document that ITP makes the most sense when used with ACCEPT_ALWAYS, maybe print a g_warning() if ITP is enabled and the cookie policy is NO_THIRD_PARTY, but let apps set it anyway.

> I don't know why we are enabling ITP in all tests either.

Doesn't matter IMO. Let's just match what Apple does.
Comment 18 Carlos Garcia Campos 2020-07-03 07:22:07 PDT
(In reply to Michael Catanzaro from comment #17)
> (In reply to Carlos Garcia Campos from comment #16)
> > Those are failing because the cookie is not set after being granted due to
> > the no-third-party accept policy (which is the default in tests for all
> > ports). Those tests don't change the ThirdPartyCookieBlockingMode, so
> > OnlyAccordingToPerDomainPolicy is used and we don't change the policy in
> > that case. 
> 
> I think we could still make this work by having the GTK API layer fudge the
> cookie policy (since the test controller will not run that code), rather
> than doing it in the network layer.

It doesn't matter, the storageAccess tests I mentioned would still fail, because no-third-party is the default, so setting the cookie after granted by storage access is rejected by the cookies jar. That's exactly what this bug is about.

> But maybe it's better to just document that ITP makes the most sense when
> used with ACCEPT_ALWAYS, maybe print a g_warning() if ITP is enabled and the
> cookie policy is NO_THIRD_PARTY, but let apps set it anyway.

Or maybe I don't understand the different ThirdPartyCookieBlockingMode, but I'm pretty sure we want to use All unconditionally, otherwise we would be less restrictive than before.

> > I don't know why we are enabling ITP in all tests either.
> 
> Doesn't matter IMO. Let's just match what Apple does.
Comment 19 Michael Catanzaro 2020-07-03 08:06:41 PDT
(In reply to Carlos Garcia Campos from comment #18)
> It doesn't matter, the storageAccess tests I mentioned would still fail,
> because no-third-party is the default, so setting the cookie after granted
> by storage access is rejected by the cookies jar. That's exactly what this
> bug is about.

Er, OK, but then if Apple is also using no-third-party by default... then the tests should be failing for it too... but they're not?
 
> Or maybe I don't understand the different ThirdPartyCookieBlockingMode, but
> I'm pretty sure we want to use All unconditionally, otherwise we would be
> less restrictive than before.

Right, we definitely want to use All unconditionally in our API. But when running tests, we want to use whatever Apple uses when running tests.

I think OnlyAccordingToPerDomainPolicy means block third-party cookies for *prevalent* domains (domains classified as trackers). I don't think it means "just follow the cookie accept policy." I guess that mode will probably be removed eventually since Google figured out it could be used as a supercookie by manipulating WebKit into classifying certain domains as trackers, similar to HSTS abuse.

I'm not sure if AllOnSitesWithoutUserInteraction was ever shipped by Safari, but it's obsoleted by All.
Comment 20 Michael Catanzaro 2020-07-03 08:13:29 PDT
(In reply to Carlos Garcia Campos from comment #18)
> It doesn't matter, the storageAccess tests I mentioned would still fail,
> because no-third-party is the default, so setting the cookie after granted
> by storage access is rejected by the cookies jar. That's exactly what this
> bug is about.

Guess: could this be bug #193458? Our HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain is equivalent to Apple's HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain. So if we both default to OnlyFromMainDocumentDomain when running tests, we wind up using a more restrictive cookie policy than Apple.
Comment 21 Michael Catanzaro 2020-07-03 14:06:40 PDT
Created attachment 403489 [details]
Patch
Comment 22 Michael Catanzaro 2020-07-03 16:35:27 PDT
(In reply to Carlos Garcia Campos from comment #16)
> This patch would break several storageAccess tests. I don't understand how
> this can work in apple ports.
> 
> Regressions: Unexpected text-only failures (4)
>  
> http/tests/storageAccess/request-and-grant-access-then-navigate-cross-site-
> should-not-have-access-database.html [ Failure ]
>  
> http/tests/storageAccess/request-and-grant-access-then-navigate-cross-site-
> should-not-have-access.html [ Failure ]

Looks like my guess was right for these two tests. You can see my debug patch changing Apple's cookie policy to match ours causes these tests to fail. We should reconsider how best to solve bug #193458. 

But something else must be wrong with the other two....
Comment 23 Diego Pino 2020-07-04 01:35:04 PDT
(In reply to Michael Catanzaro from comment #10)
> Why do we have an EWS that doesn't appear on Bugzilla...?

It's not in production yet. At this moment it's on the EWS UAT (User Acceptance Testing) server. Once the bots can process incoming requests fast enough it will be moved to production and then there will be a new EWS bubble for GTK layout-tests. In the meantime, it will be possible to peep the UAT to check if a patch may have broken some layout test.
Comment 24 Carlos Garcia Campos 2020-07-04 01:59:54 PDT
Ok, then I think we can:

1- Land this patch, as it seems to be the right thing and it only breaks 4 tests that are currently skipped.

2- File a bug report to support the no-third-party grandfathering mode.

3- Enable storageAccess tests with the four tests failing marked as failures and pointing to the bug created in 2

4- Add new APÎ to libsoup to add the new mode using your old patch.

5- Fix bug in WebKit using the new API and unskip the test passing.
Comment 25 Carlos Garcia Campos 2020-07-04 02:02:24 PDT
So, once we have the new no-third-party mode, I wonder if it would be enough to use that when ITP is enabled, instead of always.
Comment 26 Carlos Garcia Campos 2020-07-04 04:59:48 PDT
(In reply to Michael Catanzaro from comment #20)
> (In reply to Carlos Garcia Campos from comment #18)
> > It doesn't matter, the storageAccess tests I mentioned would still fail,
> > because no-third-party is the default, so setting the cookie after granted
> > by storage access is rejected by the cookies jar. That's exactly what this
> > bug is about.
> 
> Guess: could this be bug #193458? Our
> HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain is equivalent to Apple's
> HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain. So if we both
> default to OnlyFromMainDocumentDomain when running tests, we wind up using a
> more restrictive cookie policy than Apple.

Yes, I can confirm that was it. I never understood the difference between only and exclusively. So, I took your libsoup patch, updated to add a new policy instead, wrote a WebKit patch to use it when OnlyFromMainDocumentDomain is desired and all tests pass, including the storageAccess ones.
Comment 27 Michael Catanzaro 2020-07-04 07:57:12 PDT
Layout test EWS is exciting, cool. :)

(In reply to Carlos Garcia Campos from comment #25)
> So, once we have the new no-third-party mode, I wonder if it would be enough
> to use that when ITP is enabled, instead of always.

I don't think that would work, because then the cookie policy could still block cookie storage even after an approved storage access API request. I think it has to be Always.
Comment 28 Carlos Garcia Campos 2020-07-06 01:49:37 PDT
Committed r263964: <https://trac.webkit.org/changeset/263964>