Skip to content
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

Remove Uri scheme validation from HttpRequestMessage #55035

Merged
merged 7 commits into from
Jul 10, 2021
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 @@ -1950,5 +1950,23 @@ public async Task GetAsync_InvalidUrl_ExpectedExceptionThrown()
await Assert.ThrowsAsync<HttpRequestException>(() => client.GetStringAsync(invalidUri));
}
}

// HttpRequestMessage ctor guards against such Uris before .NET 6. We allow passing relative/unknown Uris to BrowserHttpHandler.
public static bool InvalidRequestUriTest_IsSupported => PlatformDetection.IsNotNetFramework && PlatformDetection.IsNotBrowser;

[ConditionalFact(nameof(InvalidRequestUriTest_IsSupported))]
public async Task SendAsync_InvalidRequestUri_Throws()
{
using var invoker = new HttpMessageInvoker(CreateHttpClientHandler());

var request = new HttpRequestMessage(HttpMethod.Get, (Uri)null);
await Assert.ThrowsAsync<InvalidOperationException>(() => invoker.SendAsync(request, CancellationToken.None));

request = new HttpRequestMessage(HttpMethod.Get, new Uri("/relative", UriKind.Relative));
await Assert.ThrowsAsync<InvalidOperationException>(() => invoker.SendAsync(request, CancellationToken.None));

request = new HttpRequestMessage(HttpMethod.Get, new Uri("foo://foo.bar"));
await Assert.ThrowsAsync<NotSupportedException>(() => invoker.SendAsync(request, CancellationToken.None));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,10 @@
<data name="PlatformNotSupported_WinHttpHandler" xml:space="preserve">
<value>WinHttpHandler is only supported on .NET Framework and .NET runtimes on Windows. It is not supported for Windows Store Applications (UWP) or Unix platforms.</value>
</data>
<data name="net_http_unsupported_requesturi_scheme" xml:space="preserve">
<value>The '{0}' scheme is not supported.</value>
</data>
<data name="net_http_client_invalid_requesturi" xml:space="preserve">
<value>An invalid request URI was provided. Either the request URI must be an absolute URI or BaseAddress must be set.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,17 @@ protected override void Dispose(bool disposing)
throw new ArgumentNullException(nameof(request), SR.net_http_handler_norequest);
}

Uri? requestUri = request.RequestUri;
if (requestUri is null || !requestUri.IsAbsoluteUri)
{
throw new InvalidOperationException(SR.net_http_client_invalid_requesturi);
}

if (requestUri.Scheme != Uri.UriSchemeHttp && requestUri.Scheme != Uri.UriSchemeHttps)
{
throw new NotSupportedException(SR.Format(SR.net_http_unsupported_requesturi_scheme, requestUri.Scheme));
}

