From 8bbeee0f8d57a7246e51c13bb411fabad7e4eff9 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 4 Dec 2020 14:42:21 -0500 Subject: [PATCH] Strip token from blob URLs when using cache Blob URLs coming from web may now contain a token for authentication with history v1. This token will change every request, which makes the URL not suitable as a cache key. Removing the token fixes that. --- .../bridge/resource/UrlResourceCache.java | 21 ++++++++++++++++-- .../bridge/resource/UrlResourceCacheTest.java | 22 ++++++++++++++++--- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/bridge/resource/UrlResourceCache.java b/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/bridge/resource/UrlResourceCache.java index e3c7911b6e..29a0cbea57 100644 --- a/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/bridge/resource/UrlResourceCache.java +++ b/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/bridge/resource/UrlResourceCache.java @@ -43,7 +43,7 @@ public class UrlResourceCache implements ResourceCache { Map fetchedUrls, Optional maxFileSize ) throws IOException, SizeLimitExceededException { - String path = dbStore.getPathForURLInProject(projectName, url); + String path = dbStore.getPathForURLInProject(projectName, getCacheKeyFromUrl(url)); byte[] contents; if (path == null) { path = newPath; @@ -118,8 +118,25 @@ public class UrlResourceCache implements ResourceCache { throw new SizeLimitExceededException( Optional.of(path), contents.length, maxFileSize.get()); } - dbStore.addURLIndexForProject(projectName, url, path); + dbStore.addURLIndexForProject(projectName, getCacheKeyFromUrl(url), path); return contents; } + /** + * Construct a suitable cache key from the given file URL. + * + * The file URL returned by the web service may contain a token parameter + * used for authentication. This token changes for every request, so we + * need to strip it from the query string before using the URL as a cache + * key. + */ + private String getCacheKeyFromUrl(String url) { + // We're not doing proper URL parsing here, but it should be enough to + // remove the token without touching the important parts of the URL. + // + // The URL looks like: + // + // https://history.overleaf.com/api/projects/:project_id/blobs/:hash?token=:token&_path=:path + return url.replaceAll("token=[^&]*", "token=REMOVED"); + } } diff --git a/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/bridge/resource/UrlResourceCacheTest.java b/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/bridge/resource/UrlResourceCacheTest.java index e6a93d8a30..1c814770cd 100644 --- a/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/bridge/resource/UrlResourceCacheTest.java +++ b/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/bridge/resource/UrlResourceCacheTest.java @@ -3,7 +3,7 @@ package uk.ac.ic.wlgitbridge.bridge.resource; import io.netty.handler.codec.http.HttpHeaders; import io.netty.handler.codec.http.DefaultHttpHeaders; import org.junit.Test; -import uk.ac.ic.wlgitbridge.bridge.db.noop.NoopDbStore; +import uk.ac.ic.wlgitbridge.bridge.db.DBStore; import uk.ac.ic.wlgitbridge.bridge.util.CastUtil; import uk.ac.ic.wlgitbridge.git.exception.SizeLimitExceededException; import uk.ac.ic.wlgitbridge.io.http.ning.NingHttpClientFacade; @@ -17,6 +17,7 @@ import java.util.concurrent.ExecutionException; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.verify; public class UrlResourceCacheTest { @@ -28,8 +29,9 @@ public class UrlResourceCacheTest { private final NingHttpClientFacade http = mock(NingHttpClientFacade.class); - private final UrlResourceCache cache - = new UrlResourceCache(new NoopDbStore(), http); + private final DBStore dbStore = mock(DBStore.class); + + private final UrlResourceCache cache = new UrlResourceCache(dbStore, http); private static HttpHeaders withContentLength(long cl) { return new DefaultHttpHeaders().add("Content-Length", String.valueOf(cl)); @@ -57,6 +59,10 @@ public class UrlResourceCacheTest { PROJ, URL, NEW_PATH, new HashMap<>(), new HashMap<>(), max); } + private void getUrl(String url) throws IOException, SizeLimitExceededException { + cache.get(PROJ, url, NEW_PATH, new HashMap<>(), new HashMap<>(), Optional.empty()); + } + private void getWithMaxLength(long max) throws IOException, SizeLimitExceededException { getWithMaxLength(Optional.of(max)); @@ -104,4 +110,14 @@ public class UrlResourceCacheTest { getWithMaxLength(5); } + @Test + public void tokenIsRemovedFromCacheKey() throws Exception { + String url = "http://history.overleaf.com/projects/1234/blobs/abdef?token=secretencryptedstuff&_path=test.tex"; + String cacheKey = "http://history.overleaf.com/projects/1234/blobs/abdef?token=REMOVED&_path=test.tex"; + respondWithContentLength(123); + getUrl(url); + verify(dbStore).getPathForURLInProject(PROJ, cacheKey); + verify(dbStore).addURLIndexForProject(PROJ, cacheKey, NEW_PATH); + } + }