Skip to content

[release/7.0] Throw on incompatible WebSocket options #75014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,10 @@
<data name="net_WebSockets_ClientWindowBitsNegotiationFailure" xml:space="preserve">
<value>The WebSocket failed to negotiate max client window bits. The client requested {0} but the server responded with {1}.</value>
</data>
<data name="net_WebSockets_OptionsIncompatibleWithCustomInvoker" xml:space="preserve">
<value>UseDefaultCredentials, Credentials, Proxy, ClientCertificates, RemoteCertificateValidationCallback and Cookies must not be set on ClientWebSocketOptions when an HttpMessageInvoker instance is also specified. These options should be set on the HttpMessageInvoker's underlying HttpMessageHandler instead.</value>
</data>
<data name="net_WebSockets_CustomInvokerRequiredForHttp2" xml:space="preserve">
<value>An HttpMessageInvoker instance must be passed to ConnectAsync when using HTTP/2.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ public sealed class ClientWebSocketOptions
private HttpVersionPolicy _versionPolicy = HttpVersionPolicy.RequestVersionOrLower;
private bool _collectHttpResponseDetails;

internal bool AreCompatibleWithCustomInvoker() =>
!UseDefaultCredentials &&
Credentials is null &&
(_clientCertificates?.Count ?? 0) == 0 &&
RemoteCertificateValidationCallback is null &&
Cookies is null &&
(Proxy is null || Proxy == WebSocketHandle.DefaultWebProxy.Instance);

internal ClientWebSocketOptions() { } // prevent external instantiation

#region HTTP Settings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,22 @@ public void Abort()
public async Task ConnectAsync(Uri uri, HttpMessageInvoker? invoker, CancellationToken cancellationToken, ClientWebSocketOptions options)
{
bool disposeHandler = false;
invoker ??= new HttpMessageInvoker(SetupHandler(options, out disposeHandler));
HttpResponseMessage? response = null;
if (invoker is null)
{
if (options.HttpVersion.Major >= 2 || options.HttpVersionPolicy == HttpVersionPolicy.RequestVersionOrHigher)
{
throw new ArgumentException(SR.net_WebSockets_CustomInvokerRequiredForHttp2, nameof(options));
}

invoker = new HttpMessageInvoker(SetupHandler(options, out disposeHandler));
}
else if (!options.AreCompatibleWithCustomInvoker())
{
// This will not throw if the Proxy is a DefaultWebProxy.
throw new ArgumentException(SR.net_WebSockets_OptionsIncompatibleWithCustomInvoker, nameof(options));
}

HttpResponseMessage? response = null;
bool disposeResponse = false;

// force non-secure request to 1.1 whenever it is possible as HttpClient does
Expand Down Expand Up @@ -237,12 +250,7 @@ private static SocketsHttpHandler SetupHandler(ClientWebSocketOptions options, o
// Create the handler for this request and populate it with all of the options.
// Try to use a shared handler rather than creating a new one just for this request, if
// the options are compatible.
if (options.Credentials == null &&
!options.UseDefaultCredentials &&
options.Proxy == null &&
options.Cookies == null &&
options.RemoteCertificateValidationCallback == null &&
(options._clientCertificates?.Count ?? 0) == 0)
if (options.AreCompatibleWithCustomInvoker() && options.Proxy is null)
{
disposeHandler = false;
handler = s_defaultHandler;
Expand Down Expand Up @@ -518,7 +526,7 @@ private static void ValidateHeader(HttpHeaders headers, string name, string expe
}

/// <summary>Used as a sentinel to indicate that ClientWebSocket should use the system's default proxy.</summary>
private sealed class DefaultWebProxy : IWebProxy
internal sealed class DefaultWebProxy : IWebProxy
{
public static DefaultWebProxy Instance { get; } = new DefaultWebProxy();
public ICredentials? Credentials { get => throw new NotSupportedException(); set => throw new NotSupportedException(); }
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ public sealed class InvokerAbortTest : AbortTest
{
public InvokerAbortTest(ITestOutputHelper output) : base(output) { }

protected override HttpMessageInvoker? GetInvoker() => new HttpMessageInvoker(new SocketsHttpHandler());
protected override bool UseCustomInvoker => true;
}

public sealed class HttpClientAbortTest : AbortTest
{
public HttpClientAbortTest(ITestOutputHelper output) : base(output) { }

protected override HttpMessageInvoker? GetInvoker() => new HttpClient(new HttpClientHandler());
protected override bool UseHttpClient => true;
}

public class AbortTest : ClientWebSocketTestBase
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ public sealed class InvokerCancelTest : CancelTest
{
public InvokerCancelTest(ITestOutputHelper output) : base(output) { }

protected override HttpMessageInvoker? GetInvoker() => new HttpMessageInvoker(new SocketsHttpHandler());
protected override bool UseCustomInvoker => true;
}

public sealed class HttpClientCancelTest : CancelTest
{
public HttpClientCancelTest(ITestOutputHelper output) : base(output) { }

protected override HttpMessageInvoker? GetInvoker() => new HttpClient(new HttpClientHandler());
protected override bool UseHttpClient => true;
}

public class CancelTest : ClientWebSocketTestBase
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,17 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Net.Test.Common;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

using Xunit;
using Xunit.Abstractions;
using System.Net.Http;
using System.Net.WebSockets.Client.Tests;
using System.Diagnostics;

namespace System.Net.WebSockets.Client.Tests
{
/// <summary>
/// ClientWebSocket tests that do require a remote server.
/// </summary>
public class ClientWebSocketTestBase
{
public static readonly object[][] EchoServers = System.Net.Test.Common.Configuration.WebSockets.EchoServers;
Expand Down Expand Up @@ -112,7 +108,38 @@ protected static async Task<WebSocketReceiveResult> ReceiveEntireMessageAsync(We
}
}

protected virtual HttpMessageInvoker? GetInvoker() => null;
protected virtual bool UseCustomInvoker => false;

protected virtual bool UseHttpClient => false;

protected bool UseSharedHandler => !UseCustomInvoker && !UseHttpClient;

protected Action<HttpClientHandler>? ConfigureCustomHandler;

internal HttpMessageInvoker? GetInvoker()
{
var handler = new HttpClientHandler();

if (PlatformDetection.IsNotBrowser)
{
handler.ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator;
}

ConfigureCustomHandler?.Invoke(handler);

if (UseCustomInvoker)
{
Debug.Assert(!UseHttpClient);
return new HttpMessageInvoker(handler);
}

if (UseHttpClient)
{
return new HttpClient(handler);
}

return null;
}

protected Task<ClientWebSocket> GetConnectedWebSocket(Uri uri, int TimeOutMilliseconds, ITestOutputHelper output) =>
WebSocketHelper.GetConnectedWebSocket(uri, TimeOutMilliseconds, output, invoker: GetInvoker());
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ public sealed class InvokerCloseTest : CloseTest
{
public InvokerCloseTest(ITestOutputHelper output) : base(output) { }

protected override HttpMessageInvoker? GetInvoker() => new HttpMessageInvoker(new SocketsHttpHandler());
protected override bool UseCustomInvoker => true;
}

public sealed class HttpClientCloseTest : CloseTest
{
public HttpClientCloseTest(ITestOutputHelper output) : base(output) { }

protected override HttpMessageInvoker? GetInvoker() => new HttpClient(new HttpClientHandler());
protected override bool UseHttpClient => true;
}

public class CloseTest : ClientWebSocketTestBase
Expand Down
Loading