Repository: netty/netty
Issue: HttpContentEncoder does not handle multiple Accept-Encoding headers correctly

### Expected behavior

When a request contains multiple `Accept-Encoding` headers, their values will be treated as if they had been sent as the comma-separated values of a single `Accept-Encoding` header.

### Actual behavior

Only the first `Accept-Encoding` header is considered and all subsequent `Accept-Encoding` headers are ignored.

### Steps to reproduce

Configure an HTTP server with `HttpContentCompressor` in its pipeline and send a request with multiple `Accept-Encoding` headers. Using curl that would look something like this:

```
 curl -H "Accept-Encoding: unknown" -H "Accept-Encoding: gzip" localhost:8080
```

Observe that the response is not gzipped.

### Minimal yet complete reproducer code (or URL to code)

```java
package com.example.demo;

import static io.netty.handler.codec.http.HttpHeaderNames.CONNECTION;
import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_LENGTH;
import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE;
import static io.netty.handler.codec.http.HttpHeaderValues.CLOSE;
import static io.netty.handler.codec.http.HttpHeaderValues.KEEP_ALIVE;
import static io.netty.handler.codec.http.HttpHeaderValues.TEXT_PLAIN;
import static io.netty.handler.codec.http.HttpResponseStatus.OK;

import io.netty.bootstrap.ServerBootstrap;
import io.netty.buffer.Unpooled;
import io.netty.channel.Channel;
import io.netty.channel.ChannelFuture;
import io.netty.channel.ChannelFutureListener;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelInitializer;
import io.netty.channel.ChannelOption;
import io.netty.channel.ChannelPipeline;
import io.netty.channel.EventLoopGroup;
import io.netty.channel.SimpleChannelInboundHandler;
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.socket.SocketChannel;
import io.netty.channel.socket.nio.NioServerSocketChannel;
import io.netty.handler.codec.http.DefaultFullHttpResponse;
import io.netty.handler.codec.http.FullHttpResponse;
import io.netty.handler.codec.http.HttpContentCompressor;
import io.netty.handler.codec.http.HttpObject;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpServerCodec;
import io.netty.handler.codec.http.HttpServerExpectContinueHandler;
import io.netty.handler.codec.http.HttpUtil;

public final class HttpHelloWorldServer {

    public static void main(String[] args) throws Exception {
        EventLoopGroup bossGroup = new NioEventLoopGroup(1);
        EventLoopGroup workerGroup = new NioEventLoopGroup();
        try {
            ServerBootstrap b = new ServerBootstrap();
            b.option(ChannelOption.SO_BACKLOG, 1024);
            b.group(bossGroup, workerGroup)
             .channel(NioServerSocketChannel.class)
             .childHandler(new HttpHelloWorldServerInitializer());
            Channel ch = b.bind(8080).sync().channel();
            ch.closeFuture().sync();
        } finally {
            bossGroup.shutdownGracefully();
            workerGroup.shutdownGracefully();
        }
    }
    
    static final class HttpHelloWorldServerInitializer extends ChannelInitializer<SocketChannel> {

        @Override
        public void initChannel(SocketChannel ch) {
            ChannelPipeline p = ch.pipeline();
            p.addLast(new HttpServerCodec());
            p.addLast(new HttpContentCompressor());
            p.addLast(new HttpServerExpectContinueHandler());
            p.addLast(new HttpHelloWorldServerHandler());
        }
        
    }
    
    static final class HttpHelloWorldServerHandler extends SimpleChannelInboundHandler<HttpObject> {
    	
        private static final byte[] CONTENT = { 'H', 'e', 'l', 'l', 'o', ' ', 'W', 'o', 'r', 'l', 'd' };

        @Override
        public void channelReadComplete(ChannelHandlerContext context) {
            context.flush();
        }

        @Override
        public void channelRead0(ChannelHandlerContext context, HttpObject message) {
            if (message instanceof HttpRequest) {
                HttpRequest req = (HttpRequest) message;

                boolean keepAlive = HttpUtil.isKeepAlive(req);
                FullHttpResponse response = new DefaultFullHttpResponse(req.protocolVersion(), OK, Unpooled.wrappedBuffer(CONTENT));
                response.headers().set(CONTENT_TYPE, TEXT_PLAIN).setInt(CONTENT_LENGTH, response.content().readableBytes());

                if (keepAlive) {
                    if (!req.protocolVersion().isKeepAliveDefault()) {
                        response.headers().set(CONNECTION, KEEP_ALIVE);
                    }
                } else {
                    // Tell the client we're going to close the connection.
                    response.headers().set(CONNECTION, CLOSE);
                }

                ChannelFuture f = context.write(response);

                if (!keepAlive) {
                    f.addListener(ChannelFutureListener.CLOSE);
                }
            }
        }

        @Override
        public void exceptionCaught(ChannelHandlerContext context, Throwable ex) {
            ex.printStackTrace();
            context.close();
        }
        
    }
    
}
```

