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

CORS redesign in 3.0 #3452

Closed
javiercn opened this issue Aug 23, 2018 · 12 comments
Closed

CORS redesign in 3.0 #3452

javiercn opened this issue Aug 23, 2018 · 12 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@javiercn
Copy link
Member

Our CORS implementation leaves some room for improvement. We keep getting issues from customers about it.

We should take 3.0 as an opportunity to revisit the scenarios customers need to support and to update our implementation to be cleaner and more straightforward as we keep getting issues from customers that are hard to fix or make our CORS implementation too complicated and confusing

@LuckyWraptor
Copy link
Contributor

LuckyWraptor commented Aug 23, 2018

The Fetch spec states that clients should only execute required CORS calls.
Frameworks like Angular always execute CORS calls for any cross-origin domains regardless of the request type, meaning some requests won't be send to the application if it was ignored by the current in-place cors filter.

Leaving out the filtering of cors calls will reduce load-times slightly. It also allows non-compliant clients to function properly.

In my opinion having no filtering and some non-compliant work is better than having the filtering, being compliant but forcing clients to resort to other methods to access the application.

@pranavkm
Copy link
Contributor

@FlyingWraptor what do you mean by filtering?

@LuckyWraptor
Copy link
Contributor

LuckyWraptor commented Aug 24, 2018

@pranavkm Filtering, enforcing...
The current CORS implementation validates whether or not the call should be made regarding cors (Effectively doing the work the client is supposed to do again). This is perfect handling, but there have been a couple of issues around it when the clients do not fully conform with the specs and netcore doesn't reply resulting in the http request not being made by the client. I've personally experienced this with Angular. Yes it's not netcores fault, but since cors is meant for clients to decide whether or not the request should be made I personally think having the extra checks/filtering/enforcing is unnecessary.

@pranavkm
Copy link
Contributor

Ahh, yup. I entirely agree. I spent a some time a couple weekends ago to see what it would look like to clean up the filtering - aspnet/CORS@14edbf3, just haven't gotten to verify if it's spec compliant.

@javiercn
Copy link
Member Author

@pranavkm Let's queue all these things for 3.0 and revamp the implementation.

@brockallen
Copy link

brockallen commented Aug 24, 2018

Frameworks like Angular always execute CORS calls for any cross-origin domains regardless of the request type

Are you certain frameworks have this sort of control over the inherent browser implementation of things like CORS and same-origin policy? IOW, frameworks should be subject to the browser behavior, not that frameworks instructor browsers how to behave for such features.

@LuckyWraptor
Copy link
Contributor

@brockallen I am not, but it does this for both my simple Cross-Origin GET and POST requests, (Which is what alarmed me about this issue). After some testing (I do not fully remember correctly) I believe it required valid cors responses for any requests going to a different dns name.

@mkArtakMSFT mkArtakMSFT added this to the 3.0.0 milestone Sep 27, 2018
@mkArtakMSFT mkArtakMSFT added 1 - Ready enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Sep 27, 2018
@wdolek
Copy link

wdolek commented Oct 5, 2018

@javiercn, @mkArtakMSFT, is there any proposal of improvement(s)?

I'm currently facing inability to extend CORS as is. There's one corner-case: imagine you have n domains like: contoso.com, contoso.ca, contoso.co.uk, contoso.fr, contoso.de, contoso.cz, contoso. ... you name it. What I really don't want to do is to specify tens of domains (with subdomain wildcard).

I was thinking of:

  • extract interface ICorsPolicy, keep its default (current) implementation CorsPolicy, builders etc.
  • use HashSet<T> instead of List<T> for list of origins (maybe just stick with ICollection<T>?) (O(1) lookup for direct match)

This way, I would be able to create my own MyCorsPolicy which could work for example with compiled regular expression... (NB! such change would also require change in Microsoft.AspNetCore.Mvc.Cors)

Is it worth opening PR now?

@javiercn
Copy link
Member Author

javiercn commented Oct 5, 2018

@wdolek Thanks for contacting us. We haven't started gathering information to do this yet.

Regarding your specific issue, is it something like contoso.* what you want? I believe you can already set a delegate on CorsOptions to do this.

I would hold off on sending a PR for this as we want to take our time to look at all scenarios and how they fit together.

@wdolek
Copy link

wdolek commented Oct 5, 2018

@javiercn Thank you for fast response! Yes, I can somehow do it even now, sort of, but everything what came to my mind was bit hackish (closures to hold settings?). I will try to rethink it and apply solution to current CorsPolicy.

Is there any board where I could check progress on decisions/ideas/direction of CORS improvements?

@javiercn
Copy link
Member Author

javiercn commented Oct 5, 2018

Not really, I filed an issue to keep track of this, but I don't know if we'll do it for 3.0

@Eilon Eilon added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed repo:CORS labels Nov 26, 2018
@mkArtakMSFT
Copy link
Member

Closing this as there is no real need for this and we've had a bunch of improvements in this area recently.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

7 participants