smart HTTP: Return errors inside payload

When the client is clearly making a smart HTTP request to our smart
HTTP server, return any errors like RepositoryNotFoundException or
ServiceNotEnabledException inside of the payload as a Git level ERR
message, rather than an HTTP error code.

This prevents the C Git command line client from retrying a failed
"$URL/info/refs?service=git-upload-pack" request without the smart
service URL, only to fail again with "403 Forbidden" when the dumb
as-is service has been disabled by the server configuration, or is
unavailable because the repository is not on the local filesystem.

Change-Id: I57e8756d5026e885e0ca615979bfcd729703be6c
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

diff --git a/org.eclipse.jgit.http.server/resources/org/eclipse/jgit/http/server/HttpServerText.properties b/org.eclipse.jgit.http.server/resources/org/eclipse/jgit/http/server/HttpServerText.properties
index 6232f47f0537039e758832991487281d7cb01d32..a7643c5f9e2c77fb74639b23979de9855c08245f 100644 (file)
--- a/org.eclipse.jgit.http.server/resources/org/eclipse/jgit/http/server/HttpServerText.properties
+++ b/org.eclipse.jgit.http.server/resources/org/eclipse/jgit/http/server/HttpServerText.properties
@@ -13,6 +13,8 @@ noResolverAvailable=No resolver available
 parameterNotSet=Parameter {0} not set
 pathForParamNotFound={0} (for {1}) not found
 pathNotSupported={0} not supported
+repositoryAccessForbidden=Git access forbidden
+repositoryNotFound=Git repository not found
 servletAlreadyInitialized=Servlet already initialized
 servletMustNotBeNull=servlet must not be null
 servletWasAlreadyBound=servlet was already bound

diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/HttpServerText.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/HttpServerText.java
index fc1001442766a90e90d55fb40af4e44d40f63155..18743989bad9a402481baaddd6f9e5d4a4b7941d 100644 (file)
--- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/HttpServerText.java
+++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/HttpServerText.java
@@ -73,6 +73,8 @@ public class HttpServerText extends TranslationBundle {
        /***/ public String parameterNotSet;
        /***/ public String pathForParamNotFound;
        /***/ public String pathNotSupported;
+       /***/ public String repositoryAccessForbidden;
+       /***/ public String repositoryNotFound;
        /***/ public String servletAlreadyInitialized;
        /***/ public String servletMustNotBeNull;
        /***/ public String servletWasAlreadyBound;

diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java
index 69b5aec25e7841dee1cca6530c21aa16810bcb5c..192050a17c0cf85b3351b2cc8e790ed450c0648a 100644 (file)
--- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java
+++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java
@@ -128,7 +128,7 @@ class ReceivePackServlet extends HttpServlet {
                                return;
 
                        } catch (ServiceNotEnabledException e) {
-                               rsp.sendError(SC_FORBIDDEN);
+                               RepositoryFilter.sendError(SC_FORBIDDEN, req, rsp);
                                return;
                        }
 

diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/RepositoryFilter.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/RepositoryFilter.java
index 1097932ece321752b5bda408dc52104903f931fc..3e0a572564831a354f003277849f185bc9f8c21d 100644 (file)
--- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/RepositoryFilter.java
+++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/RepositoryFilter.java
@@ -48,6 +48,7 @@ import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
 import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
 import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
 import static org.eclipse.jgit.http.server.ServletUtils.ATTRIBUTE_REPOSITORY;
+import static org.eclipse.jgit.util.HttpSupport.HDR_ACCEPT;
 
 import java.io.IOException;
 import java.text.MessageFormat;
@@ -64,6 +65,7 @@ import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.transport.PacketLineOut;
 import org.eclipse.jgit.transport.resolver.RepositoryResolver;
 import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException;
 import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException;
@@ -131,14 +133,14 @@ public class RepositoryFilter implements Filter {
                try {
                        db = resolver.open(req, name);
                } catch (RepositoryNotFoundException e) {
-                       ((HttpServletResponse) rsp).sendError(SC_NOT_FOUND);
+                       sendError(SC_NOT_FOUND, req, (HttpServletResponse) rsp);
+                       return;
+               } catch (ServiceNotEnabledException e) {
+                       sendError(SC_FORBIDDEN, req, (HttpServletResponse) rsp);
                        return;
                } catch (ServiceNotAuthorizedException e) {
                        ((HttpServletResponse) rsp).sendError(SC_UNAUTHORIZED);
                        return;
-               } catch (ServiceNotEnabledException e) {
-                       ((HttpServletResponse) rsp).sendError(SC_FORBIDDEN);
-                       return;
                }
                try {
                        request.setAttribute(ATTRIBUTE_REPOSITORY, db);
@@ -148,4 +150,54 @@ public class RepositoryFilter implements Filter {
                        db.close();
                }
        }
+
+       static void sendError(int statusCode, HttpServletRequest req,
+                       HttpServletResponse rsp) throws IOException {
+               String svc = req.getParameter("service");
+               String accept = req.getHeader(HDR_ACCEPT);
+
+               if (svc != null && svc.startsWith("git-") && accept != null
+                               && accept.contains("application/x-" + svc + "-advertisement")) {
+                       // Smart HTTP service request, use an ERR response.
+                       rsp.setContentType("application/x-" + svc + "-advertisement");
+
+                       SmartOutputStream buf = new SmartOutputStream(req, rsp);
+                       PacketLineOut out = new PacketLineOut(buf);
+                       out.writeString("# service=" + svc + "\n");
+                       out.end();
+                       out.writeString("ERR " + translate(statusCode));
+                       buf.close();
+                       return;
+               }
+
+               if (accept != null && accept.contains(UploadPackServlet.RSP_TYPE)) {
+                       // An upload-pack wants ACK or NAK, return ERR
+                       // and the client will print this instead.
+                       rsp.setContentType(UploadPackServlet.RSP_TYPE);
+                       SmartOutputStream buf = new SmartOutputStream(req, rsp);
+                       PacketLineOut out = new PacketLineOut(buf);
+                       out.writeString("ERR " + translate(statusCode));
+                       buf.close();
+                       return;
+               }
+
+               // Otherwise fail with an HTTP error code instead of an
+               // application level message. This may not be as pretty
+               // of a result for the user, but its better than nothing.
+               //
+               rsp.sendError(statusCode);
+       }
+
+       private static String translate(int statusCode) {
+               switch (statusCode) {
+               case SC_NOT_FOUND:
+                       return HttpServerText.get().repositoryNotFound;
+
+               case SC_FORBIDDEN:
+                       return HttpServerText.get().repositoryAccessForbidden;
+
+               default:
+                       return String.valueOf(statusCode);
+               }
+       }
 }

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 e60c5068cf36e2b0c9b32cad1ca756b9741d0e5e..1ceb0965a3860f82c8ed307a8a9e235c3b82ca43 100644 (file)
--- 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
@@ -77,7 +77,7 @@ import org.eclipse.jgit.transport.resolver.UploadPackFactory;
 class UploadPackServlet extends HttpServlet {
        private static final String REQ_TYPE = "application/x-git-upload-pack-request";
 
-       private static final String RSP_TYPE = "application/x-git-upload-pack-result";
+       static final String RSP_TYPE = "application/x-git-upload-pack-result";
 
        private static final long serialVersionUID = 1L;
 
@@ -130,7 +130,7 @@ class UploadPackServlet extends HttpServlet {
                                return;
 
                        } catch (ServiceNotEnabledException e) {
-                               rsp.sendError(SC_FORBIDDEN);
+                               RepositoryFilter.sendError(SC_FORBIDDEN, req, rsp);
                                return;
                        }
 

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 209f161c9bdf4c3fa41ac7105bfdbd0a24ec94e0..c3590a44fb923daabee85a3a5be4585aac194d53 100644 (file)
--- 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
@@ -55,6 +55,7 @@ import static org.junit.Assert.fail;
 
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.net.URISyntaxException;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -73,6 +74,7 @@ import org.eclipse.jetty.servlet.FilterMapping;
 import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jgit.JGitText;
+import org.eclipse.jgit.errors.RemoteRepositoryException;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.http.server.GitServlet;
@@ -223,6 +225,36 @@ public class SmartClientSmartServerTest extends HttpTestCase {
                assertEquals("gzip", info.getResponseHeader(HDR_CONTENT_ENCODING));
        }
 
+       @Test
+       public void testListRemote_BadName() throws IOException, URISyntaxException {
+               Repository dst = createBareRepository();
+               URIish uri = new URIish(this.remoteURI.toString() + ".invalid");
+               Transport t = Transport.open(dst, uri);
+               try {
+                       try {
+                               t.openFetch();
+                               fail("fetch connection opened");
+                       } catch (RemoteRepositoryException notFound) {
+                               assertEquals(uri + ": Git repository not found",
+                                               notFound.getMessage());
+                       }
+               } finally {
+                       t.close();
+               }
+
+               List<AccessEvent> requests = getRequests();
+               assertEquals(1, requests.size());
+
+               AccessEvent info = requests.get(0);
+               assertEquals("GET", info.getMethod());
+               assertEquals(join(uri, "info/refs"), info.getPath());
+               assertEquals(1, info.getParameters().size());
+               assertEquals("git-upload-pack", info.getParameter("service"));
+               assertEquals(200, info.getStatus());
+               assertEquals("application/x-git-upload-pack-advertisement",
+                               info.getResponseHeader(HDR_CONTENT_TYPE));
+       }
+
        @Test
        public void testInitialClone_Small() throws Exception {
                Repository dst = createBareRepository();