dotnet / dotnet/aspnetcore

CorsPolicyBuilder does not correctly combine policy with DefaultIsOriginAllowed

Open
#43,319 5 comments 1 reaction 1 assignee Claimed by @BrennanConroy View on GitHub
area-minimal area-mvc bug
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 10h
Merged PRs (30d)
281

Description

### Is there an existing issue for this?

- [X] I have searched the existing issues

### Describe the bug

A CorsPolicy has per default a DefaultIsOriginAllowed func which is responsible for evaluating if an origin is allowed. Because this func [encapsulates a member access to `Origins`](https://github.com/dotnet/aspnetcore/blob/7b5da426f9bd42fa515a6b67e0bd1504eacbc6c7/src/Middleware/CORS/src/Infrastructure/CorsPolicy.cs#L177), cors policy builder will not correctly combine two cors policies.
Following sample code demonstrates the problem. It is the Program.cs of a new .net 6 Web API Project.

```
using Microsoft.AspNetCore.Cors.Infrastructure;

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.

builder.Services.AddControllers();
// Learn more about configuring Swagger/OpenAPI at https://aka.ms/aspnetcore/swashbuckle
builder.Services.AddEndpointsApiExplorer();
builder.Services.AddSwaggerGen();

// does not work
var policy1 = new CorsPolicyBuilder(new CorsPolicy())
.AllowAnyHeader()
.AllowAnyMethod()
.WithOrigins("https://www.microsoft.com")
.Build();
//builder.Services.AddCors(options => {
// options.AddDefaultPolicy(policy1);
//});

// works
var policy2 = new CorsPolicyBuilder("https://www.microsoft.com")
.AllowAnyHeader()
.AllowAnyMethod()
.Build();

// will not work if cors request is made for origin https://www.visualstudio.com
//var policy3 = new CorsPolicyBuilder(policy2)
// .WithOrigins("https://www.visualstudio.com")
// .Build();

builder.Services.AddCors(options => {
options.AddDefaultPolicy(policy2);
});

Console.WriteLine(policy1.IsOriginAllowed("https://www.microsoft.com")); // => false
Console.WriteLine(policy2.IsOriginAllowed("https://www.microsoft.com")); // => true

var app = builder.Build();

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment()) {
app.UseSwagger();
app.UseSwaggerUI();
}

app.UseHttpsRedirection();
app.UseCors();
app.UseAuthorization();
app.MapControllers();
app.Run();
```

### Expected Behavior

Combining two policies using [`CorsPolicyBuilder`](https://github.com/dotnet/aspnetcore/blob/7b5da426f9bd42fa515a6b67e0bd1504eacbc6c7/src/Middleware/CORS/src/Infrastructure/CorsPolicyBuilder.cs#L29) where each policy has allowed origins should combine correctly.
One solution would be, that if the first policy has `IsDefaultIsOriginAllowed` set to true then the IsOriginAllowed func of the second policy should be kept in the [Combine](https://github.com/dotnet/aspnetcore/blob/7b5da426f9bd42fa515a6b67e0bd1504eacbc6c7/src/Middleware/CORS/src/Infrastructure/CorsPolicyBuilder.cs#L260) function. A prettier solution would probably be to consider changes around the IsDefaultIsOriginAllowed / IsOriginAllowed func approach, but might be more difficult to implement without breaking changes.

### Steps To Reproduce

Run following unit test.
```
[TestClass]
public class UnitTest1 {
[TestMethod]
public void CorsBuilder_should_combine_correctly() {
var policy1 = new CorsPolicyBuilder(new CorsPolicy())
.AllowAnyHeader()
.AllowAnyMethod()
.WithOrigins("https://www.microsoft.com")
.Build();

var policy2 = new CorsPolicyBuilder("https://www.microsoft.com")
.AllowAnyHeader()
.AllowAnyMethod()
.Build();

var policy3 = new CorsPolicyBuilder(policy2)
.WithOrigins("https://www.visualstudio.com")
.Build();

Assert.IsTrue(policy1.IsOriginAllowed("https://www.microsoft.com")); // fails
Assert.IsTrue(policy2.IsOriginAllowed("https://www.microsoft.com")); // works
Assert.IsTrue(policy3.IsOriginAllowed("https://www.microsoft.com")); // works
Assert.IsTrue(policy3.IsOriginAllowed("https://www.visualstudio.com")); // fails
}
}

```

### Exceptions (if any)

_No response_

### .NET Version

6.0.203

### Anything else?

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.