diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java index 1388931e233..a51aea42b9b 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java @@ -23,6 +23,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -217,6 +218,8 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler path = request.getPath(); } Request newRequest = client.copyRequest(request, requestURI); + // Disable the timeout so that only the one from the initial request applies. + newRequest.timeout(0, TimeUnit.MILLISECONDS); if (path != null) newRequest.path(path); diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java index 7b9fa122f93..688ffbf569e 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java @@ -928,7 +928,7 @@ public class HttpClient extends ContainerLifeCycle } /** - * @return the max number of HTTP redirects that are followed + * @return the max number of HTTP redirects that are followed in a conversation * @see #setMaxRedirects(int) */ public int getMaxRedirects() @@ -937,7 +937,7 @@ public class HttpClient extends ContainerLifeCycle } /** - * @param maxRedirects the max number of HTTP redirects that are followed + * @param maxRedirects the max number of HTTP redirects that are followed in a conversation, or -1 for unlimited redirects * @see #setFollowRedirects(boolean) */ public void setMaxRedirects(int maxRedirects) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConversation.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConversation.java index a555d700965..3f57de2b9fc 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConversation.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConversation.java @@ -25,9 +25,13 @@ import java.util.concurrent.ConcurrentLinkedDeque; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.util.AttributesMap; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.Logger; public class HttpConversation extends AttributesMap { + private static final Logger LOG = Log.getLogger(HttpConversation.class); + private final Deque exchanges = new ConcurrentLinkedDeque<>(); private volatile List listeners; @@ -118,6 +122,7 @@ public class HttpConversation extends AttributesMap HttpExchange lastExchange = exchanges.peekLast(); if (firstExchange == lastExchange) { + // We don't have a conversation, just a single request. if (overrideListener != null) listeners.add(overrideListener); else @@ -125,13 +130,16 @@ public class HttpConversation extends AttributesMap } else { - // Order is important, we want to notify the last exchange first + // We have a conversation (e.g. redirect, authentication). + // Order is important, we want to notify the last exchange first. listeners.addAll(lastExchange.getResponseListeners()); if (overrideListener != null) listeners.add(overrideListener); else listeners.addAll(firstExchange.getResponseListeners()); } + if (LOG.isDebugEnabled()) + LOG.debug("Exchanges in conversation {}, override={}, listeners={}", exchanges.size(), overrideListener, listeners); this.listeners = listeners; } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java index 58fde88a489..44a14a6bb9b 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java @@ -490,8 +490,12 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest exchanges.size(), connectionPool); } - - // The TimeoutTask that expires when the next check of expiry is needed + + /** + * This class enforces the total timeout for exchanges that are still in the queue. + * The total timeout for exchanges that are not in the destination queue is enforced + * by {@link HttpChannel}. + */ private class TimeoutTask extends CyclicTimeout { private final AtomicLong nextTimeout = new AtomicLong(Long.MAX_VALUE); @@ -504,6 +508,9 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest @Override public void onTimeoutExpired() { + if (LOG.isDebugEnabled()) + LOG.debug("{} timeout expired", this); + nextTimeout.set(Long.MAX_VALUE); long now = System.nanoTime(); long nextExpiresAt = Long.MAX_VALUE; @@ -536,12 +543,16 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest if (timeoutAt != expiresAt) { long delay = expiresAt - System.nanoTime(); - if (LOG.isDebugEnabled()) - LOG.debug("Scheduled timeout in {} ms", TimeUnit.NANOSECONDS.toMillis(delay)); if (delay <= 0) + { onTimeoutExpired(); + } else + { schedule(delay, TimeUnit.NANOSECONDS); + if (LOG.isDebugEnabled()) + LOG.debug("{} scheduled timeout in {} ms", this, TimeUnit.NANOSECONDS.toMillis(delay)); + } } } } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRedirector.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRedirector.java index 24bdc74d8c6..9bd9f208a88 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRedirector.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRedirector.java @@ -21,8 +21,10 @@ package org.eclipse.jetty.client; import java.net.URI; import java.net.URISyntaxException; import java.util.List; +import java.util.Objects; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -101,11 +103,11 @@ public class HttpRedirector /** * Redirects the given {@code response}, blocking until the redirect is complete. * - * @param request the original request that triggered the redirect + * @param request the original request that triggered the redirect * @param response the response to the original request * @return a {@link Result} object containing the request to the redirected location and its response * @throws InterruptedException if the thread is interrupted while waiting for the redirect to complete - * @throws ExecutionException if the redirect failed + * @throws ExecutionException if the redirect failed * @see #redirect(Request, Response, Response.CompleteListener) */ public Result redirect(Request request, Response response) throws InterruptedException, ExecutionException @@ -144,7 +146,7 @@ public class HttpRedirector /** * Redirects the given {@code response} asynchronously. * - * @param request the original request that triggered the redirect + * @param request the original request that triggered the redirect * @param response the response to the original request * @param listener the listener that receives response events * @return the request to the redirected location @@ -157,6 +159,14 @@ public class HttpRedirector URI newURI = extractRedirectURI(response); if (newURI != null) { + boolean absolute = newURI.isAbsolute(); + URI uri = request.getURI(); + if (absolute && newURI.equals(uri) || + !absolute && Objects.equals(newURI.getPath(), uri.getPath())) + { + fail(request, response, new HttpResponseException("Redirect to same URI: " + location, response)); + return null; + } if (LOG.isDebugEnabled()) LOG.debug("Redirecting to {} (Location: {})", newURI, location); return redirect(request, response, listener, newURI); @@ -292,7 +302,8 @@ public class HttpRedirector Integer redirects = (Integer)conversation.getAttribute(ATTRIBUTE); if (redirects == null) redirects = 0; - if (redirects < client.getMaxRedirects()) + int maxRedirects = client.getMaxRedirects(); + if (maxRedirects < 0 || redirects < maxRedirects) { ++redirects; conversation.setAttribute(ATTRIBUTE, redirects); @@ -310,19 +321,17 @@ public class HttpRedirector try { Request redirect = client.copyRequest(httpRequest, location); + // Disable the timeout so that only the one from the initial request applies. + redirect.timeout(0, TimeUnit.MILLISECONDS); // Use given method redirect.method(method); - redirect.onRequestBegin(new Request.BeginListener() + redirect.onRequestBegin(request -> { - @Override - public void onBegin(Request redirect) - { - Throwable cause = httpRequest.getAbortCause(); - if (cause != null) - redirect.abort(cause); - } + Throwable cause = httpRequest.getAbortCause(); + if (cause != null) + request.abort(cause); }); redirect.send(listener); diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java b/jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java index ced60b57239..7a3121a755b 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java @@ -26,7 +26,6 @@ import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.api.Result; import org.eclipse.jetty.io.CyclicTimeout; -import org.eclipse.jetty.util.component.Destroyable; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.thread.Scheduler; @@ -47,7 +46,7 @@ public class TimeoutCompleteListener extends CyclicTimeout implements Response.C { Request request = this.request.getAndSet(null); if (LOG.isDebugEnabled()) - LOG.debug("Total timeout {} ms elapsed for {}", request.getTimeout(), request); + LOG.debug("Total timeout {} ms elapsed for {} on {}", request.getTimeout(), request, this); if (request != null) request.abort(new TimeoutException("Total timeout " + request.getTimeout() + " ms elapsed")); } @@ -60,7 +59,7 @@ public class TimeoutCompleteListener extends CyclicTimeout implements Response.C { boolean cancelled = cancel(); if (LOG.isDebugEnabled()) - LOG.debug("Cancelled ({}) timeout for {}", cancelled, request); + LOG.debug("Cancelled ({}) timeout for {} on {}", cancelled, request, this); } } @@ -69,12 +68,16 @@ public class TimeoutCompleteListener extends CyclicTimeout implements Response.C if (this.request.compareAndSet(null, request)) { long delay = timeoutAt - System.nanoTime(); - if (LOG.isDebugEnabled()) - LOG.debug("Scheduled timeout in {} ms for {}", TimeUnit.NANOSECONDS.toMillis(delay), request); if (delay <= 0) + { onTimeoutExpired(); + } else + { schedule(delay, TimeUnit.NANOSECONDS); + if (LOG.isDebugEnabled()) + LOG.debug("Scheduled timeout in {} ms for {} on {}", TimeUnit.NANOSECONDS.toMillis(delay), request, this); + } } } } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/api/Request.java b/jetty-client/src/main/java/org/eclipse/jetty/client/api/Request.java index 97bcf4c9014..09f35d9ec12 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/api/Request.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/api/Request.java @@ -261,12 +261,14 @@ public interface Request Request idleTimeout(long timeout, TimeUnit unit); /** - * @return the total timeout for this request, in milliseconds + * @return the total timeout for this request, in milliseconds; + * zero or negative if the timeout is disabled */ long getTimeout(); /** - * @param timeout the total timeout for the request/response conversation + * @param timeout the total timeout for the request/response conversation; + * use zero or a negative value to disable the timeout * @param unit the timeout unit * @return this request object */ diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java index 5e3307746a2..672b6272501 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java @@ -44,9 +44,11 @@ import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.api.Response.Listener; import org.eclipse.jetty.client.api.Result; +import org.eclipse.jetty.client.util.AbstractAuthentication; import org.eclipse.jetty.client.util.BasicAuthentication; import org.eclipse.jetty.client.util.DeferredContentProvider; import org.eclipse.jetty.client.util.DigestAuthentication; +import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.security.Authenticator; import org.eclipse.jetty.security.ConstraintMapping; @@ -57,7 +59,6 @@ import org.eclipse.jetty.security.authentication.BasicAuthenticator; import org.eclipse.jetty.security.authentication.DigestAuthenticator; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Server; -import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.IO; @@ -67,6 +68,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ArgumentsSource; +import static org.eclipse.jetty.client.api.Authentication.ANY_REALM; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalToIgnoringCase; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -137,7 +139,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest { startBasic(scenario, new EmptyServerHandler()); URI uri = URI.create(scenario.getScheme() + "://localhost:" + connector.getLocalPort()); - test_Authentication(scenario, new BasicAuthentication(uri, Authentication.ANY_REALM, "basic", "basic")); + test_Authentication(scenario, new BasicAuthentication(uri, ANY_REALM, "basic", "basic")); } @ParameterizedTest @@ -155,7 +157,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest { startDigest(scenario, new EmptyServerHandler()); URI uri = URI.create(scenario.getScheme() + "://localhost:" + connector.getLocalPort()); - test_Authentication(scenario, new DigestAuthentication(uri, Authentication.ANY_REALM, "digest", "digest")); + test_Authentication(scenario, new DigestAuthentication(uri, ANY_REALM, "digest", "digest")); } private void test_Authentication(final Scenario scenario, Authentication authentication) throws Exception @@ -227,16 +229,19 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest @ArgumentsSource(ScenarioProvider.class) public void test_BasicAuthentication_ThenRedirect(Scenario scenario) throws Exception { - startBasic(scenario, new AbstractHandler() + startBasic(scenario, new EmptyServerHandler() { private final AtomicInteger requests = new AtomicInteger(); @Override - public void handle(String target, org.eclipse.jetty.server.Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException + protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { - baseRequest.setHandled(true); - if (requests.incrementAndGet() == 1) - response.sendRedirect(URIUtil.newURI(scenario.getScheme(), request.getServerName(), request.getServerPort(), request.getRequestURI(), null)); + int r = requests.incrementAndGet(); + if (r == 1) + { + String path = request.getRequestURI() + "/" + r; + response.sendRedirect(URIUtil.newURI(scenario.getScheme(), request.getServerName(), request.getServerPort(), path, null)); + } } }); @@ -269,12 +274,11 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest @ArgumentsSource(ScenarioProvider.class) public void test_Redirect_ThenBasicAuthentication(Scenario scenario) throws Exception { - startBasic(scenario, new AbstractHandler() + startBasic(scenario, new EmptyServerHandler() { @Override - public void handle(String target, org.eclipse.jetty.server.Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException + protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { - baseRequest.setHandled(true); if (request.getRequestURI().endsWith("/redirect")) response.sendRedirect(URIUtil.newURI(scenario.getScheme(), request.getServerName(), request.getServerPort(), "/secure", null)); } @@ -571,61 +575,57 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest assertTrue(resultLatch.await(5, TimeUnit.SECONDS)); } - private static class GeneratingContentProvider implements ContentProvider + @ParameterizedTest + @ArgumentsSource(ScenarioProvider.class) + public void test_InfiniteAuthentication(Scenario scenario) throws Exception { - private static final ByteBuffer DONE = ByteBuffer.allocate(0); - - private final IntFunction generator; - - private GeneratingContentProvider(IntFunction generator) + String authType = "Authenticate"; + start(scenario, new EmptyServerHandler() { - this.generator = generator; - } - - @Override - public long getLength() - { - return -1; - } - - @Override - public boolean isReproducible() - { - return true; - } - - @Override - public Iterator iterator() - { - return new Iterator() + @Override + protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response) { - private int index; - public ByteBuffer current; + // Always reply with a 401 to see if the client + // can handle an infinite authentication loop. + response.setStatus(HttpStatus.UNAUTHORIZED_401); + response.setHeader(HttpHeader.WWW_AUTHENTICATE.asString(), authType); + } + }); - @Override - @SuppressWarnings("ReferenceEquality") - public boolean hasNext() + AuthenticationStore authenticationStore = client.getAuthenticationStore(); + URI uri = URI.create(scenario.getScheme() + "://localhost:" + connector.getLocalPort()); + authenticationStore.addAuthentication(new AbstractAuthentication(uri, Authentication.ANY_REALM) + { + @Override + public String getType() + { + return authType; + } + + @Override + public Result authenticate(Request request, ContentResponse response, HeaderInfo headerInfo, Attributes context) + { + return new Result() { - if (current == null) + @Override + public URI getURI() { - current = generator.apply(index++); - if (current == null) - current = DONE; + return uri; } - return current != DONE; - } - @Override - public ByteBuffer next() - { - ByteBuffer result = current; - current = null; - if (result == null) - throw new NoSuchElementException(); - return result; - } - }; - } + @Override + public void apply(Request request) + { + } + }; + } + }); + + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(scenario.getScheme()) + .send(); + + assertEquals(HttpStatus.UNAUTHORIZED_401, response.getStatus()); } @Test @@ -801,4 +801,61 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest assertEquals("thermostat", headerInfo.getParameter("realm")); assertEquals(headerInfo.getParameter("nonce"), "1523430383="); } + + private static class GeneratingContentProvider implements ContentProvider + { + private static final ByteBuffer DONE = ByteBuffer.allocate(0); + + private final IntFunction generator; + + private GeneratingContentProvider(IntFunction generator) + { + this.generator = generator; + } + + @Override + public long getLength() + { + return -1; + } + + @Override + public boolean isReproducible() + { + return true; + } + + @Override + public Iterator iterator() + { + return new Iterator() + { + private int index; + public ByteBuffer current; + + @Override + @SuppressWarnings("ReferenceEquality") + public boolean hasNext() + { + if (current == null) + { + current = generator.apply(index++); + if (current == null) + current = DONE; + } + return current != DONE; + } + + @Override + public ByteBuffer next() + { + ByteBuffer result = current; + current = null; + if (result == null) + throw new NoSuchElementException(); + return result; + } + }; + } + } } diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java index 9038ec52f95..f4eb3090a07 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java @@ -18,14 +18,6 @@ package org.eclipse.jetty.client; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; - import java.io.IOException; import java.net.URLDecoder; import java.nio.ByteBuffer; @@ -34,7 +26,9 @@ import java.nio.charset.StandardCharsets; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -48,13 +42,20 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.Request; -import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.toolchain.test.IO; import org.hamcrest.Matchers; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ArgumentsSource; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + public class HttpClientRedirectTest extends AbstractHttpClientServerTest { @ParameterizedTest @@ -128,14 +129,13 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest { start(scenario, new RedirectHandler()); - ExecutionException x = assertThrows(ExecutionException.class, ()->{ - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scenario.getScheme()) - .method(HttpMethod.DELETE) - .path("/301/localhost/done") - .timeout(5, TimeUnit.SECONDS) - .send(); - }); + ExecutionException x = assertThrows(ExecutionException.class, () -> + client.newRequest("localhost", connector.getLocalPort()) + .scheme(scenario.getScheme()) + .method(HttpMethod.DELETE) + .path("/301/localhost/done") + .timeout(5, TimeUnit.SECONDS) + .send()); HttpResponseException xx = (HttpResponseException)x.getCause(); Response response = xx.getResponse(); assertNotNull(response); @@ -170,13 +170,12 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest start(scenario, new RedirectHandler()); client.setMaxRedirects(1); - ExecutionException x = assertThrows(ExecutionException.class, ()->{ - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scenario.getScheme()) - .path("/303/localhost/302/localhost/done") - .timeout(5, TimeUnit.SECONDS) - .send(); - }); + ExecutionException x = assertThrows(ExecutionException.class, () -> + client.newRequest("localhost", connector.getLocalPort()) + .scheme(scenario.getScheme()) + .path("/303/localhost/302/localhost/done") + .timeout(5, TimeUnit.SECONDS) + .send()); HttpResponseException xx = (HttpResponseException)x.getCause(); Response response = xx.getResponse(); assertNotNull(response); @@ -269,12 +268,11 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @ArgumentsSource(ScenarioProvider.class) public void testRedirectWithWrongScheme(Scenario scenario) throws Exception { - start(scenario, new AbstractHandler() + start(scenario, new EmptyServerHandler() { @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) { - baseRequest.setHandled(true); response.setStatus(303); response.setHeader("Location", "ssh://localhost:" + connector.getLocalPort() + "/path"); } @@ -439,12 +437,11 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest public void testRedirectWithCorruptedBody(Scenario scenario) throws Exception { byte[] bytes = "ok".getBytes(StandardCharsets.UTF_8); - start(scenario, new AbstractHandler() + start(scenario, new EmptyServerHandler() { @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { - baseRequest.setHandled(true); if (target.startsWith("/redirect")) { response.setStatus(HttpStatus.SEE_OTHER_303); @@ -471,6 +468,64 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest assertArrayEquals(bytes, response.getContent()); } + @ParameterizedTest + @ArgumentsSource(ScenarioProvider.class) + public void testRedirectToSameURL(Scenario scenario) throws Exception + { + start(scenario, new EmptyServerHandler() + { + @Override + protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) + { + response.setStatus(HttpStatus.SEE_OTHER_303); + response.setHeader(HttpHeader.LOCATION.asString(), request.getRequestURI()); + } + }); + + ExecutionException x = assertThrows(ExecutionException.class, () -> + { + client.setMaxRedirects(-1); + client.newRequest("localhost", connector.getLocalPort()) + .scheme(scenario.getScheme()) + .timeout(5, TimeUnit.SECONDS) + .send(); + }); + assertThat(x.getCause(), Matchers.instanceOf(HttpResponseException.class)); + } + + @ParameterizedTest + @ArgumentsSource(ScenarioProvider.class) + public void testInfiniteRedirectLoopMustTimeout(Scenario scenario) throws Exception + { + AtomicLong counter = new AtomicLong(); + start(scenario, new EmptyServerHandler() + { + @Override + protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) + { + try + { + Thread.sleep(200); + response.setStatus(HttpStatus.SEE_OTHER_303); + response.setHeader(HttpHeader.LOCATION.asString(), "/" + counter.getAndIncrement()); + } + catch (InterruptedException x) + { + throw new RuntimeException(x); + } + } + }); + + assertThrows(TimeoutException.class, () -> + { + client.setMaxRedirects(-1); + client.newRequest("localhost", connector.getLocalPort()) + .scheme(scenario.getScheme()) + .timeout(1, TimeUnit.SECONDS) + .send(); + }); + } + private void testSameMethodRedirect(final Scenario scenario, final HttpMethod method, int redirectCode) throws Exception { testMethodRedirect(scenario, method, method, redirectCode); @@ -519,10 +574,10 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest assertEquals(200, response.getStatus()); } - private class RedirectHandler extends AbstractHandler + private class RedirectHandler extends EmptyServerHandler { @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { try { @@ -551,10 +606,6 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest // Echo content back IO.copy(request.getInputStream(), response.getOutputStream()); } - finally - { - baseRequest.setHandled(true); - } } } } diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java index b32c271ff45..a1483e769c6 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java @@ -18,16 +18,6 @@ package org.eclipse.jetty.client; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.instanceOf; -import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; - import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -106,6 +96,16 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ArgumentsSource; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + @ExtendWith(WorkDirExtension.class) public class HttpClientTest extends AbstractHttpClientServerTest {