From 9657be1c7258eab722b4c00e4a7969a779a9f6fe Mon Sep 17 00:00:00 2001 From: Konstantin Gribov Date: Tue, 13 Dec 2016 19:33:42 +0300 Subject: [PATCH] Fixes #1171 - Jetty-client throws NPE for request to IDN hosts only when `HttpClient#send(...)` is called. Added early check for null host when creating requests in HttpClient. Added test for incorrect IDN redirect. Signed-off-by: Konstantin Gribov Reviewed-by: Simone Bordet --- .../org/eclipse/jetty/client/HttpClient.java | 18 ++- .../jetty/client/HttpClientURITest.java | 124 ++++++++++++++++++ 2 files changed, 141 insertions(+), 1 deletion(-) 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 7114cd422cb..7e71b0016ce 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 @@ -468,7 +468,23 @@ public class HttpClient extends ContainerLifeCycle protected HttpRequest newHttpRequest(HttpConversation conversation, URI uri) { - return new HttpRequest(this, conversation, uri); + return new HttpRequest(this, conversation, checkHost(uri)); + } + + /** + * Check {@code uri} for non-null host + * + * @param uri to check for non-null host + * @return same {@code uri} if it's correct + * @throws IllegalArgumentException with message containing authority if {@code uri.getHost() == null} + */ + private URI checkHost(URI uri) + { + if (uri.getHost() == null) + { + throw new IllegalArgumentException(String.format("Invalid URI host: null (authority: %s)", uri.getRawAuthority())); + } + return uri; } /** diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java index 691d7840dd9..3f48d130f54 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java @@ -18,10 +18,20 @@ package org.eclipse.jetty.client; +import java.io.BufferedReader; import java.io.IOException; +import java.io.InputStreamReader; +import java.io.PrintWriter; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.ServerSocket; +import java.net.Socket; import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; import java.util.Locale; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -29,6 +39,8 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.client.api.Request; +import org.eclipse.jetty.http.HttpField; +import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.handler.AbstractHandler; @@ -63,6 +75,51 @@ public class HttpClientURITest extends AbstractHttpClientServerTest Assert.assertEquals(HttpStatus.OK_200, request.send().getStatus()); } + @Test(expected = IllegalArgumentException.class) + public void testIDNHost() throws Exception + { + startClient(); + client.newRequest("http://пример.рф"); // example.com-like host in IDN domain + } + + @Test(expected = IllegalArgumentException.class) + public void testIDNRedirect() throws Exception + { + String exampleHost = "http://пример.рф"; // example.com-like host in IDN domain + String incorrectlyDecoded = new String(exampleHost.getBytes(StandardCharsets.UTF_8), StandardCharsets.ISO_8859_1); + + IDNRedirectServer server = new IDNRedirectServer(exampleHost); + startClient(); + server.start(); + + ContentResponse response = client.newRequest("localhost", server.localPort()) + .timeout(5, TimeUnit.SECONDS) + .followRedirects(false) + .send(); + + HttpField location = response.getHeaders().getField(HttpHeader.LOCATION); + Assert.assertEquals(incorrectlyDecoded, location.getValue()); + + try + { + client.newRequest("localhost", server.localPort()) + .timeout(5, TimeUnit.SECONDS) + .followRedirects(true) + .send(); + } + catch (ExecutionException e) + { + if (e.getCause() instanceof IllegalArgumentException) + { + throw (IllegalArgumentException) e.getCause(); + } + } + finally + { + server.stop(); + } + } + @Test public void testPath() throws Exception { @@ -504,4 +561,71 @@ public class HttpClientURITest extends AbstractHttpClientServerTest Assert.assertEquals(HttpStatus.OK_200, response.getStatus()); } + + private static class IDNRedirectServer + { + private final Thread serverThread; + private final AtomicInteger port; + + private IDNRedirectServer(final String redirect) + { + this.port = new AtomicInteger(0); + this.serverThread = new Thread(new Runnable() + { + @Override + public void run() + { + try (ServerSocket serverSocket = new ServerSocket()) + { + serverSocket.bind(new InetSocketAddress(InetAddress.getLocalHost(), 0)); + port.set(serverSocket.getLocalPort()); + + while (!Thread.currentThread().isInterrupted()) + { + try (Socket clientSocket = serverSocket.accept(); + BufferedReader reader = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()))) + { + while (!reader.readLine().equals("")) + { + // ignore request + } + + try (PrintWriter writer = new PrintWriter(clientSocket.getOutputStream(), false)) + { + writer.append("HTTP/1.1 302 Found\r\n") + .append("Location: ").append(redirect).append("\r\n") + .append("Content-Length: 0\r\n") + .append("\r\n"); + writer.flush(); + } + } + } + } + catch (Exception e) + { + throw new RuntimeException(e); + } + } + }); + } + + public void start() + { + serverThread.start(); + while (port.get() == 0) + { + // await serverThread started + } + } + + public void stop() + { + serverThread.interrupt(); + } + + public int localPort() + { + return port.get(); + } + } }