Support internal hostnames (such as http://localhost) in domain-based deletion

Support internal hostnames (such as http://localhost) in domain-based deletion

Until now, RegistrableDomainFilterBuilder allowed the filters to contain only
eTLD+1 and IP addresses. This excluded internal hostnames, such as
http://localhost.

Since domain scoped cookies can not be set on domains that do not contain
at least one dot (two dots according to RFC 2109, but the leading dot
is automatically prepended), cookies for "localhost" will never be visible
from other hosts. In particular, they will not be visible from
"subdomain.localhost", therefore it is sufficient if the filter requires
exact hostname match in these cases.

Note that "subdomain.localhost" allows domain cookies, and these will be
visible from "third-level-domain.subdomain.localhost". GetDomainAndRegistry()
treats "subdomain.localhost" as an eTLD+1 from an unknown registry, and
RegistrableDomainFilterBuilder in this case works as usual.

BUG=589586
Committed: https://crrev.com/dba4dbf970d73e637514fb1b3a7b464f632d2222
Cr-Commit-Position: refs/heads/master@{#404128}

[msramek, 2016-07-05 14:26:50]

Patchset #1 (id:1) has been deleted

[msramek, 2016-07-05 14:28:24]

msramek@chromium.org changed reviewers:
+ dmurph@chromium.org, mkwst@chromium.org

[msramek, 2016-07-05 14:28:24]

Hi Mike and Dan,

please have a look and let me know if this makes sense :)

Thanks,
Martin

[Mike West, 2016-07-06 06:48:00]

https://codereview.chromium.org/2127603002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/registrable_domain_filter_builder.h (right):

https://codereview.chromium.org/2127603002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder.h:44: // will
never be visible on "*.localhost" or vice versa.
This is pretty weird. Does our cookie implementation have the same behavior?
That is, if I set a cookie on `localhost`, are we sure it doesn't span across
`*.localhost`? More saliently, are we sure that a response from
`subsubdomain.subdomain.not-an-actual-tld` can't set a cookie for
`subdomain.not-an-actual-tld` or `not-an-actual-tld`?

[msramek, 2016-07-06 08:58:29]

https://codereview.chromium.org/2127603002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/registrable_domain_filter_builder.h (right):

https://codereview.chromium.org/2127603002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder.h:44: // will
never be visible on "*.localhost" or vice versa.
On 2016/07/06 06:48:00, Mike West wrote:
> This is pretty weird. Does our cookie implementation have the same behavior?
> That is, if I set a cookie on `localhost`, are we sure it doesn't span across
> `*.localhost`? More saliently, are we sure that a response from
> `subsubdomain.subdomain.not-an-actual-tld` can't set a cookie for
> `subdomain.not-an-actual-tld` or `not-an-actual-tld`?

Below are the results of my experiments. They're consistent
with what I wrote; and indeed, the boundary between "localhost"
and "*.localhost" seems to be necessitated by the RFC 2109
requirement that the domain attribute must have an embedded dot,
therefore there can be no domain cookies on localhost.

In addition, it is also consistent with GetDomainAndRegistry(),
where:
GetDomainAndRegistry("subdomain.localhost") == "subdomain.localhost"
GetDomainAndRegistry("localhost") == ""

========================================================

-----> http://localhost

> document.cookie="a=1"
> document.cookie
< "a=1"  // visible

> document.cookie="b=2; domain=.localhost"
> document.cookie
< "a=1"  // not set; domain cookies don't work on localhost

> document.cookie="c=3; domain=.test.localhost"
> document.cookie
< "a=1"  // not set; domain cooies don't work on localhost

-----> http://test.localhost

> document.cookie
< ""     // nothing, because c and d were not set

-----> http://www.test.localhost

> document.cookie
< ""     // nothing, because c and d were not set

========================================================

-----> http://www.test.localhost

> document.cookie="d=4"
> document.cookie="e=5; domain=.test.localhost"
> document.cookie="f=6; domain=.localhost"
> document.cookie
< "d=4; e=5"    // d and e match; f is not supposed to work

-----> http://test.localhost

> document.cookie
< "e=5"         // only e matches, because d was a different domain

-----> http://localhost

> document.cookie
< "a=1"         // confirmed that f did not work

========================================================

Another observation is that in most cases setting domain with the
leading dot ("e=5; domain=.test.localhost") works the same as omitting it
("e=5; domain=test.localhost"), but this is not true on localhost.

On localhost, using "domain=localhost" works the same way as omitting
the domain, i.e. the cookie is set and only visible from localhost.
Using "domain=localhost" from other domains (such as www.test.localhost)
won't set the cookie (same way as "domain=.localhost" wouldn't).

[Mike West, 2016-07-06 09:30:09]

Thanks for the explanation and tests. LGTM with a test nit.

https://codereview.chromium.org/2127603002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/registrable_domain_filter_builder_unittest.cc
(right):

https://codereview.chromium.org/2127603002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder_unittest.cc:146:
{"http://website.fileserver/foo/bar", false },
Could you add tests for the `subsubdomain.subdomain.fileserver` behavior
discussed below (`www.test.localhost`, etc).

[dmurph, 2016-07-06 18:46:21]

This lgtm. I wish I noticed that the blacklist/whitelist differences w/
testcases was a simple inversion of the results earlier, as the test cases would
be /2. Sorry about that!

[msramek, 2016-07-07 10:44:35]

Patchset #2 (id:40001) has been deleted

[msramek, 2016-07-07 10:46:29]

Thanks!

I made sure to keep the pairs of test symmetrical, so I can simplify them in a
followup CL :)

