From bdb7357228c6611cea2d266255c7751bd9ed368e Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Wed, 13 May 2020 20:06:23 +0200 Subject: [PATCH] TransportHttp: abort on time-out or on SocketException Avoid trying other authentication methods on SocketException or on InterruptedIOException. SocketException is rather fatal, such as nothing listening on the peer's port, connection reset, or it could be a connection time-out. Time-outs enforced by Timeout{Input,Output}Stream may result in InterruptedIOException being thrown. In both cases, it makes no sense to try other authentication methods, and doing so may wrongly report "authentication not supported" or "cannot open git-upload-pack" or some such instead of reporting a time-out. Bug: 563138 Change-Id: I0191b1e784c2471035e550205abd06ec9934fd00 Signed-off-by: Thomas Wolf --- .../http/test/SmartClientSmartServerTest.java | 76 +++++++++++++++++++ .../eclipse/jgit/transport/TransportHttp.java | 14 ++++ 2 files changed, 90 insertions(+) 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 51a7a8ddc..8d1870a87 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 @@ -112,6 +112,10 @@ public class SmartClientSmartServerTest extends AllFactoriesHttpTestCase { private URIish authOnPostURI; + private URIish slowURI; + + private URIish slowAuthURI; + private RevBlob A_txt; private RevCommit A, B, unreachableCommit; @@ -152,6 +156,10 @@ public void setUp() throws Exception { ServletContextHandler authOnPost = addAuthContext(gs, "pauth", "POST"); + ServletContextHandler slow = addSlowContext(gs, "slow", false); + + ServletContextHandler slowAuth = addSlowContext(gs, "slowAuth", true); + server.setUp(); remoteRepository = src.getRepository(); @@ -160,6 +168,8 @@ public void setUp() throws Exception { redirectURI = toURIish(redirect, srcName); authURI = toURIish(auth, srcName); authOnPostURI = toURIish(authOnPost, srcName); + slowURI = toURIish(slow, srcName); + slowAuthURI = toURIish(slowAuth, srcName); A_txt = src.blob("A"); A = src.commit().add("A_txt", A_txt).create(); @@ -372,6 +382,43 @@ public void destroy() { return redirect; } + private ServletContextHandler addSlowContext(GitServlet gs, String path, + boolean auth) { + ServletContextHandler slow = server.addContext('/' + path); + slow.addFilter(new FilterHolder(new Filter() { + + @Override + public void init(FilterConfig filterConfig) + throws ServletException { + // empty + } + + // Simply delays the servlet for two seconds. Used for timeout + // tests, which use a one-second timeout. + @Override + public void doFilter(ServletRequest request, + ServletResponse response, FilterChain chain) + throws IOException, ServletException { + try { + Thread.sleep(2000); + } catch (InterruptedException e) { + throw new IOException(e); + } + chain.doFilter(request, response); + } + + @Override + public void destroy() { + // empty + } + }), "/*", EnumSet.of(DispatcherType.REQUEST)); + slow.addServlet(new ServletHolder(gs), "/*"); + if (auth) { + return server.authBasic(slow); + } + return slow; + } + @Test public void testListRemote() throws IOException { assertEquals("http", remoteURI.getScheme()); @@ -488,6 +535,35 @@ protected Map getAdvertisedRefs(Repository repository, } } + @Test + public void testTimeoutExpired() throws Exception { + try (Repository dst = createBareRepository(); + Transport t = Transport.open(dst, slowURI)) { + t.setTimeout(1); + TransportException expected = assertThrows(TransportException.class, + () -> t.fetch(NullProgressMonitor.INSTANCE, + mirror(master))); + assertTrue("Unexpected exception message: " + expected.toString(), + expected.getMessage().contains("time")); + } + } + + @Test + public void testTimeoutExpiredWithAuth() throws Exception { + try (Repository dst = createBareRepository(); + Transport t = Transport.open(dst, slowAuthURI)) { + t.setTimeout(1); + t.setCredentialsProvider(testCredentials); + TransportException expected = assertThrows(TransportException.class, + () -> t.fetch(NullProgressMonitor.INSTANCE, + mirror(master))); + assertTrue("Unexpected exception message: " + expected.toString(), + expected.getMessage().contains("time")); + assertFalse("Unexpected exception message: " + expected.toString(), + expected.getMessage().contains("auth")); + } + } + @Test public void testInitialClone_Small() throws Exception { try (Repository dst = createBareRepository(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java index c3c1f2a40..16169f028 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java @@ -39,11 +39,13 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.io.InterruptedIOException; import java.io.OutputStream; import java.net.HttpCookie; import java.net.MalformedURLException; import java.net.Proxy; import java.net.ProxySelector; +import java.net.SocketException; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; @@ -573,6 +575,14 @@ private HttpConnection connect(String service) } } catch (NotSupportedException | TransportException e) { throw e; + } catch (InterruptedIOException e) { + // Timeout!? Don't try other authentication methods. + throw new TransportException(uri, MessageFormat.format( + JGitText.get().connectionTimeOut, u.getHost()), e); + } catch (SocketException e) { + // Nothing on other end, timeout, connection reset, ... + throw new TransportException(uri, + JGitText.get().connectionFailed, e); } catch (SSLHandshakeException e) { handleSslFailure(e); continue; // Re-try @@ -1501,6 +1511,10 @@ void sendRequest() throws IOException { } catch (SSLHandshakeException e) { handleSslFailure(e); continue; // Re-try + } catch (SocketException | InterruptedIOException e) { + // Timeout!? Must propagate; don't try other authentication + // methods. + throw e; } catch (IOException e) { if (authenticator == null || authMethod .getType() != HttpAuthMethod.Type.NONE) {