From 8984e1f663b5b58f74cd0381043133f0059e004d Mon Sep 17 00:00:00 2001 From: Sven Selberg Date: Tue, 12 Apr 2022 11:36:58 +0200 Subject: 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 --- .../eclipse/jgit/http/test/HttpClientTests.java | 6 ++++- .../jgit/http/test/SmartClientSmartServerTest.java | 27 +++++++++++++++------- 2 files changed, 24 insertions(+), 9 deletions(-) (limited to 'org.eclipse.jgit.http.test') 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 df093c185b..3438c52c8d 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.File; 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 class HttpClientTests extends AllFactoriesHttpTestCase { 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 887e970a0c..8f3888e4d5 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.ServletContextHandler; 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 class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { 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 class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { 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 class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { Collections.singletonList( new RefSpec(unreachableCommit.name())))); assertTrue(e.getMessage().contains( - "want " + unreachableCommit.name() + " not valid")); + "Bad Request")); } + assertLastRequestStatusCode(400); } @Test @@ -558,8 +560,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { () -> 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 class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { } catch (TransportException e) { assertTrue(e.getMessage().contains("301")); } + assertLastRequestStatusCode(301); } @Test @@ -934,6 +938,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { assertTrue( e.getMessage().contains("http.followRedirects is false")); } + assertLastRequestStatusCode(301); } private void assertFetchRequests(List requests, int index) { @@ -1605,8 +1610,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { 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 class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { 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 class SmartClientSmartServerTest extends AllProtocolsHttpTestCase { .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); -- cgit v1.2.3