From 8984e1f663b5b58f74cd0381043133f0059e004d Mon Sep 17 00:00:00 2001 From: Sven Selberg Date: Tue, 12 Apr 2022 11:36:58 +0200 Subject: [PATCH] HTTP Smart: set correct HTTP status on error Previous behavior was that status code was automatically set to 200 regardless of reported status and according to HTTP Smart protocol[1]: If there is no repository at $GIT_URL, or the resource pointed to by a location matching $GIT_URL does not exist, the server MUST NOT respond with 200 OK response. A server SHOULD respond with 404 Not Found, 410 Gone, or any other suitable HTTP status code which does not imply the resource exists as requested. Since the jgit HTTP client isn't able to handle reading content from a response reporting an error (calling HttpURLConnection#getInputStream on a "failed" connection throws an exception and the internal interface HttpConnection does not expose HttpURLConnection#getErrorStream) the SmartClientSmartServerTest needed to be rewritten to expect the generic response messages. [1] https://git-scm.com/docs/http-protocol#_general_request_processing Bug: 579676 Change-Id: Ibb942d02124a0bc279df09600b091354019ce064 --- .../jgit/http/server/GitSmartHttpTools.java | 25 +++++++++-------- .../jgit/http/server/UploadPackServlet.java | 21 ++++++++++++--- .../jgit/http/test/HttpClientTests.java | 6 ++++- .../http/test/SmartClientSmartServerTest.java | 27 +++++++++++++------ 4 files changed, 56 insertions(+), 23 deletions(-) diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java index 012f9a3dc..f1155dcf5 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java @@ -158,11 +158,11 @@ public static void sendError(HttpServletRequest req, } if (isInfoRefs(req)) { - sendInfoRefsError(req, res, textForGit); + sendInfoRefsError(req, res, textForGit, httpStatus); } else if (isUploadPack(req)) { - sendUploadPackError(req, res, textForGit); + sendUploadPackError(req, res, textForGit, httpStatus); } else if (isReceivePack(req)) { - sendReceivePackError(req, res, textForGit); + sendReceivePackError(req, res, textForGit, httpStatus); } else { if (httpStatus < 400) ServletUtils.consumeRequestBody(req); @@ -171,29 +171,32 @@ public static void sendError(HttpServletRequest req, } private static void sendInfoRefsError(HttpServletRequest req, - HttpServletResponse res, String textForGit) throws IOException { + HttpServletResponse res, String textForGit, int httpStatus) + throws IOException { ByteArrayOutputStream buf = new ByteArrayOutputStream(128); PacketLineOut pck = new PacketLineOut(buf); String svc = req.getParameter("service"); pck.writeString("# service=" + svc + "\n"); pck.end(); pck.writeString("ERR " + textForGit); - send(req, res, infoRefsResultType(svc), buf.toByteArray()); + send(req, res, infoRefsResultType(svc), buf.toByteArray(), httpStatus); } private static void sendUploadPackError(HttpServletRequest req, - HttpServletResponse res, String textForGit) throws IOException { + HttpServletResponse res, String textForGit, int httpStatus) + throws IOException { // Do not use sideband. Sideband is acceptable only while packfile is // being sent. Other places, like acknowledgement section, do not // support sideband. Use an error packet. ByteArrayOutputStream buf = new ByteArrayOutputStream(128); PacketLineOut pckOut = new PacketLineOut(buf); writePacket(pckOut, textForGit); - send(req, res, UPLOAD_PACK_RESULT_TYPE, buf.toByteArray()); + send(req, res, UPLOAD_PACK_RESULT_TYPE, buf.toByteArray(), httpStatus); } private static void sendReceivePackError(HttpServletRequest req, - HttpServletResponse res, String textForGit) throws IOException { + HttpServletResponse res, String textForGit, int httpStatus) + throws IOException { ByteArrayOutputStream buf = new ByteArrayOutputStream(128); PacketLineOut pckOut = new PacketLineOut(buf); @@ -212,7 +215,7 @@ private static void sendReceivePackError(HttpServletRequest req, writeSideBand(buf, textForGit); else writePacket(pckOut, textForGit); - send(req, res, RECEIVE_PACK_RESULT_TYPE, buf.toByteArray()); + send(req, res, RECEIVE_PACK_RESULT_TYPE, buf.toByteArray(), httpStatus); } private static boolean isReceivePackSideBand(HttpServletRequest req) { @@ -246,9 +249,9 @@ private static void writePacket(PacketLineOut pckOut, String textForGit) } private static void send(HttpServletRequest req, HttpServletResponse res, - String type, byte[] buf) throws IOException { + String type, byte[] buf, int httpStatus) throws IOException { ServletUtils.consumeRequestBody(req); - res.setStatus(HttpServletResponse.SC_OK); + res.setStatus(httpStatus); res.setContentType(type); res.setContentLength(buf.length); try (OutputStream os = res.getOutputStream()) { diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java index 77ed09cdf..d3437b7eb 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java @@ -10,6 +10,7 @@ package org.eclipse.jgit.http.server; +import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; @@ -48,6 +49,7 @@ import org.eclipse.jgit.transport.ServiceMayNotContinueException; import org.eclipse.jgit.transport.UploadPack; import org.eclipse.jgit.transport.UploadPackInternalServerErrorException; +import org.eclipse.jgit.transport.WantNotValidException; import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException; import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; import org.eclipse.jgit.transport.resolver.UploadPackFactory; @@ -151,6 +153,16 @@ public void destroy() { } } + private static int statusCodeForThrowable(Throwable error) { + if (error instanceof ServiceNotEnabledException) { + return SC_FORBIDDEN; + } + if (error instanceof WantNotValidException) { + return SC_BAD_REQUEST; + } + return SC_INTERNAL_SERVER_ERROR; + } + private final UploadPackErrorHandler handler; UploadPackServlet(@Nullable UploadPackErrorHandler handler) { @@ -216,9 +228,12 @@ private void defaultUploadPackHandler(HttpServletRequest req, log(up.getRepository(), e); if (!rsp.isCommitted()) { rsp.reset(); - String msg = e instanceof PackProtocolException ? e.getMessage() - : null; - sendError(req, rsp, SC_INTERNAL_SERVER_ERROR, msg); + String msg = null; + if (e instanceof PackProtocolException + || e instanceof ServiceNotEnabledException) { + msg = e.getMessage(); + } + sendError(req, rsp, statusCodeForThrowable(e), msg); } } } diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java index df093c185..3438c52c8 100644 --- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java @@ -23,6 +23,7 @@ import java.io.OutputStream; import java.net.URI; import java.net.URL; +import java.text.MessageFormat; import java.util.List; import javax.servlet.http.HttpServletRequest; @@ -308,7 +309,10 @@ public void testListRemote_Smart_UploadPackDisabled() throws Exception { fail("connection opened even though service disabled"); } catch (TransportException err) { String exp = smartAuthNoneURI + ": " - + JGitText.get().serviceNotEnabledNoName; + + MessageFormat.format( + JGitText.get().serviceNotPermitted, + smartAuthNoneURI.toString() + "/", + "git-upload-pack"); assertEquals(exp, err.getMessage()); } } diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java index 887e970a0..8f3888e4d 100644 --- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java @@ -57,7 +57,7 @@ import org.eclipse.jetty.servlet.ServletHolder; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.TransportConfigCallback; -import org.eclipse.jgit.errors.RemoteRepositoryException; +import org.eclipse.jgit.errors.NoRemoteRepositoryException; import org.eclipse.jgit.errors.TransportException; import org.eclipse.jgit.errors.UnsupportedCredentialItem; import org.eclipse.jgit.http.server.GitServlet; @@ -496,8 +496,9 @@ public void testListRemote_BadName() throws IOException, URISyntaxException { try { t.openFetch(); fail("fetch connection opened"); - } catch (RemoteRepositoryException notFound) { - assertEquals(uri + ": Git repository not found", + } catch (NoRemoteRepositoryException notFound) { + assertEquals(uri + ": " + uri + + "/info/refs?service=git-upload-pack not found: Not Found", notFound.getMessage()); } } @@ -510,7 +511,7 @@ public void testListRemote_BadName() throws IOException, URISyntaxException { assertEquals(join(uri, "info/refs"), info.getPath()); assertEquals(1, info.getParameters().size()); assertEquals("git-upload-pack", info.getParameter("service")); - assertEquals(200, info.getStatus()); + assertEquals(404, info.getStatus()); assertEquals("application/x-git-upload-pack-advertisement", info.getResponseHeader(HDR_CONTENT_TYPE)); } @@ -536,8 +537,9 @@ public void testFetchBySHA1Unreachable() throws Exception { Collections.singletonList( new RefSpec(unreachableCommit.name())))); assertTrue(e.getMessage().contains( - "want " + unreachableCommit.name() + " not valid")); + "Bad Request")); } + assertLastRequestStatusCode(400); } @Test @@ -558,8 +560,9 @@ protected Map getAdvertisedRefs(Repository repository, () -> t.fetch(NullProgressMonitor.INSTANCE, Collections.singletonList(new RefSpec(A.name())))); assertTrue( - e.getMessage().contains("want " + A.name() + " not valid")); + e.getMessage().contains("Bad Request")); } + assertLastRequestStatusCode(400); } @Test @@ -916,6 +919,7 @@ public void testInitialClone_RedirectOnPostForbidden() throws Exception { } catch (TransportException e) { assertTrue(e.getMessage().contains("301")); } + assertLastRequestStatusCode(301); } @Test @@ -934,6 +938,7 @@ public void testInitialClone_RedirectForbidden() throws Exception { assertTrue( e.getMessage().contains("http.followRedirects is false")); } + assertLastRequestStatusCode(301); } private void assertFetchRequests(List requests, int index) { @@ -1605,8 +1610,9 @@ public void testInvalidWant() throws Exception { fail("Server accepted want " + id.name()); } catch (TransportException err) { assertTrue(err.getMessage() - .contains("want " + id.name() + " not valid")); + .contains("Bad Request")); } + assertLastRequestStatusCode(400); } @Test @@ -1650,7 +1656,7 @@ public void testFetch_RefsUnreadableOnUpload() throws Exception { fail("Successfully served ref with value " + c.getRef(master)); } catch (TransportException err) { assertTrue("Unexpected exception message " + err.getMessage(), - err.getMessage().contains("Internal server error")); + err.getMessage().contains("Server Error")); } } finally { noRefServer.tearDown(); @@ -1821,6 +1827,11 @@ public void testPush_ChunkedEncoding() throws Exception { .getResponseHeader(HDR_CONTENT_TYPE)); } + private void assertLastRequestStatusCode(int statusCode) { + List requests = getRequests(); + assertEquals(statusCode, requests.get(requests.size() - 1).getStatus()); + } + private void enableReceivePack() throws IOException { final StoredConfig cfg = remoteRepository.getConfig(); cfg.setBoolean("http", null, "receivepack", true);