Bug 204137 – [GTK][WPE] Add same-site cookie support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Patrick Griffis]

[GTK][WPE] Add same-site cookie support

[Patrick Griffis]

Created attachment 383419 [details]
Patch

[Patrick Griffis]

Note that this requires libsoup to be merged: https://gitlab.gnome.org/GNOME/libsoup/merge_requests/36

Once merged do we build the development release in JHBuild?

[Patrick Griffis]

Created attachment 383420 [details]
Patch

[Michael Catanzaro]

(In reply to Patrick Griffis from comment #2)
> Once merged do we build the development release in JHBuild?

You'll want to update the jhbuild moduleset in your patch here to ensure you don't break the test expectations with your expectations changes.

[Michael Catanzaro]

*** Bug 184955 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Comment on attachment 383420 [details]
Patch

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

r- because tests will fail, we need to update the libsoup in jhbuild too.

> Source/WebCore/ChangeLog:11
> +        Added an httpMethod argument to cookie lookup functions for the
> +        soup API to use. Modified other backends to simply ignore this.

Do you know why other ports don't need the http method?

> Source/WebCore/loader/CookieJar.cpp:84
> +        result = session->cookiesForDOM(document.firstPartyForCookies(), sameSiteInfo(document), url, frameID, pageID, includeSecureCookies, httpMethod);

We only need the http method to determine whether it's safe, right? and it's only used by same site cookies, right? If that's the case maybe it would be easier to add bool isSafeHTTPMethod to SameSitoInfo and we don't need to add a new parameter. We still have time to change the libsoup API or we can use any safe HTTP method when it's safe, since it's only used for that.

> Source/WebCore/platform/network/soup/CookieSoup.cpp:35
> +#if SOUP_CHECK_VERSION(2, 69, 0)

Better use the first version including the new api, even if it's an unstable release. That way we don't need to wait until the next stable release to test this.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:486
> +        httpMethodStr = httpMethod->ascii().data();

This is a temporary, either cache the CString returned by ascii() or do httpMethod ? httpMethod->ascii().data() : nullptr directly in the call to soup_cookie_jar_get_cookie_list_with_same_site_info().

> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:187
> +        URL cookieURL = soupURIToURL(siteForCookies);
> +        setIsSameSite(areRegistrableDomainsEqual(cookieURL, m_url));

This could be one line.

> LayoutTests/ChangeLog:9
> +        Updated GTK/WPE test expectations to pass most same-site cookie tests
> +        matching the Apple ports.

To do this, we need to wait for the next libsoup unstable release and use it in out jhbuild, otherwise the tests will fail in the bots that use a previous version.

[Patrick Griffis]

Created attachment 389653 [details]
Patch

[Patrick Griffis]

GNOME ftp doesn't have the tarball yet, not sure how long that is cached for.

[Patrick Griffis]

Created attachment 389654 [details]
Patch

[Michael Catanzaro]

Comment on attachment 389654 [details]
Patch

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

> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:189
> +    else
> +#endif
> +        m_sameSiteDisposition = SameSiteDisposition::Unspecified;

I can tell you Carlos doesn't like to mix #if #endif and runtime conditionals like this. -Wmisleading-indentation won't like it either.

Look how much cleaner it is this way, with just one line of code duplication:

#if SOUP_CHECK_VERSION(2, 69, 90)
    setIsTopSite(soup_message_get_is_top_level_navigation(soupMessage));

    if (SoupURI* siteForCookies = soup_message_get_site_for_cookies(soupMessage))
        setIsSameSite(areRegistrableDomainsEqual(soupURIToURL(siteForCookies), m_url));
    else
        m_sameSiteDisposition = SameSiteDisposition::Unspecified;
#else
    m_sameSiteDisposition = SameSiteDisposition::Unspecified;
#endif

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:688
> +    if (isTopLevelNavigation()) {
> +        request.setFirstPartyForCookies(request.url());
> +#if SOUP_CHECK_VERSION(2, 69, 90)
> +        soup_message_set_is_top_level_navigation(m_soupMessage.get(), true);
> +    }
> +
> +    if (request.isSameSite()) {
> +        GUniquePtr<SoupURI> requestURI = urlToSoupURI(request.url());
> +        soup_message_set_site_for_cookies(m_soupMessage.get(), requestURI.get());
> +#endif
> +    }

if (isTopLevelNavigation()) {
        request.setFirstPartyForCookies(request.url());
#if SOUP_CHECK_VERSION(2, 69, 90)
        soup_message_set_is_top_level_navigation(m_soupMessage.get(), true);
#endif
    }

#if SOUP_CHECK_VERSION(2, 69, 90)
    if (request.isSameSite()) {
        GUniquePtr<SoupURI> requestURI = urlToSoupURI(request.url());
        soup_message_set_site_for_cookies(m_soupMessage.get(), requestURI.get());
    }
#endif

> Tools/gtk/jhbuild.modules:56
>    <repository type="tarball" name="ftp.gnome.org"
>        href="http://ftp.gnome.org"/>

Future work: everything should switch to use download.gnome.org.

[Carlos Garcia Campos]

Comment on attachment 389654 [details]
Patch

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

> Source/WebCore/platform/network/SameSiteInfo.cpp:36
> +static bool httpMethodIsSafe(const String& method)
> +{
> +    return method == "GET" || method == "HEAD" || method == "OPTIONS" || method == "TRACE";
> +}

I think this could be moved to HTTPParsers.cpp as isSafeMethod(). We should also use equalLettersIgnoringASCIICase() instead of checking the uppercase method names directly.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:459
> +    GUniquePtr<SoupURI> cookieURI = sameSiteInfo.isSameSite ? urlToSoupURI(url) : nullptr;

Move this after the if (!firstPartyURI) because this is not needed if we return early.

[Patrick Griffis]

Created attachment 389666 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 389666 [details]
Patch

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

LGTM

> Source/WebCore/platform/network/HTTPParsers.cpp:988
> +bool isSafeMethod(const String& method)

Add a comment linking to https://tools.ietf.org/html/rfc7231#section-4.2.1

[Patrick Griffis]

Created attachment 389855 [details]
Patch

[Carlos Garcia Campos]

Has libsoup 2.69.90 been released? the tarball doesn't exist, at least in the url https://download.gnome.org/libsoup/2.69/libsoup-2.69.90.tar.xz

[Carlos Garcia Campos]

Ok, the right url is https://download.gnome.org/sources/libsoup/2.69/libsoup-2.69.90.tar.xz so the repo in the jhbuild moduleset is wrong

[Carlos Garcia Campos]

Comment on attachment 389855 [details]
Patch

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

> Tools/gtk/jhbuild.modules:58
> +  <repository type="tarball" name="download.gnome.org"
> +      href="https://download.gnome.org/sources"/>

Maybe thi needs a trailing slash? https://download.gnome.org/sources/

[Patrick Griffis]

Created attachment 389945 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389945 [details]:

imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html bug 207335 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 389945 [details]
Patch

Clearing flags on attachment: 389945

Committed r256013: <https://trac.webkit.org/changeset/256013>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.