Repository: netty/netty (Java)

Issue:
WebsocketClientHandshaker can fail if multiple X-Websocket-Key headers are present on the upgrade request
### Expected behavior

A websockets handshake should not fail if a `X-Websockets-Key` header exists in the custom headers passed to the handshaker.

### Actual behavior

The handshake fails if the server picks the incorrect key on which to compute its challenge response (as opposed to the key that is set by the `WebsocketsClientHandshaker`).

### Steps to reproduce

Use the `WebsocketsClientHandshaker` to send a request to a websockets server along with passing custom headers that also contain an `X-Websockets-Key` header.

If the server happens to pick the _last_ `X-Websockets-Key` in the request headers on which to compute the `X-Websockets-Accept` value, the client handshake will fail with the following, as the `X-Websockets-Accept` value it expects was calculated on the _first_ `X-Websockets-Key`:

```
20:04:35.727 [nioEventLoopGroup-3-1] WARN  i.n.channel.DefaultChannelPipeline - An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.
io.netty.handler.codec.http.websocketx.WebSocketHandshakeException: Invalid challenge. Actual: w7cyq/iodDhF6MfTw9LCsDqUyBQ=. Expected: +RND8EYF2d5MkuthghnnAyg9DN0=
	at io.netty.handler.codec.http.websocketx.WebSocketClientHandshaker13.verify(WebSocketClientHandshaker13.java:223)
	at io.netty.handler.codec.http.websocketx.WebSocketClientHandshaker.finishHandshake(WebSocketClientHandshaker.java:216)
	at io.netty.handler.codec.http.websocketx.WebSocketClientProtocolHandshakeHandler.channelRead(WebSocketClientProtocolHandshakeHandler.java:57)
... snip ...
```

### Minimal yet complete reproducer code (or URL to code)

Reproducer [here](https://github.com/nicktrav/netty/commit/219d1dd5c00a2d2d92ef8224392f88a2a386668c).

### Netty version

HEAD of 4.1 branch, as of 8a8576150.

### JVM version (e.g. `java -version`)

```
$ java -version
java version "1.8.0_162"
Java(TM) SE Runtime Environment (build 1.8.0_162-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.162-b12, mixed mode)
```

### OS version (e.g. `uname -a`)

```
$ uname -a
Darwin nickt.local 17.3.0 Darwin Kernel Version 17.3.0: Thu Nov  9 18:09:22 PST 2017; root:xnu-4570.31.3~1/RELEASE_X86_64 x86_64
```


Hints:
One way to fix this could be to add the custom headers _first_, and the call `HttpHeaders#set` for each of the websocket-specific headers. It would require some re-ordering of calls in `WebSocketClientHandshaker13#newHandshakeRequest` (and variants).

One downside to this approach is that is would overwrite any header values that are present. I don't know if that's an issue though, as there should never really be duplicate handshake headers on the upgrade request. I couldn't see anything in the spec about this either.

Another option could be to throw, although that feels a little heavy handed.

If either of these sound reasonable, I can open up a PR with a fix.
I think I would go with the first approach 

> Am 26.05.2018 um 05:16 schrieb Nick Travers <notifications@github.com>:
> 
> One way to fix this could be to add the custom headers first, and the call HttpHeaders#set for each of the websocket-specific headers. It would require some re-ordering of calls in WebSocketClientHandshaker13#newHandshakeRequest (and variants).
> 
> One downside to this approach is that is would overwrite any header values that are present. I don't know if that's an issue though, as there should never really be duplicate handshake headers on the upgrade request. I couldn't see anything in the spec about this either.
> 
> Another option could be to throw, although that feels a little heavy handed.
> 
> If either of these sound reasonable, I can open up a PR with a fix.
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or mute the thread.


Patch:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00.java
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00.java
@@ -131,22 +131,23 @@ protected FullHttpRequest newHandshakeRequest() {
         // Format request
         FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, path);
         HttpHeaders headers = request.headers();
-        headers.add(HttpHeaderNames.UPGRADE, WEBSOCKET)
-               .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
-               .add(HttpHeaderNames.HOST, websocketHostValue(wsURL))
-               .add(HttpHeaderNames.ORIGIN, websocketOriginValue(wsURL))
-               .add(HttpHeaderNames.SEC_WEBSOCKET_KEY1, key1)
-               .add(HttpHeaderNames.SEC_WEBSOCKET_KEY2, key2);
-
-        String expectedSubprotocol = expectedSubprotocol();
-        if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) {
-            headers.add(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol);
-        }
 
         if (customHeaders != null) {
             headers.add(customHeaders);
         }
 
