From 651105c73b7d007b8e8123ef8260acb6579112b9 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 14 Feb 2013 21:31:57 +0100 Subject: [PATCH] 400848 - Redirect fails with non-encoded location URIs. --- .../eclipse/jetty/client/HttpConnection.java | 2 +- .../org/eclipse/jetty/client/HttpRequest.java | 2 +- .../jetty/client/RedirectProtocolHandler.java | 113 +++++++++++++----- .../jetty/client/HttpClientRedirectTest.java | 31 +++++ 4 files changed, 117 insertions(+), 31 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java index e291532df4c..5c5d487291d 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java @@ -131,8 +131,8 @@ public class HttpConnection extends AbstractConnection implements Connection HttpConversation conversation = client.getConversation(request.getConversationID(), true); HttpExchange exchange = new HttpExchange(conversation, this, request, listeners); - setExchange(exchange); conversation.getExchanges().offer(exchange); + setExchange(exchange); sender.send(exchange); } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java index c0c320eb255..fefd4b8b75c 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java @@ -82,7 +82,7 @@ public class HttpRequest implements Request scheme = uri.getScheme(); host = uri.getHost(); port = client.normalizePort(scheme, uri.getPort()); - path = uri.getPath(); + path = uri.getRawPath(); String query = uri.getRawQuery(); if (query != null) { 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 bd37ca3c50c..826ace6a797 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 @@ -19,7 +19,10 @@ package org.eclipse.jetty.client; import java.net.URI; +import java.net.URISyntaxException; import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; @@ -28,6 +31,14 @@ import org.eclipse.jetty.http.HttpMethod; public class RedirectProtocolHandler extends Response.Listener.Empty implements ProtocolHandler { + private static String SCHEME_REGEXP = "(^https?)"; + private static String AUTHORITY_REGEXP = "([^/\\?#]+)"; + // The location may be relative so the scheme://authority part may be missing + private static String DESTINATION_REGEXP = "(" + SCHEME_REGEXP + "://" + AUTHORITY_REGEXP + ")?"; + private static String PATH_REGEXP = "([^\\?#]*)"; + private static String QUERY_REGEXP = "([^#]*)"; + private static String FRAGMENT_REGEXP = "(.*)"; + private static Pattern URI_PATTERN = Pattern.compile(DESTINATION_REGEXP + PATH_REGEXP + QUERY_REGEXP + FRAGMENT_REGEXP); private static final String ATTRIBUTE = RedirectProtocolHandler.class.getName() + ".redirects"; private final HttpClient client; @@ -69,40 +80,47 @@ public class RedirectProtocolHandler extends Response.Listener.Empty implements String location = response.getHeaders().get("location"); if (location != null) { - URI newURI = URI.create(location); - if (!newURI.isAbsolute()) - newURI = request.getURI().resolve(newURI); - - int status = response.getStatus(); - switch (status) + URI newURI = sanitize(location); + if (newURI != null) { - case 301: + if (!newURI.isAbsolute()) + newURI = request.getURI().resolve(newURI); + + int status = response.getStatus(); + switch (status) { - if (request.getMethod() == HttpMethod.GET || request.getMethod() == HttpMethod.HEAD) + case 301: + { + if (request.getMethod() == HttpMethod.GET || request.getMethod() == HttpMethod.HEAD) + redirect(result, request.getMethod(), newURI); + else + fail(result, new HttpResponseException("HTTP protocol violation: received 301 for non GET or HEAD request", response)); + break; + } + case 302: + case 303: + { + // Redirect must be done using GET + redirect(result, HttpMethod.GET, newURI); + break; + } + case 307: + { + // Keep same method redirect(result, request.getMethod(), newURI); - else - fail(result, new HttpResponseException("HTTP protocol violation: received 301 for non GET or HEAD request", response)); - break; - } - case 302: - case 303: - { - // Redirect must be done using GET - redirect(result, HttpMethod.GET, newURI); - break; - } - case 307: - { - // Keep same method - redirect(result, request.getMethod(), newURI); - break; - } - default: - { - fail(result, new HttpResponseException("Unhandled HTTP status code " + status, response)); - break; + break; + } + default: + { + fail(result, new HttpResponseException("Unhandled HTTP status code " + status, response)); + break; + } } } + else + { + fail(result, new HttpResponseException("Malformed Location header " + location, response)); + } } else { @@ -115,6 +133,43 @@ public class RedirectProtocolHandler extends Response.Listener.Empty implements } } + private URI sanitize(String location) + { + // Redirects should be valid, absolute, URIs, with properly escaped paths and encoded + // query parameters. However, shit happens, and here we try our best to recover. + + try + { + // Direct hit first: if passes, we're good + return new URI(location); + } + catch (URISyntaxException x) + { + Matcher matcher = URI_PATTERN.matcher(location); + if (matcher.matches()) + { + String scheme = matcher.group(2); + String authority = matcher.group(3); + String path = matcher.group(4); + String query = matcher.group(5); + if (query.length() == 0) + query = null; + String fragment = matcher.group(6); + if (fragment.length() == 0) + fragment = null; + try + { + return new URI(scheme, authority, path, query, fragment); + } + catch (URISyntaxException xx) + { + // Give up + } + } + return null; + } + } + private void redirect(Result result, HttpMethod method, URI location) { final Request request = result.getRequest(); 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 2727c160b77..d3023966e26 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 @@ -19,6 +19,7 @@ package org.eclipse.jetty.client; import java.io.IOException; +import java.net.URLDecoder; import java.nio.ByteBuffer; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -211,6 +212,32 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest Assert.assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString())); } + @Test + public void testAbsoluteURIPathWithSpaces() throws Exception + { + Response response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(scheme) + .path("/303/localhost/a+space?decode=true") + .timeout(5, TimeUnit.SECONDS) + .send(); + Assert.assertNotNull(response); + Assert.assertEquals(200, response.getStatus()); + Assert.assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString())); + } + + @Test + public void testRelativeURIPathWithSpaces() throws Exception + { + Response response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(scheme) + .path("/303/localhost/a+space?relative=true&decode=true") + .timeout(5, TimeUnit.SECONDS) + .send(); + Assert.assertNotNull(response); + Assert.assertEquals(200, response.getStatus()); + Assert.assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString())); + } + private class RedirectHandler extends AbstractHandler { @Override @@ -228,6 +255,10 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest boolean relative = Boolean.parseBoolean(request.getParameter("relative")); String location = relative ? "" : request.getScheme() + "://" + host + ":" + request.getServerPort(); location += "/" + path; + + if (Boolean.parseBoolean(request.getParameter("decode"))) + location = URLDecoder.decode(location, "UTF-8"); + response.setHeader("Location", location); if (Boolean.parseBoolean(request.getParameter("close")))