CORS preflight request with URL Path versioning results in `ArgumentNullException`

[richarddbarnett]

I have a .NET Framework 4.5 project using OWIN & ASP.NET Web API, with CORS support using Microsoft.AspNet.WebApi.Cors. Runtime is Mono 6.x.

When I add URL Path versioning, preflight requests fail with ArgumentNullException: Value cannot be null.\nParameter name: response, and stack

  at System.Web.Http.HttpResponseException..ctor (System.Net.Http.HttpResponseMessage response) [0x00018] in <4f8d88aa02d546f6963914471854cb7c>:0 
  at Microsoft.Web.Http.Dispatcher.HttpResponseExceptionFactory.CreateBadRequest (Microsoft.Web.Http.ApiVersion requestedVersion) [0x00007] in <603f310054da465a9f47899e3083913f>:0 
  at Microsoft.Web.Http.Dispatcher.HttpResponseExceptionFactory.CreateBadRequest (Microsoft.Web.Http.Dispatcher.ControllerSelectionResult conventionRouteResult, Microsoft.Web.Http.Dispatcher.ControllerSelectionResult directRouteResult) [0x00027] in <603f310054da465a9f47899e3083913f>:0 
  at Microsoft.Web.Http.Dispatcher.HttpResponseExceptionFactory.NewNotFoundOrBadRequestException (Microsoft.Web.Http.Dispatcher.ControllerSelectionResult conventionRouteResult, Microsoft.Web.Http.Dispatcher.ControllerSelectionResult directRouteResult) [0x00000] in <603f310054da465a9f47899e3083913f>:0 
  at Microsoft.Web.Http.Dispatcher.ApiVersionControllerSelector.SelectController (System.Net.Http.HttpRequestMessage request) [0x00143] in <603f310054da465a9f47899e3083913f>:0 
  at System.Web.Http.Cors.AttributeBasedPolicyProviderFactory.SelectAction (System.Net.Http.HttpRequestMessage request, System.Web.Http.Routing.IHttpRouteData routeData, System.Web.Http.HttpConfiguration config) [0x00018] in <a9bb4129cf4d431da372d3b3dbf7a190>:0 
  at System.Web.Http.Cors.AttributeBasedPolicyProviderFactory.GetCorsPolicyProvider (System.Net.Http.HttpRequestMessage request) [0x000f9] in <a9bb4129cf4d431da372d3b3dbf7a190>:0 
  at System.Web.Http.Cors.CorsMessageHandler.GetCorsPolicyAsync (System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) [0x0001e] in <a9bb4129cf4d431da372d3b3dbf7a190>:0 
  at System.Web.Http.Cors.CorsMessageHandler.HandleCorsPreflightRequestAsync (System.Net.Http.HttpRequestMessage request, System.Web.Cors.CorsRequestContext corsRequestContext, System.Threading.CancellationToken cancellationToken) [0x0010e] in <a9bb4129cf4d431da372d3b3dbf7a190>:0 
  at System.Web.Http.Cors.CorsMessageHandler.SendAsync (System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) [0x000a2] in <a9bb4129cf4d431da372d3b3dbf7a190>:0

[richarddbarnett]

Minimal repro solution: CorsVersioningExample.zip

Debug build

This has CORS support only.

$ curl -v -X OPTIONS -H 'Access-Control-Request-Method: POST' -H 'Access-Control-Request-Headers: content-type,authorization' -H 'Origin: https://example.com' http://localhost:9999/test/

> OPTIONS /test/ HTTP/1.1
> Host: localhost:9999
> User-Agent: curl/7.64.1
> Accept: */*
> Access-Control-Request-Method: POST
> Access-Control-Request-Headers: content-type,authorization
> Origin: https://example.com
>
< HTTP/1.1 200 OK
< Access-Control-Allow-Origin: https://example.com
< Access-Control-Allow-Credentials: true
< Access-Control-Allow-Headers: content-type,authorization
< Server: Mono-HTTPAPI/1.0
< Date: Thu, 16 Apr 2020 06:36:19 GMT
< Content-Length: 0
< Keep-Alive: timeout=15,max=100
<

Release build

This has CORS support & URL Path versioning.

$ curl -v -X OPTIONS -H 'Access-Control-Request-Method: POST' -H 'Access-Control-Request-Headers: content-type,authorization' -H 'Origin: https://example.com' http://localhost:9999/v1/test/

> OPTIONS /v1/test/ HTTP/1.1
> Host: localhost:9999
> User-Agent: curl/7.64.1
> Accept: */*
> Access-Control-Request-Method: POST
> Access-Control-Request-Headers: content-type,authorization
> Origin: https://example.com
>
< HTTP/1.1 500 Internal Server Error
< Content-Type: application/json; charset=utf-8
< Server: Mono-HTTPAPI/1.0
< Date: Thu, 16 Apr 2020 06:38:19 GMT
< Content-Length: 2415
< Connection: close
<
{"Message":"An error has occurred.", ... }

[richarddbarnett]

Possibly related:

• Looks like CORS is not supported #393
• UrlSegementApiVersionReader doesn't work with Microsoft.AspNet.WebApi.Cors #402

[commonsensesoftware]

Thanks for the repro. I was able to recreate your conditions.

It took some digging and debugging, but I was able to track down where the problem occurs. It actually comes from the CORS library itself. For some reason it's doing something that it definitely should not:

AttributeBasedPolicyProviderFactory (Line 49)

