diff --git a/.github/workflows/builds.yml b/.github/workflows/builds.yml index 2586cf3c6..a3ebdd7cc 100644 --- a/.github/workflows/builds.yml +++ b/.github/workflows/builds.yml @@ -30,7 +30,7 @@ jobs: distribution: 'corretto' java-version: '11' - name: Run Tests - run: ./mvnw -B -ntp test + run: ./mvnw -B -ntp test -Ddocker.tests=true RunOnMacOs: runs-on: macos-latest diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 1f49d3112..473f1686f 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -27,8 +27,8 @@ jobs: with: distribution: 'corretto' java-version: '11' - - name: Run Tests - run: ./mvnw -B -ntp clean test + - name: Run Tests (force Docker tests) + run: ./mvnw -B -ntp clean test -Ddocker.tests=true RunOnMacOs: runs-on: macos-latest @@ -40,8 +40,8 @@ jobs: with: distribution: 'corretto' java-version: '11' - - name: Run Tests - run: ./mvnw -B -ntp clean test + - name: Run Tests (force Docker tests) + run: ./mvnw -B -ntp clean test -Ddocker.tests=true RunOnWindows: runs-on: windows-latest diff --git a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java index 8d13361ae..e59daadd1 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java @@ -20,7 +20,6 @@ import io.netty.channel.Channel; import io.netty.channel.ChannelFactory; import io.netty.channel.ChannelHandler; -import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInitializer; import io.netty.channel.ChannelOption; import io.netty.channel.ChannelPipeline; @@ -508,14 +507,10 @@ public Future getBootstrap(Uri uri, NameResolver nameRes nameResolver.resolve(proxy.getHost()).addListener((Future whenProxyAddress) -> { if (whenProxyAddress.isSuccess()) { socksBootstrap.handler(new ChannelInitializer() { - @Override - public void handlerAdded(ChannelHandlerContext ctx) throws Exception { - httpBootstrapHandler.handlerAdded(ctx); - super.handlerAdded(ctx); - } - @Override protected void initChannel(Channel channel) throws Exception { + channel.pipeline().addLast(httpBootstrapHandler); + InetSocketAddress proxyAddress = new InetSocketAddress(whenProxyAddress.get(), proxy.getPort()); Realm realm = proxy.getRealm(); String username = realm != null ? realm.getPrincipal() : null; diff --git a/client/src/test/java/org/asynchttpclient/proxy/SocksProxyTest.java b/client/src/test/java/org/asynchttpclient/proxy/SocksProxyTest.java index e1870721a..75b2e7e74 100644 --- a/client/src/test/java/org/asynchttpclient/proxy/SocksProxyTest.java +++ b/client/src/test/java/org/asynchttpclient/proxy/SocksProxyTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2024 AsyncHttpClient Project. All rights reserved. + * Copyright (c) 2024-2026 AsyncHttpClient Project. All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,18 +23,24 @@ import org.eclipse.jetty.server.handler.AbstractHandler; import org.junit.jupiter.api.Test; +import java.io.IOException; +import java.net.ServerSocket; import java.time.Duration; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import static org.asynchttpclient.Dsl.asyncHttpClient; import static org.asynchttpclient.Dsl.config; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.fail; /** * Tests for SOCKS proxy support with both HTTP and HTTPS. + * Validates fix for GitHub issue #2139 (SOCKS proxy support broken). */ public class SocksProxyTest extends AbstractBasicTest { @@ -43,6 +49,15 @@ public AbstractHandler configureHandler() throws Exception { return new ProxyTest.ProxyHandler(); } + /** + * Returns a port that is not in use by binding to port 0 and then closing the socket. + */ + private static int findFreePort() throws IOException { + try (ServerSocket socket = new ServerSocket(0)) { + return socket.getLocalPort(); + } + } + @RepeatedIfExceptionsTest(repeats = 5) public void testSocks4ProxyWithHttp() throws Exception { // Start SOCKS proxy in background thread @@ -70,183 +85,133 @@ public void testSocks4ProxyWithHttp() throws Exception { } } - @RepeatedIfExceptionsTest(repeats = 5) - public void testSocks5ProxyWithHttp() throws Exception { - // Start SOCKS proxy in background thread - Thread socksProxyThread = new Thread(() -> { - try { - new SocksProxy(60000); - } catch (Exception e) { - logger.error("Failed to establish SocksProxy", e); - } - }); - socksProxyThread.start(); - - // Give the proxy time to start - Thread.sleep(1000); + /** + * Validates that when a SOCKS5 proxy is configured at an address where no + * SOCKS server is running, the HTTP request FAILS instead of silently + * bypassing the proxy and using normal routing. + * This is the core regression test for GitHub issue #2139. + */ + @Test + public void testSocks5ProxyNotRunningMustFailHttp() throws Exception { + int freePort = findFreePort(); - try (AsyncHttpClient client = asyncHttpClient()) { + try (AsyncHttpClient client = asyncHttpClient(config() + .setConnectTimeout(Duration.ofMillis(5000)) + .setRequestTimeout(Duration.ofMillis(10000)))) { String target = "http://localhost:" + port1 + '/'; Future f = client.prepareGet(target) - .setProxyServer(new ProxyServer.Builder("localhost", 8000).setProxyType(ProxyType.SOCKS_V5)) + .setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort) + .setProxyType(ProxyType.SOCKS_V5)) .execute(); - - Response response = f.get(60, TimeUnit.SECONDS); - assertNotNull(response); - assertEquals(200, response.getStatusCode()); + assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS), + "Request should fail when SOCKS5 proxy is not running, not bypass proxy"); } } + /** + * Validates that when a SOCKS4 proxy is configured at an address where no + * SOCKS server is running, the HTTP request FAILS instead of silently + * bypassing the proxy and using normal routing. + */ @Test - public void testSocks5ProxyWithHttpsDoesNotThrowException() throws Exception { - // This test specifically verifies that HTTPS requests through SOCKS5 proxy - // do not throw NoSuchElementException: socks anymore - - // Start SOCKS proxy in background thread - Thread socksProxyThread = new Thread(() -> { - try { - new SocksProxy(10000); // shorter time for test - } catch (Exception e) { - logger.error("Failed to establish SocksProxy", e); - } - }); - socksProxyThread.start(); - - // Give the proxy time to start - Thread.sleep(1000); + public void testSocks4ProxyNotRunningMustFailHttp() throws Exception { + int freePort = findFreePort(); try (AsyncHttpClient client = asyncHttpClient(config() - .setProxyServer(new ProxyServer.Builder("localhost", 8000).setProxyType(ProxyType.SOCKS_V5)) .setConnectTimeout(Duration.ofMillis(5000)) .setRequestTimeout(Duration.ofMillis(10000)))) { - - // This would previously throw: java.util.NoSuchElementException: socks - // We expect this to fail with connection timeout (since we don't have a real HTTPS target) - // but NOT with NoSuchElementException - - try { - Future f = client.prepareGet("https://httpbin.org/get").execute(); - f.get(8, TimeUnit.SECONDS); - // If we reach here, great! The SOCKS proxy worked - } catch (Exception e) { - // We should NOT see NoSuchElementException: socks anymore - String message = e.getMessage(); - if (message != null && message.contains("socks") && message.contains("NoSuchElementException")) { - throw new AssertionError("NoSuchElementException: socks still occurs", e); - } - // Other exceptions like connection timeout are expected since we don't have a real working SOCKS proxy setup - logger.info("Expected exception (not the SOCKS handler bug): " + e.getClass().getSimpleName() + ": " + message); - } + String target = "http://localhost:" + port1 + '/'; + Future f = client.prepareGet(target) + .setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort) + .setProxyType(ProxyType.SOCKS_V4)) + .execute(); + assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS), + "Request should fail when SOCKS4 proxy is not running, not bypass proxy"); } } + /** + * Validates that when a SOCKS5 proxy is configured at an address where no + * SOCKS server is running, an HTTPS request FAILS instead of silently + * bypassing the proxy and using normal routing. + */ @Test - public void testSocks4ProxyWithHttpsDoesNotThrowException() throws Exception { - // This test specifically verifies that HTTPS requests through SOCKS4 proxy - // do not throw NoSuchElementException: socks anymore - - // Start SOCKS proxy in background thread - Thread socksProxyThread = new Thread(() -> { - try { - new SocksProxy(10000); // shorter time for test - } catch (Exception e) { - logger.error("Failed to establish SocksProxy", e); - } - }); - socksProxyThread.start(); - - // Give the proxy time to start - Thread.sleep(1000); + public void testSocks5ProxyNotRunningMustFailHttps() throws Exception { + int freePort = findFreePort(); try (AsyncHttpClient client = asyncHttpClient(config() - .setProxyServer(new ProxyServer.Builder("localhost", 8000).setProxyType(ProxyType.SOCKS_V4)) .setConnectTimeout(Duration.ofMillis(5000)) .setRequestTimeout(Duration.ofMillis(10000)))) { - - // This would previously throw: java.util.NoSuchElementException: socks - // We expect this to fail with connection timeout (since we don't have a real HTTPS target) - // but NOT with NoSuchElementException - - try { - Future f = client.prepareGet("https://httpbin.org/get").execute(); - f.get(8, TimeUnit.SECONDS); - // If we reach here, great! The SOCKS proxy worked - } catch (Exception e) { - // We should NOT see NoSuchElementException: socks anymore - String message = e.getMessage(); - if (message != null && message.contains("socks") && message.contains("NoSuchElementException")) { - throw new AssertionError("NoSuchElementException: socks still occurs", e); - } - // Other exceptions like connection timeout are expected since we don't have a real working SOCKS proxy setup - logger.info("Expected exception (not the SOCKS handler bug): " + e.getClass().getSimpleName() + ": " + message); - } + String target = "https://localhost:" + port2 + '/'; + Future f = client.prepareGet(target) + .setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort) + .setProxyType(ProxyType.SOCKS_V5)) + .execute(); + assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS), + "Request should fail when SOCKS5 proxy is not running, not bypass proxy"); } } + /** + * Validates that when a SOCKS4 proxy is configured at an address where no + * SOCKS server is running, an HTTPS request FAILS instead of silently + * bypassing the proxy and using normal routing. + */ @Test - public void testIssue1913NoSuchElementExceptionSocks5() throws Exception { - // Reproduces the exact issue from GitHub issue #1913 with SOCKS5 - // This uses the exact code pattern from the issue report - var proxyServer = new ProxyServer.Builder("127.0.0.1", 1081) - .setProxyType(ProxyType.SOCKS_V5); + public void testSocks4ProxyNotRunningMustFailHttps() throws Exception { + int freePort = findFreePort(); - try (var client = asyncHttpClient(config() - .setProxyServer(proxyServer.build()) - .setConnectTimeout(Duration.ofMillis(2000)) - .setRequestTimeout(Duration.ofMillis(5000)))) { - - // This would previously throw: java.util.NoSuchElementException: socks - // We expect this to fail with connection timeout (since proxy doesn't exist) - // but NOT with NoSuchElementException - - try { - var response = client.prepareGet("https://cloudflare.com/cdn-cgi/trace").execute().get(); - // If we reach here, great! The fix worked and proxy connection succeeded - logger.info("Connection successful: " + response.getStatusCode()); - } catch (Exception e) { - // Check that we don't get the NoSuchElementException: socks anymore - Throwable cause = e.getCause(); - String message = cause != null ? cause.getMessage() : e.getMessage(); - - // This should NOT contain the original error - if (message != null && message.contains("socks") && - (e.toString().contains("NoSuchElementException") || cause != null && cause.toString().contains("NoSuchElementException"))) { - throw new AssertionError("NoSuchElementException: socks still occurs - fix didn't work: " + e.toString()); - } - - // Other exceptions like connection timeout are expected since we don't have a working SOCKS proxy - logger.info("Expected exception (not the SOCKS handler bug): " + e.getClass().getSimpleName() + ": " + message); - } + try (AsyncHttpClient client = asyncHttpClient(config() + .setConnectTimeout(Duration.ofMillis(5000)) + .setRequestTimeout(Duration.ofMillis(10000)))) { + String target = "https://localhost:" + port2 + '/'; + Future f = client.prepareGet(target) + .setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort) + .setProxyType(ProxyType.SOCKS_V4)) + .execute(); + assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS), + "Request should fail when SOCKS4 proxy is not running, not bypass proxy"); } } - @Test - public void testIssue1913NoSuchElementExceptionSocks4() throws Exception { - // Reproduces the exact issue from GitHub issue #1913 with SOCKS4 - // This uses the exact code pattern from the issue report - var proxyServer = new ProxyServer.Builder("127.0.0.1", 1081) - .setProxyType(ProxyType.SOCKS_V4); - - try (var client = asyncHttpClient(config() - .setProxyServer(proxyServer.build()) - .setConnectTimeout(Duration.ofMillis(2000)) - .setRequestTimeout(Duration.ofMillis(5000)))) { + /** + * Validates that per-request SOCKS5 proxy config with a non-existent proxy + * also correctly fails the request. + */ + @Test + public void testPerRequestSocks5ProxyNotRunningMustFail() throws Exception { + int freePort = findFreePort(); - try { - var response = client.prepareGet("https://cloudflare.com/cdn-cgi/trace").execute().get(); - logger.info("Connection successful: " + response.getStatusCode()); - } catch (Exception e) { - // Check that we don't get the NoSuchElementException: socks anymore - Throwable cause = e.getCause(); - String message = cause != null ? cause.getMessage() : e.getMessage(); + try (AsyncHttpClient client = asyncHttpClient(config() + .setConnectTimeout(Duration.ofMillis(5000)) + .setRequestTimeout(Duration.ofMillis(10000)))) { + String target = "http://localhost:" + port1 + '/'; + Future f = client.prepareGet(target) + .setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort) + .setProxyType(ProxyType.SOCKS_V5)) + .execute(); + assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS), + "Per-request SOCKS5 proxy config should not be silently ignored"); + } + } - if (message != null && message.contains("socks") && - (e.toString().contains("NoSuchElementException") || cause != null && cause.toString().contains("NoSuchElementException"))) { - throw new AssertionError("NoSuchElementException: socks still occurs - fix didn't work: " + e.toString()); - } + /** + * Validates that client-level SOCKS5 proxy config with a non-existent proxy + * also correctly fails the request. + */ + @Test + public void testClientLevelSocks5ProxyNotRunningMustFail() throws Exception { + int freePort = findFreePort(); - logger.info("Expected exception (not the SOCKS handler bug): " + e.getClass().getSimpleName() + ": " + message); - } + try (AsyncHttpClient client = asyncHttpClient(config() + .setProxyServer(new ProxyServer.Builder("127.0.0.1", freePort) + .setProxyType(ProxyType.SOCKS_V5)) + .setConnectTimeout(Duration.ofMillis(5000)) + .setRequestTimeout(Duration.ofMillis(10000)))) { + String target = "http://localhost:" + port1 + '/'; + Future f = client.prepareGet(target).execute(); + assertThrows(ExecutionException.class, () -> f.get(10, TimeUnit.SECONDS), + "Client-level SOCKS5 proxy config should not be silently ignored"); } } } diff --git a/client/src/test/java/org/asynchttpclient/proxy/SocksProxyTestcontainersIntegrationTest.java b/client/src/test/java/org/asynchttpclient/proxy/SocksProxyTestcontainersIntegrationTest.java index 4308f388e..420dfff7e 100644 --- a/client/src/test/java/org/asynchttpclient/proxy/SocksProxyTestcontainersIntegrationTest.java +++ b/client/src/test/java/org/asynchttpclient/proxy/SocksProxyTestcontainersIntegrationTest.java @@ -77,6 +77,7 @@ static void checkDockerAvailability() { // Allow force-disabling Docker tests if ("true".equals(System.getProperty("no.docker.tests"))) { LOGGER.info("Docker tests disabled via -Dno.docker.tests=true"); + dockerAvailable = false; return; } // Only start container if Docker is available @@ -89,7 +90,7 @@ static void checkDockerAvailability() { ) .withExposedPorts(SOCKS_PORT) .withLogConsumer(new Slf4jLogConsumer(LOGGER).withPrefix("DANTE")) - .waitingFor(Wait.forLogMessage(".*sockd.*", 1) + .waitingFor(Wait.forLogMessage(".*danted.*running.*", 1) .withStartupTimeout(Duration.ofMinutes(2))); socksProxy.start(); LOGGER.info("Dante SOCKS proxy started successfully on port {}", socksProxy.getMappedPort(SOCKS_PORT)); diff --git a/client/src/test/resources/dante/Dockerfile b/client/src/test/resources/dante/Dockerfile index a98658439..23029f7bc 100644 --- a/client/src/test/resources/dante/Dockerfile +++ b/client/src/test/resources/dante/Dockerfile @@ -5,15 +5,11 @@ RUN apt-get update && \ apt-get install -y dante-server && \ rm -rf /var/lib/apt/lists/* -# Copy dante configuration -COPY sockd.conf /etc/sockd.conf - -# Create run directory -RUN mkdir -p /var/run/sockd && \ - chmod 755 /var/run/sockd +# Copy dante configuration (danted uses /etc/danted.conf) +COPY sockd.conf /etc/danted.conf # Expose SOCKS port EXPOSE 1080 -# Run dante server (sockd binary is in /usr/sbin) -CMD ["/usr/sbin/sockd", "-f", "/etc/sockd.conf", "-D"] +# Run dante server (binary is /usr/sbin/danted) +CMD ["/usr/sbin/danted"] diff --git a/client/src/test/resources/dante/sockd.conf b/client/src/test/resources/dante/sockd.conf index e4f7ed0fd..7cdd7d01e 100644 --- a/client/src/test/resources/dante/sockd.conf +++ b/client/src/test/resources/dante/sockd.conf @@ -1,23 +1,25 @@ -# Basic SOCKS proxy configuration for testing -# Allow all connections and methods for testing purposes +# Dante SOCKS proxy configuration for testing +# Allow all connections without authentication + +logoutput: stderr -# Server configuration - listen on all interfaces internal: 0.0.0.0 port = 1080 external: eth0 -# Authentication method - no authentication for testing socksmethod: none +clientmethod: none + +user.privileged: root +user.unprivileged: nobody -# Clients allowed to connect (all for testing) client pass { from: 0.0.0.0/0 to: 0.0.0.0/0 log: error } -# Rules for SOCKS requests socks pass { from: 0.0.0.0/0 to: 0.0.0.0/0 protocol: tcp udp - method: none + socksmethod: none log: error }