diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncConnectExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncConnectExec.java index 2ce6a97d19..97c600a981 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncConnectExec.java +++ b/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) { diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ConnectExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ConnectExec.java index 422e3bb943..54e37b32a3 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ConnectExec.java +++ b/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) { diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncConnectExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncConnectExec.java index 14fd8b4a2f..a6f3cd79af 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncConnectExec.java +++ b/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 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()); + } + } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestConnectExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestConnectExec.java index b1266d0c80..a2335dc668 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestConnectExec.java +++ b/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);