Apply PSL to "first-party for cookies" ancestor restrictions.

Apply PSL to "first-party for cookies" ancestor restrictions.

Both Chrome and Firefox currently allow 'talkgadget.google.com' to
receive cookies when embedded within 'plus.google.com'. This patch pokes
at 'Document::firstPartyForCookies' to relax the ancestor restriction
from an exact origin match to an eTLD match in order to bring our behavior
into line with this particular portion of the status quo.

BUG=477578
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195457

[Mike West, 2015-05-12 07:51:27]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, rsleevi@chromium.org

[Mike West, 2015-05-12 07:51:28]

WDYT, Jochen?

Ryan: this feels hacky. I'm doing it because 'StaticCookiePolicy::CanGetCookies'
already uses the PSL list for "first-party" matching, but it seems like there
ought to be something simpler we could do here. WDYT?

[Ryan Sleevi, 2015-05-12 23:15:23]

Not LGTM.

m_hostIsIPAddress in OriginAccessEntry is bugged, especially with respect to
IPv6 addresses, at least from reading the source.

That is, using RFC 2732 as the test cases, there are numerous ways (with or
without) brackets for m_HostIsIPAddress to match

I agree with the intent (although where it's spec'd, I don't see -_-), but am
concerned w/ the implementation.

[Mike West, 2015-05-13 05:06:14]

On 2015/05/12 at 23:15:23, rsleevi wrote:
> m_hostIsIPAddress in OriginAccessEntry is bugged, especially with respect to
IPv6 addresses, at least from reading the source.
> 
> That is, using RFC 2732 as the test cases, there are numerous ways (with or
without) brackets for m_HostIsIPAddress to match

Hrm. In that case, what do you suggest? Do we need to fix OriginAccessEntry (in
which case it would be wonderful if you filed bugs!), or do we need to expose
some other interface? I don't actually think we should do anything at all with
IP addresses; they shouldn't match anything other than themselves; would you be
happy with this code if we explicitly excluded IP addresses from the
OriginAccessEntry check?

(Note that this is pretty much the only PSL-like interface we have in Blink, and
it's already being used in exactly this way for `document.domain` checks.)

> I agree with the intent (although where it's spec'd, I don't see -_-), but am
concerned w/ the implementation.

Working on the spec today; basically, I wanted to know if you hated the idea
before I tried to figure out how to get PSL-like language into the ID.

[Ryan Sleevi, 2015-05-13 06:30:57]

On 2015/05/13 05:06:14, Mike West wrote:
> On 2015/05/12 at 23:15:23, rsleevi wrote:
> > m_hostIsIPAddress in OriginAccessEntry is bugged, especially with respect to
> IPv6 addresses, at least from reading the source.
> > 
> > That is, using RFC 2732 as the test cases, there are numerous ways (with or
> without) brackets for m_HostIsIPAddress to match
> 
> Hrm. In that case, what do you suggest? Do we need to fix OriginAccessEntry
(in
> which case it would be wonderful if you filed bugs!),

Well, I don't know the full implications, and wanted to make sure I was reading
the code.

It looks to me there are bugs, and if we don't fix them, we will trade one
disconnect with Firefox for another, potentially worse one.

Can you add unit tests for IPv6? If Blink could use GURL, then this would be so
easy to do safely. If you're guaranteed a canonicalized URL (no clue if you
are), then you can do tests like the bracket-or-colon test. But I don't know the
best way here.

>  or do we need to expose
> some other interface? I don't actually think we should do anything at all with
> IP addresses; they shouldn't match anything other than themselves; would you
be
> happy with this code if we explicitly excluded IP addresses from the
> OriginAccessEntry check?

Yeah, I threw the flag down because it seems broken for IPs. If this is used in
other security checks, you'll probably have a bad time there too ;)

> 
> (Note that this is pretty much the only PSL-like interface we have in Blink,
and
> it's already being used in exactly this way for `document.domain` checks.)
> 
> > I agree with the intent (although where it's spec'd, I don't see -_-), but
am
> concerned w/ the implementation.
> 
> Working on the spec today; basically, I wanted to know if you hated the idea
> before I tried to figure out how to get PSL-like language into the ID.

In general, I hate all new uses of the PSL. I don't see this as belong in a
rational platform, but because cookies/document.domain make it irrational, then
that's what we live with. This doesn't seem like an ideal long-term solution,
but likely reflects the reality of the Web today. Sadly.

[Mike West, 2015-05-15 14:47:38]

Given https://codereview.chromium.org/1128913004, are you less unhappy about
this CL, Ryan?

[Ryan Sleevi, 2015-05-15 21:33:42]

LGTM % I have no idea what I'm doing here ;)

https://codereview.chromium.org/1133223002/diff/1/Source/web/tests/WebDocumen...
File Source/web/tests/WebDocumentTest.cpp (right):

https://codereview.chromium.org/1133223002/diff/1/Source/web/tests/WebDocumen...
Source/web/tests/WebDocumentTest.cpp:211: const char* baseURLOriginSubA =
"http://subdomain.example.test:0/";
An invalid origin, FWIW ;)

(:0 = oh noes)

[Mike West, 2015-05-18 08:02:01]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-05-18 08:02:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133223002/1

[commit-bot: I haz the power, 2015-05-18 09:21:06]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195457