What's the actual web (extension?) API being used here?  document.cookie doesn't let you specify the URL.

Cookies can be set on valid domains.  The leading dot when setting a cookie is magic that means set it on the domain without a cookie, and all subdomains of that domain...  But it's not actually a domain you can set a cookie on.

If we want magic to make this valid, I'd argue it should not be at the cookie store layer, as it's unclear what this actually means, so it should be up to the consumer to make a reasonable query.

Unrestricting as there doesn't appear to be anything secret.

There's no web API being used here; this is coming from WebView's Java API exposed to the embedding app: https://developer.android.com/reference/android/webkit/CookieManager.html

The call is getting a cookie - there isn't a meaningful behaviour with a leading dot for that, right? Though, even for setting one with this API the expectation is that you pass the actual URL here, and specify the specific domain you want to set the cookie for with the usual Set-Cookie parameters in the actual value string.

For gross legacy reasons the implementation of CookieManager passes all URLs used in set/get operations through "fixupUrl" which parses the provided string with android.net.WebAddress and then converts it back to string - I would have expected this to remove the leading dot, but I guess it doesn't (not sure what normalisation this actually does internally; we kept it for compatibility with the old webview).

The documentation for how to actually use CookieManager to do anything nontrivial is really lacking. We should probably fix that and provide actual examples of usage (e.g. how to actually set a wildcard domain cookie).

What is the actual result here, when DCHECKs aren't enabled? Does it just return the cookies for facebook.com? Or no cookies at all? Or something else? We could probably normalise this on the input side but exactly how we'd want to do that depends on the current behaviour that people might unfortunately be relying on..

Nate, do you have bandwidth to look into this one a bit and see if we can fix this up and improve the docs? Or mmenke/rdsmith, do you have opinions, or want to know more about what's going on here from us?

"Set-Cookie: foo=bar; domain=.a.com" means that for both a.com and a.com subdomains (Like c.b.a.com) can all see the cookie.  The cookie isn't actually set on .a.com.

For a request to a.com with "Set-Cookie: foo=bar", the cookie will only be visible for requests to a.com itself.  (domain=a.com is technically incorrect syntax, but will be corrected to domain=.a.com).

It's not clear what getting a cookie for http://.a.com/ means.  Does it include a.com cookies?  Only .a.com cookies?  There's no spec, because it's not a valid domain.  There's also no web API to see only requests set on a.com and its subdomains only  (i.e. excluding cookies set without a domain property, which are only visible to a.com itself).

Oh, and I have no idea what the result is - the result depends on the caller.  GetCookieDomainWithString just figures out what domain to get cookies from, it doesn't actually do any real work itself.

Yeah, that's what I figured. I suspect that the lacking documentation on CookieManager means that some people who want to set a cookie for subdomains might try doing it by calling one of:

1) CookieManager.setCookie("http://.a.com", "foo=bar");
2) CookieManager.setCookie("http://.a.com", "foo=bar; domain=.a.com");

depending how they guess. Both of these are wrong; it should be:

3) CookieManager.setCookie("http://a.com", "foo=bar; domain=.a.com");

(i.e. you should pass a real URL of the hypothetical page that would be returning this Set-Cookie header)

I have no idea what the current behaviour of the wrong versions is. It wouldn't surprise me if at least 2) actually works and does the right thing right now through some combination of tolerant parsing at different levels. If setting them in one or more dubious ways appears to largely work then I also wouldn't be surprised to see the exact same URL strings being used to query cookies, even though this isn't really meaningful as you say.

The "ideal" thing of just parsing this more strictly at the android.webkit.CookieManager API level and throwing an exception for malformed URLs would be nice but I suspect will break existing apps.. I think we should experiment to figure out what bad cases people appear to be using and what the current behaviour of those cases is, and decide what to do to rationalise this based on that.

It looks like through the set-cookie path, 1) would actually work without the DCHECK, at least with the current code, and treat is the same as if it had a "domain=" parameter.  Less sure about the get-cookie path, but wouldn't be surprised if it only returned cookies with a "domain=" parameter.

I'll take a look at the real behavior sometime tomorrow and see if it's reasonable to support or if we should just drop the API calls. I can also send a CL to fix up CookieManager documentation.

I haven't had any further time to investigate this, but I saw this error pop up again in  issue 788137 . The URL they're passing is ".quora.com".

Just a quick nit on cookie line format, which probably isn't relevant for this bug: There is no difference at the HTTP level between:

    Set-Cookie: a=b; domain=example.com

and 

    Set-Cookie: a=b; domain=.example.com

