IssueTracker

Malloc debug has been rewritten in the latest aosp release so this doesn't apply any more.

Hang on, this Stack Overflow says our interpretation might be wrong:
http://stackoverflow.com/questions/1612177/are-http-cookies-port-specific

Regardless of whether cookies are port specific: the implementation just doesn't work.

If you put a cookie for http://localhost:4123 in, you should get the same cookie out when querying http://localhost:4123. Instead, you get null with the current implementation.

Agree this looks wrong, either:

1) The CookieStoreImpl was implemented to ignore the port (as in add()), in which case get() should also ignore the port.
2) The CookieStoreImpl was implemented to honor the port, in which case add() should not ignore the port.

The RFC suggests the port should be ignored, but it's possible that CookieStoreImpl was trying to implement some kind of same-origin-policy.

Looking more deeply at this, I think there's an implicit assumption in the current implementation that the domain is (and other properties like Path are) set on the HttpCookie before it is passed to CookieStore. This has previously led to CookieManager finding these cookies even when the port is not being handled correctly: although the initial URI match fails, the follow-up "domain" match it performs does find it iff the domain is set.

The reason the sample code above isn't working is because it is using CookieStoreImpl directly, which brings the behavior of each component more clearly into focus for developers.

CookieManager on Android sets the cookie domain properties automatically from the URI's host if it hasn't been set. RFC 6265 section 4.1.2.3 suggests this is incorrect, although Android is documented to follow RFC 2965 not RFC 6265. RFC 2965 states:

   Domain  Defaults to the effective request-host.  (Note that because
           there is no dot at the beginning of effective request-host,
           the default Domain can only domain-match itself.)

So I think Android is RFC 2965 compliant in that regard.

Using CookieManager directly; this code works as I would expect:

    public void testCookieManagerGet_portChecks() throws Exception {
        CookieManager cookieManager = new CookieManager();

        cookieManager.put(new URI("http://a.com:443/"), cookieHeaders("a1=android"));
        cookieManager.put(new URI("http://a.com:8080/"), cookieHeaders("a2=android"));
        cookieManager.put(new URI("http://a.com:8080/"), cookieHeaders("a3=android; Port=8080"));

        assertManagerCookiesMatch(cookieManager, "http://a.com/", "a1=android; a2=android");
        assertManagerCookiesMatch(cookieManager, "http://a.com:8080/",
                "a1=android; a2=android; a3=android");
    }

    private static Map<String, List<String>> cookieHeaders(String... headers) {
        return Collections.singletonMap("Set-Cookie", Arrays.asList(headers));
    }


    private static void assertManagerCookiesMatch(CookieManager cookieManager, String url,
        String expectedCookieRequestHeader) throws Exception {

        Map<String, List<String>> cookieHeaders =
                cookieManager.get(new URI(url), EMPTY_COOKIES_MAP);
        if (expectedCookieRequestHeader == null) {
            assertTrue(cookieHeaders.isEmpty());
            return;
        }

        assertEquals(1, cookieHeaders.size());
        List<String> actualCookieHeaderStrings = cookieHeaders.get("Cookie");

        // For simplicity, we concatenate the cookie header strings if there are multiple ones.
        String actualCookieRequestHeader = actualCookieHeaderStrings.get(0);
        for (int i = 1; i < actualCookieHeaderStrings.size(); i++) {
            actualCookieRequestHeader += "; " + actualCookieHeaderStrings.get(i);
        }
        assertEquals(expectedCookieRequestHeader, actualCookieRequestHeader);
    }


I agree there is an issue here, and I it looks like the fix is as suggested above. I don't *think* it's an issue for developers using CookieHandler/CookieManager, only those using CookieStoreImpl directly. Please correct me if you disagree.

Generally: CookieStore is poorly defined IMO: the responsibilities of this class vs CookieHandler are very unclear.

Given what I now understand about this class, I think developers should avoid *using* CookieStore(Impl) directly: later implementations may spread responsibilities around differently, leading to app-compatibility issues. If you are implementing your own CookieStore then I wish you luck: it may be possible to produce a very conservative implementation that covers all possible eventualities but I'm not sure.

For example, handling of the correct location for handling the "secure" attribute are not specified at all: you may find different implementations make different assumptions. Right now Android ignores "secure" and enforces the check in CookieManager. CookieStoreImpl could also enforce this check on Android (i.e. in addition to CookieManager) but currently does not, and CookieManager assumes it has to.

CookieManager/CookieHandler is probably better specified because it acts as a facade over the whole mess and the contract is effectively "do the correct thing with cookies". If you are implementing CookieStore, I would recommend implementing the whole CookieHandler instead if you want to control cookie handling on Android. I appreciate that's a lot of work but I can't think of another way that would insulate you from changes across Android versions in future.

This also breaks for https cookies (version 23 emulator, verified in the source). When setting a cookie it will happily convert the storage uri to http without port, but when looking up cookies, it will not do so. As such it is unable to find cookies for non-http locations such as https or custom ports.

Thank you for your feedback.  We assure you that we are doing our best to address all issues reported. For now, we will be closing the issue as won't fix obsolete. If this issue currently still exists, we request that you log a new issue along with the bug report here https://goo.gl/TbMiIO and reference this bug for context.

Reopening issues that had been marked with hotlistid:4240 to prevent bulk closure.