The request pipeline uses the Flyweight pattern to pass the request around as a monad. You can see from this referenced code, however, that the CORS library is creating an entirely new request and forwarding it on. The comments claim that this is done due to things in the request property bag that shouldn't be passed along (but I don't know why). IMO, if this is really required, then the properties should be removed, the original request forwarded, and then re-add the properties after the return. This would maintain the monad.

This behavior breaks API Versioning, specifically, in the case of versioning by URL segment (which I don't recommend). This is because the only time API Versioning knows which route parameter value contains the API version is when the ApiVersionRouteConstraint is evaluated. When the route constraint matches, it gets or adds the ApiVersioningProperties from the request's property bag. Since there are two different request instances, there are two different property bags and two different sets of ApiVersioningProperties, one of whom does not have the API version because it wasn't the one used by the route constraint. Ultimately, this is a sequencing problem in the CORS library design.

NOTE: Other methods for API versioning are not affected by this because their readers can easily extract the scalar API version value from any request via a query string, header, or media type. Versioning by URL requires using a route constraint to avoid magic string parsing.

Honestly, I feel like this is a bug in the CORS library as I don't see any real compelling reason why a new request has to be created. This is certainly not something that extensions would expect to encounter. I only discovered this was the case by first noticing the property was missing later in the pipeline and that the hash code of the request objects didn't match at different points in the pipeline.

Ideally, it would be great if API Versioning can handle this situation, despite it being something that should never happen. I'm not sure what that solution is yet and it may take a little bit.

In the meantime, there are at least two possible workarounds:

• Use a different method of versioning
• Fork the AttributeBasedPolicyProviderFactory class, prevent it from creating a new request, and replace the default implementation in your configuration
  • I haven't tested this, but I expect it to work because simply setting the API version to the correct value while debugging made everything happy

Probably not the answers your hoping for, but that will hopefully unblock you while something more out-of-the-box is evaluated.

BTW: I noticed you had a lot of conflicting and unnecessary transitive dependencies in your repro. To use Autofac with the latest OWIN and Web API stacks, you need to target .NET 4.6.1. I'm not sure if there's a reason why you can't, but there are warnings of conflicting versions. You also don't need to list all of your transitive dependencies. The following subset resolves the same references:

<PropertyGroup>
 <TargetFramework>net461</TargetFramework>
</PropertyGroup>
<ItemGroup>
 <PackageReference Include="Autofac.WebApi2.Owin" Version="5.0.0" />
 <PackageReference Include="Microsoft.AspNet.WebApi.Cors" Version="5.2.7" />
 <PackageReference Include="Microsoft.AspNet.WebApi.OwinSelfHost" Version="5.2.7" />
 <PackageReference Include="Microsoft.AspNet.WebApi.Versioning.ApiExplorer" Version="4.0.0" />
 <PackageReference Include="Microsoft.Owin.Security.Jwt" Version="4.1.0" />
 <PackageReference Include="Newtonsoft.Json" Version="12.0.3" />
 <PackageReference Include="Owin.Logging.Common" Version="1.0.5506.19675" />
</ItemGroup>

[richarddbarnett]

Thanks @commonsensesoftware for this amazingly quick & comprehensive response. I'll need a bit of time to digest this & work out what to do.

[commonsensesoftware]

A quick follow up. I looked at this just a bit more. The new request is definitely the culprit, but I figured out exactly why. AttributeBasedPolicyProviderFactory.cs (Line 72) does not account for GetRouteData possibly having side effects. That method ultimately triggers the evaluation of route constraints, but on the original request. As a result, this process never happens on the new request object and, therefore, the new request doesn't have the API version information.

I need to test and flush out whether it's the right thing, but it looks like I can tell if this has happened in a generic way by checking for the presence of the MS_RoutingContext key in the property bag. If it's not there, I can force route constraint evaluation by calling GetRouteData. It's not yet clear what other side effects that might have.

If I'm right, this means the ApiVersioningProperties extension method can change to something like this:

public static ApiVersionRequestProperties ApiVersionProperties(this HttpRequestMessage request)
{
 if ( request == null )
 {
  throw new ArgumentNullException( nameof( request ) );
 }

 if ( !request.Properties.TryGetValue(
        ApiVersionPropertiesKey,
        out ApiVersionRequestProperties properties ) )
 {
  // assume route constraints haven't been evaluated if this key isn't present
  var forceRouteConstraintEvaluation =
    !request.Properties.ContainsKey( RoutingContextKey );

  // add to the property bag FIRST because the ApiVersionRouteConstraint will re-enter,
  // resulting in StackOverflowException
  request.Properties[ApiVersionPropertiesKey] = properties =
    new ApiVersionRequestProperties( request );

  if ( forceRouteConstraintEvaluation )
  {
   // now force evaluation; reentrancy is safe at this point
   request.GetConfiguration().Routes.GetRouteData( request );
  }
 }

 return properties;
}

Testing against your repro seemed to behave as expected. I'll need to put this against the full suite, but hopefully I can get this patched in the next day or two. In the meantime, the other workarounds are still applicable.

[commonsensesoftware]

Checking back in. Did the workaround work for you? I've incorporated my proposed generic solution, but it won't go out until the next release. If the workaround worked for you, I'll close this issue so I know it's complete and add the details to the release notes. Thanks.

[richarddbarnett]

Hi @commonsensesoftware. We ended up going with a different method of versioning. When you've released the fix we'll reconsider whether to revert to the original approach. Thanks for checking in.

[commonsensesoftware]

Thanks for confirming. I'll mark this is an answered, but leave it open. It will auto-close once the changes are merged. That will be time to consider/reconsider incorporating things. Glad you got something working.