Repository: netty/netty
Issue: HttpProxyHandler incorrectly formats IPv6 host and port in CONNECT request
Labels: defect

### Expected behavior
The HttpProxyHandler is expected to be capable of issuing a valid CONNECT request for a tunneled connection to an IPv6 host. In this case we are passing an IPv6 address (eg fd00:c0de:42::c:293a:5736) rather than a host name.

### Actual behavior
The HttpProxyHandler does not properly concatenate the IPv6 address and port. The resulting error after we fail to connect will show you the problem:

```
io.netty.handler.proxy.ProxyConnectException: http, none, /fd00:c0de:42:0:5:562d:d54:1:3128 => /fd00:c0de:42:0:50:5694:2fda:1:4287, status: 503 Service Unavailable
```

In both cases you can see the IPv6 address is formatted without brackets: `fd00:c0de:42:0:50:5694:2fda:1:4287` should be `[fd00:c0de:42:0:50:5694:2fda:1]:4287`. This is just an exception message so it doesn't prove it's formatting incorrectly. However, if you look at the request on the wire you can see it is certainly wrong:

```
    CONNECT fd00:c0de:42:0:50:5694:2fda:1:4287 HTTP/1.1\r\n
    host: fd00:c0de:42:0:50:5694:2fda:1:4287\r\n
    \r\n
```

Here is the problem method from HttpProxyHandler:

```
    @Override
    protected Object newInitialMessage(ChannelHandlerContext ctx) throws Exception {
        InetSocketAddress raddr = destinationAddress();
        String rhost;
        if (raddr.isUnresolved()) {
            rhost = raddr.getHostString();
        } else {
            rhost = raddr.getAddress().getHostAddress();
        }

        final String host = rhost + ':' + raddr.getPort();
        FullHttpRequest req = new DefaultFullHttpRequest(
                HttpVersion.HTTP_1_1, HttpMethod.CONNECT,
                host,
                Unpooled.EMPTY_BUFFER, false);

        req.headers().set(HttpHeaderNames.HOST, host);

        if (authorization != null) {
            req.headers().set(HttpHeaderNames.PROXY_AUTHORIZATION, authorization);
        }

        return req;
    }
```

Specifically: ` final String host = rhost + ':' + raddr.getPort();`

### Steps to reproduce
* Setup an HTTP Proxy with IPv6.
* Setup a target server with an IPv6 address.
* Attempt to establish a connection to the target server through the proxy, giving the HttpProxyHandler an IPv6 IP address for the target.
** We can successfully connect to the proxy server with IPv6. The problem seems to be specific to the target of the tunneled connection using an IPv6 address.

### Minimal yet complete reproducer code (or URL to code)
* I am not able to create a reproducer or patch with my company at this time. I think the issue is relatively straightforward though.

### Netty version
4.1.4.Final

### JVM version (e.g. `java -version`)
Java 8 update 92

### OS version (e.g. `uname -a`)
Attempted on Ubuntu Linux 14.04 and Mac OSX. I don't think the OS matters in this case.

Issue comments:
@ZuluForce maybe you can come up with a PR that fixes the issue for you ?
We should consider using [toAddressString](https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/NetUtil.java#L948) for the address portion which follows the RFC 5952 formatting (last time I checked java didn't). We could also add a method to format a address+port which handles ipv6 and ipv4 correctly ... this is common enough.
@Scottmitch +1 Want to take a stab ?
Thanks for the replies @normanmaurer  and @Scottmitch. I would need to get the time from my employer to work on this and I currently don't have the Netty development environment setup or approval for the CCLA. If I could submit something it wouldn't be until after the holidays (ie early to mid January).
@ZuluForce @Scottmitch let me take care.
@ZuluForce PTAL https://github.com/netty/netty/pull/6195
Fixed by https://github.com/netty/netty/pull/6195

Pull request review comments:
I wonder if it would should be more explicit about the string concatenation operations used here. For example it seems unlikely JIT will be able to combine all of these conditional concatenation operations into a single `StringBuilder`. Can we create a single `StringBuilder` and manually do the `append` calls (e.g. including what is now building `rhost`) to be sure we don't create multiple `String`/`StringBuilder` operations?
`enought` -> `enough`
also remove the space before the period?
`enought` -> `enough`

also remove the space before the period?
nit: no need for `else`
I added the space before to make it clear it is not part of it... But I guess its ok to remove it as well.

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/NetUtil.java b/common/src/main/java/io/netty/util/NetUtil.java
index a645cd70d63..a442ff4ce9b 100644
--- a/common/src/main/java/io/netty/util/NetUtil.java
+++ b/common/src/main/java/io/netty/util/NetUtil.java
@@ -25,6 +25,7 @@
 import java.net.Inet4Address;
 import java.net.Inet6Address;
 import java.net.InetAddress;
+import java.net.InetSocketAddress;
 import java.net.NetworkInterface;
 import java.net.SocketException;
 import java.net.UnknownHostException;
@@ -933,6 +934,38 @@ public static Inet6Address getByName(CharSequence ip, boolean ipv4Mapped) {
         }
     }
 