### Netty version

4.1.39

### JVM version (e.g. `java -version`)

```
openjdk version "1.8.0_202"
OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_202-b08)
OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.202-b08, mixed mode)
```

### OS version (e.g. `uname -a`)

Darwin Andys-MacBook-Pro.local 17.7.0 Darwin Kernel Version 17.7.0: Sun Jun  2 20:31:42 PDT 2019; root:xnu-4570.71.46~1/RELEASE_X86_64 x86_64

Issue comments:
@wilkinsona sounds like a bug... are you interested in providing a PR ?
@normanmaurer  Hi, problem that we handle only first `accept-encoding` header value, also we need to consider the q-value for each, or combine to one header-value by spec https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
@amizurov @wilkinsona  I more then happy to review PRs ... Currently I am a bit busy so I will put this on my backlog if no-one else will fix it in the meantime 
@normanmaurer No problem if @wilkinsona don't mind I will do a pull request soon.
@amizurov If you have the time then please go ahead. If not, I should have some cycles later this week once we've released Spring Boot 2.2 M6.

Pull request review comments:
nit: maybe we should make this:

```java
switch (acceptedEncodingHeaders.size()) {
    case 0:
        acceptedEncoding = HttpContentDecoder.IDENTITY;
        break;
    case 1: 
        acceptedEncoding = acceptEncodingHeaders.get(0);
        break;
    default:
        // Multiple message-header fields https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
       acceptedEncoding = StringUtil.join(",", acceptEncodingHeaders.iterator());
       break;

}
```
assert return value
assert return value
maybe just make this `Iterable` ?
I wonder if we should optimise this a but to only build the StringBuilder etc when there are at least 2 elements in ?
This could also be simplified to just `new StringBuilder(elements.next())`.
Better to just return `builder`, no need to copy to a `String` yet
It's a shame that `HttpHeaders` doesn't have `get`/`getAll` methods which return `CharSequence`s rather than `String`s. These would be quite easy to add I think and would help cut down on copying and garbage.

(this is a passing comment and not directly related to the PR change!)
Don't mind, done
Done
Done
Done
All done :)
Done

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
index 7be3b8b92e9..871caef5be2 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
@@ -22,11 +22,14 @@
 import io.netty.handler.codec.DecoderResult;
 import io.netty.handler.codec.MessageToMessageCodec;
 import io.netty.util.ReferenceCountUtil;
+import io.netty.util.internal.StringUtil;
 
 import java.util.ArrayDeque;
 import java.util.List;
 import java.util.Queue;
 
+import static io.netty.handler.codec.http.HttpHeaderNames.*;
+
 /**
  * Encodes the content of the outbound {@link HttpResponse} and {@link HttpContent}.
  * The original content is replaced with the new content encoded by the
@@ -71,21 +74,30 @@ public boolean acceptOutboundMessage(Object msg) throws Exception {
     }
 
     @Override
-    protected void decode(ChannelHandlerContext ctx, HttpRequest msg, List<Object> out)
-            throws Exception {
-        CharSequence acceptedEncoding = msg.headers().get(HttpHeaderNames.ACCEPT_ENCODING);
-        if (acceptedEncoding == null) {
-            acceptedEncoding = HttpContentDecoder.IDENTITY;
+    protected void decode(ChannelHandlerContext ctx, HttpRequest msg, List<Object> out) throws Exception {
+        CharSequence acceptEncoding;
+        List<String> acceptEncodingHeaders = msg.headers().getAll(ACCEPT_ENCODING);
+        switch (acceptEncodingHeaders.size()) {
+        case 0:
+            acceptEncoding = HttpContentDecoder.IDENTITY;
+            break;
+        case 1:
+            acceptEncoding = acceptEncodingHeaders.get(0);
+            break;
+        default:
+            // Multiple message-header fields https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
+            acceptEncoding = StringUtil.join(",", acceptEncodingHeaders);
+            break;
         }
 
         HttpMethod method = msg.method();
         if (HttpMethod.HEAD.equals(method)) {
-            acceptedEncoding = ZERO_LENGTH_HEAD;
+            acceptEncoding = ZERO_LENGTH_HEAD;
         } else if (HttpMethod.CONNECT.equals(method)) {
-            acceptedEncoding = ZERO_LENGTH_CONNECT;
+            acceptEncoding = ZERO_LENGTH_CONNECT;
         }
 
-        acceptEncodingQueue.add(acceptedEncoding);
+        acceptEncodingQueue.add(acceptEncoding);
         out.add(ReferenceCountUtil.retain(msg));
     }
 
diff --git a/common/src/main/java/io/netty/util/internal/StringUtil.java b/common/src/main/java/io/netty/util/internal/StringUtil.java
index e81a024ddc1..2b107d71b21 100644
--- a/common/src/main/java/io/netty/util/internal/StringUtil.java
+++ b/common/src/main/java/io/netty/util/internal/StringUtil.java
@@ -17,6 +17,7 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
 
 import static io.netty.util.internal.ObjectUtil.*;
@@ -597,6 +598,36 @@ public static CharSequence trimOws(CharSequence value) {
         return start == 0 && end == length - 1 ? value : value.subSequence(start, end + 1);
     }
 
+    /**
+     * Returns a char sequence that contains all {@code elements} joined by a given separator.
+     *
+     * @param separator for each element
+     * @param elements to join together
+     *
+     * @return a char sequence joined by a given separator.
+     */
+    public static CharSequence join(CharSequence separator, Iterable<? extends CharSequence> elements) {
+        ObjectUtil.checkNotNull(separator, "separator");
+        ObjectUtil.checkNotNull(elements, "elements");
+
+        Iterator<? extends CharSequence> iterator = elements.iterator();
+        if (!iterator.hasNext()) {
+            return EMPTY_STRING;
+        }
+
+        CharSequence firstElement = iterator.next();
+        if (!iterator.hasNext()) {
+            return firstElement;
+        }
+
+        StringBuilder builder = new StringBuilder(firstElement);
+        do {
+            builder.append(separator).append(iterator.next());
+        } while (iterator.hasNext());
+
+        return builder;
+    }
+
     /**
      * @return {@code length} if no OWS is found.
      */
