Repository: netty/netty
Issue: When more than one connection header is present in h2c upgrade request, upgrade fails
Labels: defect

### Expected behavior
$subject occurs, with this change https://github.com/netty/netty/pull/7824. (@bryce-anderson and @normanmaurer )

Now that more than one connection header is allowed with this https://github.com/netty/netty/pull/7824,
 line https://github.com/netty/netty/blob/737519314153f9146eaf532cd4e945a621cf3fa5/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java#L295 returns false and the upgrade fails since the connection header value returned from this is not the upgrade but some other connection header value. 

If we are allowing multiple connection headers, then in HttpServerUpgradeHandler I think we need to check  whether any of the connection header value is upgrade and then remove other connection headers. https://tools.ietf.org/html/rfc7540#section-8.1.2.2

### Steps to reproduce
Send an upgrade request with multiple conection headers to HTTP/2 server and the request hangs.

Example request:
GET / HTTP/1.1
Host: localhost:9000
connection: keep-alive
forwarded: by=127.0.0.1
content-length: 0
upgrade: h2c
HTTP2-Settings: AAEAABAAAAIAAAABAAN_____AAQAAP__AAUAAEAAAAYAACAA
connection: HTTP2-Settings,upgrade

### Netty version
From 4.1.33.Final (This issue  started to occur from From 4.1.23.Final onwards)




Issue comments:
@arukshani sounds about right... Are you interested in providing a PR to fix it ? /cc @bryce-anderson 
Yup. Will send a PR with the fix @normanmaurer 
Sent the PR https://github.com/netty/netty/pull/8848 to fix this.

Pull request review comments:
This requires that Upgrade exists in the same Connection header value as requiredHeaders. But each value could be put in its own Connection header. It would be better to join all the values together with a `,` and then splitHeader(). @normanmaurer, is there not a utility to do that?
@ejona86 thats a good point... I think there is no such utility yet (or at least I could not remember and also could not find any). /cc @arukshani can you do the adjustment and also add a unit test for this ?
@ejona86 and @normanmaurer I have done the necessary changes and added a unit test as well for that. Please review.
call `concatenatedConnectionValue.setLength(concatenatedConnectionValue.length() -1)` to strip of the last comma
@arukshani seems like the test code is completely identical except the different `upgradeString`. Consider moving all the code into an extra method and just pass in the `upgradeString` from both of the test methods to remove all the code-duplication
Done
@normanmaurer Done
nit: you could also move all the `setUpServerChannel()` calls
Done

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
index f1f3efcb1de..2b54b0e4b21 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
@@ -14,9 +14,6 @@
  */
 package io.netty.handler.codec.http;
 
-import static io.netty.util.AsciiString.containsContentEqualsIgnoreCase;
-import static io.netty.util.AsciiString.containsAllContentEqualsIgnoreCase;
-
 import io.netty.buffer.Unpooled;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
@@ -30,7 +27,10 @@
 
 import static io.netty.handler.codec.http.HttpResponseStatus.SWITCHING_PROTOCOLS;
 import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
