motiejus
/
jgit
1
Fork 0
Code Releases Activity

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
This commit is contained in:
Sven Selberg 2022-04-12 11:36:58 +02:00 committed by Matthias Sohn
parent 8f7ef245f0
commit 8984e1f663
4 changed files with 56 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java
View File

@ -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()) {

21
org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java
View File

@ -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);
}
}
}

6
org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java
View File

@ -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());
}
}

27
org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
View File

@ -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<String, Ref> 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<AccessEvent> 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<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);