+        headers.set(HttpHeaderNames.UPGRADE, WEBSOCKET)
+               .set(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
+               .set(HttpHeaderNames.HOST, websocketHostValue(wsURL))
+               .set(HttpHeaderNames.ORIGIN, websocketOriginValue(wsURL))
+               .set(HttpHeaderNames.SEC_WEBSOCKET_KEY1, key1)
+               .set(HttpHeaderNames.SEC_WEBSOCKET_KEY2, key2);
+
+        String expectedSubprotocol = expectedSubprotocol();
+        if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) {
+            headers.set(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol);
+        }
+
         // Set Content-Length to workaround some known defect.
         // See also: http://www.ietf.org/mail-archive/web/hybi/current/msg02149.html
         headers.set(HttpHeaderNames.CONTENT_LENGTH, key3.length);
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07.java
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07.java
@@ -145,22 +145,22 @@ protected FullHttpRequest newHandshakeRequest() {
         FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, path);
         HttpHeaders headers = request.headers();
 
-        headers.add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET)
-               .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
-               .add(HttpHeaderNames.SEC_WEBSOCKET_KEY, key)
-               .add(HttpHeaderNames.HOST, websocketHostValue(wsURL))
-               .add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(wsURL));
+        if (customHeaders != null) {
+            headers.add(customHeaders);
+        }
+
+        headers.set(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET)
+               .set(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
+               .set(HttpHeaderNames.SEC_WEBSOCKET_KEY, key)
+               .set(HttpHeaderNames.HOST, websocketHostValue(wsURL))
+               .set(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(wsURL));
 
         String expectedSubprotocol = expectedSubprotocol();
         if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) {
-            headers.add(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol);
+            headers.set(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol);
         }
 
-        headers.add(HttpHeaderNames.SEC_WEBSOCKET_VERSION, "7");
-
-        if (customHeaders != null) {
-            headers.add(customHeaders);
-        }
+        headers.set(HttpHeaderNames.SEC_WEBSOCKET_VERSION, "7");
         return request;
     }
 
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08.java
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08.java
@@ -146,22 +146,22 @@ protected FullHttpRequest newHandshakeRequest() {
         FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, path);
         HttpHeaders headers = request.headers();
 
-        headers.add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET)
-               .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
-               .add(HttpHeaderNames.SEC_WEBSOCKET_KEY, key)
-               .add(HttpHeaderNames.HOST, websocketHostValue(wsURL))
-               .add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(wsURL));
+        if (customHeaders != null) {
+            headers.add(customHeaders);
+        }
+
+        headers.set(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET)
+               .set(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
+               .set(HttpHeaderNames.SEC_WEBSOCKET_KEY, key)
+               .set(HttpHeaderNames.HOST, websocketHostValue(wsURL))
+               .set(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(wsURL));
 
         String expectedSubprotocol = expectedSubprotocol();
         if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) {
-            headers.add(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol);
+            headers.set(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol);
         }
 
-        headers.add(HttpHeaderNames.SEC_WEBSOCKET_VERSION, "8");
-
-        if (customHeaders != null) {
-            headers.add(customHeaders);
-        }
+        headers.set(HttpHeaderNames.SEC_WEBSOCKET_VERSION, "8");
         return request;
     }
 
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13.java
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13.java
@@ -146,22 +146,22 @@ protected FullHttpRequest newHandshakeRequest() {
         FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, path);
         HttpHeaders headers = request.headers();
 
-        headers.add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET)
-               .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
-               .add(HttpHeaderNames.SEC_WEBSOCKET_KEY, key)
-               .add(HttpHeaderNames.HOST, websocketHostValue(wsURL))
-               .add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(wsURL));
+        if (customHeaders != null) {
+            headers.add(customHeaders);
+        }
+
+        headers.set(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET)
+               .set(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
+               .set(HttpHeaderNames.SEC_WEBSOCKET_KEY, key)
+               .set(HttpHeaderNames.HOST, websocketHostValue(wsURL))
+               .set(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(wsURL));
 
         String expectedSubprotocol = expectedSubprotocol();
         if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) {
-            headers.add(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol);
+            headers.set(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol);
         }
 
