The current ManagedWebSocket.IsValidCloseStatus implementation may reject certain valid status codes when it shouldn't

[chenxinyanc]

I don't have a clean code snippet for this (suspected) bug but the current implementation of IsValidCloseStatus will reject WebSocket close status code 1014 "Bad Gateway". Specifically, if remote-side initiates WS closure with status code 1014, ManagedWebSocket will immediately abort the connection.

Suppose we have client-side code as follows, and WS server closes WebSocket proactively after a while, with status code 1014

using var wsClient = new ClientWebSocket();
await wsClient.ConnectAsync(new Uri("..."), default);
var result = await wsClient.ReceiveAsync(Memory<byte>.Empty, default);

There will be an Exception thrown from client side, wsClient enters Aborted state, and server-side also observes the client-side has aborted connection instead of replying with a Close handshake message.

System.Net.WebSockets.WebSocketException
An exception caused the WebSocket to enter the Aborted state. Please see the InnerException, if present, for more details.
   at System.Net.WebSockets.ManagedWebSocket.CloseWithReceiveErrorAndThrowAsync(WebSocketCloseStatus closeStatus, WebSocketError error, String errorMessage, Exception innerException)
   at System.Net.WebSockets.ManagedWebSocket.HandleReceivedCloseAsync(MessageHeader header, CancellationToken cancellationToken)
   at System.Net.WebSockets.ManagedWebSocket.ReceiveAsyncPrivate[TResult](Memory`1 payloadBuffer, CancellationToken cancellationToken)
   at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1.StateMachineBox`1.System.Threading.Tasks.Sources.IValueTaskSource<TResult>.GetResult(Int16 token)
   at 
...

It seems that currently ManagedWebSocket.IsValidCloseStatus will reject all the WS close status code falling into the range of 1000-2999 but not within the (most of the) WebSocketCloseStatus enum values. This is too strict and not resilient to any possible future extensions.

I think the point is, as long as the WS close frame received from remote-side is valid, is it possible for ManagedWebSocket just to relay whatever status code received from remote side to the API consumer, and let them decide whether they'd like to abort the connection? I understand there are some status codes never intended to be sent (e.g., 1005 "Empty" or 1006 "Abnormal closure"), but that doesn't sound fit to me to reject all the other status codes in 1000-2999 because of this.

runtime/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs

Lines 1030 to 1067 in 5415cd1

	/// <summary>Check whether a close status is valid according to the RFC.</summary>
	/// <param name="closeStatus">The status to validate.</param>
	/// <returns>true if the status if valid; otherwise, false.</returns>
	private static bool IsValidCloseStatus(WebSocketCloseStatus closeStatus)
	{
	// 0-999: "not used"
	// 1000-2999: reserved for the protocol; we need to check individual codes manually
	// 3000-3999: reserved for use by higher-level code
	// 4000-4999: reserved for private use
	// 5000-: not mentioned in RFC
	if (closeStatus < (WebSocketCloseStatus)1000 || closeStatus >= (WebSocketCloseStatus)5000)
	{
	return false;
	}
	if (closeStatus >= (WebSocketCloseStatus)3000)
	{
	return true;
	}
	switch (closeStatus) // check for the 1000-2999 range known codes
	{
	case WebSocketCloseStatus.EndpointUnavailable:
	case WebSocketCloseStatus.InternalServerError:
	case WebSocketCloseStatus.InvalidMessageType:
	case WebSocketCloseStatus.InvalidPayloadData:
	case WebSocketCloseStatus.MandatoryExtension:
	case WebSocketCloseStatus.MessageTooBig:
	case WebSocketCloseStatus.NormalClosure:
	case WebSocketCloseStatus.PolicyViolation:
	case WebSocketCloseStatus.ProtocolError:
	return true;
	default:
	return false;
	}
	}

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I don't have a clean code snippet for this (suspected) bug but the current implementation of IsValidCloseStatus will reject WebSocket close status code 1014 "Bad Gateway". Specifically, if remote-side initiates WS closure with status code 1014, ManagedWebSocket will immediately abort the connection.

Suppose we have client-side code as follows, and WS server closes WebSocket proactively after a while, with status code 1014

using var wsClient = new ClientWebSocket();
await wsClient.ConnectAsync(new Uri("..."), default);
var result = await wsClient.ReceiveAsync(Memory<byte>.Empty, default);

There will be an Exception thrown from client side, wsClient enters Aborted state, and server-side also observes the client-side has aborted connection instead of replying with a Close handshake message.

System.Net.WebSockets.WebSocketException
An exception caused the WebSocket to enter the Aborted state. Please see the InnerException, if present, for more details.
   at System.Net.WebSockets.ManagedWebSocket.CloseWithReceiveErrorAndThrowAsync(WebSocketCloseStatus closeStatus, WebSocketError error, String errorMessage, Exception innerException)
   at System.Net.WebSockets.ManagedWebSocket.HandleReceivedCloseAsync(MessageHeader header, CancellationToken cancellationToken)
   at System.Net.WebSockets.ManagedWebSocket.ReceiveAsyncPrivate[TResult](Memory`1 payloadBuffer, CancellationToken cancellationToken)
   at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1.StateMachineBox`1.System.Threading.Tasks.Sources.IValueTaskSource<TResult>.GetResult(Int16 token)
   at 
...

It seems that currently ManagedWebSocket.IsValidCloseStatus will reject all the WS close status code falling into the range of 1000-2999 but not within the (most of the) WebSocketCloseStatus enum values. This is too strict and not resilient to any possible future extensions.

I think the point is, as long as the WS close frame received from remote-side is valid, is it possible for ManagedWebSocket just to relay whatever status code received from remote side to the API consumer, and let them decide whether they'd like to abort the connection? I understand there are some status codes never intended to be sent (e.g., 1005 "Empty" or 1006 "Abnormal closure"), but that doesn't sound fit to me to reject all the other status codes in 1000-2999 because of this.

runtime/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs

Lines 1030 to 1067 in 5415cd1

	/// <summary>Check whether a close status is valid according to the RFC.</summary>
	/// <param name="closeStatus">The status to validate.</param>
	/// <returns>true if the status if valid; otherwise, false.</returns>
	private static bool IsValidCloseStatus(WebSocketCloseStatus closeStatus)
	{
	// 0-999: "not used"
	// 1000-2999: reserved for the protocol; we need to check individual codes manually
	// 3000-3999: reserved for use by higher-level code
	// 4000-4999: reserved for private use
	// 5000-: not mentioned in RFC
	if (closeStatus < (WebSocketCloseStatus)1000 || closeStatus >= (WebSocketCloseStatus)5000)
	{
	return false;
	}
	if (closeStatus >= (WebSocketCloseStatus)3000)
	{
	return true;
	}
	switch (closeStatus) // check for the 1000-2999 range known codes
	{
	case WebSocketCloseStatus.EndpointUnavailable:
	case WebSocketCloseStatus.InternalServerError:
	case WebSocketCloseStatus.InvalidMessageType:
	case WebSocketCloseStatus.InvalidPayloadData:
	case WebSocketCloseStatus.MandatoryExtension:
	case WebSocketCloseStatus.MessageTooBig:
	case WebSocketCloseStatus.NormalClosure:
	case WebSocketCloseStatus.PolicyViolation:
	case WebSocketCloseStatus.ProtocolError:
	return true;
	default:
	return false;
	}
	}

Author:	chenxinyanc
Assignees:	-
Labels:	area-System.Net, untriaged
Milestone:	-

[CarnaViire]

I see that these codes (1012, 1013, 1014) are reserved, but not a part of any official RFC yet... I wonder what is our policy for such cases @stephentoub @dotnet/ncl

The problem IMO is not only that it throws, but it also loses the information about the actual close status and everything that was in the close frame

runtime/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs

Lines 984 to 987 in 57bfe47

	if (!IsValidCloseStatus(closeStatus))
	{
	await CloseWithReceiveErrorAndThrowAsync(WebSocketCloseStatus.ProtocolError, WebSocketError.Faulted).ConfigureAwait(false);
	}

Also, we can send 1014&co but can't receive it -- the checks for received close status and for to-be-sent close status are different, which is additionally troublesome.

[CarnaViire]

Triage: we decided that we will allow the reserved codes (1012, 1013, 1014), but we won't add them to the WebSocketCloseStatus enum, unless there would be much customer ask. This way we won't need to change public API.

The fix should be straightforward. We are happy to accept contributions.

Putting to Future, since it's the first issue about it in quite a long time.

[IDisposable]

In building out the tests for this it becomes evident that not adding the three new acceptable close status codes to the WebSocketCloseStatus enum makes the testing break. It seems like it's just better not to play spooky magic with those three integer values since testing them gives errors.

Additionally, while the RFC doesn't spell these codes out, the Mozilla doc does and they are reasonably likely to never be usurped.

[IDisposable]

PR #83713 (sorry for the mess above... just trying to neaten up the commit comment and broke everything, Mea Culpa)

[CarnaViire]

In building out the tests for this it becomes evident that not adding the three new acceptable close status codes to the WebSocketCloseStatus enum makes the testing break.

How exactly does it break testing?

(let's move the conversation to your PR though, I've put some comments there)

[IDisposable]

I'm reworking this now per the feedback on the PR