Repository: netty/netty
Issue: WebSocketClientExtensionHandler doesn't check if extensions are already present in sec-websocket-extensions header

### Expected behavior

Netty verifies that extensions are not already present in sec-websocket-extensions header before appending them.

### Actual behavior

Netty doesn't check, possibly resulting in duplicated values.

### Steps to reproduce

Just read existing code: https://github.com/netty/netty/blob/netty-4.1.54.Final/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketClientExtensionHandler.java#L65-L73

### Netty version

4.1.54

### JVM version (e.g. `java -version`)

Irrelevant

### OS version (e.g. `uname -a`)

Irrelevant

Issue comments:
Will provide a PR, probably next week.
@slandelle any update ?
@normanmaurer PTAL #10951

Pull request review comments:
Does this need to be a `LinkedHashSet` ?
is the default size of 64 good ?
Parameters order is not supposed to be significant. Moreover, `extractExtensions` (in the same class) uses `HashMap` for the parsing side, so using one here too is consistent.
The size greatly varies depending on the parameters. The real world example (client side deflate) is have in the tests is 149 chars with 4 extensions ranging from 15 to 70 chars.
Do you have any suggestion? Hardcode 150?
yep +1 
done

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketClientExtensionHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketClientExtensionHandler.java
index 165c1cdfc88..47073a7b234 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketClientExtensionHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketClientExtensionHandler.java
@@ -63,14 +63,15 @@ public void write(final ChannelHandlerContext ctx, Object msg, ChannelPromise pr
         if (msg instanceof HttpRequest && WebSocketExtensionUtil.isWebsocketUpgrade(((HttpRequest) msg).headers())) {
             HttpRequest request = (HttpRequest) msg;
             String headerValue = request.headers().getAsString(HttpHeaderNames.SEC_WEBSOCKET_EXTENSIONS);
-
+            List<WebSocketExtensionData> extraExtensions =
+              new ArrayList<WebSocketExtensionData>(extensionHandshakers.size());
             for (WebSocketClientExtensionHandshaker extensionHandshaker : extensionHandshakers) {
-                WebSocketExtensionData extensionData = extensionHandshaker.newRequestData();
-                headerValue = WebSocketExtensionUtil.appendExtension(headerValue,
-                        extensionData.name(), extensionData.parameters());
+                extraExtensions.add(extensionHandshaker.newRequestData());
             }
+            String newHeaderValue = WebSocketExtensionUtil
+              .computeMergeExtensionsHeaderValue(headerValue, extraExtensions);
 
-            request.headers().set(HttpHeaderNames.SEC_WEBSOCKET_EXTENSIONS, headerValue);
+            request.headers().set(HttpHeaderNames.SEC_WEBSOCKET_EXTENSIONS, newHeaderValue);
         }
 
         super.write(ctx, msg, promise);
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketExtensionUtil.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketExtensionUtil.java
index 567e49e6bf3..01f1c0036c1 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketExtensionUtil.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketExtensionUtil.java
@@ -72,25 +72,53 @@ public static List<WebSocketExtensionData> extractExtensions(String extensionHea
         }
     }
 
