Bug 170798 - Make WebCore::topPrivatelyControlledDomain() return "localhost" for localhost
Summary: Make WebCore::topPrivatelyControlledDomain() return "localhost" for localhost
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-12 18:00 PDT by John Wilander
Modified: 2017-04-18 17:04 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.50 KB, patch)
2017-04-12 18:19 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (4.09 KB, patch)
2017-04-17 15:46 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (4.27 KB, patch)
2017-04-18 16:21 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2017-04-12 18:00:23 PDT
WebCore::topPrivatelyControlledDomain() is used for partitioning and should return "localhost" if the host domain is "localhost".
Comment 1 Radar WebKit Bug Importer 2017-04-12 18:04:10 PDT
<rdar://problem/31595108>
Comment 2 John Wilander 2017-04-12 18:19:28 PDT
Created attachment 306959 [details]
Patch
Comment 3 Alex Christensen 2017-04-12 22:24:20 PDT
Comment on attachment 306959 [details]
Patch

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

> Source/WebCore/platform/mac/PublicSuffixMac.mm:48
> +    if ([domain _web_looksLikeIPAddress] || [domain isEqualToString:@"localhost"])

Let's do a case-insensitive comparison and add a test for LocalHost.
Comment 4 John Wilander 2017-04-17 15:46:30 PDT
Created attachment 307309 [details]
Patch
Comment 5 Alex Christensen 2017-04-18 14:34:01 PDT
Comment on attachment 307309 [details]
Patch

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

> Source/WebCore/platform/mac/PublicSuffixMac.mm:54
> +    const auto& lowercaseDomain = domain.convertToASCIILowercase();

I guess punycode encoding should not happen here.  This seems correct.

> Tools/TestWebKitAPI/Tests/mac/PublicSuffix.mm:64
> +    EXPECT_FALSE(isPublicSuffix(""));

Is this the UTF-8 encoding if an uppercase non-ASCII character?  Careful when committing, because sometimes our tools like webkit-patch UTF-8 encode non-ASCII characters.  svn  commit doesn't change the characters.
We should probably include a test with an uppercase non-ASCII character and a test with a lowercase non-ASCII character just for fun.

> Tools/TestWebKitAPI/Tests/mac/PublicSuffix.mm:90
> +    EXPECT_EQ(String("example.com"), topPrivatelyControlledDomain("ExamPle.com"));

And let's add a test with a.b.subdomain.Example.com
Comment 6 John Wilander 2017-04-18 16:21:12 PDT
Created attachment 307433 [details]
Patch for landing
Comment 7 John Wilander 2017-04-18 16:22:08 PDT
(In reply to Alex Christensen from comment #5)
> Comment on attachment 307309 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307309&action=review
> 
> > Source/WebCore/platform/mac/PublicSuffixMac.mm:54
> > +    const auto& lowercaseDomain = domain.convertToASCIILowercase();
> 
> I guess punycode encoding should not happen here.  This seems correct.
> 
> > Tools/TestWebKitAPI/Tests/mac/PublicSuffix.mm:64
> > +    EXPECT_FALSE(isPublicSuffix(""));
> 
> Is this the UTF-8 encoding if an uppercase non-ASCII character?  Careful
> when committing, because sometimes our tools like webkit-patch UTF-8 encode
> non-ASCII characters.  svn  commit doesn't change the characters.
> We should probably include a test with an uppercase non-ASCII character and
> a test with a lowercase non-ASCII character just for fun.
> 
> > Tools/TestWebKitAPI/Tests/mac/PublicSuffix.mm:90
> > +    EXPECT_EQ(String("example.com"), topPrivatelyControlledDomain("ExamPle.com"));
> 
> And let's add a test with a.b.subdomain.Example.com

All done. Thanks, Alex!
Comment 8 WebKit Commit Bot 2017-04-18 17:04:54 PDT
Comment on attachment 307433 [details]
Patch for landing

Clearing flags on attachment: 307433

Committed r215489: <http://trac.webkit.org/changeset/215489>
Comment 9 WebKit Commit Bot 2017-04-18 17:04:56 PDT
All reviewed patches have been landed.  Closing bug.