Repository: netty/netty
Issue: HttpConversionUtil.toHttp2Headers should strip http/2 incompatible headers
Labels: defect

### Expected behavior
When servers receive an h2c upgrade request, it's an http/1.1 message, and it quickly gets turned into an h2c message.

### Actual behavior
If the upgrade request has illegal headers, like `te: deflate,gzip;q=0.3` then it doesn't handle it gracefully and throws an exception.

```
W 1030 21:49:47.059 THREAD175 com.twitter.finagle.netty4.channel.ChannelExceptionHandler.exceptionCaught: Unhandled exception in connection with /10.56.221.119:54149, shutting down connection
java.lang.IllegalArgumentException: Invalid value for te: deflate,gzip;q=0.3
        at io.netty.handler.codec.http2.HttpConversionUtil.toHttp2Headers(HttpConversionUtil.java:423)
        at io.netty.handler.codec.http2.HttpConversionUtil.toHttp2Headers(HttpConversionUtil.java:399)
        at io.netty.handler.codec.http2.InboundHttpToHttp2Adapter.handle(InboundHttpToHttp2Adapter.java:61)
        at io.netty.handler.codec.http2.Http2FrameCodec.userEventTriggered(Http2FrameCodec.java:250)
        at io.netty.channel.AbstractChannelHandlerContext.invokeUserEventTriggered(AbstractChannelHandlerContext.java:329)
        at io.netty.channel.AbstractChannelHandlerContext.invokeUserEventTriggered(AbstractChannelHandlerContext.java:315)
        at io.netty.channel.AbstractChannelHandlerContext.fireUserEventTriggered(AbstractChannelHandlerContext.java:307)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler$1.operationComplete(HttpServerUpgradeHandler.java:329)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler$1.operationComplete(HttpServerUpgradeHandler.java:318)
        at io.netty.util.concurrent.DefaultPromise.notifyListener0(DefaultPromise.java:507)
        at io.netty.util.concurrent.DefaultPromise.notifyListenersNow(DefaultPromise.java:481)
        at io.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:420)
        at io.netty.util.concurrent.DefaultPromise.addListener(DefaultPromise.java:163)
        at io.netty.channel.DefaultChannelPromise.addListener(DefaultChannelPromise.java:93)
        at io.netty.channel.DefaultChannelPromise.addListener(DefaultChannelPromise.java:28)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler.upgrade(HttpServerUpgradeHandler.java:318)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler.decode(HttpServerUpgradeHandler.java:239)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler.decode(HttpServerUpgradeHandler.java:40)
        at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:88)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
        at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:438)
        at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:310)
        at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:284)
        at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:253)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
        at io.netty.channel.ChannelInboundHandlerAdapter.channelRead(ChannelInboundHandlerAdapter.java:86)
        at com.twitter.finagle.netty4.channel.ChannelStatsHandler.channelRead(ChannelStatsHandler.scala:106)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
        at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1359)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
        at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1359)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:935)
        at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:797)
        at io.netty.channel.epoll.AbstractEpollChannel$AbstractEpollUnsafe$1.run(AbstractEpollChannel.java:412)
        at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163)
        at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:403)
        at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:309)
        at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at com.twitter.finagle.util.BlockingTimeTrackingThreadFactory$$anon$1.run(BlockingTimeTrackingThreadFactory.scala:23)
        at java.lang.Thread.run(Thread.java:748)
```

### Steps to reproduce
Send an h2c upgrade request to a server that has illegal h2c headers (like `te: deflate,gzip;q=0.3`).

### Minimal yet complete reproducer code (or URL to code)
Haven't had a chance to repro

### Netty version
4.1.16.Final

### JVM version (e.g. `java -version`)
jdk8

### OS version (e.g. `uname -a`)
Darwin tw-mbp13-mnakamura.local 16.7.0 Darwin Kernel Version 16.7.0: Thu Jun 15 17:36:27 PDT 2017; root:xnu-3789.70.16~2/RELEASE_X86_64 x86_64

