Repository: netty/netty
Issue: sec-websocket-origin should mention HTTPS
Labels: defect

I'm using Netty WebSockets with HTTPS. When performing the handshake, the header field `sec-websocket-origin` is set to a value starting with `http://`, which looks weird.
There is nothing about `sec-websocket-origin` in the RFC. 
The string `http` is set inconditionnally in `io.netty.handler.codec.http.websocketx.WebSocketClientHandshaker13`, line 165.
I'm using Netty-4.1.0.CR7.


Issue comments:
@caillette yet that sounds not right... let me come up with a fix quickly.

Fixed https://github.com/netty/netty/pull/5419


Pull request review comments:
`HttpScheme.HTTPS.port()`

consider using `HttpScheme` here

nit: you could just do `return originValue + ':' + wsPort;`

nit: HttpScheme also provides the `https` portion with `HttpScheme.name()`.


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java
index 4f88bc7cbad..f91e4c7be0d 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java
@@ -32,6 +32,7 @@
 import io.netty.handler.codec.http.HttpRequestEncoder;
 import io.netty.handler.codec.http.HttpResponse;
 import io.netty.handler.codec.http.HttpResponseDecoder;
+import io.netty.handler.codec.http.HttpScheme;
 import io.netty.util.ReferenceCountUtil;
 import io.netty.util.internal.EmptyArrays;
 import io.netty.util.internal.StringUtil;
@@ -444,4 +445,26 @@ static String rawPath(URI wsURL) {
 
         return path == null || path.isEmpty() ? "/" : path;
     }
+
+    static int websocketPort(URI wsURL) {
+        // Format request
+        int wsPort = wsURL.getPort();
+        // check if the URI contained a port if not set the correct one depending on the schema.
+        // See https://github.com/netty/netty/pull/1558
+        if (wsPort == -1) {
+            return "wss".equals(wsURL.getScheme()) ? HttpScheme.HTTPS.port() : HttpScheme.HTTP.port();
+        }
+        return wsPort;
+    }
+
+    static CharSequence websocketOriginValue(String host, int wsPort) {
+        String originValue = (wsPort == HttpScheme.HTTPS.port() ?
+                HttpScheme.HTTPS.name() : HttpScheme.HTTP.name()) + "://" + host;
+        if (wsPort != HttpScheme.HTTP.port() && wsPort != HttpScheme.HTTPS.port()) {
+            // if the port is not standard (80/443) its needed to add the port to the header.
+            // See http://tools.ietf.org/html/rfc6454#section-6.2
+            return originValue + ':' + wsPort;
+        }
+        return originValue;
+    }
 }
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
index b077f9f543b..d0660690648 100644
--- 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
@@ -127,23 +127,16 @@ protected FullHttpRequest newHandshakeRequest() {
         // Get path
         URI wsURL = uri();
         String path = rawPath(wsURL);
+        int wsPort = websocketPort(wsURL);
+        String host = wsURL.getHost();
 
         // 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, wsURL.getHost());
-
-        int wsPort = wsURL.getPort();
-        String originValue = "http://" + wsURL.getHost();
-        if (wsPort != 80 && wsPort != 443) {
-            // if the port is not standard (80/443) its needed to add the port to the header.
-            // See http://tools.ietf.org/html/rfc6454#section-6.2
-            originValue = originValue + ':' + wsPort;
-        }
-
-        headers.add(HttpHeaderNames.ORIGIN, originValue)
+               .add(HttpHeaderNames.HOST, host)
+               .add(HttpHeaderNames.ORIGIN, websocketOriginValue(host, wsPort))
                .add(HttpHeaderNames.SEC_WEBSOCKET_KEY1, key1)
                .add(HttpHeaderNames.SEC_WEBSOCKET_KEY2, key2);
 
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
index c2618b2e47a..d8e670f87a2 100644
--- 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
@@ -141,6 +141,9 @@ protected FullHttpRequest newHandshakeRequest() {
                     key, expectedChallengeResponseString);
         }
 
+        int wsPort = websocketPort(wsURL);
+        String host = wsURL.getHost();
+
         // Format request
         FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, path);
         HttpHeaders headers = request.headers();
@@ -148,16 +151,8 @@ protected FullHttpRequest newHandshakeRequest() {
         headers.add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET)
                .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
                .add(HttpHeaderNames.SEC_WEBSOCKET_KEY, key)
-               .add(HttpHeaderNames.HOST, wsURL.getHost());
-
-        int wsPort = wsURL.getPort();
-        String originValue = "http://" + wsURL.getHost();
-        if (wsPort != 80 && wsPort != 443) {
-            // if the port is not standard (80/443) its needed to add the port to the header.
-            // See http://tools.ietf.org/html/rfc6454#section-6.2
-            originValue = originValue + ':' + wsPort;
-        }
-        headers.add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, originValue);
+               .add(HttpHeaderNames.HOST, host)
+               .add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(host, wsPort));
 
         String expectedSubprotocol = expectedSubprotocol();
         if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) {
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
index d589b7729c6..e5ada055b4b 100644
--- 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
@@ -142,6 +142,9 @@ protected FullHttpRequest newHandshakeRequest() {
                     key, expectedChallengeResponseString);
         }
 
