From 3f828867740d1b9f2c7dadc3df33014d120b666b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 11 Mar 2016 11:31:10 +0100 Subject: [PATCH] Issue #266 (jetty-client redirection process is aborted if redirect response have corrupt body) Fixed by disabling content decode, since we are discarding the content anway. --- .../jetty/client/RedirectProtocolHandler.java | 10 ++ .../jetty/client/HttpClientRedirectTest.java | 97 ++++++++++++++----- 2 files changed, 83 insertions(+), 24 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/RedirectProtocolHandler.java b/jetty-client/src/main/java/org/eclipse/jetty/client/RedirectProtocolHandler.java index f6c8018e34f..5a2241294cf 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/RedirectProtocolHandler.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/RedirectProtocolHandler.java @@ -21,6 +21,8 @@ package org.eclipse.jetty.client; 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.http.HttpField; +import org.eclipse.jetty.http.HttpHeader; /** *

A protocol handler that handles redirect status codes 301, 302, 303, 307 and 308.

@@ -54,6 +56,14 @@ public class RedirectProtocolHandler extends Response.Listener.Adapter implement return this; } + @Override + public boolean onHeader(Response response, HttpField field) + { + // Avoid that the content is decoded, which could generate + // errors, since we are discarding the content anyway. + return field.getHeader() != HttpHeader.CONTENT_ENCODING; + } + @Override public void onComplete(Result 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 10052b936ba..667e291cb9b 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 @@ -22,6 +22,7 @@ import java.io.IOException; import java.net.URLDecoder; import java.nio.ByteBuffer; import java.nio.channels.UnresolvedAddressException; +import java.nio.charset.StandardCharsets; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -44,7 +45,6 @@ import org.eclipse.jetty.toolchain.test.IO; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.hamcrest.Matchers; import org.junit.Assert; -import org.junit.Before; import org.junit.Ignore; import org.junit.Test; @@ -55,15 +55,11 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest super(sslContextFactory); } - @Before - public void prepare() throws Exception - { - start(new RedirectHandler()); - } - @Test public void test_303() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .path("/303/localhost/done") @@ -77,6 +73,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void test_303_302() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .path("/303/localhost/302/localhost/done") @@ -90,6 +88,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void test_303_302_OnDifferentDestinations() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .path("/303/127.0.0.1/302/localhost/done") @@ -103,6 +103,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void test_301() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .method(HttpMethod.HEAD) @@ -117,6 +119,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void test_301_WithWrongMethod() throws Exception { + start(new RedirectHandler()); + try { client.newRequest("localhost", connector.getLocalPort()) @@ -140,6 +144,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void test_307_WithRequestContent() throws Exception { + start(new RedirectHandler()); + byte[] data = new byte[]{0, 1, 2, 3, 4, 5, 6, 7}; ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) @@ -157,6 +163,7 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void testMaxRedirections() throws Exception { + start(new RedirectHandler()); client.setMaxRedirects(1); try @@ -181,6 +188,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void test_303_WithConnectionClose_WithBigRequest() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .path("/303/localhost/done?close=true") @@ -194,6 +203,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void testDontFollowRedirects() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .followRedirects(false) @@ -208,6 +219,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void testRelativeLocation() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .path("/303/localhost/done?relative=true") @@ -221,6 +234,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void testAbsoluteURIPathWithSpaces() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .path("/303/localhost/a+space?decode=true") @@ -234,6 +249,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void testRelativeURIPathWithSpaces() throws Exception { + start(new RedirectHandler()); + Response response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .path("/303/localhost/a+space?relative=true&decode=true") @@ -247,7 +264,6 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void testRedirectWithWrongScheme() throws Exception { - dispose(); start(new AbstractHandler() { @Override @@ -264,14 +280,10 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest .scheme(scheme) .path("/path") .timeout(5, TimeUnit.SECONDS) - .send(new Response.CompleteListener() + .send(result -> { - @Override - public void onComplete(Result result) - { - Assert.assertTrue(result.isFailed()); - latch.countDown(); - } + Assert.assertTrue(result.isFailed()); + latch.countDown(); }); Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); } @@ -281,6 +293,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest public void testRedirectFailed() throws Exception { // TODO this test is failing with timout after an ISP upgrade?? DNS dependent? + start(new RedirectHandler()); + try { client.newRequest("localhost", connector.getLocalPort()) @@ -370,6 +384,7 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest @Test public void testHttpRedirector() throws Exception { + start(new RedirectHandler()); final HttpRedirector redirector = new HttpRedirector(client); org.eclipse.jetty.client.api.Request request1 = client.newRequest("localhost", connector.getLocalPort()) @@ -390,20 +405,52 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest Assert.assertTrue(redirector.isRedirect(response2)); final CountDownLatch latch = new CountDownLatch(1); - redirector.redirect(request2, response2, new Response.CompleteListener() + redirector.redirect(request2, response2, r -> { - @Override - public void onComplete(Result result) - { - Response response3 = result.getResponse(); - Assert.assertEquals(200, response3.getStatus()); - Assert.assertFalse(redirector.isRedirect(response3)); - latch.countDown(); - } + Response response3 = r.getResponse(); + Assert.assertEquals(200, response3.getStatus()); + Assert.assertFalse(redirector.isRedirect(response3)); + latch.countDown(); }); Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); } + @Test + public void testRedirectWithCorruptedBody() throws Exception + { + byte[] bytes = "ok".getBytes(StandardCharsets.UTF_8); + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + if (target.startsWith("/redirect")) + { + response.setStatus(HttpStatus.SEE_OTHER_303); + response.setHeader(HttpHeader.LOCATION.asString(), scheme + "://localhost:" + connector.getLocalPort() + "/ok"); + // Say that we send gzipped content, but actually don't. + response.setHeader(HttpHeader.CONTENT_ENCODING.asString(), "gzip"); + response.getOutputStream().write("redirect".getBytes(StandardCharsets.UTF_8)); + } + else + { + response.setStatus(HttpStatus.OK_200); + response.getOutputStream().write(bytes); + } + } + }); + + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(scheme) + .path("/redirect") + .timeout(5, TimeUnit.SECONDS) + .send(); + + Assert.assertEquals(200, response.getStatus()); + Assert.assertArrayEquals(bytes, response.getContent()); + } + private void testSameMethodRedirect(final HttpMethod method, int redirectCode) throws Exception { testMethodRedirect(method, method, redirectCode); @@ -416,6 +463,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest private void testMethodRedirect(final HttpMethod requestMethod, final HttpMethod redirectMethod, int redirectCode) throws Exception { + start(new RedirectHandler()); + final AtomicInteger passes = new AtomicInteger(); client.getRequestListeners().add(new org.eclipse.jetty.client.api.Request.Listener.Adapter() {