Cache uses just request url as a key

[Mugenor]

The current cache implementation uses request url as a key even when Cache-Control policy is private.
This might lead to leaking data from one user to another, creating security holes in an application.

• HTTP spec
• The bug place

I would expect it to cache based on request url + authorisation header in case the cache control policy is private, e.g. a hashed combination of these values

[Mugenor]

Here is a code example:

public final class OkHttpCacheProbeMain {

    private static final String DEFAULT_BASE_URL = "https://api.github.com";
    private static final String DEFAULT_PATH = "/repos/<org>/<repo>"; // should be private
    private static final long CACHE_SIZE_BYTES = 100L * 1024L * 1024L;

    private OkHttpCacheProbeMain() {
    }

    public static void main(String[] args) throws Exception {
        Map<String, String> env = System.getenv();

        // has access to the private repo
        String tokenA = env.getOrDefault("GITHUB_TOKEN_A", "YOUR_TOKEN");
        // doesn't have access to the private remo
        String tokenB = env.getOrDefault("GITHUB_TOKEN_B", "YOUR_TOKEN");
        String baseUrl = env.getOrDefault("GITHUB_API_URL", DEFAULT_BASE_URL);
        String path = env.getOrDefault("GITHUB_CACHE_PROBE_PATH", DEFAULT_PATH);
        boolean clearCache = Boolean.parseBoolean(env.getOrDefault("GITHUB_CACHE_PROBE_CLEAR", "true"));

        File cacheDir = new File(System.getProperty("java.io.tmpdir"), "okhttp-cache-probe");
        if (clearCache) {
            clearDirectory(cacheDir.toPath());
        }
        Files.createDirectories(cacheDir.toPath());

        Dispatcher dispatcher = new Dispatcher();
        dispatcher.setMaxRequests(32);
        dispatcher.setMaxRequestsPerHost(32);

        Cache cache = new Cache(cacheDir, CACHE_SIZE_BYTES);
        OkHttpClient client = new OkHttpClient.Builder()
                .dispatcher(dispatcher)
                .connectionPool(new ConnectionPool(8, 5, TimeUnit.MINUTES))
                .cache(cache)
                .build();

        String url = joinUrl(baseUrl, path);

        System.out.println("OkHttp cache probe");
        System.out.println("URL: " + url);
        System.out.println("Cache dir: " + cacheDir.getAbsolutePath());
        System.out.println("Cache cleared at startup: " + clearCache);
        System.out.println();

        runRequest(client, url, tokenA, "tokenA-first");
        runRequest(client, url, tokenA, "tokenA-second");
        runRequest(client, url, tokenB, "tokenB-first");
        runRequest(client, url, tokenB, "tokenB-second");

        try {
            System.out.println("\n===\nSleeping for 70s...\n===\n");
            Thread.sleep(Duration.ofSeconds(70));
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
        }

        runRequest(client, url, tokenA, "tokenA-third");
        runRequest(client, url, tokenA, "tokenA-forth");
        runRequest(client, url, tokenB, "tokenB-third");
        runRequest(client, url, tokenB, "tokenB-forth");


        System.out.println();
        System.out.println("Cache stats: requestCount=" + cache.requestCount()
                + ", networkCount=" + cache.networkCount()
                + ", hitCount=" + cache.hitCount());

        cache.close();
    }

    private static void runRequest(OkHttpClient client, String url, String token, String label) throws IOException {
        Request request = new Request.Builder()
                .url(url)
                .header("Authorization", "Bearer " + token)
                .header("Accept", "application/vnd.github.v3+json")
                .build();

        long startNs = System.nanoTime();
        try (Response response = client.newCall(request).execute()) {
            long durationMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNs);
            String source = describeSource(response);
            String cacheControl = response.header("Cache-Control", "<none>");
            String vary = response.header("Vary", "<none>");
            String etag = response.header("ETag", "<none>");

            System.out.println("=== " + label + " ===");
            System.out.println("status=" + response.code()
                    + ", source=" + source
                    + ", durationMs=" + durationMs);
            System.out.println("requestAuthHash=" + Integer.toHexString(request.header("Authorization").hashCode()));
            System.out.println("cacheControl=" + cacheControl);
            System.out.println("vary=" + vary);
            System.out.println("etag=" + etag);
            if (response.networkResponse() != null) {
                System.out.println("networkStatus=" + response.networkResponse().code());
            }
            if (response.cacheResponse() != null) {
                System.out.println("cacheStatus=" + response.cacheResponse().code());
            }
            System.out.println();
        }
    }

    private static String describeSource(Response response) {
        if (response.networkResponse() != null && response.cacheResponse() != null) {
            return "conditional-cache";
        }
        if (response.cacheResponse() != null) {
            return "cache";
        }
        if (response.networkResponse() != null) {
            return "network";
        }
        return "unknown";
    }

    private static String joinUrl(String baseUrl, String path) {
        if (path.startsWith("http://") || path.startsWith("https://")) {
            return path;
        }
        String normalizedBase = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl;
        String normalizedPath = path.startsWith("/") ? path : "/" + path;
        return normalizedBase + normalizedPath;
    }

    private static void clearDirectory(Path directory) throws IOException {
        if (!Files.exists(directory)) {
            return;
        }
        try (var paths = Files.walk(directory)) {
            paths.sorted(Comparator.reverseOrder())
                    .forEach(path -> deletePath(path));
        }
    }

    private static void deletePath(Path path) {
        try {
            Files.deleteIfExists(path);
        } catch (IOException e) {
            throw new RuntimeException("Failed to delete cache path: " + path, e);
        }
    }
}

So it effectively results in:

1. first call with tokenA: 200 via network
2. second call with tokenA: 200 from cache
3. first call with tokenB: 404 via network
4. second call wtih tokenB: 404 via network
5. third call with tokenA: 404 from cache <- this is a problem

[swankjesse]

Could you tell me a bit more about how you’re using OkHttp? Presumably it isn’t a single-user application in your case?

There’s a few other places in the library where we make a single-user expectation, including the CookieStore.

A simple-enough work around is to create an independent Cache for each user. The drawback of this is that you’ll get a lower hit-rate for non-private HTTP responses.