Bug 207984 - [WPE][GTK] googleapis.com is a public suffix, defeating isGoogle() check in UserAgentQuirks.cpp
Summary: [WPE][GTK] googleapis.com is a public suffix, defeating isGoogle() check in U...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-19 19:12 PST by Michael Catanzaro
Modified: 2021-02-17 07:07 PST (History)
3 users (show)

See Also:


Attachments
Patch (3.88 KB, patch)
2020-02-20 14:34 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (4.01 KB, patch)
2020-02-21 08:55 PST, 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 Michael Catanzaro 2020-02-19 19:12:01 PST
At first I thought this was a URLParser bug, but no, for whatever ungodly reason, googleapis.com has been added to the public suffix list: https://github.com/publicsuffix/list/blob/7922d7c20e246552be418e8f72e577899fd30d99/public_suffix_list.dat#L11922

So while, for example, soup_tld_domain_is_public_suffix() would normally only return TRUE for values like "org", "com", "co.uk", etc., it will also return TRUE for "googleapis.com". Notably, this means that the base domain of "fonts.googleapis.com" is actually "fonts.googleapis.com", not "googleapis.com" as one would naively expect. Our base domain test in UserAgentQuirks.cpp is broken. I think we need to change it from:

    if (baseDomain.startsWith("google."))
        return true;
    if (baseDomain == "gstatic.com")
        return true;
    if (baseDomain == "googleapis.com")
        return true;
    if (baseDomain == "googleusercontent.com")
        return true;

To:

    if (baseDomain.startsWith("google."))
        return true;
    if (baseDomain == "gstatic.com")
        return true;
    if (baseDomain.endsWith("googleapis.com"))
        return true;
    if (baseDomain == "googleusercontent.com")
        return true;

Requires testing.
Comment 1 Michael Catanzaro 2020-02-20 14:34:15 PST
Created attachment 391335 [details]
Patch
Comment 2 Daniel Bates 2020-02-20 20:03:49 PST
Comment on attachment 391335 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Fix the check for googleapis.com. Since it's now a public suffix, we can no longer check the

This is ok as-is. A better commit message would reference the public suffix change for convenience and grepping

> Source/WebCore/platform/UserAgentQuirks.cpp:40
> +    String baseDomain = topPrivatelyControlledDomain(domain);

This is ok as-is. The optimal solution would be for topPriva...() to take a StringView because it avoids a copy.

> Source/WebCore/platform/UserAgentQuirks.cpp:56
> +    if (domain.endsWith(".googleapis.com"))

Is this case-insensitive?If not, should it be?
Comment 3 Michael Catanzaro 2020-02-21 07:45:20 PST
(In reply to Daniel Bates from comment #2)
> This is ok as-is. A better commit message would reference the public suffix
> change for convenience and grepping

It's not simple to figure out which commit added it, because GitHub times out when blaming this file.

> > Source/WebCore/platform/UserAgentQuirks.cpp:40
> > +    String baseDomain = topPrivatelyControlledDomain(domain);
> 
> This is ok as-is. The optimal solution would be for topPriva...() to take a
> StringView because it avoids a copy.

Are you requesting a new overload of topPrivatelyControlledDomain? Like this:

WEBCORE_EXPORT String topPrivatelyControlledDomain(const String& domain);
WEBCORE_EXPORT StringView topPrivatelyControlledDomain(const StringView& domain);

?

> > Source/WebCore/platform/UserAgentQuirks.cpp:56
> > +    if (domain.endsWith(".googleapis.com"))
> 
> Is this case-insensitive?If not, should it be?

It doesn't need to be case-insensitive. It would only need to be case-insensitive (a) if some Google property were to actually use uppercase letters when loading this domain, *and* (b) if that resource load happened to be the particular resource load that was discriminating against WebKit based on user agent. In practice, (a) is never true, so no need to for it to be case-insensitive.
Comment 4 WebKit Commit Bot 2020-02-21 07:47:09 PST
Comment on attachment 391335 [details]
Patch

Rejecting attachment 391335 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 391335, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=391335&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=207984&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 391335 from bug 207984.
Fetching: https://bugs.webkit.org/attachment.cgi?id=391335
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Daniel Bates']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 4 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/UserAgentQuirks.cpp
Hunk #1 succeeded at 36 with fuzz 1.
Hunk #2 succeeded at 48 (offset -1 lines).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/Tests/WebCore/UserAgentQuirks.cpp
Hunk #1 FAILED at 85.
1 out of 1 hunk FAILED -- saving rejects to file Tools/TestWebKitAPI/Tests/WebCore/UserAgentQuirks.cpp.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Daniel Bates']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/13326249
Comment 5 Michael Catanzaro 2020-02-21 08:55:50 PST
Created attachment 391405 [details]
Patch
Comment 6 WebKit Commit Bot 2020-02-21 09:22:42 PST
Comment on attachment 391405 [details]
Patch

Clearing flags on attachment: 391405

Committed r257139: <https://trac.webkit.org/changeset/257139>
Comment 7 WebKit Commit Bot 2020-02-21 09:22:44 PST
All reviewed patches have been landed.  Closing bug.