From 14545aac26de7178cf37ddb9aa1fe57396834a34 Mon Sep 17 00:00:00 2001 From: Max Weber Date: Sat, 29 May 2021 15:01:58 -0600 Subject: [PATCH] rl-client: never cache 4/5xx requests --- .../java/net/runelite/client/RuneLite.java | 24 +++- .../client/OkHttpCacheSanityTest.java | 125 ++++++++++++++++++ 2 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 runelite-client/src/test/java/net/runelite/client/OkHttpCacheSanityTest.java diff --git a/runelite-client/src/main/java/net/runelite/client/RuneLite.java b/runelite-client/src/main/java/net/runelite/client/RuneLite.java index 49ea8383a6..42b57e6366 100644 --- a/runelite-client/src/main/java/net/runelite/client/RuneLite.java +++ b/runelite-client/src/main/java/net/runelite/client/RuneLite.java @@ -73,6 +73,7 @@ import net.runelite.client.ui.overlay.worldmap.WorldMapOverlay; import net.runelite.http.api.RuneLiteAPI; import okhttp3.Cache; import okhttp3.OkHttpClient; +import okhttp3.Response; import org.slf4j.LoggerFactory; @Singleton @@ -191,8 +192,8 @@ public class RuneLite } }); - OkHttpClient.Builder okHttpClientBuilder = RuneLiteAPI.CLIENT.newBuilder() - .cache(new Cache(new File(CACHE_DIR, "okhttp"), MAX_OKHTTP_CACHE_SIZE)); + OkHttpClient.Builder okHttpClientBuilder = RuneLiteAPI.CLIENT.newBuilder(); + setupCache(okHttpClientBuilder, new File(CACHE_DIR, "okhttp")); final boolean insecureSkipTlsVerification = options.has("insecure-skip-tls-verification"); if (insecureSkipTlsVerification || RuneLiteProperties.isInsecureSkipTlsVerification()) @@ -381,6 +382,25 @@ public class RuneLite } } + @VisibleForTesting + static void setupCache(OkHttpClient.Builder builder, File cacheDir) + { + builder.cache(new Cache(cacheDir, MAX_OKHTTP_CACHE_SIZE)) + .addNetworkInterceptor(chain -> + { + // This has to be a network interceptor so it gets hit before the cache tries to store stuff + Response res = chain.proceed(chain.request()); + if (res.code() >= 400 && "GET".equals(res.request().method())) + { + // if the request 404'd we don't want to cache it because its probably temporary + res = res.newBuilder() + .header("Cache-Control", "no-store") + .build(); + } + return res; + }); + } + private static void setupInsecureTrustManager(OkHttpClient.Builder okHttpClientBuilder) { try diff --git a/runelite-client/src/test/java/net/runelite/client/OkHttpCacheSanityTest.java b/runelite-client/src/test/java/net/runelite/client/OkHttpCacheSanityTest.java new file mode 100644 index 0000000000..2478042ed2 --- /dev/null +++ b/runelite-client/src/test/java/net/runelite/client/OkHttpCacheSanityTest.java @@ -0,0 +1,125 @@ +/* + * Copyright (c) 2020 Abex + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package net.runelite.client; + +import java.io.IOException; +import java.time.Instant; +import java.time.ZoneId; +import java.time.format.DateTimeFormatter; +import java.util.Locale; +import java.util.concurrent.TimeUnit; +import net.runelite.http.api.RuneLiteAPI; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.Response; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +public class OkHttpCacheSanityTest +{ + @Rule + public TemporaryFolder cacheFolder = new TemporaryFolder(); + + @Rule + public MockWebServer server = new MockWebServer(); + + private static final DateTimeFormatter TIME_FMT = DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss z", Locale.US) + .withZone(ZoneId.of("GMT")); + + private static final String BODY_404 = "404 not found"; + private static final String BODY_200 = "{success:true}"; + + /** + * The specific thing we are trying to catch here is when a 404 + * response is served by cloudflare after the upstream has gotten + * an updated document. CF serves the 404 response with a Date header, + * but no ETag or Last-Modified. OkHttp then uses the Date header, which + * is from after the upstream was edited, in a If-Modified-Since request, + * which does hit the upstream, returning 304 Not Modified. Since there + * is no body OkHttp serves the cached 404 as up-to-date. Better yet, since + * there is now an ETag in the 304 response it requests subsequent updates + * with the ETag, which will always 304 Not Modified, causing clients in + * this state to get incorrect 404s until the ETag changes. + */ + @Test + public void testCacheSanity() throws IOException, InterruptedException + { + OkHttpClient.Builder builder = RuneLiteAPI.CLIENT.newBuilder(); + RuneLite.setupCache(builder, cacheFolder.getRoot()); + OkHttpClient client = builder.build(); + + Instant lastModified = Instant.now().minusSeconds(20); + + server.enqueue(new MockResponse() + .setResponseCode(404) + .setHeader("Content-Type", "text/html") + .setHeader("Date", TIME_FMT.format(Instant.now().minusSeconds(10))) + .setBody(BODY_404)); + try (Response res = client.newCall(new Request.Builder() + .url(server.url("/manifest")) + .build()).execute()) + { + Assert.assertEquals(404, res.code()); + } + RecordedRequest req = server.takeRequest(1, TimeUnit.SECONDS); + Assert.assertNotNull("cache did not make a initial request", req); + + server.enqueue(new MockResponse() + .setResponseCode(200) + .setHeader("Content-Type", "text/html") + .setHeader("Date", TIME_FMT.format(Instant.now().minusSeconds(5))) + .setHeader("Last-Modified", TIME_FMT.format(lastModified)) + .setBody(BODY_200)); + try (Response res = client.newCall(new Request.Builder() + .url(server.url("/manifest")) + .build()).execute()) + { + Assert.assertEquals(200, res.code()); + } + req = server.takeRequest(1, TimeUnit.SECONDS); + Assert.assertNotNull("cache did not make a request", req); + Assert.assertNull(req.getHeader("If-Modified-Since")); + + server.enqueue(new MockResponse() + .setResponseCode(304) + .setHeader("Content-Type", "text/html") + .setHeader("Date", TIME_FMT.format(Instant.now())) + .setHeader("Last-Modified", TIME_FMT.format(lastModified))); + try (Response res = client.newCall(new Request.Builder() + .url(server.url("/manifest")) + .build()).execute()) + { + Assert.assertEquals(200, res.code()); + } + req = server.takeRequest(1, TimeUnit.SECONDS); + Assert.assertNotNull("cache did not make a conditional request", req); + Assert.assertNotNull(req.getHeader("If-Modified-Since")); + } +}