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:
@normanmaurer This was missing in the previous PR.

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..2013cafd698 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) {
@@ -147,8 +147,8 @@ public void operationComplete(ChannelFuture future) {
                     }
                 });
 
-                if (headerValue != null) {
-                    headers.set(HttpHeaderNames.SEC_WEBSOCKET_EXTENSIONS, headerValue);
+                if (newHeaderValue != null) {
+                    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);
     }
 }