Skip to content

Commit bdd9067

Browse files
hyperxprohyperxpro
authored
Add BootstrapHandler inside initChannel for SOCKS (#2140)
Motivation: SOCKS bootstrap pipeline wasn't wiring the HTTP handler correctly, causing proxy handshakes to fail or silently bypass proxies. Modification: - ChannelManager: Remove handlerAdded override and add httpBootstrapHandler directly into the SOCKS bootstrap pipeline. - Tests: Add findFreePort, replace flaky startup-based tests with deterministic assertions that requests fail when SOCKS (v4/v5) proxies are unreachable, and add timeouts. Result: Fixes #2139
1 parent ae59f51 commit bdd9067

File tree

7 files changed

+134
-175
lines changed

7 files changed

+134
-175
lines changed

‎.github/workflows/builds.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ jobs:
3030
distribution: 'corretto'
3131
java-version: '11'
3232
- name: Run Tests
33-
run: ./mvnw -B -ntp test
33+
run: ./mvnw -B -ntp test -Ddocker.tests=true
3434

3535
RunOnMacOs:
3636
runs-on: macos-latest

‎.github/workflows/maven.yml‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ jobs:
2727
with:
2828
distribution: 'corretto'
2929
java-version: '11'
30-
- name: Run Tests
31-
run: ./mvnw -B -ntp clean test
30+
- name: Run Tests (force Docker tests)
31+
run: ./mvnw -B -ntp clean test -Ddocker.tests=true
3232

3333
RunOnMacOs:
3434
runs-on: macos-latest
@@ -40,8 +40,8 @@ jobs:
4040
with:
4141
distribution: 'corretto'
4242
java-version: '11'
43-
- name: Run Tests
44-
run: ./mvnw -B -ntp clean test
43+
- name: Run Tests (force Docker tests)
44+
run: ./mvnw -B -ntp clean test -Ddocker.tests=true
4545

4646
RunOnWindows:
4747
runs-on: windows-latest

‎client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import io.netty.channel.Channel;
2121
import io.netty.channel.ChannelFactory;
2222
import io.netty.channel.ChannelHandler;
23-
import io.netty.channel.ChannelHandlerContext;
2423
import io.netty.channel.ChannelInitializer;
2524
import io.netty.channel.ChannelOption;
2625
import io.netty.channel.ChannelPipeline;
@@ -508,14 +507,10 @@ public Future<Bootstrap> getBootstrap(Uri uri, NameResolver<InetAddress> nameRes
508507
nameResolver.resolve(proxy.getHost()).addListener((Future<InetAddress> whenProxyAddress) -> {
509508
if (whenProxyAddress.isSuccess()) {
510509
socksBootstrap.handler(new ChannelInitializer<Channel>() {
511-
@Override
512-
public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
513-
httpBootstrapHandler.handlerAdded(ctx);
514-
super.handlerAdded(ctx);
515-
}
516-
517510
@Override
518511
protected void initChannel(Channel channel) throws Exception {
512+
channel.pipeline().addLast(httpBootstrapHandler);
513+
519514
InetSocketAddress proxyAddress = new InetSocketAddress(whenProxyAddress.get(), proxy.getPort());
520515
Realm realm = proxy.getRealm();
521516
String username = realm != null ? realm.getPrincipal() : null;

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

Lines changed: 112 additions & 147 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2024 AsyncHttpClient Project. All rights reserved.
2+
* Copyright (c) 2024-2026 AsyncHttpClient Project. All rights reserved.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -23,18 +23,24 @@
2323
import org.eclipse.jetty.server.handler.AbstractHandler;
2424
import org.junit.jupiter.api.Test;
2525

26+
import java.io.IOException;
27+
import java.net.ServerSocket;
2628
import java.time.Duration;
2729
import java.util.concurrent.ExecutionException;
2830
import java.util.concurrent.Future;
2931
import java.util.concurrent.TimeUnit;
32+
import java.util.concurrent.TimeoutException;
3033

3134
import static org.asynchttpclient.Dsl.asyncHttpClient;
3235
import static org.asynchttpclient.Dsl.config;
3336
import static org.junit.jupiter.api.Assertions.assertEquals;
3437
import static org.junit.jupiter.api.Assertions.assertNotNull;
38+
import static org.junit.jupiter.api.Assertions.assertThrows;
39+
import static org.junit.jupiter.api.Assertions.fail;
3540

3641
/**
3742
* Tests for SOCKS proxy support with both HTTP and HTTPS.
43+
* Validates fix for GitHub issue #2139 (SOCKS proxy support broken).
3844
*/
3945
public class SocksProxyTest extends AbstractBasicTest {
4046

@@ -43,6 +49,15 @@ public AbstractHandler configureHandler() throws Exception {
4349
return new ProxyTest.ProxyHandler();
4450
}
4551

52+
/**
53+
* Returns a port that is not in use by binding to port 0 and then closing the socket.
54+
*/
55+
private static int findFreePort() throws IOException {
56+
try (ServerSocket socket = new ServerSocket(0)) {
57+
return socket.getLocalPort();
58+
}
59+
}
60+
4661
@RepeatedIfExceptionsTest(repeats = 5)
4762
public void testSocks4ProxyWithHttp() throws Exception {
4863
// Start SOCKS proxy in background thread
@@ -70,183 +85,133 @@ public void testSocks4ProxyWithHttp() throws Exception {
7085
}
7186
}
7287

73-
@RepeatedIfExceptionsTest(repeats = 5)
74-
public void testSocks5ProxyWithHttp() throws Exception {
75-
// Start SOCKS proxy in background thread
76-
Thread socksProxyThread = new Thread(() -> {
77-
try {
78-
new SocksProxy(60000);
79-
} catch (Exception e) {
80-
logger.error("Failed to establish SocksProxy", e);
81-
}
82-
});
83-
socksProxyThread.start();
84-
85-
// Give the proxy time to start
86-
Thread.sleep(1000);
88+
/**
89+
* Validates that when a SOCKS5 proxy is configured at an address where no
90+
* SOCKS server is running, the HTTP request FAILS instead of silently
91+
* bypassing the proxy and using normal routing.
92+
* This is the core regression test for GitHub issue #2139.
93+
*/
94+
@Test
95+
public void testSocks5ProxyNotRunningMustFailHttp() throws Exception {
96+
int freePort = findFreePort();
8797

88-
try (AsyncHttpClient client = asyncHttpClient()) {
98+
try (AsyncHttpClient client = asyncHttpClient(config()
99+
.setConnectTimeout(Duration.ofMillis(5000))
100+
.setRequestTimeout(Duration.ofMillis(10000)))) {
89101
String target = "http://localhost:" + port1 + '/';
90102
Future<Response> f = client.prepareGet(target)
91-
.setProxyServer(new ProxyServer.Builder("localhost", 8000).setProxyType(ProxyType.SOCKS_V5))
103+
.setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort)
104+
.setProxyType(ProxyType.SOCKS_V5))
92105
.execute();
93-
94-
Response response = f.get(60, TimeUnit.SECONDS);
95-
assertNotNull(response);
96-
assertEquals(200, response.getStatusCode());
106+
assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS),
107+
"Request should fail when SOCKS5 proxy is not running, not bypass proxy");
97108
}
98109
}
99110

111+
/**
112+
* Validates that when a SOCKS4 proxy is configured at an address where no
113+
* SOCKS server is running, the HTTP request FAILS instead of silently
114+
* bypassing the proxy and using normal routing.
115+
*/
100116
@Test
101-
public void testSocks5ProxyWithHttpsDoesNotThrowException() throws Exception {
102-
// This test specifically verifies that HTTPS requests through SOCKS5 proxy
103-
// do not throw NoSuchElementException: socks anymore
104-
105-
// Start SOCKS proxy in background thread
106-
Thread socksProxyThread = new Thread(() -> {
107-
try {
108-
new SocksProxy(10000); // shorter time for test
109-
} catch (Exception e) {
110-
logger.error("Failed to establish SocksProxy", e);
111-
}
112-
});
113-
socksProxyThread.start();
114-
115-
// Give the proxy time to start
116-
Thread.sleep(1000);
117+
public void testSocks4ProxyNotRunningMustFailHttp() throws Exception {
118+
int freePort = findFreePort();
117119

118120
try (AsyncHttpClient client = asyncHttpClient(config()
119-
.setProxyServer(new ProxyServer.Builder("localhost", 8000).setProxyType(ProxyType.SOCKS_V5))
120121
.setConnectTimeout(Duration.ofMillis(5000))
121122
.setRequestTimeout(Duration.ofMillis(10000)))) {
122-
123-
// This would previously throw: java.util.NoSuchElementException: socks
124-
// We expect this to fail with connection timeout (since we don't have a real HTTPS target)
125-
// but NOT with NoSuchElementException
126-
127-
try {
128-
Future<Response> f = client.prepareGet("https://httpbin.org/get").execute();
129-
f.get(8, TimeUnit.SECONDS);
130-
// If we reach here, great! The SOCKS proxy worked
131-
} catch (Exception e) {
132-
// We should NOT see NoSuchElementException: socks anymore
133-
String message = e.getMessage();
134-
if (message != null && message.contains("socks") && message.contains("NoSuchElementException")) {
135-
throw new AssertionError("NoSuchElementException: socks still occurs", e);
136-
}
137-
// Other exceptions like connection timeout are expected since we don't have a real working SOCKS proxy setup
138-
logger.info("Expected exception (not the SOCKS handler bug): " + e.getClass().getSimpleName() + ": " + message);
139-
}
123+
String target = "http://localhost:" + port1 + '/';
124+
Future<Response> f = client.prepareGet(target)
125+
.setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort)
126+
.setProxyType(ProxyType.SOCKS_V4))
127+
.execute();
128+
assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS),
129+
"Request should fail when SOCKS4 proxy is not running, not bypass proxy");
140130
}
141131
}
142132

133+
/**
134+
* Validates that when a SOCKS5 proxy is configured at an address where no
135+
* SOCKS server is running, an HTTPS request FAILS instead of silently
136+
* bypassing the proxy and using normal routing.
137+
*/
143138
@Test
144-
public void testSocks4ProxyWithHttpsDoesNotThrowException() throws Exception {
145-
// This test specifically verifies that HTTPS requests through SOCKS4 proxy
146-
// do not throw NoSuchElementException: socks anymore
147-
148-
// Start SOCKS proxy in background thread
149-
Thread socksProxyThread = new Thread(() -> {
150-
try {
151-
new SocksProxy(10000); // shorter time for test
152-
} catch (Exception e) {
153-
logger.error("Failed to establish SocksProxy", e);
154-
}
155-
});
156-
socksProxyThread.start();
157-
158-
// Give the proxy time to start
159-
Thread.sleep(1000);
139+
public void testSocks5ProxyNotRunningMustFailHttps() throws Exception {
140+
int freePort = findFreePort();
160141

161142
try (AsyncHttpClient client = asyncHttpClient(config()
162-
.setProxyServer(new ProxyServer.Builder("localhost", 8000).setProxyType(ProxyType.SOCKS_V4))
163143
.setConnectTimeout(Duration.ofMillis(5000))
164144
.setRequestTimeout(Duration.ofMillis(10000)))) {
165-
166-
// This would previously throw: java.util.NoSuchElementException: socks
167-
// We expect this to fail with connection timeout (since we don't have a real HTTPS target)
168-
// but NOT with NoSuchElementException
169-
170-
try {
171-
Future<Response> f = client.prepareGet("https://httpbin.org/get").execute();
172-
f.get(8, TimeUnit.SECONDS);
173-
// If we reach here, great! The SOCKS proxy worked
174-
} catch (Exception e) {
175-
// We should NOT see NoSuchElementException: socks anymore
176-
String message = e.getMessage();
177-
if (message != null && message.contains("socks") && message.contains("NoSuchElementException")) {
178-
throw new AssertionError("NoSuchElementException: socks still occurs", e);
179-
}
180-
// Other exceptions like connection timeout are expected since we don't have a real working SOCKS proxy setup
181-
logger.info("Expected exception (not the SOCKS handler bug): " + e.getClass().getSimpleName() + ": " + message);
182-
}
145+
String target = "https://localhost:" + port2 + '/';
146+
Future<Response> f = client.prepareGet(target)
147+
.setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort)
148+
.setProxyType(ProxyType.SOCKS_V5))
149+
.execute();
150+
assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS),
151+
"Request should fail when SOCKS5 proxy is not running, not bypass proxy");
183152
}
184153
}
185154

