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

Add support of CORS #949

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

manifest
Copy link

Pull request for issue 947 "Make Cowboy CORS friendly".

@essen
Copy link
Member

essen commented Feb 15, 2016

Thanks, a few comments. Note however that this will break in a week or two when I finally merge back my current set of changes (huge).

  • I will not merge until I review the CORS specs, so please be patient
  • I will not merge until there's tests or I have time to write some; those must be based on real-world use cases and added to a relevant Common Test suite (or a new suite must be created, probably this)
  • It seems that there's parsing code in this PR. Header parsing code should be in cow_http_hd in cowlib (along with tests and benchmarks)
  • I am not sure the function binary_join is necessary
  • Use maps, the whole proplists converted to a record is completely unnecessary

@manifest
Copy link
Author

I will write the tests. You're right about maps, it's a better choice. And we need the binary_join function at least for exposed headers.

@essen
Copy link
Member

essen commented Mar 31, 2016

Hey, I will try to get to this this week-end. I finally read the CORS spec so we can move forward.

This seems like a good first step, but I think we should also better integrate CORS support with cowboy_rest. Most of the info can be filled automatically there, except the origin check, max-age and exposed-headers, so it could be very nice to have.


-spec match_cors_credentials(boolean(), {binary(), binary(), 0..65535} | reference() | '*') -> boolean().
match_cors_credentials(true, '*') ->
throw({bad_credentials, true, '*'});
Copy link
Member

Choose a reason for hiding this comment

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

Guessing this is for 'The string "" cannot be used for a resource that supports credentials.' but I'm not sure this is very useful, more useful would be sending Origin back instead of "" if credentials are allowed.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, it's how it works now. User will get an Origin back if credentials are allowed. The only way he can get the "*" in the response the parser didn't fully recognize the Origin value or the value was "null" string, thus the parser returned an reference that can't be returned to the user. So we need this validation for the last case.

@hachreak
Copy link

hachreak commented Jun 4, 2016

Hi at all,
are there news about CORS for cowboy?
Thanks :)

@manifest
Copy link
Author

manifest commented Jun 4, 2016

@hachreak, the main conversation is here.

@hachreak
Copy link

hi @manifest, I saw that the PR for cowlib have been merged.
It's mean that also this PR will be merged too?
Thanks :)

@manifest
Copy link
Author

@hachreak, this PR is currently obsoleted. You can implement the support of CORS in a way described in the last few comments here, using new functions of cowlib. Will it work for you?

@hachreak
Copy link

if you have time, can you add a example that show how it works? :)
This will be really useful to anyone want use it.

@manifest
Copy link
Author

@hachreak, have you seen an example here? Is it not clear how you can implement CORS support from that example or you mean an example in the docs?

@hachreak
Copy link

I mean an example inside examples/ directory.

@manifest
Copy link
Author

Ok, will do it.

@hachreak
Copy link

@manifest thanks a lot! :D

@essen essen added this to the 2.0 New Features milestone Feb 4, 2017
@essen essen modified the milestones: 2.0 New Features, After 2.0 Oct 2, 2017
@essen essen added this to the 2.4 "CORS" milestone Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants