Skip to content

Fix for #147 #185

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 6 commits into from
Aug 21, 2018
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
32 changes: 27 additions & 5 deletions src/System.Web.Http.Cors/CorsHttpConfigurationExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,38 @@ public static class CorsHttpConfigurationExtensions
/// <param name="httpConfiguration">The <see cref="HttpConfiguration"/>.</param>
public static void EnableCors(this HttpConfiguration httpConfiguration)
{
EnableCors(httpConfiguration, null);
EnableCors(httpConfiguration, null, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI we've got a meeting scheduled next Tuesday to decide whether default should be true i.e. whether we should break compatibility to ensure IExceptionHandler sees what it should. Will let you know…

Copy link
Author

Choose a reason for hiding this comment

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

Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Decision was to avoid breaking changes and stick with the approach you've implemented here.

}

/// <summary>
/// Enables the support for CORS.
/// </summary>
/// <param name="httpConfiguration">The <see cref="HttpConfiguration"/>.</param>
/// <param name="rethrowExceptions">Indicates whether upstream exceptions should be rethrown</param>
public static void EnableCors(this HttpConfiguration httpConfiguration, bool rethrowExceptions)
{
EnableCors(httpConfiguration, null, rethrowExceptions);
}

/// <summary>
/// Enables the support for CORS.
/// </summary>
/// <param name="httpConfiguration">The <see cref="HttpConfiguration"/>.</param>
/// <param name="defaultPolicyProvider">The default <see cref="ICorsPolicyProvider"/>.</param>
/// <exception cref="System.ArgumentNullException">httpConfiguration</exception>
public static void EnableCors(this HttpConfiguration httpConfiguration, ICorsPolicyProvider defaultPolicyProvider)
{
EnableCors(httpConfiguration, defaultPolicyProvider, false);
}

/// <summary>
/// Enables the support for CORS.
/// </summary>
/// <param name="httpConfiguration">The <see cref="HttpConfiguration"/>.</param>
/// <param name="defaultPolicyProvider">The default <see cref="ICorsPolicyProvider"/>.</param>
/// <param name="rethrowExceptions">Indicates whether upstream exceptions should be rethrown</param>
/// <exception cref="System.ArgumentNullException">httpConfiguration</exception>
public static void EnableCors(this HttpConfiguration httpConfiguration, ICorsPolicyProvider defaultPolicyProvider,
bool rethrowExceptions)
{
if (httpConfiguration == null)
{
Expand All @@ -49,11 +71,11 @@ public static void EnableCors(this HttpConfiguration httpConfiguration, ICorsPol
httpConfiguration.SetCorsPolicyProviderFactory(policyProviderFactory);
}

AddCorsMessageHandler(httpConfiguration);
AddCorsMessageHandler(httpConfiguration, rethrowExceptions);
}

[SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Caller owns the disposable object")]
private static void AddCorsMessageHandler(this HttpConfiguration httpConfiguration)
private static void AddCorsMessageHandler(this HttpConfiguration httpConfiguration, bool rethrowExceptions)
{
object corsEnabled;
if (!httpConfiguration.Properties.TryGetValue(CorsEnabledKey, out corsEnabled))
Expand All @@ -64,7 +86,7 @@ private static void AddCorsMessageHandler(this HttpConfiguration httpConfigurati
if (!config.Properties.TryGetValue(CorsEnabledKey, out corsEnabled))
{
// Execute this in the Initializer to ensure that the CorsMessageHandler is added last.
config.MessageHandlers.Add(new CorsMessageHandler(config));
config.MessageHandlers.Add(new CorsMessageHandler(config, rethrowExceptions));

ITraceWriter traceWriter = config.Services.GetTraceWriter();

Expand Down
19 changes: 18 additions & 1 deletion src/System.Web.Http.Cors/CorsMessageHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,32 @@ namespace System.Web.Http.Cors
public class CorsMessageHandler : DelegatingHandler
{
private HttpConfiguration _httpConfiguration;
private bool _rethrowExceptions;

/// <summary>
/// Initializes a new instance of the <see cref="CorsMessageHandler"/> class.
/// </summary>
/// <param name="httpConfiguration">The <see cref="HttpConfiguration"/>.</param>
/// <exception cref="System.ArgumentNullException">httpConfiguration</exception>
public CorsMessageHandler(HttpConfiguration httpConfiguration)
public CorsMessageHandler(HttpConfiguration httpConfiguration) : this(httpConfiguration, false)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="CorsMessageHandler"/> class.
/// </summary>
/// <param name="httpConfiguration">The <see cref="HttpConfiguration"/>.</param>
/// <param name="rethrowExceptions">Indicates whether upstream exceptions should be rethrown</param>
/// <exception cref="System.ArgumentNullException">httpConfiguration</exception>
public CorsMessageHandler(HttpConfiguration httpConfiguration, bool rethrowExceptions)
{
if (httpConfiguration == null)
{
throw new ArgumentNullException("httpConfiguration");
}

_httpConfiguration = httpConfiguration;
_rethrowExceptions = rethrowExceptions;
}

/// <summary>
Expand Down Expand Up @@ -60,6 +72,11 @@ protected async override Task<HttpResponseMessage> SendAsync(HttpRequestMessage
}
catch (Exception exception)
{
if (_rethrowExceptions)
{
throw;
}

return HandleException(request, exception);
}
}
Expand Down
11 changes: 11 additions & 0 deletions test/System.Web.Http.Cors.Test/Controllers/ThrowingController.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
namespace System.Web.Http.Cors
{
[EnableCors("*", "*", "*")]
public class ThrowingController : ApiController
{
public string Get()
{
throw new Exception();
}
}
}
43 changes: 43 additions & 0 deletions test/System.Web.Http.Cors.Test/CorsMessageHandlerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Threading;
using System.Threading.Tasks;
using System.Web.Cors;
using System.Web.Http.ExceptionHandling;
using System.Web.Http.Hosting;
using Microsoft.TestCommon;

Expand Down Expand Up @@ -180,6 +181,40 @@ public async Task SendAsync_HandlesExceptions_ThrownDuringPreflight()
Assert.Equal(HttpStatusCode.MethodNotAllowed, response.StatusCode);
}

[Fact]
public async Task SendAsync_Preflight_RethrowsExceptions_WhenRethrowFlagIsTrue()
{
HttpConfiguration config = new HttpConfiguration();
config.Routes.MapHttpRoute("default", "{controller}");
HttpServer server = new HttpServer(config);
CorsMessageHandler corsHandler = new CorsMessageHandler(config, true);
corsHandler.InnerHandler = server;
HttpMessageInvoker invoker = new HttpMessageInvoker(corsHandler);
HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Options, "http://localhost/sample");
request.SetConfiguration(config);
request.Headers.Add(CorsConstants.Origin, "http://localhost");
request.Headers.Add(CorsConstants.AccessControlRequestMethod, "RandomMethod");

await Assert.ThrowsAsync<HttpResponseException>(() => invoker.SendAsync(request, CancellationToken.None));
}

[Fact]
public async Task SendAsync_RethrowsExceptions_WhenRethrowFlagIsTrue()
{
HttpConfiguration config = new HttpConfiguration();
config.Routes.MapHttpRoute("default", "{controller}");
config.Services.Replace(typeof(IExceptionHandler), new PassthroughExceptionHandler());
Copy link
Author

Choose a reason for hiding this comment

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

This is needed to reproduce the exact scenario of original issue, otherwise default implementation of IExceptionHandler maps Exceptions to just 500 status code

HttpServer server = new HttpServer(config);
CorsMessageHandler corsHandler = new CorsMessageHandler(config, true);
corsHandler.InnerHandler = server;
HttpMessageInvoker invoker = new HttpMessageInvoker(corsHandler);
HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/throwing");
request.SetConfiguration(config);
request.Headers.Add(CorsConstants.Origin, "http://localhost");

await Assert.ThrowsAsync<Exception>(() => invoker.SendAsync(request, CancellationToken.None));
}

[Fact]
public Task HandleCorsRequestAsync_NullConfig_Throws()
{
Expand Down Expand Up @@ -238,5 +273,13 @@ public Task HandleCorsPreflightRequestAsync_NullContext_Throws()
() => corsHandler.HandleCorsPreflightRequestAsync(new HttpRequestMessage(), null, CancellationToken.None),
"corsRequestContext");
}

private class PassthroughExceptionHandler : IExceptionHandler
{
public Task HandleAsync(ExceptionHandlerContext context, CancellationToken cancellationToken)
{
throw context.Exception;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
<Compile Include="Controllers\PerControllerConfigController.cs" />
<Compile Include="Controllers\SampleController.cs" />
<Compile Include="Controllers\DefaultController.cs" />
<Compile Include="Controllers\ThrowingController.cs" />
<Compile Include="CorsMessageHandlerTest.cs" />
<Compile Include="DisableCorsAttributeTest.cs" />
<Compile Include="EnableCorsAttributeTest.cs" />
Expand Down