Skip to content

Commit 2409335

Browse files
jasonjoo2010jasonjoo2010
committed
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]>
1 parent 6afba08 commit 2409335

File tree

3 files changed

+101
-17
lines changed

3 files changed

+101
-17
lines changed

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.asynchttpclient.netty.channel.ChannelManager;
2929
import org.asynchttpclient.netty.channel.Channels;
3030
import org.asynchttpclient.netty.request.NettyRequestSender;
31+
import org.asynchttpclient.util.HttpConstants.ResponseStatusCodes;
3132

3233
import java.io.IOException;
3334
import java.net.InetSocketAddress;
@@ -39,9 +40,12 @@ public HttpHandler(AsyncHttpClientConfig config, ChannelManager channelManager,
3940
super(config, channelManager, requestSender);
4041
}
4142

42-
private boolean abortAfterHandlingStatus(AsyncHandler<?> handler,
43+
private boolean abortAfterHandlingStatus(AsyncHandler<?> handler, HttpMethod httpMethod,
4344
NettyResponseStatus status) throws Exception {
44-
return handler.onStatusReceived(status) == State.ABORT;
45+
// For non-200 response of a CONNECT request, it's still unconnected.
46+
// We need to either close the connection or reuse it but send CONNECT request again.
47+
// The former one is easier or we have to attach more state to Channel.
48+
return handler.onStatusReceived(status) == State.ABORT || httpMethod == HttpMethod.CONNECT && status.getStatusCode() != ResponseStatusCodes.OK_200;
4549
}
4650

4751
private boolean abortAfterHandlingHeaders(AsyncHandler<?> handler,
@@ -75,7 +79,7 @@ private void handleHttpResponse(final HttpResponse response, final Channel chann
7579
HttpHeaders responseHeaders = response.headers();
7680

7781
if (!interceptors.exitAfterIntercept(channel, future, handler, response, status, responseHeaders)) {
78-
boolean abort = abortAfterHandlingStatus(handler, status) || //
82+
boolean abort = abortAfterHandlingStatus(handler, httpRequest.method(), status) || //
7983
abortAfterHandlingHeaders(handler, responseHeaders) || //
8084
abortAfterHandlingReactiveStreams(channel, future, handler);
8185

‎client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,13 @@ public NettyRequestSender(AsyncHttpClientConfig config,
8383
this.clientState = clientState;
8484
requestFactory = new NettyRequestFactory(config);
8585
}
86+
87+
// needConnect returns true if the request is secure/websocket and a HTTP proxy is set
88+
private boolean needConnect(final Request request, final ProxyServer proxyServer) {
89+
return proxyServer != null
90+
&& proxyServer.getProxyType().isHttp()
91+
&& (request.getUri().isSecured() || request.getUri().isWebSocket());
92+
}
8693

8794
public <T> ListenableFuture<T> sendRequest(final Request request,
8895
final AsyncHandler<T> asyncHandler,
@@ -97,10 +104,7 @@ public <T> ListenableFuture<T> sendRequest(final Request request,
97104
ProxyServer proxyServer = getProxyServer(config, request);
98105

99106
// WebSockets use connect tunneling to work with proxies
100-
if (proxyServer != null
101-
&& proxyServer.getProxyType().isHttp()
102-
&& (request.getUri().isSecured() || request.getUri().isWebSocket())
103-
&& !isConnectAlreadyDone(request, future)) {
107+
if (needConnect(request, proxyServer) && !isConnectAlreadyDone(request, future)) {
104108
// Proxy with HTTPS or WebSocket: CONNECT for sure
105109
if (future != null && future.isConnectAllowed()) {
106110
// Perform CONNECT
@@ -117,6 +121,8 @@ public <T> ListenableFuture<T> sendRequest(final Request request,
117121

118122
private boolean isConnectAlreadyDone(Request request, NettyResponseFuture<?> future) {
119123
return future != null
124+
// If the channel can't be reused or closed, a CONNECT is still required
125+
&& future.isReuseChannel() && Channels.isChannelActive(future.channel())
120126
&& future.getNettyRequest() != null
121127
&& future.getNettyRequest().getHttpRequest().method() == HttpMethod.CONNECT
122128
&& !request.getMethod().equals(CONNECT);
@@ -132,15 +138,19 @@ private <T> ListenableFuture<T> sendRequestWithCertainForceConnect(Request reque
132138
NettyResponseFuture<T> future,
133139
ProxyServer proxyServer,
134140
boolean performConnectRequest) {
135-
136-
NettyResponseFuture<T> newFuture = newNettyRequestAndResponseFuture(request, asyncHandler, future, proxyServer,
137-
performConnectRequest);
138-
139-
Channel channel = getOpenChannel(future, request, proxyServer, asyncHandler);
140-
141-
return Channels.isChannelActive(channel)
142-
? sendRequestWithOpenChannel(newFuture, asyncHandler, channel)
143-
: sendRequestWithNewChannel(request, proxyServer, newFuture, asyncHandler);
141+
Channel channel = getOpenChannel(future, request, proxyServer, asyncHandler);
142+
if (Channels.isChannelActive(channel)) {
143+
NettyResponseFuture<T> newFuture = newNettyRequestAndResponseFuture(request, asyncHandler, future,
144+
proxyServer, performConnectRequest);
145+
return sendRequestWithOpenChannel(newFuture, asyncHandler, channel);
146+
} else {
147+
// A new channel is not expected when performConnectRequest is false. We need to
148+
// revisit the condition of sending
149+
// the CONNECT request to the new channel.
150+
NettyResponseFuture<T> newFuture = newNettyRequestAndResponseFuture(request, asyncHandler, future,
151+
proxyServer, needConnect(request, proxyServer));
152+
return sendRequestWithNewChannel(request, proxyServer, newFuture, asyncHandler);
153+
}
144154
}
145155

146156
/**

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

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@
1313
package org.asynchttpclient.proxy;
1414

1515
import org.asynchttpclient.*;
16+
import org.asynchttpclient.proxy.ProxyServer.Builder;
1617
import org.asynchttpclient.request.body.generator.ByteArrayBodyGenerator;
1718
import org.asynchttpclient.test.EchoHandler;
19+
import org.asynchttpclient.util.HttpConstants;
1820
import org.eclipse.jetty.proxy.ConnectHandler;
1921
import org.eclipse.jetty.server.Server;
2022
import org.eclipse.jetty.server.ServerConnector;
@@ -23,11 +25,21 @@
2325
import org.testng.annotations.BeforeClass;
2426
import org.testng.annotations.Test;
2527

28+
import io.netty.handler.codec.http.DefaultHttpHeaders;
29+
2630
import static org.asynchttpclient.Dsl.*;
2731
import static org.asynchttpclient.test.TestUtils.LARGE_IMAGE_BYTES;
2832
import static org.asynchttpclient.test.TestUtils.addHttpConnector;
2933
import static org.asynchttpclient.test.TestUtils.addHttpsConnector;
3034
import static org.testng.Assert.assertEquals;
35+
import static org.testng.Assert.assertThrows;
36+
37+
import java.io.IOException;
38+
import java.util.concurrent.ExecutionException;
39+
40+
import javax.servlet.ServletException;
41+
import javax.servlet.http.HttpServletRequest;
42+
import javax.servlet.http.HttpServletResponse;
3143

3244
/**
3345
* Proxy usage tests.
@@ -37,7 +49,7 @@ public class HttpsProxyTest extends AbstractBasicTest {
3749
private Server server2;
3850

3951
public AbstractHandler configureHandler() throws Exception {
40-
return new ConnectHandler();
52+
return new ProxyHandler();
4153
}
4254

4355
@BeforeClass(alwaysRun = true)
@@ -129,4 +141,62 @@ public void testPooledConnectionsWithProxy() throws Exception {
129141
assertEquals(r2.getStatusCode(), 200);
130142
}
131143
}
144+
145+
@Test
146+
public void testFailedConnectWithProxy() throws Exception {
147+
try (AsyncHttpClient asyncHttpClient = asyncHttpClient(config().setFollowRedirect(true).setUseInsecureTrustManager(true).setKeepAlive(true))) {
148+
Builder proxyServer = proxyServer("localhost", port1);
149+
proxyServer.setCustomHeaders(r -> new DefaultHttpHeaders().set(ProxyHandler.HEADER_FORBIDDEN, "1"));
150+
RequestBuilder rb = get(getTargetUrl2()).setProxyServer(proxyServer);
151+
152+
Response response1 = asyncHttpClient.executeRequest(rb.build()).get();
153+
assertEquals(403, response1.getStatusCode());
154+
155+
Response response2 = asyncHttpClient.executeRequest(rb.build()).get();
156+
assertEquals(403, response2.getStatusCode());
157+
158+
Response response3 = asyncHttpClient.executeRequest(rb.build()).get();
159+
assertEquals(403, response3.getStatusCode());
160+
}
161+
}
162+
163+
@Test
164+
public void testClosedConnectionWithProxy() throws Exception {
165+
try (AsyncHttpClient asyncHttpClient = asyncHttpClient(
166+
config().setFollowRedirect(true).setUseInsecureTrustManager(true).setKeepAlive(true))) {
167+
Builder proxyServer = proxyServer("localhost", port1);
168+
proxyServer.setCustomHeaders(r -> new DefaultHttpHeaders().set(ProxyHandler.HEADER_FORBIDDEN, "2"));
169+
RequestBuilder rb = get(getTargetUrl2()).setProxyServer(proxyServer);
170+
171+
assertThrows(ExecutionException.class, () -> asyncHttpClient.executeRequest(rb.build()).get());
172+
assertThrows(ExecutionException.class, () -> asyncHttpClient.executeRequest(rb.build()).get());
173+
assertThrows(ExecutionException.class, () -> asyncHttpClient.executeRequest(rb.build()).get());
174+
}
175+
}
176+
177+
public static class ProxyHandler extends ConnectHandler {
178+
final static String HEADER_FORBIDDEN = "X-REJECT-REQUEST";
179+
180+
@Override
181+
public void handle(String s, org.eclipse.jetty.server.Request r, HttpServletRequest request,
182+
HttpServletResponse response) throws ServletException, IOException {
183+
if (HttpConstants.Methods.CONNECT.equalsIgnoreCase(request.getMethod())) {
184+
String headerValue = request.getHeader(HEADER_FORBIDDEN);
185+
if (headerValue == null) {
186+
headerValue = "";
187+
}
188+
switch (headerValue) {
189+
case "1":
190+
response.setStatus(HttpServletResponse.SC_FORBIDDEN);
191+
r.setHandled(true);
192+
return;
193+
case "2":
194+
r.getHttpChannel().getConnection().close();
195+
r.setHandled(true);
196+
return;
197+
}
198+
}
199+
super.handle(s, r, request, response);
200+
}
201+
}
132202
}

0 commit comments

Comments
 (0)