[icedtea-web] PR1474: Can't get javaws to use SOCKS proxy

[Jiri Vanek]

On 09/23/2013 07:46 PM, Omair Majid wrote:
> Hi,
>
> The attached patch improves how manual SOCKS proxy configuration is
> handled. Specifically, if there is a socks proxy specified, it is used
> even for http(s) and ftp, unless a more specific proxy for those is
> specified. 'sameProxy' now affects the https/http/ftp protocols, but not
> the socket protocol.
>
> Okay to push?
>
> Thanks,
> Omair
> -- PGP Key: 66484681 (http://pgp.mit.edu/) Fingerprint = F072 555B 0A17 3957 4E95 0056 F286 F14F 6648 4681
>
>
> pr1474-01.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> --- a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> +++ b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> @@ -129,6 +129,7 @@
>
>       /** the proxy type. possible values are {@code JNLPProxySelector.PROXY_TYPE_*} */
>       public static final String KEY_PROXY_TYPE = "deployment.proxy.type";

I would add an empty line here , just for readibility

> +    /** if true, the http host/port should be used https and ftp as well */
>       public static final String KEY_PROXY_SAME = "deployment.proxy.same";
>       public static final String KEY_PROXY_AUTO_CONFIG_URL = "deployment.proxy.auto.config.url";
>       public static final String KEY_PROXY_BYPASS_LIST = "deployment.proxy.bypass.list";
> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPProxySelector.java b/netx/net/sourceforge/jnlp/runtime/JNLPProxySelector.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPProxySelector.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPProxySelector.java
> @@ -298,27 +298,28 @@
>           String scheme = uri.getScheme();
>
>           if (sameProxy) {
> -            SocketAddress sa = new InetSocketAddress(proxyHttpHost, proxyHttpPort);
> -            Proxy proxy;
> -            if (scheme.equals("socket")) {
> -                proxy = new Proxy(Type.SOCKS, sa);
> -            } else {
> -                proxy = new Proxy(Type.HTTP, sa);
> +            if (proxyHttpHost != null&&  (scheme.equals("https") || scheme.equals("http") || scheme.equals("ftp"))) {
> +                SocketAddress sa = new InetSocketAddress(proxyHttpHost, proxyHttpPort);
> +                Proxy proxy = new Proxy(Type.HTTP, sa);
> +                proxies.add(proxy);
>               }
> -            proxies.add(proxy);
> -        } else if (scheme.equals("http")) {
> +        } else if (scheme.equals("http")&&  proxyHttpHost != null) {

There should be spaces before &&, but I do not inissts. Is the null check really wonted? Is not NPE better ?

>               SocketAddress sa = new InetSocketAddress(proxyHttpHost, proxyHttpPort);
>               proxies.add(new Proxy(Type.HTTP, sa));
> -        } else if (scheme.equals("https")) {
> +        } else if (scheme.equals("https")&&  proxyHttpsHost != null) {
>               SocketAddress sa = new InetSocketAddress(proxyHttpsHost, proxyHttpsPort);
>               proxies.add(new Proxy(Type.HTTP, sa));
> -        } else if (scheme.equals("ftp")) {
> +        } else if (scheme.equals("ftp")&&  proxyFtpHost != null) {
>               SocketAddress sa = new InetSocketAddress(proxyFtpHost, proxyFtpPort);
>               proxies.add(new Proxy(Type.HTTP, sa));
> -        } else if (scheme.equals("socket")) {
> +        }
> +
> +        if (proxySocks4Host != null) {
>               SocketAddress sa = new InetSocketAddress(proxySocks4Host, proxySocks4Port);
>               proxies.add(new Proxy(Type.SOCKS, sa));
> -        } else {

Is this removed else intentional? " but not the socket protocol" ?
> +        }
> +
> +        if (proxies.size() == 0) {
>               proxies.add(Proxy.NO_PROXY);
>           }
>
> diff --git a/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPProxySelectorTest.java b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPProxySelectorTest.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPProxySelectorTest.java
> +++ b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPProxySelectorTest.java
> @@ -45,7 +45,6 @@
>   import java.net.InetSocketAddress;
>   import java.net.Proxy;
>   import java.net.Proxy.Type;
> -import java.net.SocketAddress;
>   import java.net.URI;
>   import java.net.URISyntaxException;
>   import java.net.UnknownHostException;
> @@ -116,9 +115,11 @@
>           assertEquals(Proxy.NO_PROXY, result.get(0));
>       }
>
> +    // TODO implement this
> +    @Ignore("Implement this")
hmhm - how did test suite survived ignored test? /me probably never tested. But I think if xmls/htmls/dailyreport will crash on ot, then it is best reason to finally fix them :)
Known to fial is not option here?

>       @Test
>       public void testLocalProxyBypassListIsIgnoredForNonLocal() {
> -
> +        fail();
>       }
>
>       @Test
> @@ -210,6 +211,23 @@
>       }
>
>       @Test
> +    public void testHttpFallsBackToManualSocksProxy() throws URISyntaxException {
> +        String SOCKS_HOST = "example.org";
> +        int SOCKS_PORT = 42;
> +
> +        DeploymentConfiguration config = new DeploymentConfiguration();
> +        config.setProperty(DeploymentConfiguration.KEY_PROXY_TYPE, String.valueOf(JNLPProxySelector.PROXY_TYPE_MANUAL));
> +        config.setProperty(DeploymentConfiguration.KEY_PROXY_SOCKS4_HOST, SOCKS_HOST);
> +        config.setProperty(DeploymentConfiguration.KEY_PROXY_SOCKS4_PORT, String.valueOf(SOCKS_PORT));
> +
> +        JNLPProxySelector selector = new TestProxySelector(config);
> +        List<Proxy>  result = selector.select(new URI("http://example.org/"));
> +
> +        assertEquals(1, result.size());
> +        assertEquals(new Proxy(Type.SOCKS, new InetSocketAddress(SOCKS_HOST, SOCKS_PORT)), result.get(0));
> +    }
> +
> +    @Test
>       public void testManualUnknownProtocolProxy() throws URISyntaxException {
>           DeploymentConfiguration config = new DeploymentConfiguration();
>           config.setProperty(DeploymentConfiguration.KEY_PROXY_TYPE, String.valueOf(JNLPProxySelector.PROXY_TYPE_MANUAL));
> @@ -239,11 +257,6 @@
>
>           assertEquals(1, result.size());
>           assertEquals(new Proxy(Type.HTTP, new InetSocketAddress(HTTP_HOST, HTTP_PORT)), result.get(0));
> -
> -        result = selector.select(new URI("socket://example.org/"));
> -
> -        assertEquals(1, result.size());
> -        assertEquals(new Proxy(Type.SOCKS, new InetSocketAddress(HTTP_HOST, HTTP_PORT)), result.get(0));
>       }
>
>       @Test

Otherwise good enough to go. Are you planing also 1,4 backport?

Thank you for this,
   J.