https://codereview.chromium.org/2127603002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/registrable_domain_filter_builder_unittest.cc
(right):

https://codereview.chromium.org/2127603002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder_unittest.cc:146:
{"http://website.fileserver/foo/bar", false },
On 2016/07/06 09:30:09, Mike West wrote:
> Could you add tests for the `subsubdomain.subdomain.fileserver` behavior
> discussed below (`www.test.localhost`, etc).

Done.

Right, I already defined "fakedomain.faketld" above, just forgot to follow
through in the rest of tests.

I renamed it to "second-level-domain.fileserver" and test that it is recognized
as a valid registrable domain, and that its subdomains are recognized, but that
it itself is not recognized as a subdomain of fileserver.

[msramek, 2016-07-07 10:46:35]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-07-07 10:46:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/2127603002/#ps60001
(title: "More tests")

[commit-bot: I haz the power, 2016-07-07 10:46:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 11:19:07]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-07-07 11:19:09]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 11:22:10]

Description was changed from

==========
Support internal hostnames (such as http://localhost) in domain-based deletion

Until now, RegistrableDomainFilterBuilder allowed the filters to contain only
eTLD+1 and IP addresses. This excluded internal hostnames, such as
http://localhost.

Since domain scoped cookies can not be set on domains that do not contain
at least one dot (two dots according to RFC 2109, but the leading dot
is automatically prepended), cookies for "localhost" will never be visible
from other hosts. In particular, they will not be visible from
"subdomain.localhost", therefore it is sufficient if the filter requires
exact hostname match in these cases.

Note that "subdomain.localhost" allows domain cookies, and these will be
visible from "third-level-domain.subdomain.localhost". GetDomainAndRegistry()
treats "subdomain.localhost" as an eTLD+1 from an unknown registry, and
RegistrableDomainFilterBuilder in this case works as usual.

BUG=589586
==========

to

==========
Support internal hostnames (such as http://localhost) in domain-based deletion

Until now, RegistrableDomainFilterBuilder allowed the filters to contain only
eTLD+1 and IP addresses. This excluded internal hostnames, such as
http://localhost.

Since domain scoped cookies can not be set on domains that do not contain
at least one dot (two dots according to RFC 2109, but the leading dot
is automatically prepended), cookies for "localhost" will never be visible
from other hosts. In particular, they will not be visible from
"subdomain.localhost", therefore it is sufficient if the filter requires
exact hostname match in these cases.

Note that "subdomain.localhost" allows domain cookies, and these will be
visible from "third-level-domain.subdomain.localhost". GetDomainAndRegistry()
treats "subdomain.localhost" as an eTLD+1 from an unknown registry, and
RegistrableDomainFilterBuilder in this case works as usual.

BUG=589586

Committed: https://crrev.com/dba4dbf970d73e637514fb1b3a7b464f632d2222
Cr-Commit-Position: refs/heads/master@{#404128}
==========

[commit-bot: I haz the power, 2016-07-07 11:22:11]

Patchset 2 (id:??) landed as
https://crrev.com/dba4dbf970d73e637514fb1b3a7b464f632d2222
Cr-Commit-Position: refs/heads/master@{#404128}