Repository: netty/netty
Issue: HttpClientUpgradeHandler may clobber existing connection headers
Labels: defect

### Expected behavior
`Connection` headers on a request that goes through the `HttpClientUpgradeHandler` should be preserved.

### Actual behavior
They are clobbered by the `Connection: Upgrade` header.

### Minimal yet complete reproducer code (or URL to code)
Test case for `HttpClientUpgradeHandlerTest`:
```java
@Test
    public void dontStripConnectionHeaders() {
        HttpClientUpgradeHandler.SourceCodec sourceCodec = new FakeSourceCodec();
        HttpClientUpgradeHandler.UpgradeCodec upgradeCodec = new FakeUpgradeCodec();
        HttpClientUpgradeHandler handler = new HttpClientUpgradeHandler(sourceCodec, upgradeCodec, 1024);
        UserEventCatcher catcher = new UserEventCatcher();
        EmbeddedChannel channel = new EmbeddedChannel(catcher);
        channel.pipeline().addFirst("upgrade", handler);

        DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "netty.io");
        request.headers().add("connection", "extra");
        request.headers().add("extra", "value");
        assertTrue(channel.writeOutbound(request));
        FullHttpRequest readRequest = channel.readOutbound();

        List<String> connectionHeaders = readRequest.headers().getAll("connection");
        assertTrue(connectionHeaders.contains("extra"));
    }
```

### Netty version
branch 4.1, SHA 5ebee9426f05b0, March 27.

### JVM version (e.g. `java -version`)
openjdk version "1.8.0_162"

### OS version (e.g. `uname -a`)
Darwin Kernel Version 17.5.0

Issue comments:
The problem is pretty simple, it is that we're using `.set(CONNECTION, value)` [here](https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java#L280) where we should probably be using `.add(CONNECTION, value)`.

I'll open a PR, and if it turns out I've misunderstood the spec, I'll close it. 😄

Pull request review comments:
You need to release this one
Also call channel.finish() and assert the return value

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java
index 235dbb1b5d8..bf2cd5529ac 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java
@@ -277,6 +277,6 @@ private void setUpgradeRequestHeaders(ChannelHandlerContext ctx, HttpRequest req
             builder.append(',');
         }
         builder.append(HttpHeaderValues.UPGRADE);
-        request.headers().set(HttpHeaderNames.CONNECTION, builder.toString());
+        request.headers().add(HttpHeaderNames.CONNECTION, builder.toString());
     }
 }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientUpgradeHandlerTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientUpgradeHandlerTest.java
index e342ef73e28..a90ee575dd9 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientUpgradeHandlerTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientUpgradeHandlerTest.java
@@ -21,6 +21,7 @@
 
 import java.util.Collection;
 import java.util.Collections;
+import java.util.List;
 
 import org.junit.Test;
 
@@ -174,4 +175,25 @@ public void testEarlyBailout() {
         assertEquals(HttpResponseStatus.OK, response.status());
         assertFalse(channel.finish());
     }
+
+    @Test
+    public void dontStripConnectionHeaders() {
+        HttpClientUpgradeHandler.SourceCodec sourceCodec = new FakeSourceCodec();
+        HttpClientUpgradeHandler.UpgradeCodec upgradeCodec = new FakeUpgradeCodec();
+        HttpClientUpgradeHandler handler = new HttpClientUpgradeHandler(sourceCodec, upgradeCodec, 1024);
+        UserEventCatcher catcher = new UserEventCatcher();
+        EmbeddedChannel channel = new EmbeddedChannel(catcher);
+        channel.pipeline().addFirst("upgrade", handler);
+
+        DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "netty.io");
+        request.headers().add("connection", "extra");
+        request.headers().add("extra", "value");
+        assertTrue(channel.writeOutbound(request));
+        FullHttpRequest readRequest = channel.readOutbound();
+
+        List<String> connectionHeaders = readRequest.headers().getAll("connection");
+        assertTrue(connectionHeaders.contains("extra"));
+        assertTrue(readRequest.release());
+        assertFalse(channel.finish());
+    }
 }