155+
/**
156+
* Validates that when a SOCKS4 proxy is configured at an address where no
157+
* SOCKS server is running, an HTTPS request FAILS instead of silently
158+
* bypassing the proxy and using normal routing.
159+
*/
186160
@Test
187-
public void testIssue1913NoSuchElementExceptionSocks5() throws Exception {
188-
// Reproduces the exact issue from GitHub issue #1913 with SOCKS5
189-
// This uses the exact code pattern from the issue report
190-
var proxyServer = new ProxyServer.Builder("127.0.0.1", 1081)
191-
.setProxyType(ProxyType.SOCKS_V5);
161+
public void testSocks4ProxyNotRunningMustFailHttps() throws Exception {
162+
int freePort = findFreePort();
192163

193-
try (var client = asyncHttpClient(config()
194-
.setProxyServer(proxyServer.build())
195-
.setConnectTimeout(Duration.ofMillis(2000))
196-
.setRequestTimeout(Duration.ofMillis(5000)))) {
197-
198-
// This would previously throw: java.util.NoSuchElementException: socks
199-
// We expect this to fail with connection timeout (since proxy doesn't exist)
200-
// but NOT with NoSuchElementException
201-
202-
try {
203-
var response = client.prepareGet("https://cloudflare.com/cdn-cgi/trace").execute().get();
204-
// If we reach here, great! The fix worked and proxy connection succeeded
205-
logger.info("Connection successful: " + response.getStatusCode());
206-
} catch (Exception e) {
207-
// Check that we don't get the NoSuchElementException: socks anymore
208-
Throwable cause = e.getCause();
209-
String message = cause != null ? cause.getMessage() : e.getMessage();
210-
211-
// This should NOT contain the original error
212-
if (message != null && message.contains("socks") &&
213-
(e.toString().contains("NoSuchElementException") || cause != null && cause.toString().contains("NoSuchElementException"))) {
214-
throw new AssertionError("NoSuchElementException: socks still occurs - fix didn't work: " + e.toString());
215-
}
216-
217-
// Other exceptions like connection timeout are expected since we don't have a working SOCKS proxy
218-
logger.info("Expected exception (not the SOCKS handler bug): " + e.getClass().getSimpleName() + ": " + message);
219-
}
164+
try (AsyncHttpClient client = asyncHttpClient(config()
165+
.setConnectTimeout(Duration.ofMillis(5000))
166+
.setRequestTimeout(Duration.ofMillis(10000)))) {
167+
String target = "https://localhost:" + port2 + '/';
168+
Future<Response> f = client.prepareGet(target)
169+
.setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort)
170+
.setProxyType(ProxyType.SOCKS_V4))
171+
.execute();
172+
assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS),
173+
"Request should fail when SOCKS4 proxy is not running, not bypass proxy");
220174
}
221175
}
222176

