Repository: netty/netty
Issue: Check for valid status codes should be done in CloseWebSocketFrame instead of WebSocketCloseStatus

According to the specification https://tools.ietf.org/html/rfc6455#section-7.4

```
1006 is a reserved value and MUST NOT be set as a status code in a
Close control frame by an endpoint.  It is designated for use in
applications expecting a status code to indicate that the
connection was closed abnormally, e.g., without sending or
receiving a Close control frame.
```

I want to use `WebSocketCloseStatus` in the applications in order to indicate that the
connection was closed abnormally.

I do not intent to send `CloseWebSocketFrame` with such `WebSocketCloseStatus` (with 1006 status code).

However the current implementation checks the validity of the status codes in `WebSocketCloseStatus`
instead of `CloseWebSocketFrame`.

Isn't it more correct if the check for a valid status code is in `CloseWebSocketFrame`?

### Expected behavior
To be able to create `WebSocketCloseStatus` with status code 1006
and `CloseWebSocketFrame ` to fail when constructed with `WebSocketCloseStatus` with status code 1006

### Actual behavior
`WebSocketCloseStatus` fails when created with status code 1006

### Proposed solution

I can create a PR with the patch below

```
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java
index 7231bde8f4..834d0e6eee 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java
@@ -40,7 +40,7 @@ public class CloseWebSocketFrame extends WebSocketFrame {
      *            example, <tt>1000</tt> indicates normal closure.
      */
     public CloseWebSocketFrame(WebSocketCloseStatus status) {
-        this(status.code(), status.reasonText());
+        this(requireValidStatusCode(status), status.reasonText());
     }
 
     /**
@@ -53,7 +53,7 @@ public class CloseWebSocketFrame extends WebSocketFrame {
      *            Reason text. Set to null if no text.
      */
     public CloseWebSocketFrame(WebSocketCloseStatus status, String reasonText) {
-        this(status.code(), reasonText);
+        this(requireValidStatusCode(status), reasonText);
     }
 
     /**
@@ -201,4 +201,13 @@ public class CloseWebSocketFrame extends WebSocketFrame {
         super.touch(hint);
         return this;
     }
+
+    static int requireValidStatusCode(WebSocketCloseStatus status) {
+        if (WebSocketCloseStatus.isValidStatusCode(status.code())) {
+            return status.code();
+        }
+        else {
+            throw new IllegalArgumentException("WebSocket close status code does NOT comply with RFC-6455: " + status.code());
+        }
+    }
 }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java
index 92b1dbaef5..f634240ef5 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java
@@ -208,16 +208,26 @@ public final class WebSocketCloseStatus implements Comparable<WebSocketCloseStat
 
     // 1004, 1005, 1006, 1015 are reserved and should never be used by user
     //public static final WebSocketCloseStatus SPECIFIC_MEANING = register(1004, "...");
-    //public static final WebSocketCloseStatus EMPTY = register(1005, "Empty");
-    //public static final WebSocketCloseStatus ABNORMAL_CLOSURE = register(1006, "Abnormal closure");
-    //public static final WebSocketCloseStatus TLS_HANDSHAKE_FAILED(1015, "TLS handshake failed");
+
+    public static final WebSocketCloseStatus EMPTY =
+        new WebSocketCloseStatus(1005, "Empty");
+
+    public static final WebSocketCloseStatus ABNORMAL_CLOSURE =
+        new WebSocketCloseStatus(1006, "Abnormal closure");
+
+    public static final WebSocketCloseStatus TLS_HANDSHAKE_FAILED =
+        new WebSocketCloseStatus(1015, "TLS handshake failed");
 
     private final int statusCode;
     private final String reasonText;
     private String text;
 
     public WebSocketCloseStatus(int statusCode, String reasonText) {
-        if (!isValidStatusCode(statusCode)) {
+        this(statusCode, reasonText, true);
+    }
+
+    public WebSocketCloseStatus(int statusCode, String reasonText, boolean validate) {
+        if (validate && !isValidStatusCode(statusCode)) {
             throw new IllegalArgumentException(
                 "WebSocket close status code does NOT comply with RFC-6455: " + statusCode);
         }
```

### Netty version
4.1.55.Final-SNAPSHOT


Issue comments:
Also  for `CloseWebSocketFrame` constructors where you can provide directly `int` for the status code, I see that the status code is checked when reading the frame, but isn't it better to check it also when constructing `CloseWebSocketFrame`? (My Netty server was able to create and send such `CloseWebSocketFrame` and only my Netty client complained about the status code)

`new CloseWebSocketFrame(1006, "Abnormal closure")`

