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

Handle redirects in the pipeline #22876

Merged
merged 5 commits into from
Jul 26, 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
11 changes: 9 additions & 2 deletions sdk/core/Azure.Core.TestFramework/src/MockResponse.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,26 @@ public void SetContent(byte[] content)
ContentStream = new MemoryStream(content, 0, content.Length, false, true);
}

public void SetContent(string content)
public MockResponse SetContent(string content)
{
SetContent(Encoding.UTF8.GetBytes(content));
return this;
}

public void AddHeader(HttpHeader header)
public MockResponse AddHeader(string name, string value)
{
return AddHeader(new HttpHeader(name, value));
}

public MockResponse AddHeader(HttpHeader header)
{
if (!_headers.TryGetValue(header.Name, out List<string> values))
{
_headers[header.Name] = values = new List<string>();
}

values.Add(header.Value);
return this;
}

#if HAS_INTERNALS_VISIBLE_CORE
Expand Down
21 changes: 21 additions & 0 deletions sdk/core/Azure.Core/src/Diagnostics/AzureCoreEventSource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ internal sealed class AzureCoreEventSource : AzureEventSource
private const int RequestEvent = 1;
private const int RequestContentEvent = 2;
private const int RequestContentTextEvent = 17;
private const int RequestRedirectEvent = 20;
private const int RequestRedirectBlockedEvent = 21;
private const int RequestRedirectCountExceededEvent = 22;
private const int ResponseEvent = 5;
private const int ResponseContentEvent = 6;
private const int ResponseDelayEvent = 7;
Expand Down Expand Up @@ -133,5 +136,23 @@ public void ExceptionResponse(string requestId, string exception)
{
WriteEvent(ExceptionResponseEvent, requestId, exception);
}

[Event(RequestRedirectEvent, Level = EventLevel.Verbose, Message = "Request [{0}] Redirecting from {1} to {2} in response to status code {3}")]
public void RequestRedirect(string requestId, string from, string to, int status)
{
WriteEvent(RequestRedirectEvent, requestId, from, to, status);
}

[Event(RequestRedirectBlockedEvent, Level = EventLevel.Warning, Message = "Request [{0}] Insecure HTTPS to HTTP redirect from {1} to {2} was blocked.")]
public void RequestRedirectBlocked(string requestId, string from, string to)
{
WriteEvent(RequestRedirectBlockedEvent, requestId, from, to);
}

[Event(RequestRedirectCountExceededEvent, Level = EventLevel.Warning, Message = "Request [{0}] Exceeded max number of redirects. Redirect from {1} to {2} blocked.")]
public void RequestRedirectCountExceeded(string requestId, string from, string to)
{
WriteEvent(RequestRedirectCountExceededEvent, requestId, from, to);
}
}
}
1 change: 1 addition & 0 deletions sdk/core/Azure.Core/src/HttpMessage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Net.Http;
using System.Threading;
using Azure.Core.Pipeline;

Expand Down
12 changes: 9 additions & 3 deletions sdk/core/Azure.Core/src/Pipeline/HttpClientTransport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ private static HttpClient CreateDefaultClient()
return new HttpClient(httpMessageHandler)
{
// Timeouts are handled by the pipeline
Timeout = Timeout.InfiniteTimeSpan
Timeout = Timeout.InfiniteTimeSpan,
};
}

Expand All @@ -153,9 +153,15 @@ private static HttpMessageHandler CreateDefaultHandler()
}

#if NETCOREAPP
return new SocketsHttpHandler();
return new SocketsHttpHandler()
{
AllowAutoRedirect = false
};
#else
return new HttpClientHandler();
return new HttpClientHandler()
{
AllowAutoRedirect = false
};
Comment on lines -156 to +164
Copy link
Member

Choose a reason for hiding this comment

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

nit:

#if NETCOREAPP
return new SocketsHttpHandler()
#else
return new HttpClientHandler();
#endif
{
    AllowAutoRedirect = false
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's evil :)

#endif
}