223-
@Test
224-
public void testIssue1913NoSuchElementExceptionSocks4() throws Exception {
225-
// Reproduces the exact issue from GitHub issue #1913 with SOCKS4
226-
// This uses the exact code pattern from the issue report
227-
var proxyServer = new ProxyServer.Builder("127.0.0.1", 1081)
228-
.setProxyType(ProxyType.SOCKS_V4);
229-
230-
try (var client = asyncHttpClient(config()
231-
.setProxyServer(proxyServer.build())
232-
.setConnectTimeout(Duration.ofMillis(2000))
233-
.setRequestTimeout(Duration.ofMillis(5000)))) {
177+
/**
178+
* Validates that per-request SOCKS5 proxy config with a non-existent proxy
179+
* also correctly fails the request.
180+
*/
181+
@Test
182+
public void testPerRequestSocks5ProxyNotRunningMustFail() throws Exception {
183+
int freePort = findFreePort();
234184

235-
try {
236-
var response = client.prepareGet("https://cloudflare.com/cdn-cgi/trace").execute().get();
237-
logger.info("Connection successful: " + response.getStatusCode());
238-
} catch (Exception e) {
239-
// Check that we don't get the NoSuchElementException: socks anymore
240-
Throwable cause = e.getCause();
241-
String message = cause != null ? cause.getMessage() : e.getMessage();
185+
try (AsyncHttpClient client = asyncHttpClient(config()
186+
.setConnectTimeout(Duration.ofMillis(5000))
187+
.setRequestTimeout(Duration.ofMillis(10000)))) {
188+
String target = "http://localhost:" + port1 + '/';
189+
Future<Response> f = client.prepareGet(target)
190+
.setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort)
191+
.setProxyType(ProxyType.SOCKS_V5))
192+
.execute();
193+
assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS),
194+
"Per-request SOCKS5 proxy config should not be silently ignored");
195+
}
196+
}
242197

