From a6e4c8e5c7b4e3d3cac23449d472f275399a6222 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Mon, 11 Nov 2019 00:10:43 +0100 Subject: Fix NPE when no action can be parsed from the URL Renames `static final` variables according to convention to be in all upper case. That makes it easier to see that in an `equals` comparison the final variable should come first as it will not trigger a NPE. Also strip parameters from the URL when extracting the repository name from it. Parameters can not be part of a repository name, and this way an empty repository name can be detected. Fixes #1092 --- .../gitblit/servlet/AccessRestrictionFilter.java | 7 +++ src/main/java/com/gitblit/servlet/GitFilter.java | 68 ++++++++++++---------- .../java/com/gitblit/tests/GitServletTest.java | 54 +++++++++++++++++ 3 files changed, 99 insertions(+), 30 deletions(-) (limited to 'src') diff --git a/src/main/java/com/gitblit/servlet/AccessRestrictionFilter.java b/src/main/java/com/gitblit/servlet/AccessRestrictionFilter.java index e1d76db0..f83f1608 100644 --- a/src/main/java/com/gitblit/servlet/AccessRestrictionFilter.java +++ b/src/main/java/com/gitblit/servlet/AccessRestrictionFilter.java @@ -167,6 +167,7 @@ public abstract class AccessRestrictionFilter extends AuthenticationFilter { String fullUrl = getFullUrl(httpRequest); String repository = extractRepositoryName(fullUrl); if (StringUtils.isEmpty(repository)) { + logger.info("ARF: Rejecting request, empty repository name in URL {}", fullUrl); httpResponse.setStatus(HttpServletResponse.SC_BAD_REQUEST); return; } @@ -181,6 +182,12 @@ public abstract class AccessRestrictionFilter extends AuthenticationFilter { String fullSuffix = fullUrl.substring(repository.length()); String urlRequestType = getUrlRequestAction(fullSuffix); + if (StringUtils.isEmpty(urlRequestType)) { + logger.info("ARF: Rejecting request for {}, no supported action found in URL {}", repository, fullSuffix); + httpResponse.setStatus(HttpServletResponse.SC_BAD_REQUEST); + return; + } + UserModel user = getUser(httpRequest); // Load the repository model diff --git a/src/main/java/com/gitblit/servlet/GitFilter.java b/src/main/java/com/gitblit/servlet/GitFilter.java index 9522893e..4b32b433 100644 --- a/src/main/java/com/gitblit/servlet/GitFilter.java +++ b/src/main/java/com/gitblit/servlet/GitFilter.java @@ -46,14 +46,14 @@ import com.gitblit.utils.StringUtils; @Singleton public class GitFilter extends AccessRestrictionFilter { - protected static final String gitReceivePack = "/git-receive-pack"; + static final String GIT_RECEIVE_PACK = "/git-receive-pack"; - protected static final String gitUploadPack = "/git-upload-pack"; + static final String GIT_UPLOAD_PACK = "/git-upload-pack"; - protected static final String gitLfs = "/info/lfs"; + static final String GIT_LFS = "/info/lfs"; - protected static final String[] suffixes = { gitReceivePack, gitUploadPack, "/info/refs", "/HEAD", - "/objects", gitLfs }; + static final String[] SUFFIXES = {GIT_RECEIVE_PACK, GIT_UPLOAD_PACK, "/info/refs", "/HEAD", + "/objects", GIT_LFS}; private IStoredSettings settings; @@ -76,13 +76,19 @@ public class GitFilter extends AccessRestrictionFilter { /** * Extract the repository name from the url. * - * @param cloneUrl + * @param url + * Request URL with repository name in it * @return repository name */ - public static String getRepositoryName(String value) { - String repository = value; + public static String getRepositoryName(String url) { + String repository = url; + // strip off parameters from the URL + if (repository.contains("?")) { + repository = repository.substring(0, repository.indexOf("?")); + } + // get the repository name from the url by finding a known url suffix - for (String urlSuffix : suffixes) { + for (String urlSuffix : SUFFIXES) { if (repository.indexOf(urlSuffix) > -1) { repository = repository.substring(0, repository.indexOf(urlSuffix)); } @@ -111,18 +117,18 @@ public class GitFilter extends AccessRestrictionFilter { @Override protected String getUrlRequestAction(String suffix) { if (!StringUtils.isEmpty(suffix)) { - if (suffix.startsWith(gitReceivePack)) { - return gitReceivePack; - } else if (suffix.startsWith(gitUploadPack)) { - return gitUploadPack; + if (suffix.startsWith(GIT_RECEIVE_PACK)) { + return GIT_RECEIVE_PACK; + } else if (suffix.startsWith(GIT_UPLOAD_PACK)) { + return GIT_UPLOAD_PACK; } else if (suffix.contains("?service=git-receive-pack")) { - return gitReceivePack; + return GIT_RECEIVE_PACK; } else if (suffix.contains("?service=git-upload-pack")) { - return gitUploadPack; - } else if (suffix.startsWith(gitLfs)) { - return gitLfs; + return GIT_UPLOAD_PACK; + } else if (suffix.startsWith(GIT_LFS)) { + return GIT_LFS; } else { - return gitUploadPack; + return GIT_UPLOAD_PACK; } } return null; @@ -150,9 +156,11 @@ public class GitFilter extends AccessRestrictionFilter { */ @Override protected boolean isCreationAllowed(String action) { - + // Unknown action, don't allow creation + if (action == null) return false; + //Repository must already exist before large files can be deposited - if (action.equals(gitLfs)) { + if (GIT_LFS.equals(action)) { return false; } @@ -170,7 +178,7 @@ public class GitFilter extends AccessRestrictionFilter { protected boolean isActionAllowed(RepositoryModel repository, String action, String method) { // the log here has been moved into ReceiveHook to provide clients with // error messages - if (gitLfs.equals(action)) { + if (GIT_LFS.equals(action)) { if (!method.matches("GET|POST|PUT|HEAD")) { return false; } @@ -194,13 +202,13 @@ public class GitFilter extends AccessRestrictionFilter { */ @Override protected boolean requiresAuthentication(RepositoryModel repository, String action, String method) { - if (gitUploadPack.equals(action)) { + if (GIT_UPLOAD_PACK.equals(action)) { // send to client return repository.accessRestriction.atLeast(AccessRestrictionType.CLONE); - } else if (gitReceivePack.equals(action)) { + } else if (GIT_RECEIVE_PACK.equals(action)) { // receive from client return repository.accessRestriction.atLeast(AccessRestrictionType.PUSH); - } else if (gitLfs.equals(action)) { + } else if (GIT_LFS.equals(action)) { if (method.matches("GET|HEAD")) { return repository.accessRestriction.atLeast(AccessRestrictionType.CLONE); @@ -227,10 +235,10 @@ public class GitFilter extends AccessRestrictionFilter { // Git Servlet disabled return false; } - if (action.equals(gitReceivePack)) { + if (GIT_RECEIVE_PACK.equals(action)) { // push permissions are enforced in the receive pack return true; - } else if (action.equals(gitUploadPack)) { + } else if (GIT_UPLOAD_PACK.equals(action)) { // Clone request if (user.canClone(repository)) { return true; @@ -255,9 +263,9 @@ public class GitFilter extends AccessRestrictionFilter { */ @Override protected RepositoryModel createRepository(UserModel user, String repository, String action) { - boolean isPush = !StringUtils.isEmpty(action) && gitReceivePack.equals(action); + boolean isPush = !StringUtils.isEmpty(action) && GIT_RECEIVE_PACK.equals(action); - if (action.equals(gitLfs)) { + if (GIT_LFS.equals(action)) { //Repository must already exist for any filestore actions return null; } @@ -325,7 +333,7 @@ public class GitFilter extends AccessRestrictionFilter { @Override protected String getAuthenticationHeader(HttpServletRequest httpRequest, String action) { - if (action.equals(gitLfs)) { + if (GIT_LFS.equals(action)) { if (hasContentInRequestHeader(httpRequest, "Accept", FilestoreServlet.GIT_LFS_META_MIME)) { return "LFS-Authenticate"; } @@ -344,7 +352,7 @@ public class GitFilter extends AccessRestrictionFilter { protected boolean hasValidRequestHeader(String action, HttpServletRequest request) { - if (action.equals(gitLfs) && request.getMethod().equals("POST")) { + if (GIT_LFS.equals(action) && request.getMethod().equals("POST")) { if ( !hasContentInRequestHeader(request, "Accept", FilestoreServlet.GIT_LFS_META_MIME) || !hasContentInRequestHeader(request, "Content-Type", FilestoreServlet.GIT_LFS_META_MIME)) { return false; diff --git a/src/test/java/com/gitblit/tests/GitServletTest.java b/src/test/java/com/gitblit/tests/GitServletTest.java index f9a1bec4..c433ab18 100644 --- a/src/test/java/com/gitblit/tests/GitServletTest.java +++ b/src/test/java/com/gitblit/tests/GitServletTest.java @@ -25,6 +25,11 @@ import java.util.Date; import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.commons.io.IOUtils; +import org.apache.http.HttpResponse; +import org.apache.http.client.HttpClient; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.impl.client.HttpClientBuilder; import org.eclipse.jgit.api.CloneCommand; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.MergeCommand.FastForwardMode; @@ -928,4 +933,53 @@ public class GitServletTest extends GitblitUnitTest { GitBlitSuite.close(repository); assertTrue("Repository has an empty push log!", pushes.size() > 0); } + + + + @Test + public void testInvalidURLNoRepoName() throws IOException { + final String testURL = GitBlitSuite.gitServletUrl + "/?service=git-upload-pack"; + + HttpClient client = HttpClientBuilder.create().build(); + HttpGet request = new HttpGet(testURL); + + HttpResponse response = client.execute(request); + assertEquals("Expected BAD REQUEST due to missing repository string", 400, response.getStatusLine().getStatusCode()); + } + + @Test + public void testInvalidURLNoRepoName2() throws IOException { + final String testURL = GitBlitSuite.gitServletUrl + "//info/refs"; + + HttpClient client = HttpClientBuilder.create().build(); + HttpGet request = new HttpGet(testURL); + + HttpResponse response = client.execute(request); + assertEquals("Expected BAD REQUEST due to missing repository string", 400, response.getStatusLine().getStatusCode()); + } + + + @Test + public void testURLUnknownRepo() throws IOException { + final String testURL = GitBlitSuite.url + "/r/foobar.git/info/refs"; + + HttpClient client = HttpClientBuilder.create().build(); + HttpGet request = new HttpGet(testURL); + + HttpResponse response = client.execute(request); + assertEquals(401, response.getStatusLine().getStatusCode()); + } + + @Test + public void testURLUnknownAction() throws IOException { + final String testURL = GitBlitSuite.gitServletUrl + "/helloworld.git/something/unknown"; + + HttpClient client = HttpClientBuilder.create().build(); + HttpGet request = new HttpGet(testURL); + + HttpResponse response = client.execute(request); + + assertEquals(400, response.getStatusLine().getStatusCode()); + } + } -- cgit v1.2.3