// Check for invalid combinations of properties.
if (_proxy != null && _windowsProxyUsePolicy != WindowsProxyUsePolicy.UseCustomProxy)
{
Expand Down
9 changes: 3 additions & 6 deletions src/libraries/System.Net.Http/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,10 @@
<value>The base address must be an absolute URI.</value>
</data>
<data name="net_http_client_invalid_requesturi" xml:space="preserve">
<value>An invalid request URI was provided. The request URI must either be an absolute URI or BaseAddress must be set.</value>
<value>An invalid request URI was provided. Either the request URI must be an absolute URI or BaseAddress must be set.</value>
</data>
<data name="net_http_client_http_baseaddress_required" xml:space="preserve">
<value>Only 'http' and 'https' schemes are allowed.</value>
</data>
<data name="net_http_client_http_browser_baseaddress_required" xml:space="preserve">
<value>Only 'http', 'https', and 'blob' schemes are allowed.</value>
<data name="net_http_unsupported_requesturi_scheme" xml:space="preserve">
<value>The '{0}' scheme is not supported.</value>
</data>
<data name="net_http_parser_invalid_base64_string" xml:space="preserve">
<value>Value '{0}' is not a valid Base64 string. Error: {1}</value>
Expand Down
4 changes: 1 addition & 3 deletions src/libraries/System.Net.Http/src/System.Net.Http.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
<Compile Include="System\Net\Http\HttpResponseMessage.cs" />
<Compile Include="System\Net\Http\HttpRuleParser.cs" />
<Compile Include="System\Net\Http\HttpTelemetry.cs" />
<Compile Include="System\Net\Http\HttpUtilities.cs" />
<Compile Include="System\Net\Http\HttpVersionPolicy.cs" />
<Compile Include="System\Net\Http\MessageProcessingHandler.cs" />
<Compile Include="System\Net\Http\MultipartContent.cs" />
Expand Down Expand Up @@ -180,6 +179,7 @@
<Compile Include="System\Net\Http\SocketsHttpHandler\HttpContentStream.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\HttpContentWriteStream.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\HttpKeepAlivePingPolicy.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\HttpUtilities.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\IHttpTrace.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\IMultiWebProxy.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\MultiProxy.cs" />
Expand All @@ -188,7 +188,6 @@
<Compile Include="System\Net\Http\SocketsHttpHandler\SocketsHttpConnectionContext.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\SocketsHttpHandler.cs" />
<Compile Include="System\Net\Http\HttpTelemetry.AnyOS.cs" />
<Compile Include="System\Net\Http\HttpUtilities.AnyOS.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\SystemProxyInfo.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\SocksHelper.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\SocksException.cs" />
Expand Down Expand Up @@ -614,7 +613,6 @@
<Compile Include="System\Net\Http\BrowserHttpHandler\SocketsHttpHandler.cs" />
<Compile Include="System\Net\Http\BrowserHttpHandler\BrowserHttpHandler.cs" />
<Compile Include="System\Net\Http\BrowserHttpHandler\HttpTelemetry.Browser.cs" />
<Compile Include="System\Net\Http\BrowserHttpHandler\HttpUtilities.Browser.cs" />
<Compile Include="$(CommonPath)System\Net\Http\HttpHandlerDefaults.cs"
Link="Common\System\Net\Http\HttpHandlerDefaults.cs" />
</ItemGroup>
Expand Down

This file was deleted.

31 changes: 9 additions & 22 deletions src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ public partial class HttpClient : HttpMessageInvoker

private CancellationTokenSource _pendingRequestsCts;
private HttpRequestHeaders? _defaultRequestHeaders;
private Version _defaultRequestVersion = HttpUtilities.DefaultRequestVersion;
private HttpVersionPolicy _defaultVersionPolicy = HttpUtilities.DefaultVersionPolicy;
private Version _defaultRequestVersion = HttpRequestMessage.DefaultRequestVersion;
private HttpVersionPolicy _defaultVersionPolicy = HttpRequestMessage.DefaultVersionPolicy;

private Uri? _baseAddress;
private TimeSpan _timeout;
Expand Down Expand Up @@ -78,7 +78,12 @@ public HttpVersionPolicy DefaultVersionPolicy
get => _baseAddress;
set
{
CheckBaseAddress(value, nameof(value));
// It's OK to not have a base address specified, but if one is, it needs to be absolute.
if (value is not null && !value.IsAbsoluteUri)
{
throw new ArgumentException(SR.net_http_client_absolute_baseaddress_required, nameof(value));
}

CheckDisposedOrStarted();

if (NetEventSource.Log.IsEnabled()) NetEventSource.UriBaseAddress(this, value);
Expand Down Expand Up @@ -621,7 +626,7 @@ private void HandleFailure(Exception e, bool telemetryStarted, HttpResponseMessa

private static bool StartSend(HttpRequestMessage request)
{
if (HttpTelemetry.Log.IsEnabled() && request.RequestUri != null)
if (HttpTelemetry.Log.IsEnabled())
{
HttpTelemetry.Log.RequestStart(request);
return true;
Expand Down Expand Up @@ -810,24 +815,6 @@ private void PrepareRequestMessage(HttpRequestMessage request)
return (pendingRequestsCts, DisposeTokenSource: false, pendingRequestsCts);
}

private static void CheckBaseAddress(Uri? baseAddress, string parameterName)
{
if (baseAddress == null)
{
return; // It's OK to not have a base address specified.
}

if (!baseAddress.IsAbsoluteUri)
{
throw new ArgumentException(SR.net_http_client_absolute_baseaddress_required, parameterName);
}

if (!HttpUtilities.IsHttpUri(baseAddress))
{
throw new ArgumentException(HttpUtilities.InvalidUriMessage, parameterName);
}
}

private static bool IsNativeHandlerEnabled()
{
if (!AppContext.TryGetSwitch("System.Net.Http.UseNativeHttpHandler", out bool isEnabled))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,15 @@ public HttpMessageInvoker(HttpMessageHandler handler, bool disposeHandler)
}

[UnsupportedOSPlatformAttribute("browser")]
public virtual HttpResponseMessage Send(HttpRequestMessage request,
CancellationToken cancellationToken)
public virtual HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
{
if (request == null)
{
throw new ArgumentNullException(nameof(request));
}
CheckDisposed();

if (HttpTelemetry.Log.IsEnabled() && !request.WasSentByHttpClient() && request.RequestUri != null)
if (ShouldSendWithTelemetry(request))
{
HttpTelemetry.Log.RequestStart(request);

Expand All @@ -67,16 +66,15 @@ public HttpMessageInvoker(HttpMessageHandler handler, bool disposeHandler)
}
}

public virtual Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
CancellationToken cancellationToken)
public virtual Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
if (request == null)
{
throw new ArgumentNullException(nameof(request));
}
CheckDisposed();

if (HttpTelemetry.Log.IsEnabled() && !request.WasSentByHttpClient() && request.RequestUri != null)
if (ShouldSendWithTelemetry(request))
{
return SendAsyncWithTelemetry(_handler, request, cancellationToken);
}
Expand All @@ -103,6 +101,12 @@ static async Task<HttpResponseMessage> SendAsyncWithTelemetry(HttpMessageHandler
}
}

private static bool ShouldSendWithTelemetry(HttpRequestMessage request) =>
HttpTelemetry.Log.IsEnabled() &&
!request.WasSentByHttpClient() &&
request.RequestUri is Uri requestUri &&
requestUri.IsAbsoluteUri;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we disable telemetry if the Uri isn't absolute?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to RequestStart that follows this check will access Uri fields that would throw for relative Uris.

Adding this check here so that you get the meaningful exception message from SocketsHttpHandler instead.


internal static bool LogRequestFailed(bool telemetryStarted)
{
if (HttpTelemetry.Log.IsEnabled() && telemetryStarted)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Net.Http.Headers;
using System.Text;
using System.Threading;
using System.Collections.Generic;
using System.Diagnostics;

namespace System.Net.Http
{
public class HttpRequestMessage : IDisposable
{
internal static Version DefaultRequestVersion => HttpVersion.Version11;
internal static HttpVersionPolicy DefaultVersionPolicy => HttpVersionPolicy.RequestVersionOrLower;

private const int MessageNotYetSent = 0;
private const int MessageAlreadySent = 1;

Expand Down Expand Up @@ -101,29 +102,12 @@ public HttpMethod Method
get { return _requestUri; }
set
{
if ((value != null) && (value.IsAbsoluteUri) && (!HttpUtilities.IsHttpUri(value)))
{
throw new ArgumentException(HttpUtilities.InvalidUriMessage, nameof(value));
}
CheckDisposed();

// It's OK to set 'null'. HttpClient will add the 'BaseAddress'. If there is no 'BaseAddress'
// sending this message will throw.
_requestUri = value;
}
}

public HttpRequestHeaders Headers
{
get
{
if (_headers == null)
{
_headers = new HttpRequestHeaders();
}
return _headers;
}
}
public HttpRequestHeaders Headers => _headers ??= new HttpRequestHeaders();

internal bool HasHeaders => _headers != null;

Expand All @@ -139,22 +123,18 @@ public HttpRequestMessage()

public HttpRequestMessage(HttpMethod method, Uri? requestUri)
{
InitializeValues(method, requestUri);
// It's OK to have a 'null' request Uri. If HttpClient is used, the 'BaseAddress' will be added.
// If there is no 'BaseAddress', sending this request message will throw.
// Note that we also allow the string to be empty: null and empty are considered equivalent.
_method = method ?? throw new ArgumentNullException(nameof(method));
_requestUri = requestUri;
_version = DefaultRequestVersion;
_versionPolicy = DefaultVersionPolicy;
}

public HttpRequestMessage(HttpMethod method, string? requestUri)
: this(method, string.IsNullOrEmpty(requestUri) ? null : new Uri(requestUri, UriKind.RelativeOrAbsolute))
{
// It's OK to have a 'null' request Uri. If HttpClient is used, the 'BaseAddress' will be added.
// If there is no 'BaseAddress', sending this request message will throw.
// Note that we also allow the string to be empty: null and empty are considered equivalent.
if (string.IsNullOrEmpty(requestUri))
{
InitializeValues(method, null);
}
else
{
InitializeValues(method, new Uri(requestUri, UriKind.RelativeOrAbsolute));
}
}

public override string ToString()
Expand All @@ -179,25 +159,6 @@ public override string ToString()
return sb.ToString();
}

[MemberNotNull(nameof(_method))]
[MemberNotNull(nameof(_version))]
private void InitializeValues(HttpMethod method, Uri? requestUri)
{
if (method is null)
{
throw new ArgumentNullException(nameof(method));
}
if ((requestUri != null) && (requestUri.IsAbsoluteUri) && (!HttpUtilities.IsHttpUri(requestUri)))
{
throw new ArgumentException(HttpUtilities.InvalidUriMessage, nameof(requestUri));
}

_method = method;
_requestUri = requestUri;
_version = HttpUtilities.DefaultRequestVersion;
_versionPolicy = HttpUtilities.DefaultVersionPolicy;
}

internal bool MarkAsSent()
{
return Interlocked.Exchange(ref _sendStatus, MessageAlreadySent) == MessageNotYetSent;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ namespace System.Net.Http
{
public class HttpResponseMessage : IDisposable
{
private const HttpStatusCode defaultStatusCode = HttpStatusCode.OK;
private const HttpStatusCode DefaultStatusCode = HttpStatusCode.OK;
private static Version DefaultResponseVersion => HttpVersion.Version11;

private HttpStatusCode _statusCode;
private HttpResponseHeaders? _headers;
Expand Down Expand Up @@ -149,7 +150,7 @@ public bool IsSuccessStatusCode
}

public HttpResponseMessage()
: this(defaultStatusCode)
: this(DefaultStatusCode)
{
}

Expand All @@ -161,7 +162,7 @@ public HttpResponseMessage(HttpStatusCode statusCode)
}

_statusCode = statusCode;
_version = HttpUtilities.DefaultResponseVersion;
_version = DefaultResponseVersion;
}

public HttpResponseMessage EnsureSuccessStatusCode()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ private void RequestStart(string scheme, string host, int port, string pathAndQu
[NonEvent]
public void RequestStart(HttpRequestMessage request)
{
Debug.Assert(request.RequestUri != null);
Debug.Assert(request.RequestUri != null && request.RequestUri.IsAbsoluteUri);

RequestStart(
request.RequestUri.Scheme,
Expand Down

This file was deleted.

Loading