Implement an efficient Android CookieJar (sqldelight?)

[swankjesse]

The JavaNetCookieJar is a hazard to use, particularly because it uses HttpCookie behind the scenes. That class unnecessarily quotes cookie values, and we need to do gross things to strip them off. This behavior is particularly bad because sometimes it doesn’t strip the quotes and we end up stripping off quotes that we shouldn’t.

[leeight]

Is it possible add QuotePreservingCookieJar.java as a replacement for JavaNetCookieJar.java
https://gist.github.com/swankjesse/e283f2ddf264abd9a0d45bc80e7ec2d8

[swankjesse]

Yeah, just copy it into your codebase.

[leeight]

Could you release a new version okhttp3? React-Native need QuotePreservingCookieJar.java to address Issue 10121.

[swankjesse]

We will not be adding QuotePreservingCookieJar.java to OkHttp. You’ll have to copy it into your own project.

[dave-r12]

Still up for this one? Mind if I take a crack at it?

I've started reviewing https://tools.ietf.org/html/rfc6265 and the internal Java (CookieManager/InMemoryCookieStore) implementations. I assume this will be replacing those things. It seems okhttp3.Cookie is already doing a lot of the heavy lifting, so thinking this shouldn't be a huge undertaking.

[swankjesse]

Yeah for sure. The most difficult part is deciding how the persistence model works. I’d love to have Sqlite but sadly we don’t. What do you think?

[dave-r12]

Ahh, I see. So this should support automatic disk persistence, in addition to in memory.

RE: SQLite - I'm still getting up to speed so I'm not entirely sure the role there. My hunch is that would provide faster access when reading cookies from disk on a fresh load. I wonder if a simple index would work there. I don't know enough yet on the access patterns, so maybe it wouldn't be enough.

[brillenheini]

I, from my perspective as a user, would be very happy with an in-memory implementation as a first addition.

[swankjesse]

There’s a handful of interesting problems we’d be able to solve with a java.util.Properties style interface:

• cookies
• HPKP
• remembering which hosts support HTTP/2?
• remembering proxy precedence?

Maybe that’s sufficient to start?

[dave-r12]

Alright, I'll attempt to draft something using the CookieJar as a first client and see where that takes me.

[dave-r12]

I did some experimenting here and realized, would it make sense to back this with DiskLruCache? It seems any key-value like storage would behave in a similar way:

• Store arbitrary data with a given key.
• Set an upper bound on the size of the store.
• Have a policy for evicting entries when size exceeds upper bound.
• Handle faulty filesystems.

All of these are supported by DiskLruCache.

[swankjesse]

You’re blowing my mind a bit, I never thought of that as an option! Yeah, for sure.

Keys would be domain names?

[franmontiel]

In case it can be of any help you can check my implementation of a PersistentCookieJar.

Some problems that I see with it for the moment:

• It uses SharedPreferences to store the cookies so is not platform independent.
• It loads all cookies in memory, maybe not problematic with the majority of apps but I suppose it won't scale well with cookie intensive usage (a browser for example).

Both problems can be solved with your proposals. For example using the DiskLruCache and storing all cookies of the same domain(ignoring subdomains) into .properties files and deserializing the .properties into a collection in memory after the first call to loadForRequestUrl() for any URL of the domain.

[dave-r12]

@swankjesse yes, that's my thought right now.

Thanks @franmontiel, I'll have a look.

[hey99xx]

@swankjesse I'm a little confused why QuotePreservingCookieJar does not become the standard implementation if JavaNetCookieJar is a hazard for cookies with quotes.

I have a use case for explicitly using cookie values surrounded by quotes such as x-mycookie="3NO432" but I just realized the quotes are manually stripped off.

Is there any plan for future OkHttp3 releases stopping the deletion of quotes in JavaNetCookieJar?

I'm syncing cookies between android.webkit.CookieManager and OkHttp client. In Chrome dev console network tab, the cookies are shown correctly with quotes, but with OkHttp they're incorrect.

---

Just realized 3.10 milestone on the sidebar. Is that when the new default cookie jar implementation aimed to be released in? Is the recommendation just replacing JavaNetCookieJar with QuotePreservingCookieJar until then?

