Repository: netty/netty (Java)

Issue:
When more than one connection header is present in h2c upgrade request, upgrade fails
### 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)





Hints:
@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 

Patch:
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
--- 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
--- 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());
+    }
 }