From b0ab2e07c5c119a268487e2194f07f400a416668 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 1 Aug 2019 11:01:39 +0100 Subject: [PATCH 1/4] Reject requests when the project uri begins with '/project' --- .../ic/wlgitbridge/server/Oauth2Filter.java | 22 +++++++++++++++- .../WLGitBridgeIntegrationTest.java | 26 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java b/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java index dfcfdf21be..bfbf0d6eea 100644 --- a/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java +++ b/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java @@ -40,6 +40,18 @@ public class Oauth2Filter implements Filter { @Override public void init(FilterConfig filterConfig) {} + private void sendResponse(ServletResponse servletResponse, int code, List lines) throws IOException { + HttpServletResponse response = ((HttpServletResponse) servletResponse); + response.setContentType("text/plain"); + response.setStatus(code); + PrintWriter w = response.getWriter(); + for (String line : lines) { + w.println(line); + } + w.close(); + return; + } + /** * The original request from git will not contain the Authorization header. * @@ -57,8 +69,16 @@ public class Oauth2Filter implements Filter { ServletResponse servletResponse, FilterChain filterChain ) throws IOException, ServletException { + String requestUri = ((Request) servletRequest).getRequestURI(); + if (requestUri.startsWith("/project")) { + Log.info("[{}] Invalid request URI", requestUri); + sendResponse(servletResponse,400, Arrays.asList( + "Invalid Project ID (must not have a '/project' prefix)" + )); + return; + } String project = Util.removeAllSuffixes( - ((Request) servletRequest).getRequestURI().split("/")[1], + requestUri.split("/")[1], ".git" ); // Reject v1 ids, the request will be rejected by v1 anyway diff --git a/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java b/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java index b45b27be7a..f150585f0f 100644 --- a/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java +++ b/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java @@ -865,6 +865,32 @@ public class WLGitBridgeIntegrationTest { wlgb.stop(); } + @Test + public void cannotCloneProjectWithSlash() throws IOException, GitAPIException, InterruptedException { + int gitBridgePort = 33886; + int mockServerPort = 3886; + + MockSnapshotServer server = new MockSnapshotServer(mockServerPort, getResource("/canCloneARepository").toFile()); + server.start(); + server.setState(states.get("canCloneARepository").get("state")); + GitBridgeApp wlgb = new GitBridgeApp(new String[] { + makeConfigFile(gitBridgePort, mockServerPort) + }); + + wlgb.run(); + Process gitProcess = runtime.exec("git clone http://127.0.0.1:" + gitBridgePort + "/project/1234abcd", null, dir); + assertNotEquals(0, gitProcess.waitFor()); + + List actual = Util.linesFromStream(gitProcess.getErrorStream(), 0, ""); + assertEquals(Arrays.asList( + "Cloning into '1234abcd'...", + "remote: Invalid Project ID (must not have a '/project' prefix)", + "fatal: unable to access 'http://127.0.0.1:33886/project/1234abcd/': The requested URL returned error: 400" + ), actual); + + wlgb.stop(); + } + private String makeConfigFile( int port, int apiPort From 76b349591c7877d99342e60e1c6b5c5f01341be2 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 1 Aug 2019 11:08:41 +0100 Subject: [PATCH 2/4] Refactor to use new helper to send error response --- .../uk/ac/ic/wlgitbridge/server/Oauth2Filter.java | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java b/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java index bfbf0d6eea..9eb8442086 100644 --- a/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java +++ b/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java @@ -84,11 +84,7 @@ public class Oauth2Filter implements Filter { // Reject v1 ids, the request will be rejected by v1 anyway if (project.matches("^[0-9]+[bcdfghjklmnpqrstvwxyz]{6,12}$") && !project.matches("^[0-9a-f]{24}$")) { Log.info("[{}] Request for v1 project, refusing", project); - HttpServletResponse response = ((HttpServletResponse) servletResponse); - response.setContentType("text/plain"); - response.setStatus(404); - PrintWriter w = response.getWriter(); - List l = Arrays.asList( + sendResponse(servletResponse, 404, Arrays.asList( "This project has not yet been moved into the new version", "of Overleaf. You will need to move it in order to continue working on it.", "Please visit this project online on www.overleaf.com to do this.", @@ -98,11 +94,7 @@ public class Oauth2Filter implements Filter { "", "If this is unexpected, please contact us at support@overleaf.com, or", "see https://www.overleaf.com/help/342 for more information." - ); - for (String line : l) { - w.println(line); - } - w.close(); + )); return; } Log.info("[{}] Checking if auth needed", project); From ac4f4082c8c8e3f67dbaff5e6d71ebb78648b3aa Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 2 Aug 2019 10:46:15 +0100 Subject: [PATCH 3/4] Use 404 code when rejecting invalid project id --- .../src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java | 2 +- .../ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java b/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java index 9eb8442086..631401c759 100644 --- a/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java +++ b/services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/server/Oauth2Filter.java @@ -72,7 +72,7 @@ public class Oauth2Filter implements Filter { String requestUri = ((Request) servletRequest).getRequestURI(); if (requestUri.startsWith("/project")) { Log.info("[{}] Invalid request URI", requestUri); - sendResponse(servletResponse,400, Arrays.asList( + sendResponse(servletResponse,404, Arrays.asList( "Invalid Project ID (must not have a '/project' prefix)" )); return; diff --git a/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java b/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java index f150585f0f..1a6cffbe6a 100644 --- a/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java +++ b/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java @@ -885,7 +885,7 @@ public class WLGitBridgeIntegrationTest { assertEquals(Arrays.asList( "Cloning into '1234abcd'...", "remote: Invalid Project ID (must not have a '/project' prefix)", - "fatal: unable to access 'http://127.0.0.1:33886/project/1234abcd/': The requested URL returned error: 400" + "fatal: repository 'http://127.0.0.1:33886/project/1234abcd/' not found" ), actual); wlgb.stop(); From ffcb382f0c34f1b83e365bdc8f04830a6f4d1b6a Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 2 Aug 2019 13:34:43 +0100 Subject: [PATCH 4/4] Update test to match new setup/teardown pattern --- .../wlgitbridge/application/WLGitBridgeIntegrationTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java b/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java index 1a6cffbe6a..2aeedde531 100644 --- a/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java +++ b/services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/application/WLGitBridgeIntegrationTest.java @@ -870,10 +870,10 @@ public class WLGitBridgeIntegrationTest { int gitBridgePort = 33886; int mockServerPort = 3886; - MockSnapshotServer server = new MockSnapshotServer(mockServerPort, getResource("/canCloneARepository").toFile()); + server = new MockSnapshotServer(mockServerPort, getResource("/canCloneARepository").toFile()); server.start(); server.setState(states.get("canCloneARepository").get("state")); - GitBridgeApp wlgb = new GitBridgeApp(new String[] { + wlgb = new GitBridgeApp(new String[] { makeConfigFile(gitBridgePort, mockServerPort) });