[GTK][WPE] Change the cookies accept policy when ITP is enabled

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 403038 [details]
Patch

[EWS Watchlist]

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

[Carlos Garcia Campos]

Created attachment 403051 [details]
Patch

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Diego Pino]

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.

[Carlos Garcia Campos]

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

[Michael Catanzaro]

Why do we have an EWS that doesn't appear on Bugzilla...?

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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

[Carlos Garcia Campos]

Created attachment 403364 [details]
Patch for landing

This should fix the tests failing.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 403489 [details]
Patch

[Michael Catanzaro]

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

[Diego Pino]

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

Committed r263964: <https://trac.webkit.org/changeset/263964>