Expand Down
2 changes: 2 additions & 0 deletions sdk/core/Azure.Core/src/Pipeline/HttpPipelineBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ public static HttpPipeline Build(ClientOptions options, HttpPipelinePolicy[] per
RetryOptions retryOptions = options.Retry;
policies.Add(new RetryPolicy(retryOptions.Mode, retryOptions.Delay, retryOptions.MaxDelay, retryOptions.MaxRetries));

policies.Add(RedirectPolicy.Shared);

policies.AddRange(perRetryPolicies);

policies.AddRange(options.PerRetryPolicies);
Expand Down
3 changes: 3 additions & 0 deletions sdk/core/Azure.Core/src/Pipeline/HttpWebRequestTransport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ private HttpWebRequest CreateRequest(Request messageRequest)
request.Timeout = Timeout.Infinite;
request.ReadWriteTimeout = Timeout.Infinite;

// Redirect is handled by the pipeline
request.AllowAutoRedirect = false;

// Don't disable the default proxy when there is no environment proxy configured
if (_environmentProxy != null)
{
Expand Down
172 changes: 172 additions & 0 deletions sdk/core/Azure.Core/src/Pipeline/Internal/RedirectPolicy.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Threading.Tasks;
using Azure.Core.Diagnostics;

namespace Azure.Core.Pipeline
{
internal sealed class RedirectPolicy : HttpPipelinePolicy
{
private readonly int _maxAutomaticRedirections;

public static RedirectPolicy Shared { get; } = new RedirectPolicy();

private RedirectPolicy()
{
_maxAutomaticRedirections = 50;
}

internal async ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline, bool async)
{
if (async)
{
await ProcessNextAsync(message, pipeline).ConfigureAwait(false);
}
else
{
ProcessNext(message, pipeline);
}

uint redirectCount = 0;
Uri? redirectUri;

Request request = message.Request;
Response response = message.Response;

while ((redirectUri = GetUriForRedirect(request, message.Response)) != null)
{
redirectCount++;

if (redirectCount > _maxAutomaticRedirections)
{
// If we exceed the maximum number of redirects
// then just return the 3xx response.
if (AzureCoreEventSource.Singleton.IsEnabled())
{
AzureCoreEventSource.Singleton.RequestRedirectCountExceeded(request.ClientRequestId, request.Uri.ToString(), redirectUri.ToString());
}

break;
}

response.Dispose();

// Clear the authorization header.
request.Headers.Remove(HttpHeader.Names.Authorization);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dotnet/runtime#26475 (comment)

TLDR to prevent accidental secret leaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.


if (AzureCoreEventSource.Singleton.IsEnabled())
Copy link
Member

Choose a reason for hiding this comment

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

Just curious why we don't do these checks in the eventSource instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid converting Uris to strings if the ES is disabled.

{
AzureCoreEventSource.Singleton.RequestRedirect(request.ClientRequestId, request.Uri.ToString(), redirectUri.ToString(), response.Status);
}

// Set up for the redirect
request.Uri.Reset(redirectUri);
if (RequestRequiresForceGet(response.Status, request.Method))
{
request.Method = RequestMethod.Get;
request.Content = null;
}

// Issue the redirected request.
if (async)
{
await ProcessNextAsync(message, pipeline).ConfigureAwait(false);
}
else
{
ProcessNext(message, pipeline);
}

response = message.Response;
}
}

private static Uri? GetUriForRedirect(Request request, Response response)
{
switch (response.Status)
{
case 301:
case 302:
case 303:
case 307:
case 300:
case 308:
break;

default:
return null;
}

if (!response.Headers.TryGetValue("Location", out string? locationString))
{
return null;
}

Uri location = new Uri(locationString);
Uri requestUri = request.Uri.ToUri();
// Ensure the redirect location is an absolute URI.
if (!location.IsAbsoluteUri)
{
location = new Uri(requestUri, location);
}

// Per https://tools.ietf.org/html/rfc7231#section-7.1.2, a redirect location without a
// fragment should inherit the fragment from the original URI.
string requestFragment = requestUri.Fragment;
if (!string.IsNullOrEmpty(requestFragment))
{
string redirectFragment = location.Fragment;
if (string.IsNullOrEmpty(redirectFragment))
{
location = new UriBuilder(location) { Fragment = requestFragment }.Uri;
}
}

// Disallow automatic redirection from secure to non-secure schemes
if (IsSupportedSecureScheme(requestUri.Scheme) && !IsSupportedSecureScheme(location.Scheme))
{
if (AzureCoreEventSource.Singleton.IsEnabled())
{
AzureCoreEventSource.Singleton.RequestRedirectBlocked(request.ClientRequestId, requestUri.ToString(), location.ToString());
}

return null;
}

return location;
}

private static bool RequestRequiresForceGet(int statusCode, RequestMethod requestMethod)
{
switch (statusCode)
{
case 301:
case 302:
case 300:
return requestMethod == RequestMethod.Post;
case 303:
return requestMethod != RequestMethod.Get && requestMethod != RequestMethod.Head;
default:
return false;
}
}

internal static bool IsSupportedSecureScheme(string scheme) =>
string.Equals(scheme, "https", StringComparison.OrdinalIgnoreCase) || IsSecureWebSocketScheme(scheme);

internal static bool IsSecureWebSocketScheme(string scheme) =>
string.Equals(scheme, "wss", StringComparison.OrdinalIgnoreCase);

public override ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
{
return ProcessAsync(message, pipeline, true);
}

public override void Process(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
{
ProcessAsync(message, pipeline, false).EnsureCompleted();
}
}
}
1 change: 1 addition & 0 deletions sdk/core/Azure.Core/src/Request.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Net.Http;
using Azure.Core.Pipeline;

namespace Azure.Core
Expand Down
Loading