+    /**
+     * Returns the {@link String} representation of an {@link InetSocketAddress}.
+     * <p>
+     * The output does not include Scope ID.
+     * @param addr {@link InetSocketAddress} to be converted to an address string
+     * @return {@code String} containing the text-formatted IP address
+     */
+    public static String toSocketAddressString(InetSocketAddress addr) {
+        String port = String.valueOf(addr.getPort());
+        final StringBuilder sb;
+
+        if (addr.isUnresolved()) {
+            String hostString = PlatformDependent.javaVersion() >= 7 ? addr.getHostString() : addr.getHostName();
+            sb = newSocketAddressStringBuilder(hostString, port, !isValidIpV6Address(hostString));
+        } else {
+            InetAddress address = addr.getAddress();
+            String hostString = toAddressString(address);
+            sb = newSocketAddressStringBuilder(hostString, port, address instanceof Inet4Address);
+        }
+        return sb.append(':').append(port).toString();
+    }
+
+    private static StringBuilder newSocketAddressStringBuilder(String hostString, String port, boolean ipv4) {
+        if (ipv4) {
+            // Need to include enough space for hostString:port.
+            return new StringBuilder(hostString.length() + 1 + port.length()).append(hostString);
+        }
+        // Need to include enough space for [hostString]:port.
+        return new StringBuilder(
+                hostString.length() + 3 + port.length()).append('[').append(hostString).append(']');
+    }
+
     /**
      * Returns the {@link String} representation of an {@link InetAddress}.
      * <ul>
diff --git a/handler-proxy/src/main/java/io/netty/handler/proxy/HttpProxyHandler.java b/handler-proxy/src/main/java/io/netty/handler/proxy/HttpProxyHandler.java
index 53d6fbb823d..aecf5f3e7ee 100644
--- a/handler-proxy/src/main/java/io/netty/handler/proxy/HttpProxyHandler.java
+++ b/handler-proxy/src/main/java/io/netty/handler/proxy/HttpProxyHandler.java
@@ -32,6 +32,7 @@
 import io.netty.handler.codec.http.LastHttpContent;
 import io.netty.util.AsciiString;
 import io.netty.util.CharsetUtil;
+import io.netty.util.NetUtil;
 
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
@@ -112,14 +113,7 @@ protected void removeDecoder(ChannelHandlerContext ctx) throws Exception {
     @Override
     protected Object newInitialMessage(ChannelHandlerContext ctx) throws Exception {
         InetSocketAddress raddr = destinationAddress();
-        String rhost;
-        if (raddr.isUnresolved()) {
-            rhost = raddr.getHostString();
-        } else {
-            rhost = raddr.getAddress().getHostAddress();
-        }
-
-        final String host = rhost + ':' + raddr.getPort();
+        final String host = NetUtil.toSocketAddressString(raddr);
         FullHttpRequest req = new DefaultFullHttpRequest(
                 HttpVersion.HTTP_1_1, HttpMethod.CONNECT,
                 host,


Test patch:
diff --git a/common/src/test/java/io/netty/util/NetUtilTest.java b/common/src/test/java/io/netty/util/NetUtilTest.java
index aa04b2a4c04..09afacca357 100644
--- a/common/src/test/java/io/netty/util/NetUtilTest.java
+++ b/common/src/test/java/io/netty/util/NetUtilTest.java
@@ -18,6 +18,7 @@
 import org.junit.Test;
 
 import java.net.InetAddress;
+import java.net.InetSocketAddress;
 import java.net.UnknownHostException;
 import java.util.HashMap;
 import java.util.Map;
@@ -27,6 +28,7 @@
 import static io.netty.util.NetUtil.createByteArrayFromIpAddressString;
 import static io.netty.util.NetUtil.getByName;
 import static io.netty.util.NetUtil.toAddressString;
+import static io.netty.util.NetUtil.toSocketAddressString;
 import static org.junit.Assert.*;
 
 public class NetUtilTest {
@@ -500,6 +502,22 @@ public void testinvalidIpv4MappedIp6GetByName() {
         }
     }
 
+    @Test
+    public void testIp6InetSocketAddressToString() throws UnknownHostException {
+        for (Entry<byte[], String> testEntry : ipv6ToAddressStrings.entrySet()) {
+            assertEquals('[' + testEntry.getValue() + "]:9999",
+                    toSocketAddressString(new InetSocketAddress(InetAddress.getByAddress(testEntry.getKey()), 9999)));
+        }
+    }
+
+    @Test
+    public void testIp4SocketAddressToString() throws UnknownHostException {
+        for (Entry<String, String> e : validIpV4Hosts.entrySet()) {
+            assertEquals(e.getKey() + ":9999",
+                    toSocketAddressString(new InetSocketAddress(InetAddress.getByAddress(unhex(e.getValue())), 9999)));
+        }
+    }
+
     private static void assertHexDumpEquals(String expected, byte[] actual) {
         assertEquals(expected, hex(actual));
     }
diff --git a/handler-proxy/src/test/java/io/netty/handler/proxy/HttpProxyHandlerTest.java b/handler-proxy/src/test/java/io/netty/handler/proxy/HttpProxyHandlerTest.java
new file mode 100644
index 00000000000..61fd6911a0a
--- /dev/null
+++ b/handler-proxy/src/test/java/io/netty/handler/proxy/HttpProxyHandlerTest.java
@@ -0,0 +1,80 @@
+/*
+ * Copyright 2016 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.handler.proxy;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelPromise;
+import io.netty.handler.codec.http.FullHttpRequest;
+import io.netty.handler.codec.http.HttpHeaderNames;
+import io.netty.handler.codec.http.HttpVersion;
+import io.netty.util.NetUtil;
+import org.junit.Test;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.*;
+
+public class HttpProxyHandlerTest {
+
+    @Test
+    public void testIpv6() throws Exception  {
+        InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("::1"), 8080);
+        testInitialMessage(socketAddress, "[::1]:8080");
+    }
+
+    @Test
+    public void testIpv6Unresolved() throws Exception  {
+        InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("::1", 8080);
+        testInitialMessage(socketAddress, "[::1]:8080");
+    }
+
+    @Test
+    public void testIpv4() throws Exception  {
+        InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("10.0.0.1"), 8080);
+        testInitialMessage(socketAddress, "10.0.0.1:8080");
+    }
+
+    @Test
+    public void testIpv4Unresolved() throws Exception  {
+        InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("10.0.0.1", 8080);
+        testInitialMessage(socketAddress, "10.0.0.1:8080");
+    }
+
+    private static void testInitialMessage(InetSocketAddress socketAddress, String expected) throws Exception  {
+        InetSocketAddress proxyAddress = new InetSocketAddress(NetUtil.LOCALHOST, 8080);
+
+        ChannelPromise promise = mock(ChannelPromise.class);
+        verifyNoMoreInteractions(promise);
+
+        ChannelHandlerContext ctx = mock(ChannelHandlerContext.class);
+        when(ctx.connect(same(proxyAddress), isNull(InetSocketAddress.class), same(promise))).thenReturn(promise);
+
+        HttpProxyHandler handler = new HttpProxyHandler(new InetSocketAddress(NetUtil.LOCALHOST, 8080));
+        handler.connect(ctx, socketAddress, null, promise);
+
+        FullHttpRequest request = (FullHttpRequest) handler.newInitialMessage(ctx);
+        try {
+            assertEquals(HttpVersion.HTTP_1_1, request.protocolVersion());
+            assertEquals(expected, request.uri());
+            assertEquals(expected, request.headers().get(HttpHeaderNames.HOST));
+        } finally {
+            request.release();
+        }
+        verify(ctx).connect(proxyAddress, null, promise);
+    }
+}