+import static io.netty.util.AsciiString.containsAllContentEqualsIgnoreCase;
+import static io.netty.util.AsciiString.containsContentEqualsIgnoreCase;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
+import static io.netty.util.internal.StringUtil.COMMA;
 
 /**
  * A server-side handler that receives HTTP requests and optionally performs a protocol switch if
@@ -284,16 +284,23 @@ private boolean upgrade(final ChannelHandlerContext ctx, final FullHttpRequest r
         }
 
         // Make sure the CONNECTION header is present.
-        CharSequence connectionHeader = request.headers().get(HttpHeaderNames.CONNECTION);
-        if (connectionHeader == null) {
+        List<String> connectionHeaderValues = request.headers().getAll(HttpHeaderNames.CONNECTION);
+
+        if (connectionHeaderValues == null) {
             return false;
         }
 
+        final StringBuilder concatenatedConnectionValue = new StringBuilder(connectionHeaderValues.size() * 10);
+        for (CharSequence connectionHeaderValue : connectionHeaderValues) {
+            concatenatedConnectionValue.append(connectionHeaderValue).append(COMMA);
+        }
+        concatenatedConnectionValue.setLength(concatenatedConnectionValue.length() - 1);
+
         // Make sure the CONNECTION header contains UPGRADE as well as all protocol-specific headers.
         Collection<CharSequence> requiredHeaders = upgradeCodec.requiredUpgradeHeaders();
-        List<CharSequence> values = splitHeader(connectionHeader);
+        List<CharSequence> values = splitHeader(concatenatedConnectionValue);
         if (!containsContentEqualsIgnoreCase(values, HttpHeaderNames.UPGRADE) ||
-            !containsAllContentEqualsIgnoreCase(values, requiredHeaders)) {
+                !containsAllContentEqualsIgnoreCase(values, requiredHeaders)) {
             return false;
         }
 


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/CleartextHttp2ServerUpgradeHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/CleartextHttp2ServerUpgradeHandlerTest.java
index a544054b668..129f62d7574 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/CleartextHttp2ServerUpgradeHandlerTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/CleartextHttp2ServerUpgradeHandlerTest.java
@@ -42,8 +42,15 @@
 import java.util.ArrayList;
 import java.util.List;
 
-import static org.junit.Assert.*;
-import static org.mockito.Mockito.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 
 /**
  * Tests for {@link CleartextHttp2ServerUpgradeHandler}
@@ -112,47 +119,35 @@ public void priorKnowledge() throws Exception {
 
     @Test
     public void upgrade() throws Exception {
-        setUpServerChannel();
-
         String upgradeString = "GET / HTTP/1.1\r\n" +
-                               "Host: example.com\r\n" +
-                               "Connection: Upgrade, HTTP2-Settings\r\n" +
-                               "Upgrade: h2c\r\n" +
-                               "HTTP2-Settings: AAMAAABkAAQAAP__\r\n\r\n";
-        ByteBuf upgrade = Unpooled.copiedBuffer(upgradeString, CharsetUtil.US_ASCII);
-
-        assertFalse(channel.writeInbound(upgrade));
-
-        assertEquals(1, userEvents.size());
-
-        Object userEvent = userEvents.get(0);
-        assertTrue(userEvent instanceof UpgradeEvent);
-        assertEquals("h2c", ((UpgradeEvent) userEvent).protocol());
-        ReferenceCountUtil.release(userEvent);
-
-        assertEquals(100, http2ConnectionHandler.connection().local().maxActiveStreams());
-        assertEquals(65535, http2ConnectionHandler.connection().local().flowController().initialWindowSize());
-
-        assertEquals(1, http2ConnectionHandler.connection().numActiveStreams());
-        assertNotNull(http2ConnectionHandler.connection().stream(1));
-
-        Http2Stream stream = http2ConnectionHandler.connection().stream(1);
-        assertEquals(State.HALF_CLOSED_REMOTE, stream.state());
-        assertFalse(stream.isHeadersSent());
-
-        String expectedHttpResponse = "HTTP/1.1 101 Switching Protocols\r\n" +
-                "connection: upgrade\r\n" +
-                "upgrade: h2c\r\n\r\n";
-        ByteBuf responseBuffer = channel.readOutbound();
-        assertEquals(expectedHttpResponse, responseBuffer.toString(CharsetUtil.UTF_8));
-        responseBuffer.release();
+                "Host: example.com\r\n" +
+                "Connection: Upgrade, HTTP2-Settings\r\n" +
+                "Upgrade: h2c\r\n" +
+                "HTTP2-Settings: AAMAAABkAAQAAP__\r\n\r\n";
+        validateClearTextUpgrade(upgradeString);
+    }
 
-        // Check that the preface was send (a.k.a the settings frame)
-        ByteBuf settingsBuffer = channel.readOutbound();
-        assertNotNull(settingsBuffer);
-        settingsBuffer.release();
+    @Test
+    public void upgradeWithMultipleConnectionHeaders() {
+        String upgradeString = "GET / HTTP/1.1\r\n" +
+                "Host: example.com\r\n" +
+                "Connection: keep-alive\r\n" +
+                "Connection: Upgrade, HTTP2-Settings\r\n" +
+                "Upgrade: h2c\r\n" +
+                "HTTP2-Settings: AAMAAABkAAQAAP__\r\n\r\n";
+        validateClearTextUpgrade(upgradeString);
+    }
 
-        assertNull(channel.readOutbound());
+    @Test
+    public void requiredHeadersInSeparateConnectionHeaders() {
+        String upgradeString = "GET / HTTP/1.1\r\n" +
+                "Host: example.com\r\n" +
+                "Connection: keep-alive\r\n" +
+                "Connection: HTTP2-Settings\r\n" +
+                "Connection: Upgrade\r\n" +
+                "Upgrade: h2c\r\n" +
+                "HTTP2-Settings: AAMAAABkAAQAAP__\r\n\r\n";
+        validateClearTextUpgrade(upgradeString);
     }
 
     @Test
@@ -254,4 +249,43 @@ private static ByteBuf settingsFrameBuf() {
     private static Http2Settings expectedSettings() {
         return new Http2Settings().maxConcurrentStreams(100).initialWindowSize(65535);
     }
+
+    private void validateClearTextUpgrade(String upgradeString) {
+        setUpServerChannel();
+
+        ByteBuf upgrade = Unpooled.copiedBuffer(upgradeString, CharsetUtil.US_ASCII);
+
+        assertFalse(channel.writeInbound(upgrade));
+
+        assertEquals(1, userEvents.size());
+
+        Object userEvent = userEvents.get(0);
+        assertTrue(userEvent instanceof UpgradeEvent);
+        assertEquals("h2c", ((UpgradeEvent) userEvent).protocol());
+        ReferenceCountUtil.release(userEvent);
+
+        assertEquals(100, http2ConnectionHandler.connection().local().maxActiveStreams());
+        assertEquals(65535, http2ConnectionHandler.connection().local().flowController().initialWindowSize());
+
+        assertEquals(1, http2ConnectionHandler.connection().numActiveStreams());
+        assertNotNull(http2ConnectionHandler.connection().stream(1));
+
+        Http2Stream stream = http2ConnectionHandler.connection().stream(1);
+        assertEquals(State.HALF_CLOSED_REMOTE, stream.state());
+        assertFalse(stream.isHeadersSent());
+
+        String expectedHttpResponse = "HTTP/1.1 101 Switching Protocols\r\n" +
+                "connection: upgrade\r\n" +
+                "upgrade: h2c\r\n\r\n";
+        ByteBuf responseBuffer = channel.readOutbound();
+        assertEquals(expectedHttpResponse, responseBuffer.toString(CharsetUtil.UTF_8));
+        responseBuffer.release();
+
+        // Check that the preface was send (a.k.a the settings frame)
+        ByteBuf settingsBuffer = channel.readOutbound();
+        assertNotNull(settingsBuffer);
+        settingsBuffer.release();
+
+        assertNull(channel.readOutbound());
+    }
 }