reject redirect responses to CONNECT in (Async)ConnectExec

httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncConnectExec.java

@@ -475,7 +475,7 @@ public void consumeResponse(final HttpResponse response,
                     state.challenged = true;
                 } else {
                     state.challenged = false;
-                    if (status >= HttpStatus.SC_REDIRECTION) {
+                    if (status >= HttpStatus.SC_CLIENT_ERROR) {
                         state.tunnelRefused = true;
                         entityConsumerRef.set(asyncExecCallback.handleResponse(response, entityDetails));
                     } else if (status == HttpStatus.SC_OK) {

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ConnectExec.java

@@ -282,6 +282,10 @@ private ClassicHttpResponse createTunnelToTarget(
         final int status = response.getCode();
         if (status == HttpStatus.SC_OK) {
             context.setProtocolVersion(null);
+        } else if (status < HttpStatus.SC_CLIENT_ERROR) {
+            // Only a refusal is handed back. A redirect or other non-200 2xx from the proxy
+            // must not be taken for the target's response to the original request.
+            throw new HttpException("Unexpected response to CONNECT request: " + new StatusLine(response));
         } else {
             final HttpEntity entity = response.getEntity();
             if (entity != null) {

httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncConnectExec.java

@@ -39,9 +39,12 @@
 import org.apache.hc.core5.concurrent.CancellableDependency;
 import org.apache.hc.core5.concurrent.FutureCallback;
 import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpException;
 import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.hc.core5.http.HttpHost;
 import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.message.BasicHttpResponse;
 import org.apache.hc.core5.http.nio.AsyncClientExchangeHandler;
 import org.apache.hc.core5.http.nio.RequestChannel;
 import org.apache.hc.core5.http.protocol.HttpProcessor;
@@ -77,15 +80,11 @@ void setup() {
     }
 
     /**
-     * Drives the connect exec through a secure proxy tunnel and returns the {@code CONNECT} request
-     * that the tunnelling exchange handler produces.
+     * Drives the connect exec through a secure proxy tunnel and returns the tunnelling exchange
+     * handler submitted for the {@code CONNECT} request.
      */
-    private HttpRequest tunnelConnectRequest(final AsyncConnectExec exec, final HttpRoute route,
-            final HttpVersionPolicy versionPolicy) throws Exception {
-        final HttpClientContext context = HttpClientContext.create();
-        if (versionPolicy != null) {
-            context.setHttpVersionPolicy(versionPolicy);
-        }
+    private AsyncClientExchangeHandler tunnelExchangeHandler(final AsyncConnectExec exec, final HttpRoute route,
+            final HttpClientContext context, final AsyncExecCallback callback) throws Exception {
         final HttpRequest request = BasicRequestBuilder.get("https://foo/test").build();
         final CancellableDependency dependency = Mockito.mock(CancellableDependency.class);
         final AsyncExecChain.Scope scope = new AsyncExecChain.Scope(
@@ -109,10 +108,25 @@ private HttpRequest tunnelConnectRequest(final AsyncConnectExec exec, final Http
             return Mockito.mock(Cancellable.class);
         }).when(execRuntime).execute(Mockito.anyString(), Mockito.any(), Mockito.any());
 
-        exec.execute(request, null, scope, chain, Mockito.mock(AsyncExecCallback.class));
+        exec.execute(request, null, scope, chain, callback);
 
         final AsyncClientExchangeHandler handler = handlerRef.get();
         Assertions.assertNotNull(handler, "CONNECT exchange handler must have been submitted for execution");
+        return handler;
+    }
+
+    /**
+     * Drives the connect exec through a secure proxy tunnel and returns the {@code CONNECT} request
+     * that the tunnelling exchange handler produces.
+     */
+    private HttpRequest tunnelConnectRequest(final AsyncConnectExec exec, final HttpRoute route,
+            final HttpVersionPolicy versionPolicy) throws Exception {
+        final HttpClientContext context = HttpClientContext.create();
+        if (versionPolicy != null) {
+            context.setHttpVersionPolicy(versionPolicy);
+        }
+        final AsyncClientExchangeHandler handler = tunnelExchangeHandler(exec, route, context,
+                Mockito.mock(AsyncExecCallback.class));
 
         final AtomicReference<HttpRequest> connectRef = new AtomicReference<>();
         final RequestChannel requestChannel = Mockito.mock(RequestChannel.class);
@@ -153,4 +167,22 @@ void testEstablishRouteViaProxyTunnelAlpnHeaderReflectsVersionPolicy() throws Ex
         Assertions.assertEquals("http%2F1.1", h.getValue());
     }
 
+    @Test
+    void testEstablishRouteViaProxyTunnelRedirectResponse() throws Exception {
+        // A redirect from the proxy must fail the exchange instead of being passed up the chain
+        // as the target's response, where the redirect exec would follow it.
+        final AsyncConnectExec exec = new AsyncConnectExec(proxyHttpProcessor, proxyAuthStrategy, null, true);
+        final HttpRoute route = new HttpRoute(target, null, proxy, true);
+        final HttpClientContext context = HttpClientContext.create();
+        final AsyncExecCallback callback = Mockito.mock(AsyncExecCallback.class);
+
+        final AsyncClientExchangeHandler handler = tunnelExchangeHandler(exec, route, context, callback);
+
+        final HttpResponse response = new BasicHttpResponse(307, "Temporary Redirect");
+        response.setHeader(HttpHeaders.LOCATION, "http://foo/test");
+        Assertions.assertThrows(HttpException.class, () ->
+                handler.consumeResponse(response, null, context));
+        Mockito.verify(callback, Mockito.never()).handleResponse(Mockito.any(), Mockito.any());
+    }
+
 }

httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestConnectExec.java

@@ -224,6 +224,53 @@ void testEstablishRouteViaProxyTunnelFailure() throws Exception {
         Mockito.verify(execRuntime, Mockito.atLeastOnce()).discardEndpoint();
     }
 
+    @Test
+    void testEstablishRouteViaProxyTunnelRedirectResponse() throws Exception {
+        final HttpRoute route = new HttpRoute(target, null, proxy, true);
+        final HttpClientContext context = HttpClientContext.create();
+        final ClassicHttpRequest request = new HttpGet("http://bar/test");
+        final ClassicHttpResponse response = new BasicClassicHttpResponse(307, "Temporary Redirect");
+        response.setHeader(HttpHeaders.LOCATION, "http://foo/test");
+        response.setEntity(new StringEntity(""));
+
+        final ConnectionState connectionState = new ConnectionState();
+        Mockito.doAnswer(connectionState.connectAnswer()).when(execRuntime).connectEndpoint(Mockito.any());
+        Mockito.when(execRuntime.isEndpointConnected()).thenAnswer(connectionState.isConnectedAnswer());
+        Mockito.when(execRuntime.execute(
+                Mockito.anyString(),
+                Mockito.any(),
+                Mockito.any())).thenReturn(response);
+
+        final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context);
+        Assertions.assertThrows(HttpException.class, () ->
+                exec.execute(request, scope, execChain));
+        Mockito.verify(execRuntime, Mockito.atLeastOnce()).discardEndpoint();
+        Mockito.verify(execChain, Mockito.never()).proceed(Mockito.any(), Mockito.any());
+    }
+
+    @Test
+    void testEstablishRouteViaProxyTunnelNonOkSuccessResponse() throws Exception {
+        final HttpRoute route = new HttpRoute(target, null, proxy, true);
+        final HttpClientContext context = HttpClientContext.create();
+        final ClassicHttpRequest request = new HttpGet("http://bar/test");
+        final ClassicHttpResponse response = new BasicClassicHttpResponse(203, "Non-Authoritative Information");
+        response.setEntity(new StringEntity("not from the target"));
+
+        final ConnectionState connectionState = new ConnectionState();
+        Mockito.doAnswer(connectionState.connectAnswer()).when(execRuntime).connectEndpoint(Mockito.any());
+        Mockito.when(execRuntime.isEndpointConnected()).thenAnswer(connectionState.isConnectedAnswer());
+        Mockito.when(execRuntime.execute(
+                Mockito.anyString(),
+                Mockito.any(),
+                Mockito.any())).thenReturn(response);
+
+        final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context);
+        Assertions.assertThrows(HttpException.class, () ->
+                exec.execute(request, scope, execChain));
+        Mockito.verify(execRuntime, Mockito.atLeastOnce()).discardEndpoint();
+        Mockito.verify(execChain, Mockito.never()).proceed(Mockito.any(), Mockito.any());
+    }
+
     @Test
     void testEstablishRouteViaProxyTunnelRetryOnAuthChallengePersistentConnection() throws Exception {
         final HttpRoute route = new HttpRoute(target, null, proxy, true);