243-
if (message != null && message.contains("socks") &&
244-
(e.toString().contains("NoSuchElementException") || cause != null && cause.toString().contains("NoSuchElementException"))) {
245-
throw new AssertionError("NoSuchElementException: socks still occurs - fix didn't work: " + e.toString());
246-
}
198+
/**
199+
* Validates that client-level SOCKS5 proxy config with a non-existent proxy
200+
* also correctly fails the request.
201+
*/
202+
@Test
203+
public void testClientLevelSocks5ProxyNotRunningMustFail() throws Exception {
204+
int freePort = findFreePort();
247205

248-
logger.info("Expected exception (not the SOCKS handler bug): " + e.getClass().getSimpleName() + ": " + message);
249-
}
206+
try (AsyncHttpClient client = asyncHttpClient(config()
207+
.setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort)
208+
.setProxyType(ProxyType.SOCKS_V5))
209+
.setConnectTimeout(Duration.ofMillis(5000))
210+
.setRequestTimeout(Duration.ofMillis(10000)))) {
211+
String target = "http://localhost:" + port1 + '/';
212+
Future<Response> f = client.prepareGet(target).execute();
213+
assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS),
214+
"Client-level SOCKS5 proxy config should not be silently ignored");
250215
}
251216
}
252217
}

0 commit comments

Comments
 (0)