Bug 213954 - [SOUP] Add support for HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain
Summary: [SOUP] Add support for HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 214077
  Show dependency treegraph
 
Reported: 2020-07-04 05:18 PDT by Carlos Garcia Campos
Modified: 2020-07-09 01:23 PDT (History)
9 users (show)

See Also:


Attachments
WIP (4.18 KB, patch)
2020-07-04 05:21 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP (9.13 KB, patch)
2020-07-07 07:38 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (13.86 KB, patch)
2020-07-08 05:23 PDT, Carlos Garcia Campos
mcatanzaro: review+
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-07-04 05:18:55 PDT
We currently use HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain as SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY because that's the default, but it's not really supported by libsoup. SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY is actually HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain. We need new libsoup API to add the new policy and use the right one. I'm not sure we want to expose the new one in the API though, since it's less restrictive and we now have ITP support.
Comment 1 Carlos Garcia Campos 2020-07-04 05:21:27 PDT
Created attachment 403520 [details]
WIP

This is wip, it requires new libsoup API, and a new release to add version checks to use the new policy. See https://gitlab.gnome.org/GNOME/libsoup/-/merge_requests/127
Comment 2 Carlos Garcia Campos 2020-07-04 05:22:56 PDT
After this, I agree we should rename the policies to avoid confusion between "Only" and "Exclusively" as proposed in bug #193458.
Comment 3 Michael Catanzaro 2020-07-04 08:38:12 PDT
Comment on attachment 403520 [details]
WIP

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

Looks good.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:294
> -        if (m_thirdPartyCookieBlockingMode == ThirdPartyCookieBlockingMode::All && m_cookieAcceptPolicy == HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain)
> +        if (m_thirdPartyCookieBlockingMode == ThirdPartyCookieBlockingMode::All && m_cookieAcceptPolicy == HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain)

I think we need a comment to explain why we are rejecting changes to ExclusivelyFromMainDocumentDomain but not to OnlyFromMainDocumentDomain: OnlyFromMainDocumentDomain is the default behavior when running layout tests, we have a couple tests that depend on it, and OnlyFromMainDocumentDomain cannot be set by public API users so they'll never notice.
Comment 4 Carlos Garcia Campos 2020-07-07 07:38:59 PDT
Created attachment 403687 [details]
WIP

Updated and also includes the test expectations.
Comment 5 Carlos Garcia Campos 2020-07-08 05:23:13 PDT
Created attachment 403772 [details]
Patch
Comment 6 EWS Watchlist 2020-07-08 05:41:01 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 7 Philippe Normand 2020-07-08 06:38:16 PDT
Comment on attachment 403772 [details]
Patch

I have built this, along with SDK, http tests pass in GTK/Release.
Comment 8 Carlos Garcia Campos 2020-07-08 23:45:14 PDT
(In reply to Philippe Normand from comment #7)
> Comment on attachment 403772 [details]
> Patch
> 
> I have built this, along with SDK, http tests pass in GTK/Release.

Does this mean I can land the patch and the bots will have the new libsoup already?
Comment 9 Carlos Garcia Campos 2020-07-09 00:57:09 PDT
Committed r264156: <https://trac.webkit.org/changeset/264156>
Comment 10 Philippe Normand 2020-07-09 01:23:33 PDT
SDK r264156 is deployed...