-    static String appendExtension(String currentHeaderValue, String extensionName,
-            Map<String, String> extensionParameters) {
+    static String computeMergeExtensionsHeaderValue(String userDefinedHeaderValue,
+                                                    List<WebSocketExtensionData> extraExtensions) {
+        List<WebSocketExtensionData> userDefinedExtensions =
+          userDefinedHeaderValue != null ?
+            extractExtensions(userDefinedHeaderValue) :
+            Collections.<WebSocketExtensionData>emptyList();
 
-        StringBuilder newHeaderValue = new StringBuilder(
-                currentHeaderValue != null ? currentHeaderValue.length() : extensionName.length() + 1);
-        if (currentHeaderValue != null && !currentHeaderValue.trim().isEmpty()) {
-            newHeaderValue.append(currentHeaderValue);
-            newHeaderValue.append(EXTENSION_SEPARATOR);
+        for (WebSocketExtensionData userDefined: userDefinedExtensions) {
+            WebSocketExtensionData matchingExtra = null;
+            int i;
+            for (i = 0; i < extraExtensions.size(); i ++) {
+                WebSocketExtensionData extra = extraExtensions.get(i);
+                if (extra.name().equals(userDefined.name())) {
+                    matchingExtra = extra;
+                    break;
+                }
+            }
+            if (matchingExtra == null) {
+                extraExtensions.add(userDefined);
+            } else {
+                // merge with higher precedence to user defined parameters
+                Map<String, String> mergedParameters = new HashMap<String, String>(matchingExtra.parameters());
+                mergedParameters.putAll(userDefined.parameters());
+                extraExtensions.set(i, new WebSocketExtensionData(matchingExtra.name(), mergedParameters));
+            }
         }
-        newHeaderValue.append(extensionName);
-        for (Entry<String, String> extensionParameter : extensionParameters.entrySet()) {
-            newHeaderValue.append(PARAMETER_SEPARATOR);
-            newHeaderValue.append(extensionParameter.getKey());
-            if (extensionParameter.getValue() != null) {
-                newHeaderValue.append(PARAMETER_EQUAL);
-                newHeaderValue.append(extensionParameter.getValue());
+
+        StringBuilder sb = new StringBuilder(150);
+
+        for (WebSocketExtensionData data: extraExtensions) {
+            sb.append(data.name());
+            for (Entry<String, String> parameter : data.parameters().entrySet()) {
+                sb.append(PARAMETER_SEPARATOR);
+                sb.append(parameter.getKey());
+                if (parameter.getValue() != null) {
+                    sb.append(PARAMETER_EQUAL);
+                    sb.append(parameter.getValue());
+                }
             }
+            sb.append(EXTENSION_SEPARATOR);
+        }
+
+        if (!extraExtensions.isEmpty()) {
+            sb.setLength(sb.length() - EXTENSION_SEPARATOR.length());
         }
-        return newHeaderValue.toString();
+
+        return sb.toString();
     }
 
     private WebSocketExtensionUtil() {
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketServerExtensionHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketServerExtensionHandler.java
index 9e45bcfa630..e00090bb79d 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketServerExtensionHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketServerExtensionHandler.java
@@ -124,13 +124,13 @@ private void handlePotentialUpgrade(final ChannelHandlerContext ctx,
 
             if (validExtensions != null) {
                 String headerValue = headers.getAsString(HttpHeaderNames.SEC_WEBSOCKET_EXTENSIONS);
-
+                List<WebSocketExtensionData> extraExtensions =
+                  new ArrayList<WebSocketExtensionData>(extensionHandshakers.size());
                 for (WebSocketServerExtension extension : validExtensions) {
-                    WebSocketExtensionData extensionData = extension.newReponseData();
-                    headerValue = WebSocketExtensionUtil.appendExtension(headerValue,
-                        extensionData.name(),
-                        extensionData.parameters());
+                    extraExtensions.add(extension.newReponseData());
                 }
+                String newHeaderValue = WebSocketExtensionUtil
+                  .computeMergeExtensionsHeaderValue(headerValue, extraExtensions);
                 promise.addListener(new ChannelFutureListener() {
                     @Override
                     public void operationComplete(ChannelFuture future) {
@@ -148,7 +148,7 @@ public void operationComplete(ChannelFuture future) {
                 });
 
                 if (headerValue != null) {
-                    headers.set(HttpHeaderNames.SEC_WEBSOCKET_EXTENSIONS, headerValue);
+                    headers.set(HttpHeaderNames.SEC_WEBSOCKET_EXTENSIONS, newHeaderValue);
                 }
             }
 


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketExtensionUtilTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketExtensionUtilTest.java
index 12aa06ed660..eab3b3ba46c 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketExtensionUtilTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketExtensionUtilTest.java
@@ -21,19 +21,63 @@
 import io.netty.handler.codec.http.HttpHeaders;
 import org.junit.Test;
 
+import java.util.List;
+
 import static org.junit.Assert.*;
+import static io.netty.handler.codec.http.websocketx.extensions.WebSocketExtensionUtil.*;
 
 public class WebSocketExtensionUtilTest {
 
     @Test
     public void testIsWebsocketUpgrade() {
         HttpHeaders headers = new DefaultHttpHeaders();
-        assertFalse(WebSocketExtensionUtil.isWebsocketUpgrade(headers));
+        assertFalse(isWebsocketUpgrade(headers));
 
         headers.add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET);
-        assertFalse(WebSocketExtensionUtil.isWebsocketUpgrade(headers));
+        assertFalse(isWebsocketUpgrade(headers));
 
         headers.add(HttpHeaderNames.CONNECTION, "Keep-Alive, Upgrade");
-        assertTrue(WebSocketExtensionUtil.isWebsocketUpgrade(headers));
+        assertTrue(isWebsocketUpgrade(headers));
+    }
+
+    @Test
+    public void computeMergeExtensionsHeaderValueWhenNoUserDefinedHeader() {
+        List<WebSocketExtensionData> extras = extractExtensions("permessage-deflate; client_max_window_bits," +
+          "permessage-deflate; client_no_context_takeover; client_max_window_bits," +
+          "deflate-frame," +
+          "x-webkit-deflate-frame");
+        String newHeaderValue = computeMergeExtensionsHeaderValue(null, extras);
+        assertEquals("permessage-deflate;client_max_window_bits," +
+          "permessage-deflate;client_no_context_takeover;client_max_window_bits," +
+          "deflate-frame," +
+          "x-webkit-deflate-frame", newHeaderValue);
+    }
+
+    @Test
+    public void computeMergeExtensionsHeaderValueWhenNoConflictingUserDefinedHeader() {
+        List<WebSocketExtensionData> extras = extractExtensions("permessage-deflate; client_max_window_bits," +
+          "permessage-deflate; client_no_context_takeover; client_max_window_bits," +
+          "deflate-frame," +
+          "x-webkit-deflate-frame");
+        String newHeaderValue = computeMergeExtensionsHeaderValue("foo, bar", extras);
+        assertEquals("permessage-deflate;client_max_window_bits," +
+          "permessage-deflate;client_no_context_takeover;client_max_window_bits," +
+          "deflate-frame," +
+          "x-webkit-deflate-frame," +
+          "foo," +
+          "bar", newHeaderValue);
+    }
+
+    @Test
+    public void computeMergeExtensionsHeaderValueWhenConflictingUserDefinedHeader() {
+        List<WebSocketExtensionData> extras = extractExtensions("permessage-deflate; client_max_window_bits," +
+          "permessage-deflate; client_no_context_takeover; client_max_window_bits," +
+          "deflate-frame," +
+          "x-webkit-deflate-frame");
+        String newHeaderValue = computeMergeExtensionsHeaderValue("permessage-deflate; client_max_window_bits", extras);
+        assertEquals("permessage-deflate;client_max_window_bits," +
+          "permessage-deflate;client_no_context_takeover;client_max_window_bits," +
+          "deflate-frame," +
+          "x-webkit-deflate-frame", newHeaderValue);
     }
 }