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
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 012f9a3..f1155dc 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 @@
}
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 @@
}
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 @@
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 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 77ed09c..d3437b7 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 @@
}
}
+ 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 @@
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 df093c1..3438c52 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 @@
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 887e970..8f3888e 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 @@
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 @@
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 @@
Collections.singletonList(
new RefSpec(unreachableCommit.name()))));
assertTrue(e.getMessage().contains(
- "want " + unreachableCommit.name() + " not valid"));
+ "Bad Request"));
}
+ assertLastRequestStatusCode(400);
}
@Test
@@ -558,8 +560,9 @@
() -> 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 @@
} catch (TransportException e) {
assertTrue(e.getMessage().contains("301"));
}
+ assertLastRequestStatusCode(301);
}
@Test
@@ -934,6 +938,7 @@
assertTrue(
e.getMessage().contains("http.followRedirects is false"));
}
+ assertLastRequestStatusCode(301);
}
private void assertFetchRequests(List<AccessEvent> requests, int index) {
@@ -1605,8 +1610,9 @@
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 @@
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 @@
.getResponseHeader(HDR_CONTENT_TYPE));
}
+ private void assertLastRequestStatusCode(int statusCode) {
+ List<AccessEvent> 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);
