Skip to content
This repository has been archived by the owner on Nov 22, 2018. It is now read-only.

Added override for VaryByOrigin when using asterisk. #175

Closed
wants to merge 1 commit into from
Closed

Added override for VaryByOrigin when using asterisk. #175

wants to merge 1 commit into from

Conversation

LuckyWraptor
Copy link
Contributor

@LuckyWraptor LuckyWraptor commented Jul 20, 2018

Originating issue dotnet/aspnetcore/issues/3299.

This fix allows clients to use the following 2 approaches allow the enabling of the Vary: 'Origin' header when using an asterisk in the WithOrigins() function for the CorsPolicyBuilder

services.AddCors(options =>
{
    options.AddPolicy("Policy1",
        builder => builder
            .WithOrigins("*")
            .SetIsOriginAllowed(IsOriginAllowed(env))
    );
});

And even single origin domains using a wildcard:

services.AddCors(options =>
{
    options.AddPolicy("Policy1",
        builder => builder
            .WithOrigins("*.domain.com")
    );
});

Without having to do a hacky workaround like described in the issue above.

This should be an easy fix with low to none conflicts.

@LuckyWraptor
Copy link
Contributor Author

LuckyWraptor commented Jul 20, 2018

Care to review? Thank you

@LuckyWraptor LuckyWraptor changed the title Added override for VaryByOrigin Added override for VaryByOrigin when using asterisk. Jul 23, 2018
@LuckyWraptor
Copy link
Contributor Author

@mkArtakMSFT Would you mind getting someone on this easy fix?

@javiercn
Copy link
Member

@FlyingWraptor I'm looking at it

@@ -272,7 +272,7 @@ private void AddOriginToResult(string origin, CorsPolicy policy, CorsResult resu
{
result.AllowedOrigin = origin;

if(policy.Origins.Count > 1)
if(policy.Origins.Count > 1 || policy.Origins.Any(o => o.Contains("*")))
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this isn't correct. The behavior for IsOriginAllowed is allowed to be user defined which means the user code could have just as likely said "IsOriginAllowed = origin => origin.EndsWith("blah.com");which would make this check incorrect. We should add a property toCorsPolicy` that allows configuring this and use the line above as the default implementation.

i.e. CorsPolicy.IsVaryByOriginRequired => origin => Origins.Count > 1 || origin.Contains("*")

@javiercn
Copy link
Member

@pranavkm maybe we can redesign this in 3.0 and clean it up?

@pranavkm
Copy link
Contributor

maybe we can redesign this in 3.0 and clean it up

Yes please 👍

@mkArtakMSFT
Copy link
Member

@javiercn, please file an issue to not forget about this in 3.0.

@javiercn
Copy link
Member

dotnet/aspnetcore#3452

@LuckyWraptor
Copy link
Contributor Author

Due to there still being imperfections I will close the merge request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants