Bug 207984 – [WPE][GTK] googleapis.com is a public suffix, defeating isGoogle() check in UserAgentQuirks.cpp

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 391335 [details]
Patch

[Daniel Bates]

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?

[Michael Catanzaro]

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

[WebKit Commit Bot]

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 [e-mail]...
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

[Michael Catanzaro]

Created attachment 391405 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391405 [details]
Patch

Clearing flags on attachment: 391405

Committed r257139: <https://trac.webkit.org/changeset/257139>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.