HTTPCLIENT-1344: Userinfo credentials in URI should not default to preemptive BASIC authentication

HTTPCLIENT-1345: Useinfo credentials ignored in redirect location header

git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1483383 13f79535-47bb-0310-9956-ffa450edef68

RELEASE_NOTES.txt

@@ -1,6 +1,13 @@
 Changes since release 4.3 BETA1
 -------------------
 
+* [HTTPCLIENT-1344] Userinfo credentials in URI should not default to preemptive BASIC 
+  authentication. 
+  Contributed by Oleg Kalnichevski <olegk at apache.org>
+
+* [HTTPCLIENT-1345] Useinfo credentials ignored in redirect location header. 
+  Contributed by Oleg Kalnichevski <olegk at apache.org>
+
 * [HTTPCLIENT-1294] HttpClient to rewrite host name of the redirect location URI in order 
   to avoid circular redirect exception due to host name case mismatch. 
   Contributed by Oleg Kalnichevski <olegk at apache.org>

httpclient/src/main/java/org/apache/http/client/utils/URIUtils.java

@@ -131,6 +131,9 @@ public class URIUtils {
             final HttpHost target,
             final boolean dropFragment) throws URISyntaxException {
         Args.notNull(uri, "URI");
+        if (uri.isOpaque()) {
+            return uri;
+        }
         final URIBuilder uribuilder = new URIBuilder(uri);
         if (target != null) {
             uribuilder.setScheme(target.getSchemeName());
@@ -174,14 +177,20 @@ public class URIUtils {
      */
     public static URI rewriteURI(final URI uri) throws URISyntaxException {
         Args.notNull(uri, "URI");
+        if (uri.isOpaque()) {
+            return uri;
+        }
         final URIBuilder uribuilder = new URIBuilder(uri);
-        uribuilder.setFragment(null).setUserInfo(null);
+        if (uribuilder.getUserInfo() != null) {
+            uribuilder.setUserInfo(null);
+        }
         if (TextUtils.isEmpty(uribuilder.getPath())) {
             uribuilder.setPath("/");
         }
         if (uribuilder.getHost() != null) {
             uribuilder.setHost(uribuilder.getHost().toLowerCase(Locale.ENGLISH));
         }
+        uribuilder.setFragment(null);
         return uribuilder.build();
     }
 

httpclient/src/main/java/org/apache/http/impl/client/DefaultRedirectStrategy.java

@@ -144,8 +144,6 @@ public class DefaultRedirectStrategy implements RedirectStrategy {
         // rfc2616 demands the location value be a complete URI
         // Location       = "Location" ":" absoluteURI
         try {
-            // Drop fragment
-            uri = URIUtils.rewriteURI(uri);
             if (!uri.isAbsolute()) {
                 if (!config.isRelativeRedirectsAllowed()) {
                     throw new ProtocolException("Relative redirect location '"
@@ -191,6 +189,7 @@ public class DefaultRedirectStrategy implements RedirectStrategy {
             if (TextUtils.isEmpty(path)) {
                 b.setPath("/");
             }
+            b.setFragment(null);
             return b.build();
         } catch (final URISyntaxException ex) {
             throw new ProtocolException("Invalid redirect URI: " + location, ex);

httpclient/src/main/java/org/apache/http/impl/client/InternalHttpClient.java

@@ -50,7 +50,6 @@ import org.apache.http.client.methods.HttpExecutionAware;
 import org.apache.http.client.methods.HttpRequestWrapper;
 import org.apache.http.client.params.ClientPNames;
 import org.apache.http.client.params.HttpClientParamConfig;
-import org.apache.http.client.protocol.ClientContext;
 import org.apache.http.client.protocol.HttpClientContext;
 import org.apache.http.config.Lookup;
 import org.apache.http.conn.ClientConnectionManager;
@@ -126,26 +125,26 @@ class InternalHttpClient extends CloseableHttpClient {
     }
 
     private void setupContext(final HttpClientContext context) {
-        if (context.getAttribute(ClientContext.TARGET_AUTH_STATE) == null) {
+        if (context.getAttribute(HttpClientContext.TARGET_AUTH_STATE) == null) {
-            context.setAttribute(ClientContext.TARGET_AUTH_STATE, new AuthState());
+            context.setAttribute(HttpClientContext.TARGET_AUTH_STATE, new AuthState());
         }
-        if (context.getAttribute(ClientContext.PROXY_AUTH_STATE) == null) {
+        if (context.getAttribute(HttpClientContext.PROXY_AUTH_STATE) == null) {
-            context.setAttribute(ClientContext.PROXY_AUTH_STATE, new AuthState());
+            context.setAttribute(HttpClientContext.PROXY_AUTH_STATE, new AuthState());
         }
-        if (context.getAttribute(ClientContext.AUTHSCHEME_REGISTRY) == null) {
+        if (context.getAttribute(HttpClientContext.AUTHSCHEME_REGISTRY) == null) {
-            context.setAttribute(ClientContext.AUTHSCHEME_REGISTRY, this.authSchemeRegistry);
+            context.setAttribute(HttpClientContext.AUTHSCHEME_REGISTRY, this.authSchemeRegistry);
         }
-        if (context.getAttribute(ClientContext.COOKIESPEC_REGISTRY) == null) {
+        if (context.getAttribute(HttpClientContext.COOKIESPEC_REGISTRY) == null) {
-            context.setAttribute(ClientContext.COOKIESPEC_REGISTRY, this.cookieSpecRegistry);
+            context.setAttribute(HttpClientContext.COOKIESPEC_REGISTRY, this.cookieSpecRegistry);
         }
-        if (context.getAttribute(ClientContext.COOKIE_STORE) == null) {
+        if (context.getAttribute(HttpClientContext.COOKIE_STORE) == null) {
-            context.setAttribute(ClientContext.COOKIE_STORE, this.cookieStore);
+            context.setAttribute(HttpClientContext.COOKIE_STORE, this.cookieStore);
         }
-        if (context.getAttribute(ClientContext.CREDS_PROVIDER) == null) {
+        if (context.getAttribute(HttpClientContext.CREDS_PROVIDER) == null) {
-            context.setAttribute(ClientContext.CREDS_PROVIDER, this.credentialsProvider);
+            context.setAttribute(HttpClientContext.CREDS_PROVIDER, this.credentialsProvider);
         }
-        if (context.getAttribute(ClientContext.REQUEST_CONFIG) == null) {
+        if (context.getAttribute(HttpClientContext.REQUEST_CONFIG) == null) {
-            context.setAttribute(ClientContext.REQUEST_CONFIG, this.defaultConfig);
+            context.setAttribute(HttpClientContext.REQUEST_CONFIG, this.defaultConfig);
         }
     }
 

httpclient/src/main/java/org/apache/http/impl/execchain/ProtocolExec.java

@@ -38,8 +38,9 @@ import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
 import org.apache.http.ProtocolException;
 import org.apache.http.annotation.Immutable;
-import org.apache.http.auth.AuthState;
+import org.apache.http.auth.AuthScope;
 import org.apache.http.auth.UsernamePasswordCredentials;
+import org.apache.http.client.CredentialsProvider;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpExecutionAware;
 import org.apache.http.client.methods.HttpRequestWrapper;
@@ -48,7 +49,6 @@ import org.apache.http.client.params.ClientPNames;
 import org.apache.http.client.protocol.HttpClientContext;
 import org.apache.http.client.utils.URIUtils;
 import org.apache.http.conn.routing.HttpRoute;
-import org.apache.http.impl.auth.BasicScheme;
 import org.apache.http.params.HttpParams;
 import org.apache.http.protocol.HttpProcessor;
 import org.apache.http.util.Args;
@@ -107,18 +107,23 @@ public class ProtocolExec implements ClientExecChain {
         Args.notNull(request, "HTTP request");
         Args.notNull(context, "HTTP context");
 
-        // Get user info from the URI
+        final HttpRequest original = request.getOriginal();
-        final AuthState targetAuthState = context.getTargetAuthState();
+        URI uri = null;
-        if (targetAuthState != null) {
+        if (original instanceof HttpUriRequest) {
-            final URI requestURI = request.getURI();
+            uri = ((HttpUriRequest) original).getURI();
-            if (requestURI != null) {
+        } else {
-                final String userinfo = requestURI.getUserInfo();
+            final String uriString = original.getRequestLine().getUri();
-                if (userinfo != null) {
+            try {
-                    targetAuthState.update(new BasicScheme(), new UsernamePasswordCredentials(
+                uri = URI.create(uriString);
-                        userinfo));
+            } catch (final IllegalArgumentException ex) {
+                if (this.log.isDebugEnabled()) {
+                    this.log.debug("Unable to parse '" + uriString + "' as a valid URI; " +
+                        "request URI and Host header may be inconsistent", ex);
                 }
             }
+
         }
+        request.setURI(uri);
 
         // Re-write request URI if needed
         rewriteRequestURI(request, route);
@@ -141,22 +146,6 @@ public class ProtocolExec implements ClientExecChain {
         if (virtualHost != null) {
             target = virtualHost;
         } else {
-            final HttpRequest original = request.getOriginal();
-            URI uri = null;
-            if (original instanceof HttpUriRequest) {
-                uri = ((HttpUriRequest) original).getURI();
-            } else {
-                final String uriString = original.getRequestLine().getUri();
-                try {
-                    uri = URI.create(uriString);
-                } catch (final IllegalArgumentException ex) {
-                    if (this.log.isDebugEnabled()) {
-                        this.log.debug("Unable to parse '" + uriString + "' as a valid URI; " +
-                            "request URI and Host header may be inconsistent", ex);
-                    }
-                }
-
-            }
             if (uri != null && uri.isAbsolute() && uri.getHost() != null) {
                 target = new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme());
             }
@@ -165,6 +154,17 @@ public class ProtocolExec implements ClientExecChain {
             target = route.getTargetHost();
         }
 
+        // Get user info from the URI
+        if (uri != null) {
+            final String userinfo = uri.getUserInfo();
+            if (userinfo != null) {
+                final CredentialsProvider credsProvider = context.getCredentialsProvider();
+                credsProvider.setCredentials(
+                        new AuthScope(target),
+                        new UsernamePasswordCredentials(userinfo));
+            }
+        }
+
         // Run request protocol interceptors
         context.setAttribute(HttpClientContext.HTTP_TARGET_HOST, target);
         context.setAttribute(HttpClientContext.HTTP_ROUTE, route);

httpclient/src/test/java/org/apache/http/client/utils/TestURIUtils.java

@@ -71,6 +71,8 @@ public class TestURIUtils {
                 URI.create("http://thathost")).toString());
         Assert.assertEquals("http://thathost/", URIUtils.rewriteURI(
                 URI.create("http://ThatHost")).toString());
+        Assert.assertEquals("http://That_Host/", URIUtils.rewriteURI(
+                URI.create("http://That_Host")).toString());
     }
 
     @Test

httpclient/src/test/java/org/apache/http/impl/client/integration/TestClientAuthentication.java

@@ -34,6 +34,7 @@ import org.apache.http.Consts;
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpException;
 import org.apache.http.HttpHost;
+import org.apache.http.HttpInetConnection;
 import org.apache.http.HttpRequest;
 import org.apache.http.HttpResponse;
 import org.apache.http.HttpStatus;
@@ -60,8 +61,10 @@ import org.apache.http.localserver.BasicAuthTokenExtractor;
 import org.apache.http.localserver.LocalTestServer;
 import org.apache.http.localserver.RequestBasicAuth;
 import org.apache.http.localserver.ResponseBasicUnauthorized;
+import org.apache.http.message.BasicHeader;
 import org.apache.http.protocol.HTTP;
 import org.apache.http.protocol.HttpContext;
+import org.apache.http.protocol.HttpCoreContext;
 import org.apache.http.protocol.HttpExpectationVerifier;
 import org.apache.http.protocol.HttpProcessor;
 import org.apache.http.protocol.HttpProcessorBuilder;
@@ -422,6 +425,43 @@ public class TestClientAuthentication extends IntegrationTestBase {
         EntityUtils.consume(entity);
     }
 
+    private static class RedirectHandler implements HttpRequestHandler {
+
+        public RedirectHandler() {
+            super();
+        }
+
+        public void handle(
+                final HttpRequest request,
+                final HttpResponse response,
+                final HttpContext context) throws HttpException, IOException {
+            final HttpInetConnection conn = (HttpInetConnection) context.getAttribute(HttpCoreContext.HTTP_CONNECTION);
+            final String localhost = conn.getLocalAddress().getHostName();
+            final int port = conn.getLocalPort();
+            response.setStatusCode(HttpStatus.SC_MOVED_PERMANENTLY);
+            response.addHeader(new BasicHeader("Location",
+                    "http://test:test@" + localhost + ":" + port + "/"));
+        }
+
+    }
+
+    @Test
+    public void testAuthenticationUserinfoInRedirectSuccess() throws Exception {
+        this.localServer.register("*", new AuthHandler());
+        this.localServer.register("/thatway", new RedirectHandler());
+
+        final HttpHost target = getServerHttp();
+        final HttpGet httpget = new HttpGet("http://" +  target.toHostString() + "/thatway");
+
+        this.httpclient = HttpClients.custom().build();
+
+        final HttpResponse response = this.httpclient.execute(getServerHttp(), httpget);
+        final HttpEntity entity = response.getEntity();
+        Assert.assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode());
+        Assert.assertNotNull(entity);
+        EntityUtils.consume(entity);
+    }
+
     static class CountingAuthHandler implements HttpRequestHandler {
 
         private final AtomicLong count;

httpclient/src/test/java/org/apache/http/impl/client/integration/TestRedirects.java

@@ -37,7 +37,6 @@ import org.apache.http.HttpRequest;
 import org.apache.http.HttpResponse;
 import org.apache.http.HttpStatus;
 import org.apache.http.ProtocolException;
-import org.apache.http.ProtocolVersion;
 import org.apache.http.client.CircularRedirectException;
 import org.apache.http.client.ClientProtocolException;
 import org.apache.http.client.CookieStore;
@@ -92,19 +91,18 @@ public class TestRedirects extends IntegrationTestBase {
             final HttpInetConnection conn = (HttpInetConnection) context.getAttribute(HttpCoreContext.HTTP_CONNECTION);
             final String localhost = conn.getLocalAddress().getHostName();
             final int port = conn.getLocalPort();
-            final ProtocolVersion ver = request.getRequestLine().getProtocolVersion();
             final String uri = request.getRequestLine().getUri();
             if (uri.equals("/oldlocation/")) {
-                response.setStatusLine(ver, this.statuscode);
+                response.setStatusCode(this.statuscode);
                 response.addHeader(new BasicHeader("Location",
                         "http://" + localhost + ":" + port + "/newlocation/"));
                 response.addHeader(new BasicHeader("Connection", "close"));
             } else if (uri.equals("/newlocation/")) {
-                response.setStatusLine(ver, HttpStatus.SC_OK);
+                response.setStatusCode(HttpStatus.SC_OK);
                 final StringEntity entity = new StringEntity("Successful redirect");
                 response.setEntity(entity);
             } else {
-                response.setStatusLine(ver, HttpStatus.SC_NOT_FOUND);
+                response.setStatusCode(HttpStatus.SC_NOT_FOUND);
             }
         }
 
@@ -120,16 +118,15 @@ public class TestRedirects extends IntegrationTestBase {
                 final HttpRequest request,
                 final HttpResponse response,
                 final HttpContext context) throws HttpException, IOException {
-            final ProtocolVersion ver = request.getRequestLine().getProtocolVersion();
             final String uri = request.getRequestLine().getUri();
             if (uri.startsWith("/circular-oldlocation")) {
-                response.setStatusLine(ver, HttpStatus.SC_MOVED_TEMPORARILY);
+                response.setStatusCode(HttpStatus.SC_MOVED_TEMPORARILY);
                 response.addHeader(new BasicHeader("Location", "/circular-location2"));
             } else if (uri.startsWith("/circular-location2")) {
-                response.setStatusLine(ver, HttpStatus.SC_MOVED_TEMPORARILY);
+                response.setStatusCode(HttpStatus.SC_MOVED_TEMPORARILY);
                 response.addHeader(new BasicHeader("Location", "/circular-oldlocation"));
             } else {
-                response.setStatusLine(ver, HttpStatus.SC_NOT_FOUND);
+                response.setStatusCode(HttpStatus.SC_NOT_FOUND);
             }
         }
     }
@@ -144,17 +141,16 @@ public class TestRedirects extends IntegrationTestBase {
                 final HttpRequest request,
                 final HttpResponse response,
                 final HttpContext context) throws HttpException, IOException {
-            final ProtocolVersion ver = request.getRequestLine().getProtocolVersion();
             final String uri = request.getRequestLine().getUri();
             if (uri.equals("/oldlocation/")) {
-                response.setStatusLine(ver, HttpStatus.SC_MOVED_TEMPORARILY);
+                response.setStatusCode(HttpStatus.SC_MOVED_TEMPORARILY);
                 response.addHeader(new BasicHeader("Location", "/relativelocation/"));
             } else if (uri.equals("/relativelocation/")) {
-                response.setStatusLine(ver, HttpStatus.SC_OK);
+                response.setStatusCode(HttpStatus.SC_OK);
                 final StringEntity entity = new StringEntity("Successful redirect");
                 response.setEntity(entity);
             } else {
-                response.setStatusLine(ver, HttpStatus.SC_NOT_FOUND);
+                response.setStatusCode(HttpStatus.SC_NOT_FOUND);
             }
         }
     }
@@ -169,17 +165,16 @@ public class TestRedirects extends IntegrationTestBase {
                 final HttpRequest request,
                 final HttpResponse response,
                 final HttpContext context) throws HttpException, IOException {
-            final ProtocolVersion ver = request.getRequestLine().getProtocolVersion();
             final String uri = request.getRequestLine().getUri();
             if (uri.equals("/test/oldlocation")) {
-                response.setStatusLine(ver, HttpStatus.SC_MOVED_TEMPORARILY);
+                response.setStatusCode(HttpStatus.SC_MOVED_TEMPORARILY);
                 response.addHeader(new BasicHeader("Location", "relativelocation"));
             } else if (uri.equals("/test/relativelocation")) {
-                response.setStatusLine(ver, HttpStatus.SC_OK);
+                response.setStatusCode(HttpStatus.SC_OK);
                 final StringEntity entity = new StringEntity("Successful redirect");
                 response.setEntity(entity);
             } else {
-                response.setStatusLine(ver, HttpStatus.SC_NOT_FOUND);
+                response.setStatusCode(HttpStatus.SC_NOT_FOUND);
             }
         }
     }
@@ -196,17 +191,16 @@ public class TestRedirects extends IntegrationTestBase {
                 final HttpRequest request,
                 final HttpResponse response,
                 final HttpContext context) throws HttpException, IOException {
-            final ProtocolVersion ver = request.getRequestLine().getProtocolVersion();
             final String uri = request.getRequestLine().getUri();
             if (uri.equals("/oldlocation/")) {
-                response.setStatusLine(ver, HttpStatus.SC_MOVED_TEMPORARILY);
+                response.setStatusCode(HttpStatus.SC_MOVED_TEMPORARILY);
                 response.addHeader(new BasicHeader("Location", url));
             } else if (uri.equals("/relativelocation/")) {
-                response.setStatusLine(ver, HttpStatus.SC_OK);
+                response.setStatusCode(HttpStatus.SC_OK);
                 final StringEntity entity = new StringEntity("Successful redirect");
                 response.setEntity(entity);
             } else {
-                response.setStatusLine(ver, HttpStatus.SC_NOT_FOUND);
+                response.setStatusCode(HttpStatus.SC_NOT_FOUND);
             }
         }
     }