```
io.netty.handler.codec.http.websocketx.CorruptedWebSocketFrameException: Invalid close frame getStatus code: 1006
	at io.netty.handler.codec.http.websocketx.WebSocket08FrameDecoder.protocolViolation(WebSocket08FrameDecoder.java:426)
	at io.netty.handler.codec.http.websocketx.WebSocket08FrameDecoder.protocolViolation(WebSocket08FrameDecoder.java:422)
	at io.netty.handler.codec.http.websocketx.WebSocket08FrameDecoder.checkCloseFrameBody(WebSocket08FrameDecoder.java:479)
	at io.netty.handler.codec.http.websocketx.WebSocket08FrameDecoder.decode(WebSocket08FrameDecoder.java:339)
	at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:501)
```

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java
index 7231bde8f4a..1703203902e 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java
@@ -40,7 +40,7 @@ public CloseWebSocketFrame() {
      *            example, <tt>1000</tt> indicates normal closure.
      */
     public CloseWebSocketFrame(WebSocketCloseStatus status) {
-        this(status.code(), status.reasonText());
+        this(requireValidStatusCode(status.code()), status.reasonText());
     }
 
     /**
@@ -53,7 +53,7 @@ public CloseWebSocketFrame(WebSocketCloseStatus status) {
      *            Reason text. Set to null if no text.
      */
     public CloseWebSocketFrame(WebSocketCloseStatus status, String reasonText) {
-        this(status.code(), reasonText);
+        this(requireValidStatusCode(status.code()), reasonText);
     }
 
     /**
@@ -66,7 +66,7 @@ public CloseWebSocketFrame(WebSocketCloseStatus status, String reasonText) {
      *            Reason text. Set to null if no text.
      */
     public CloseWebSocketFrame(int statusCode, String reasonText) {
-        this(true, 0, statusCode, reasonText);
+        this(true, 0, requireValidStatusCode(statusCode), reasonText);
     }
 
     /**
@@ -95,7 +95,7 @@ public CloseWebSocketFrame(boolean finalFragment, int rsv) {
      *            Reason text. Set to null if no text.
      */
     public CloseWebSocketFrame(boolean finalFragment, int rsv, int statusCode, String reasonText) {
-        super(finalFragment, rsv, newBinaryData(statusCode, reasonText));
+        super(finalFragment, rsv, newBinaryData(requireValidStatusCode(statusCode), reasonText));
     }
 
     private static ByteBuf newBinaryData(int statusCode, String reasonText) {
@@ -201,4 +201,13 @@ public CloseWebSocketFrame touch(Object hint) {
         super.touch(hint);
         return this;
     }
+
+    static int requireValidStatusCode(int statusCode) {
+        if (WebSocketCloseStatus.isValidStatusCode(statusCode)) {
+            return statusCode;
+        } else {
+            throw new IllegalArgumentException("WebSocket close status code does NOT comply with RFC-6455: " +
+                    statusCode);
+        }
+    }
 }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java
index 92b1dbaef56..2a0b7edb6da 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatus.java
@@ -208,16 +208,26 @@ public final class WebSocketCloseStatus implements Comparable<WebSocketCloseStat
 
     // 1004, 1005, 1006, 1015 are reserved and should never be used by user
     //public static final WebSocketCloseStatus SPECIFIC_MEANING = register(1004, "...");
-    //public static final WebSocketCloseStatus EMPTY = register(1005, "Empty");
-    //public static final WebSocketCloseStatus ABNORMAL_CLOSURE = register(1006, "Abnormal closure");
-    //public static final WebSocketCloseStatus TLS_HANDSHAKE_FAILED(1015, "TLS handshake failed");
+
+    public static final WebSocketCloseStatus EMPTY =
+        new WebSocketCloseStatus(1005, "Empty", false);
+
+    public static final WebSocketCloseStatus ABNORMAL_CLOSURE =
+        new WebSocketCloseStatus(1006, "Abnormal closure", false);
+
+    public static final WebSocketCloseStatus TLS_HANDSHAKE_FAILED =
+        new WebSocketCloseStatus(1015, "TLS handshake failed", false);
 
     private final int statusCode;
     private final String reasonText;
     private String text;
 
     public WebSocketCloseStatus(int statusCode, String reasonText) {
-        if (!isValidStatusCode(statusCode)) {
+        this(statusCode, reasonText, true);
+    }
+
+    public WebSocketCloseStatus(int statusCode, String reasonText, boolean validate) {
+        if (validate && !isValidStatusCode(statusCode)) {
             throw new IllegalArgumentException(
                 "WebSocket close status code does NOT comply with RFC-6455: " + statusCode);
         }
@@ -290,6 +300,10 @@ public static WebSocketCloseStatus valueOf(int code) {
                 return PROTOCOL_ERROR;
             case 1003:
                 return INVALID_MESSAGE_TYPE;
+            case 1005:
+                return EMPTY;
+            case 1006:
+                return ABNORMAL_CLOSURE;
             case 1007:
                 return INVALID_PAYLOAD_DATA;
             case 1008:
@@ -306,6 +320,8 @@ public static WebSocketCloseStatus valueOf(int code) {
                 return TRY_AGAIN_LATER;
             case 1014:
                 return BAD_GATEWAY;
+            case 1015:
+                return TLS_HANDSHAKE_FAILED;
             default:
                 return new WebSocketCloseStatus(code, "Close status #" + code);
         }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrameTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrameTest.java
new file mode 100644
index 00000000000..f1bcc3fc3d8
--- /dev/null
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrameTest.java
@@ -0,0 +1,79 @@
+/*
+ * Copyright 2020 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License, version 2.0 (the "License");
+ * you may not use this file except in compliance with the License. You may obtain a copy of the License at:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is
+ * distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and limitations under the License.
+ */
+package io.netty.handler.codec.http.websocketx;
+
+import org.assertj.core.api.ThrowableAssert;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+class CloseWebSocketFrameTest {
+
+    @Test
+    void testInvalidCode() {
+        doTestInvalidCode(new ThrowableAssert.ThrowingCallable() {
+
+            @Override
+            public void call() throws RuntimeException {
+                new CloseWebSocketFrame(WebSocketCloseStatus.ABNORMAL_CLOSURE);
+            }
+        });
+
+        doTestInvalidCode(new ThrowableAssert.ThrowingCallable() {
+
+            @Override
+            public void call() throws RuntimeException {
+                new CloseWebSocketFrame(WebSocketCloseStatus.ABNORMAL_CLOSURE, "invalid code");
+            }
+        });
+
+        doTestInvalidCode(new ThrowableAssert.ThrowingCallable() {
+
+            @Override
+            public void call() throws RuntimeException {
+                new CloseWebSocketFrame(1006, "invalid code");
+            }
+        });
+
+        doTestInvalidCode(new ThrowableAssert.ThrowingCallable() {
+
+            @Override
+            public void call() throws RuntimeException {
+                new CloseWebSocketFrame(true, 0, 1006, "invalid code");
+            }
+        });
+    }
+
+    @Test
+    void testValidCode() {
+        doTestValidCode(new CloseWebSocketFrame(WebSocketCloseStatus.NORMAL_CLOSURE),
+                WebSocketCloseStatus.NORMAL_CLOSURE.code(), WebSocketCloseStatus.NORMAL_CLOSURE.reasonText());
+
+        doTestValidCode(new CloseWebSocketFrame(WebSocketCloseStatus.NORMAL_CLOSURE, "valid code"),
+                WebSocketCloseStatus.NORMAL_CLOSURE.code(), "valid code");
+
+        doTestValidCode(new CloseWebSocketFrame(1000, "valid code"), 1000, "valid code");
+
+        doTestValidCode(new CloseWebSocketFrame(true, 0, 1000, "valid code"), 1000, "valid code");
+    }
+
+    private static void doTestInvalidCode(ThrowableAssert.ThrowingCallable callable) {
+        assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(callable);
+    }
+
+    private static void doTestValidCode(CloseWebSocketFrame frame, int expectedCode, String expectedReason) {
+        assertThat(frame.statusCode()).isEqualTo(expectedCode);
+        assertThat(frame.reasonText()).isEqualTo(expectedReason);
+    }
+}
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatusTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatusTest.java
index d8ab7f9bc98..223c12ce3d0 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatusTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketCloseStatusTest.java
@@ -18,9 +18,11 @@
 import java.util.SortedSet;
 import java.util.TreeSet;
 
+import org.assertj.core.api.ThrowableAssert;
 import org.hamcrest.Matchers;
 import org.junit.Test;
 
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotSame;
@@ -56,6 +58,8 @@ public void testKnownStatuses() {
         assertSame(ENDPOINT_UNAVAILABLE, valueOf(1001));
         assertSame(PROTOCOL_ERROR, valueOf(1002));
         assertSame(INVALID_MESSAGE_TYPE, valueOf(1003));
+        assertSame(EMPTY, valueOf(1005));
+        assertSame(ABNORMAL_CLOSURE, valueOf(1006));
         assertSame(INVALID_PAYLOAD_DATA, valueOf(1007));
         assertSame(POLICY_VIOLATION, valueOf(1008));
         assertSame(MESSAGE_TOO_BIG, valueOf(1009));
@@ -64,6 +68,7 @@ public void testKnownStatuses() {
         assertSame(SERVICE_RESTART, valueOf(1012));
         assertSame(TRY_AGAIN_LATER, valueOf(1013));
         assertSame(BAD_GATEWAY, valueOf(1014));
+        assertSame(TLS_HANDSHAKE_FAILED, valueOf(1015));
     }
 
     @Test
@@ -127,4 +132,23 @@ public void testRfc6455CodeValidation() {
         invalidCodes.retainAll(knownCodes);
         assertEquals(invalidCodes, Collections.emptySet());
     }
+
+    @Test
+    public void testValidationEnabled() {
+        assertThatExceptionOfType(IllegalArgumentException.class)
+                .isThrownBy(new ThrowableAssert.ThrowingCallable() {
+
+                    @Override
+                    public void call() throws RuntimeException {
+                        new WebSocketCloseStatus(1006, "validation disabled");
+                    }
+                });
+    }
+
+    @Test
+    public void testValidationDisabled() {
+        WebSocketCloseStatus status = new WebSocketCloseStatus(1006, "validation disabled", false);
+        assertEquals(1006, status.code());
+        assertEquals("validation disabled", status.reasonText());
+    }
 }