### workaround
I haven't found a workaround for this, so it would be lovely if you had any ideas here.  I think it just needs to be fixed in netty.  Here's what we normally do in finagle: 
https://github.com/twitter/finagle/blob/develop/finagle-http2/src/main/scala/com/twitter/finagle/http2/transport/RichHttp2ServerDowngrader.scala#L10-L20

Issue comments:
@mosesn sounds right... want to do a PR against `HttpConversionUtil.toHttp2Headers ` 
we could enhance the conversion to extract out the `trailers` value from the `TE` header instead of just throwing. +1 for a PR.
I made a PR that extracted the trailers value and also cleaned up the behavior around "connection", which I think was wrong.  https://github.com/netty/netty/pull/7399
Fixed by https://github.com/netty/netty/pull/7399

Pull request review comments:
should we use the constructor that takes the `arraySizeHint` based on `values.size()` ?
consider using an "old for" loop if the List implements `RandomAccess` to reduce GC pressure (as the foreach thingy you use here will always create an Iterator)
While it creates an iterator, it can be allocated on the stack. (Whether it _actually_ is allocated on the stack is a different discussion.)
Don't you need also need to split value on `,` and trim whitespace?
@ejona86 we saw GC issues caused by this in the past so better safe then sorry ;)
looks like it's a linked list: https://github.com/netty/netty/blob/4.1/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java#L163
Do we need to worry about trimming this? HTTP parsers will generally naturally remove whitespace (but aren't guaranteed to), but that doesn't happen in HTTP/2 (from what I've seen; maybe Netty does).
It doesn't seem like this should be special-cased or that all empty strings should be handled (by ignoring them), assuming that's its purpose.

From what I can tell, it is only possible for `start == lowerCased.length()` which is "safe" for `subSequence()`. That would happen for a string like "trailing,comma," which produces an extra empty string. But if we're concerned about that, then it seems ",leading,comma" and "inner,,comma" should also be disallowed.

While RFC 7230 §7 says:
> In any production that uses the list construct, a sender MUST NOT
   generate empty list elements.

It also says:
> For compatibility with legacy list rules, a recipient MUST parse and
   ignore a reasonable number of empty list elements

You may consider using `StringUtil.unescapeCsvFields` for this. It supports quotes, which is unnecessary here, but shouldn't hurt anything other than performance.
:( It seems `StringUtil.unescapeCsvFields` doesn't do the trimming, even though it is used by `CombinedHttpHeaders`.
Can we remove this allocation?
@imasahiro if you think we can easily please submit a PR :) Happy to review and merge
Isn't this a trash line? As in it does nothing? It doesn't seem like the object is used.
lol doh! ... I totally missed it.  Removed in 78522cf3f0
@normanmaurer Ah, you already pushed the comment! Thanks anyways :-)
oh whoops, sorry!

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java
index 03e338fad45..71932fdd808 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java
@@ -15,6 +15,7 @@
 package io.netty.handler.codec.http2;
 
 import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.codec.UnsupportedValueConverter;
 import io.netty.handler.codec.http.DefaultFullHttpRequest;
 import io.netty.handler.codec.http.DefaultFullHttpResponse;
 import io.netty.handler.codec.http.DefaultHttpRequest;
@@ -37,7 +38,9 @@
 
 import java.net.URI;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map.Entry;
+import java.util.Set;
 
 import static io.netty.handler.codec.http.HttpScheme.HTTP;
 import static io.netty.handler.codec.http.HttpScheme.HTTPS;
@@ -47,6 +50,7 @@
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
 import static io.netty.handler.codec.http2.Http2Exception.streamError;
 import static io.netty.util.AsciiString.EMPTY_STRING;
+import static io.netty.util.ByteProcessor.FIND_COMMA;
 import static io.netty.util.ByteProcessor.FIND_SEMI_COLON;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import static io.netty.util.internal.StringUtil.isNullOrEmpty;
@@ -410,19 +414,52 @@ public static Http2Headers toHttp2Headers(HttpHeaders inHeaders, boolean validat
         return out;
     }
 
+    private static CharSequenceMap<AsciiString> toLowercaseMap(List<String> values) {
+        UnsupportedValueConverter<AsciiString> valueConverter = UnsupportedValueConverter.<AsciiString>instance();
+        CharSequenceMap<AsciiString> result =
+            new CharSequenceMap<AsciiString>(true, valueConverter, values.size());
+
+        // we iterate because the underlying list is probably a linked list
+        for (CharSequence value : values) {
+            AsciiString lowerCased = AsciiString.of(value).toLowerCase();
+            try {
+                int index = lowerCased.forEachByte(FIND_COMMA);
+                if (index != -1) {
+                    int start = 0;
+                    do {
+                        result.add(lowerCased.subSequence(start, index, false).trim(), EMPTY_STRING);
+                        start = index + 1;
+                    } while (start < lowerCased.length() &&
+                             (index = lowerCased.forEachByte(start, value.length() - start, FIND_COMMA)) != -1);
+                    result.add(lowerCased.subSequence(start, lowerCased.length(), false).trim(), EMPTY_STRING);
+                } else {
+                    result.add(lowerCased.trim(), EMPTY_STRING);
+                }
+            } catch (Exception e) {
+                // This is not expect to happen because FIND_COMMA never throws but must be caught
+                // because of the ByteProcessor interface.
+                throw new IllegalStateException(e);
+            }
+        }
+        return result;
+    }
+
     public static void toHttp2Headers(HttpHeaders inHeaders, Http2Headers out) {
         Iterator<Entry<CharSequence, CharSequence>> iter = inHeaders.iteratorCharSequence();
+            new CharSequenceMap<AsciiString>();
+        CharSequenceMap<AsciiString> connectionBlacklist =
+            toLowercaseMap(inHeaders.getAll(HttpHeaderNames.CONNECTION));
         while (iter.hasNext()) {
             Entry<CharSequence, CharSequence> entry = iter.next();
             final AsciiString aName = AsciiString.of(entry.getKey()).toLowerCase();
-            if (!HTTP_TO_HTTP2_HEADER_BLACKLIST.contains(aName)) {
+            if (!HTTP_TO_HTTP2_HEADER_BLACKLIST.contains(aName) &&
+                !connectionBlacklist.contains(aName)) {
                 // https://tools.ietf.org/html/rfc7540#section-8.1.2.2 makes a special exception for TE
-                if (aName.contentEqualsIgnoreCase(HttpHeaderNames.TE) &&
-                    !AsciiString.contentEqualsIgnoreCase(entry.getValue(), HttpHeaderValues.TRAILERS)) {
-                    throw new IllegalArgumentException("Invalid value for " + HttpHeaderNames.TE + ": " +
-                                                        entry.getValue());
-                }
-                if (aName.contentEqualsIgnoreCase(HttpHeaderNames.COOKIE)) {
+                if (aName.contentEqualsIgnoreCase(HttpHeaderNames.TE)) {
+                    if (AsciiString.containsIgnoreCase(entry.getValue(), HttpHeaderValues.TRAILERS)) {
+                        out.add(HttpHeaderNames.TE, HttpHeaderValues.TRAILERS);
+                    }
+                } else if (aName.contentEqualsIgnoreCase(HttpHeaderNames.COOKIE)) {
                     AsciiString value = AsciiString.of(entry.getValue());
                     // split up cookies to allow for better compression
                     // https://tools.ietf.org/html/rfc7540#section-8.1.2.5
diff --git a/common/src/main/java/io/netty/util/AsciiString.java b/common/src/main/java/io/netty/util/AsciiString.java
index 17e6e5e533b..396fdae2dde 100644
--- a/common/src/main/java/io/netty/util/AsciiString.java
+++ b/common/src/main/java/io/netty/util/AsciiString.java
@@ -1006,7 +1006,8 @@ public AsciiString toUpperCase() {
     }
 
     /**
-     * Copies this string removing white space characters from the beginning and end of the string.
+     * Duplicates this string removing white space characters from the beginning and end of the
+     * string, without copying.
      *
      * @return a new string with characters {@code <= \\u0020} removed from the beginning and the end.
      */
diff --git a/common/src/main/java/io/netty/util/ByteProcessor.java b/common/src/main/java/io/netty/util/ByteProcessor.java
index dcf7f00a4f4..1ca1e58d7ef 100644
--- a/common/src/main/java/io/netty/util/ByteProcessor.java
+++ b/common/src/main/java/io/netty/util/ByteProcessor.java
@@ -81,10 +81,15 @@ public boolean process(byte value) {
     ByteProcessor FIND_NON_LF = new IndexNotOfProcessor((byte) '\n');
 
     /**
-     * Aborts on a {@code CR (';')}.
+     * Aborts on a semicolon {@code (';')}.
      */
     ByteProcessor FIND_SEMI_COLON = new IndexOfProcessor((byte) ';');
 
+    /**
+     * Aborts on a comma {@code (',')}.
+     */
+    ByteProcessor FIND_COMMA = new IndexOfProcessor((byte) ',');
+
     /**
      * Aborts on a {@code CR ('\r')} or a {@code LF ('\n')}.
      */


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java
index 98e563ee45e..8b665930f6c 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java
@@ -15,6 +15,10 @@
  */
 package io.netty.handler.codec.http2;
 
+import io.netty.handler.codec.http.DefaultHttpHeaders;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.HttpHeaderNames;
+import io.netty.handler.codec.http.HttpHeaderValues;
 import io.netty.util.AsciiString;
 import org.junit.Test;
 
@@ -55,4 +59,46 @@ public void setHttp2AuthorityNullOrEmpty() {
     public void setHttp2AuthorityWithEmptyAuthority() {
         HttpConversionUtil.setHttp2Authority("info@", new DefaultHttp2Headers());
     }
+
+    @Test
+    public void stripTEHeaders() {
+        HttpHeaders inHeaders = new DefaultHttpHeaders();
+        inHeaders.add(HttpHeaderNames.TE, HttpHeaderValues.GZIP);
+        Http2Headers out = new DefaultHttp2Headers();
+        HttpConversionUtil.toHttp2Headers(inHeaders, out);
+        assertTrue(out.isEmpty());
+    }
+
+    @Test
+    public void stripTEHeadersExcludingTrailers() {
+        HttpHeaders inHeaders = new DefaultHttpHeaders();
+        inHeaders.add(HttpHeaderNames.TE, HttpHeaderValues.GZIP);
+        inHeaders.add(HttpHeaderNames.TE, HttpHeaderValues.TRAILERS);
+        Http2Headers out = new DefaultHttp2Headers();
+        HttpConversionUtil.toHttp2Headers(inHeaders, out);
+        assertSame(HttpHeaderValues.TRAILERS, out.get(HttpHeaderNames.TE));
+    }
+
+    @Test
+    public void stripConnectionHeadersAndNominees() {
+        HttpHeaders inHeaders = new DefaultHttpHeaders();
+        inHeaders.add(HttpHeaderNames.CONNECTION, "foo");
+        inHeaders.add("foo", "bar");
+        Http2Headers out = new DefaultHttp2Headers();
+        HttpConversionUtil.toHttp2Headers(inHeaders, out);
+        assertTrue(out.isEmpty());
+    }
+
+    @Test
+    public void stripConnectionNomineesWithCsv() {
+        HttpHeaders inHeaders = new DefaultHttpHeaders();
+        inHeaders.add(HttpHeaderNames.CONNECTION, "foo,  bar");
+        inHeaders.add("foo", "baz");
+        inHeaders.add("bar", "qux");
+        inHeaders.add("hello", "world");
+        Http2Headers out = new DefaultHttp2Headers();
+        HttpConversionUtil.toHttp2Headers(inHeaders, out);
+        assertEquals(1, out.size());
+        assertSame("world", out.get("hello"));
+    }
 }