+        int wsPort = websocketPort(wsURL);
+        String host = wsURL.getHost();
+
         // Format request
         FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, path);
         HttpHeaders headers = request.headers();
@@ -149,16 +152,8 @@ protected FullHttpRequest newHandshakeRequest() {
         headers.add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET)
                .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE)
                .add(HttpHeaderNames.SEC_WEBSOCKET_KEY, key)
-               .add(HttpHeaderNames.HOST, wsURL.getHost());
-
-        int wsPort = wsURL.getPort();
-        String originValue = "http://" + wsURL.getHost();
-        if (wsPort != 80 && wsPort != 443) {
-            // if the port is not standard (80/443) its needed to add the port to the header.
-            // See http://tools.ietf.org/html/rfc6454#section-6.2
-            originValue = originValue + ':' + wsPort;
-        }
-        headers.add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, originValue);
+               .add(HttpHeaderNames.HOST, host)
+               .add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(host, wsPort));
 
         String expectedSubprotocol = expectedSubprotocol();
         if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) {
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
index 37a77c72aa1..a311b83fbd2 100644
--- 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
@@ -143,32 +143,16 @@ protected FullHttpRequest newHandshakeRequest() {
         }
 
         // Format request
-        int wsPort = wsURL.getPort();
-        // check if the URI contained a port if not set the correct one depending on the schema.
-        // See https://github.com/netty/netty/pull/1558
-        if (wsPort == -1) {
-            if ("wss".equals(wsURL.getScheme())) {
-                wsPort = 443;
-            } else {
-                wsPort = 80;
-            }
-        }
-
+        int wsPort = websocketPort(wsURL);
+        String host = wsURL.getHost();
         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, wsURL.getHost() + ':' + wsPort);
-
-        String originValue = "http://" + wsURL.getHost();
-        if (wsPort != 80 && wsPort != 443) {
-            // if the port is not standard (80/443) its needed to add the port to the header.
-            // See http://tools.ietf.org/html/rfc6454#section-6.2
-            originValue = originValue + ':' + wsPort;
-        }
-        headers.add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, originValue);
+               .add(HttpHeaderNames.HOST, host + ':' + wsPort)
+               .add(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN, websocketOriginValue(host, wsPort));
 
         String expectedSubprotocol = expectedSubprotocol();
         if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) {


Test patch:
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
index 168a2458d17..91963102fde 100644
--- 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
@@ -15,11 +15,41 @@
  */
 package io.netty.handler.codec.http.websocketx;
 
+import io.netty.handler.codec.http.FullHttpRequest;
+import io.netty.handler.codec.http.HttpHeaderNames;
+import org.junit.Test;
+
 import java.net.URI;
 
+import static org.junit.Assert.assertEquals;
+
 public class WebSocketClientHandshaker07Test extends WebSocketClientHandshakerTest {
     @Override
     protected WebSocketClientHandshaker newHandshaker(URI uri) {
         return new WebSocketClientHandshaker07(uri, WebSocketVersion.V07, null, false, null, 1024);
     }
+
+    @Test
+    public void testSecOriginWss() {
+        URI uri = URI.create("wss://localhost/path%20with%20ws");
+        WebSocketClientHandshaker handshaker = newHandshaker(uri);
+        FullHttpRequest request = handshaker.newHandshakeRequest();
+        try {
+            assertEquals("https://localhost", request.headers().get(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN));
+        } finally {
+            request.release();
+        }
+    }
+
+    @Test
+    public void testSecOriginWs() {
+        URI uri = URI.create("ws://localhost/path%20with%20ws");
+        WebSocketClientHandshaker handshaker = newHandshaker(uri);
+        FullHttpRequest request = handshaker.newHandshakeRequest();
+        try {
+            assertEquals("http://localhost", request.headers().get(HttpHeaderNames.SEC_WEBSOCKET_ORIGIN));
+        } finally {
+            request.release();
+        }
+    }
 }
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
index 249bd958fb0..4ce8016adda 100644
--- 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
@@ -17,7 +17,7 @@
 
 import java.net.URI;
 
-public class WebSocketClientHandshaker08Test extends WebSocketClientHandshakerTest {
+public class WebSocketClientHandshaker08Test extends WebSocketClientHandshaker07Test {
     @Override
     protected WebSocketClientHandshaker newHandshaker(URI uri) {
         return new WebSocketClientHandshaker07(uri, WebSocketVersion.V08, null, false, null, 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
index 2bc2e691b22..ad89fde6bc1 100644
--- 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
@@ -17,7 +17,7 @@
 
 import java.net.URI;
 
-public class WebSocketClientHandshaker13Test extends WebSocketClientHandshakerTest {
+public class WebSocketClientHandshaker13Test extends WebSocketClientHandshaker07Test {
     @Override
     protected WebSocketClientHandshaker newHandshaker(URI uri) {
         return new WebSocketClientHandshaker13(uri, WebSocketVersion.V13, null, false, null, 1024);