@@ -622,4 +653,5 @@ private static int indexOfLastNonOwsChar(CharSequence value, int start, int leng
     private static boolean isOws(char c) {
         return c == SPACE || c == TAB;
     }
+
 }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
index a31bc5beb89..d0676fd907a 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
@@ -500,6 +500,39 @@ public void testCompressThresholdNotCompress() throws Exception {
         assertTrue(ch.finishAndReleaseAll());
     }
 
+    @Test
+    public void testMultipleAcceptEncodingHeaders() {
+        FullHttpRequest request = newRequest();
+        request.headers().set(HttpHeaderNames.ACCEPT_ENCODING, "unknown; q=1.0")
+               .add(HttpHeaderNames.ACCEPT_ENCODING, "gzip; q=0.5")
+               .add(HttpHeaderNames.ACCEPT_ENCODING, "deflate; q=0");
+
+        EmbeddedChannel ch = new EmbeddedChannel(new HttpContentCompressor());
+
+        assertTrue(ch.writeInbound(request));
+
+        FullHttpResponse res = new DefaultFullHttpResponse(
+                HttpVersion.HTTP_1_1, HttpResponseStatus.OK,
+                Unpooled.copiedBuffer("Gzip Win", CharsetUtil.US_ASCII));
+        assertTrue(ch.writeOutbound(res));
+
+        assertEncodedResponse(ch);
+        HttpContent c = ch.readOutbound();
+        assertThat(ByteBufUtil.hexDump(c.content()), is("1f8b080000000000000072afca2c5008cfcc03000000ffff"));
+        c.release();
+
+        c = ch.readOutbound();
+        assertThat(ByteBufUtil.hexDump(c.content()), is("03001f2ebf0f08000000"));
+        c.release();
+
+        LastHttpContent last = ch.readOutbound();
+        assertThat(last.content().readableBytes(), is(0));
+        last.release();
+
+        assertThat(ch.readOutbound(), is(nullValue()));
+        assertTrue(ch.finishAndReleaseAll());
+    }
+
     private static FullHttpRequest newRequest() {
         FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/");
         req.headers().set(HttpHeaderNames.ACCEPT_ENCODING, "gzip");
diff --git a/common/src/test/java/io/netty/util/internal/StringUtilTest.java b/common/src/test/java/io/netty/util/internal/StringUtilTest.java
index 0d27a74fea3..7dd06e7bac1 100644
--- a/common/src/test/java/io/netty/util/internal/StringUtilTest.java
+++ b/common/src/test/java/io/netty/util/internal/StringUtilTest.java
@@ -534,4 +534,19 @@ public void trimOws() {
         assertEquals("", StringUtil.trimOws("\t ").toString());
         assertEquals("a b", StringUtil.trimOws("\ta b \t").toString());
     }
+
+    @Test
+    public void testJoin() {
+        assertEquals("",
+                     StringUtil.join(",", Collections.<CharSequence>emptyList()).toString());
+        assertEquals("a",
+                     StringUtil.join(",", Collections.singletonList("a")).toString());
+        assertEquals("a,b",
+                     StringUtil.join(",", Arrays.asList("a", "b")).toString());
+        assertEquals("a,b,c",
+                     StringUtil.join(",", Arrays.asList("a", "b", "c")).toString());
+        assertEquals("a,b,c,null,d",
+                     StringUtil.join(",", Arrays.asList("a", "b", "c", null, "d")).toString());
+    }
+
 }