("Set-Cookie" is the response header sent by the server when it want to set a cookie; the URL for the cookie is the URL the response is being sent for.  I presume the CookieManager interface is based on this same concept, but with an explicit URL.)

The spec mandates treating them as the same, specifically as "domain cookies" that apply to example.com URLs and all subdomains.  "Host cookies" which apply only to the precise domain of a URL are specified by leaving out the "domain" attribute. 

I.e. the effect of a leading "." in the "domain" attribute (none) is different than that of a leading "." in the URL domain (not allowed) is different than that of a leading "." in the domain stored for the cookie (magically signaling a domain rather than a host cookie).  I hope this is more clarifying than confusing :-}.

Yeah. The URL being passed is the explicit URL to replace the normally-implicit URL of the response, so it's incorrect to ever include a leading dot, but I think it's an easy mistake for app developers to make when they are intending to set it for a subdomain as I guessed in comment 5.

I investigated this a bit more, some of the theories above turned out to be right:

 - Calling `CookieManager.setCookie(url = ".httpbin.org", value="key=value")` actually sets a domain cookie for ".httpbin.org" (if DCHECKs are disabled)
 - Calling `CookieManager.getCookie(url = ".httpbin.org")` seems to be a special case that's already supported [1]. This API does not trigger any DCHECKs.

It sounds like the best fix is to modify WebView's fixup() code to include these rules:

 - ".httpbin.org" & "key=value" -> "http://httpbin.org" & "key=value; Domain=.httpbin.org"
 - "http://.httpbin.org" & "key=value" -> "http://httpbin.org" & "key=value; Domain=.httpbin.org"

If there's already a Domain attribute, our fixup code can just fix the URL instead (I don't think this is a common case).

[1] https://cs.chromium.org/chromium/src/net/cookies/canonical_cookie.cc?sq=package:chromium&l=353

Ugggh gross. I'm somewhat disappointed that the wrong syntax actually works :/

So, yeah, I guess we should just fix it up on the client side. If there's already a Domain and it's different, what happens now? I guess whichever one wins we should preserve.

In any case I think we should document how to use this API better, and *not* call out these special weird cases in the docs - just talk about how to use them with proper RFC-compliant Set-Cookie header values.

> In any case I think we should document how to use this API better

Agreed. I think we should document the url parameter a bit more carefully (maybe say it's as if an HTTP response for that URL is setting the header, and specify it *cannot* be in domain syntax).

I also think the CookieManager class might benefit from some examples (this also gives us an example to show setCookie with a domain parameter).

> If there's already a Domain and it's different, what happens now? I guess whichever one wins we should preserve.

I'm pretty sure the explicit Domain attribute wins--looking at the code, I believe we would skip the host-cookie case [1] and use the explicit cookie_domain.

[1] https://cs.chromium.org/chromium/src/net/cookies/cookie_util.cc?type=cs&sq=package:chromium&l=108

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cbfe5fc8ca5c1bb74794140a028d86083671ab96

commit cbfe5fc8ca5c1bb74794140a028d86083671ab96
Author: Nate Fischer <ntfschr@chromium.org>
Date: Mon Jan 08 23:41:19 2018

AW: cleanup malformed setCookie calls

No intended change to observable behavior.

Some applications invoke CookieManager#setCookie() with malformed URLs
(e.g. ".some.domain" and "http://.some.domain"). This hasn't been
explicitly supported, but has worked by accident (although it hits a
DCHECK).

This CL explicitly handles these invalid URLs by creating a valid URL
and adding a Domain attribute for the intended domain, matching current
behavior while no only passing well-formed URLs to the rest of chromium.

In the case of a malformed URL and an explicit Domain attribute, we
prefer the Domain attribute (also following current behavior).

This adds tests to make sure we don't regress and hit the DCHECK again.

Bug:  779887 
Test: run_webview_instrumentation_test_apk -f CookieManagerTest#testSetCookieWithDomainForUrl*
Test: manual - quora app (see bug for steps)
Change-Id: Ia1bdd6c40be84d8f5816e5879a31e9e47ec21e93
Reviewed-on: https://chromium-review.googlesource.com/846263
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527813}
[modify] https://crrev.com/cbfe5fc8ca5c1bb74794140a028d86083671ab96/android_webview/java/src/org/chromium/android_webview/AwCookieManager.java
[modify] https://crrev.com/cbfe5fc8ca5c1bb74794140a028d86083671ab96/android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java

Bulk edit: marking stale 'fixed' bugs as 'verified' since they don't need verification at this point.