[swankjesse]

We'll probably punt this beyond 3.10. Getting this right is challenging because OkHttp can't use Android-only APIs. I recommend the persistent cookie jar code linked above if you're targeting Android and not the JVM.

[cogman]

With the release of 4.x, where does this issue stand?

[arcao]

I use this implementation to store cookies in memory (Inspired by PersistentCookieJar).

class MemoryCookieJar : CookieJar {
    private val cache = mutableSetOf<WrappedCookie>()

    @Synchronized
    override fun loadForRequest(url: HttpUrl): List<Cookie> {
        val cookiesToRemove = mutableSetOf<WrappedCookie>()
        val validCookies = mutableSetOf<WrappedCookie>()

        cache.forEach { cookie ->
            if (cookie.isExpired()) {
                cookiesToRemove.add(cookie)
            } else if (cookie.matches(url)) {
                validCookies.add(cookie)
            }
        }

        cache.removeAll(cookiesToRemove)

        return validCookies.toList().map(WrappedCookie::unwrap)
    }

    @Synchronized
    override fun saveFromResponse(url: HttpUrl, cookies: List<Cookie>) {
        val cookiesToAdd = cookies.map { WrappedCookie.wrap(it) }

        cache.removeAll(cookiesToAdd)
        cache.addAll(cookiesToAdd)
    }

    @Synchronized
    fun clear() {
        cache.clear()
    }
}

class WrappedCookie private constructor(val cookie: Cookie) {
    fun unwrap() = cookie

    fun isExpired() = cookie.expiresAt < System.currentTimeMillis()

    fun matches(url: HttpUrl) = cookie.matches(url)

    override fun equals(other: Any?): Boolean {
        if (other !is WrappedCookie) return false

        return other.cookie.name == cookie.name &&
            other.cookie.domain == cookie.domain &&
            other.cookie.path == cookie.path &&
            other.cookie.secure == cookie.secure &&
            other.cookie.hostOnly == cookie.hostOnly
    }

    override fun hashCode(): Int {
        var hash = 17
        hash = 31 * hash + cookie.name.hashCode()
        hash = 31 * hash + cookie.domain.hashCode()
        hash = 31 * hash + cookie.path.hashCode()
        hash = 31 * hash + if (cookie.secure) 0 else 1
        hash = 31 * hash + if (cookie.hostOnly) 0 else 1
        return hash
    }

    companion object {
        fun wrap(cookie: Cookie) = WrappedCookie(cookie)
    }
}

Usage:

val okHttp = OkHttpClient.Builder()
    .cookieJar(MemoryCookieJar())
    .build()

[headsvk]

I would also like to see OkHttp's own CookieJar implementation. I found another issue with JavaNetCookieJar on Android caused by an Android-only change to java's CookieManager class which forced me to create my own simple CookieJar. https://issuetracker.google.com/issues/174647435

[swankjesse]

@headsvk yikes. Looks like they inexplicably introduced a regression.

[yschimke]

Arguably this paragraph in the spec says that a Set-Cookie with non matching path can be rejected

Cookies do not always provide isolation by path. Although the network-level protocol does not send cookies stored for one path to another, some user agents expose cookies via non-HTTP APIs, such as HTML's document.cookie API. Because some of these user agents (e.g., web browsers) do not isolate resources received from different paths, a resource retrieved from one path might be able to access cookies stored for another path.

But this is a stretch, and it isn't clearly spelled out in MUST, SHOULD terms. Just implied here.

[yschimke]

We'll probably punt this beyond 3.10. Getting this right is challenging because OkHttp can't use Android-only APIs. I recommend the persistent cookie jar code linked above if you're targeting Android and not the JVM.

Any opinion as of 4.9.0 of whether you'd be happy with an okhttp-android module using Android APIs here. I think we have test infra now (locally, not CI yet) to build against this correctly.

[swankjesse]

The feature we want is sqlite storage, or perhaps even MySQL storage. Doing a module that uses SQDelight to store cookies to a DB would be a nice win. Could also do similarly for HSTS, for example.

[yschimke]

@swankjesse is this worthwhile for 5.0? We can now use android APIs.

[swankjesse]

Not for 5.0. Good for the backlog.