-        headers.add(HttpHeaderNames.SEC_WEBSOCKET_VERSION, "13");
-
-        if (customHeaders != null) {
-            headers.add(customHeaders);
-        }
+        headers.set(HttpHeaderNames.SEC_WEBSOCKET_VERSION, "13");
         return request;
     }
 


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00Test.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00Test.java
--- a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00Test.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00Test.java
@@ -16,17 +16,35 @@
 package io.netty.handler.codec.http.websocketx;
 
 import io.netty.handler.codec.http.HttpHeaderNames;
+import io.netty.handler.codec.http.HttpHeaders;
 
 import java.net.URI;
 
 public class WebSocketClientHandshaker00Test extends WebSocketClientHandshakerTest {
     @Override
-    protected WebSocketClientHandshaker newHandshaker(URI uri) {
-        return new WebSocketClientHandshaker00(uri, WebSocketVersion.V00, null, null, 1024);
+    protected WebSocketClientHandshaker newHandshaker(URI uri, String subprotocol, HttpHeaders headers) {
+        return new WebSocketClientHandshaker00(uri, WebSocketVersion.V00, subprotocol, headers, 1024);
     }
 
     @Override
     protected CharSequence getOriginHeaderName() {
         return HttpHeaderNames.ORIGIN;
     }
+
+    @Override
+    protected CharSequence getProtocolHeaderName() {
+        return HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL;
+    }
+
+    @Override
+    protected CharSequence[] getHandshakeHeaderNames() {
+        return new CharSequence[] {
+                HttpHeaderNames.CONNECTION,
+                HttpHeaderNames.UPGRADE,
+                HttpHeaderNames.HOST,
+                HttpHeaderNames.ORIGIN,
+                HttpHeaderNames.SEC_WEBSOCKET_KEY1,
+                HttpHeaderNames.SEC_WEBSOCKET_KEY2,
+        };
+    }
 }
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07Test.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07Test.java
--- a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07Test.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07Test.java
@@ -16,17 +16,35 @@
 package io.netty.handler.codec.http.websocketx;
 
 import io.netty.handler.codec.http.HttpHeaderNames;
+import io.netty.handler.codec.http.HttpHeaders;
 
 import java.net.URI;
 
 public class WebSocketClientHandshaker07Test extends WebSocketClientHandshakerTest {
     @Override
-    protected WebSocketClientHandshaker newHandshaker(URI uri) {
-        return new WebSocketClientHandshaker07(uri, WebSocketVersion.V07, null, false, null, 1024);
+    protected WebSocketClientHandshaker newHandshaker(URI uri, String subprotocol, HttpHeaders headers) {
+        return new WebSocketClientHandshaker07(uri, WebSocketVersion.V07, subprotocol, false, headers, 1024);
     }
 
     @Override
     protected CharSequence getOriginHeaderName() {
         return HttpHeaderNames.SEC_WEBSOCKET_ORIGIN;
     }
+
+    @Override
+    protected CharSequence getProtocolHeaderName() {
+        return HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL;
+    }
+
+    @Override
+    protected CharSequence[] getHandshakeHeaderNames() {
+        return new CharSequence[] {
+                HttpHeaderNames.UPGRADE,
+                HttpHeaderNames.CONNECTION,
+                HttpHeaderNames.SEC_WEBSOCKET_KEY,
+                HttpHeaderNames.HOST,
+                HttpHeaderNames.SEC_WEBSOCKET_ORIGIN,
+                HttpHeaderNames.SEC_WEBSOCKET_VERSION,
+        };
+    }
 }
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08Test.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08Test.java
--- a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08Test.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08Test.java
@@ -15,11 +15,13 @@
  */
 package io.netty.handler.codec.http.websocketx;
 
+import io.netty.handler.codec.http.HttpHeaders;
+
 import java.net.URI;
 
 public class WebSocketClientHandshaker08Test extends WebSocketClientHandshaker07Test {
     @Override
-    protected WebSocketClientHandshaker newHandshaker(URI uri) {
-        return new WebSocketClientHandshaker08(uri, WebSocketVersion.V08, null, false, null, 1024);
+    protected WebSocketClientHandshaker newHandshaker(URI uri, String subprotocol, HttpHeaders headers) {
+        return new WebSocketClientHandshaker08(uri, WebSocketVersion.V08, subprotocol, false, headers, 1024);
     }
 }
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13Test.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13Test.java
--- a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13Test.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13Test.java
@@ -15,11 +15,13 @@
  */
 package io.netty.handler.codec.http.websocketx;
 
+import io.netty.handler.codec.http.HttpHeaders;
+
 import java.net.URI;
 
 public class WebSocketClientHandshaker13Test extends WebSocketClientHandshaker07Test {
     @Override
-    protected WebSocketClientHandshaker newHandshaker(URI uri) {
-        return new WebSocketClientHandshaker13(uri, WebSocketVersion.V13, null, false, null, 1024);
+    protected WebSocketClientHandshaker newHandshaker(URI uri, String subprotocol, HttpHeaders headers) {
+        return new WebSocketClientHandshaker13(uri, WebSocketVersion.V13, subprotocol, false, headers, 1024);
     }
 }
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshakerTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshakerTest.java
--- a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshakerTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshakerTest.java
@@ -21,11 +21,13 @@
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.SimpleChannelInboundHandler;
 import io.netty.channel.embedded.EmbeddedChannel;
+import io.netty.handler.codec.http.DefaultHttpHeaders;
 import io.netty.handler.codec.http.EmptyHttpHeaders;
 import io.netty.handler.codec.http.FullHttpRequest;
 import io.netty.handler.codec.http.FullHttpResponse;
 import io.netty.handler.codec.http.HttpClientCodec;
 import io.netty.handler.codec.http.HttpHeaderNames;
+import io.netty.handler.codec.http.HttpHeaders;
 import io.netty.handler.codec.http.HttpObjectAggregator;
 import io.netty.handler.codec.http.HttpRequestEncoder;
 import io.netty.handler.codec.http.HttpResponseDecoder;
@@ -35,14 +37,21 @@
 
 import java.net.URI;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
 
 public abstract class WebSocketClientHandshakerTest {
-    protected abstract WebSocketClientHandshaker newHandshaker(URI uri);
+    protected abstract WebSocketClientHandshaker newHandshaker(URI uri, String subprotocol, HttpHeaders headers);
+
+    protected WebSocketClientHandshaker newHandshaker(URI uri) {
+        return newHandshaker(uri, null, null);
+    }
 
     protected abstract CharSequence getOriginHeaderName();
 
+    protected abstract CharSequence getProtocolHeaderName();
+
+    protected abstract CharSequence[] getHandshakeHeaderNames();
+
     @Test
     public void hostHeaderWs() {
         for (String scheme : new String[]{"ws://", "http://"}) {
@@ -292,4 +301,36 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpResponse msg) thr
             frame.release();
         }
     }
+
+    @Test
+    public void testDuplicateWebsocketHandshakeHeaders() {
+        URI uri = URI.create("ws://localhost:9999/foo");
+
+        HttpHeaders inputHeaders = new DefaultHttpHeaders();
+        String bogusSubProtocol = "bogusSubProtocol";
+        String bogusHeaderValue = "bogusHeaderValue";
+
+        // add values for the headers that are reserved for use in the websockets handshake
+        for (CharSequence header : getHandshakeHeaderNames()) {
+            inputHeaders.add(header, bogusHeaderValue);
+        }
+        inputHeaders.add(getProtocolHeaderName(), bogusSubProtocol);
+
+        String realSubProtocol = "realSubProtocol";
+        WebSocketClientHandshaker handshaker = newHandshaker(uri, realSubProtocol, inputHeaders);
+        FullHttpRequest request = handshaker.newHandshakeRequest();
+        HttpHeaders outputHeaders = request.headers();
+
+        // the header values passed in originally have been replaced with values generated by the Handshaker
+        for (CharSequence header : getHandshakeHeaderNames()) {
+            assertEquals(1, outputHeaders.getAll(header).size());
+            assertNotEquals(bogusHeaderValue, outputHeaders.get(header));
+        }
+
+        // the subprotocol header value is that of the subprotocol string passed into the Handshaker
+        assertEquals(1, outputHeaders.getAll(getProtocolHeaderName()).size());
+        assertEquals(realSubProtocol, outputHeaders.get(getProtocolHeaderName()));
+
+        request.release();
+    }
 }