fix: inappropriate connection reuse when using HTTP proxy

There is an extra CONNECT request needs to send before the real request to the HTTP proxy and the 2nd request only happens if the CONNECT request succeeds. When CONNECT failed, the connection should be dropped as it's not in connected state.

Signed-off-by: Jason Joo <[email protected]>

Author: jasonjoo2010

client/src/main/java/org/asynchttpclient/netty/handler/HttpHandler.java

@@ -28,6 +28,7 @@
 import org.asynchttpclient.netty.channel.ChannelManager;
 import org.asynchttpclient.netty.channel.Channels;
 import org.asynchttpclient.netty.request.NettyRequestSender;
+import org.asynchttpclient.util.HttpConstants.ResponseStatusCodes;
 
 import java.io.IOException;
 import java.net.InetSocketAddress;
@@ -39,9 +40,12 @@ public HttpHandler(AsyncHttpClientConfig config, ChannelManager channelManager,
     super(config, channelManager, requestSender);
   }
 
-  private boolean abortAfterHandlingStatus(AsyncHandler<?> handler,
+  private boolean abortAfterHandlingStatus(AsyncHandler<?> handler, HttpMethod httpMethod,
                                            NettyResponseStatus status) throws Exception {
-    return handler.onStatusReceived(status) == State.ABORT;
+    // For non-200 response of a CONNECT request, it's still unconnected.
+    // We need to either close the connection or reuse it but send CONNECT request again.
+    // The former one is easier or we have to attach more state to Channel.
+    return handler.onStatusReceived(status) == State.ABORT || httpMethod == HttpMethod.CONNECT && status.getStatusCode() != ResponseStatusCodes.OK_200;
   }
 
   private boolean abortAfterHandlingHeaders(AsyncHandler<?> handler,
@@ -75,7 +79,7 @@ private void handleHttpResponse(final HttpResponse response, final Channel chann
     HttpHeaders responseHeaders = response.headers();
 
     if (!interceptors.exitAfterIntercept(channel, future, handler, response, status, responseHeaders)) {
-      boolean abort = abortAfterHandlingStatus(handler, status) || //
+      boolean abort = abortAfterHandlingStatus(handler, httpRequest.method(), status) || //
               abortAfterHandlingHeaders(handler, responseHeaders) || //
               abortAfterHandlingReactiveStreams(channel, future, handler);
 

client/src/test/java/org/asynchttpclient/proxy/HttpsProxyTest.java

@@ -13,8 +13,10 @@
 package org.asynchttpclient.proxy;
 
 import org.asynchttpclient.*;
+import org.asynchttpclient.proxy.ProxyServer.Builder;
 import org.asynchttpclient.request.body.generator.ByteArrayBodyGenerator;
 import org.asynchttpclient.test.EchoHandler;
+import org.asynchttpclient.util.HttpConstants;
 import org.eclipse.jetty.proxy.ConnectHandler;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
@@ -29,6 +31,12 @@
 import static org.asynchttpclient.test.TestUtils.addHttpsConnector;
 import static org.testng.Assert.assertEquals;
 
+import java.io.IOException;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
 /**
  * Proxy usage tests.
  */
@@ -37,7 +45,7 @@ public class HttpsProxyTest extends AbstractBasicTest {
   private Server server2;
 
   public AbstractHandler configureHandler() throws Exception {
-    return new ConnectHandler();
+    return new ProxyHandler();
   }
 
   @BeforeClass(alwaysRun = true)
@@ -129,4 +137,39 @@ public void testPooledConnectionsWithProxy() throws Exception {
       assertEquals(r2.getStatusCode(), 200);
     }
   }
+  
+  @Test
+  public void testFailedConnectWithProxy() throws Exception {
+      try (AsyncHttpClient asyncHttpClient = asyncHttpClient(config().setFollowRedirect(true).setUseInsecureTrustManager(true).setKeepAlive(true))) {
+          Builder proxyServer = proxyServer("localhost", port1);
+          proxyServer.setCustomHeaders(r -> r.getHeaders().add(ProxyHandler.HEADER_FORBIDDEN, "1"));
+          RequestBuilder rb = get(getTargetUrl2()).setProxyServer(proxyServer);
+
+          Response response1 = asyncHttpClient.executeRequest(rb.build()).get();
+          assertEquals(403, response1.getStatusCode());
+
+          Response response2 = asyncHttpClient.executeRequest(rb.build()).get();
+          assertEquals(403, response2.getStatusCode());
+
+          Response response3 = asyncHttpClient.executeRequest(rb.build()).get();
+          assertEquals(403, response3.getStatusCode());
+      }
+  }
+  
+  public static class ProxyHandler extends ConnectHandler {
+      final static String HEADER_FORBIDDEN = "X-REJECT-REQUEST";
+
+      @Override
+      public void handle(String s, org.eclipse.jetty.server.Request r, HttpServletRequest request,
+              HttpServletResponse response) throws ServletException, IOException {
+          if (HttpConstants.Methods.CONNECT.equalsIgnoreCase(request.getMethod())) {
+              if (request.getHeader(HEADER_FORBIDDEN) != null) {
+                  response.setStatus(HttpServletResponse.SC_FORBIDDEN);
+                  r.setHandled(true);
+                  return;
+              }
+          }
+          super.handle(s, r, request, response);
+      }
+  }
 }