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

Allow multiple origin domains for CORS #1774

Closed

Conversation

danielvijge
Copy link

Summary

When several domains consume an API, the CORS header Access-Control-Allow-Origin is not as useful, because it only allows one domain, or all domains (with "*"). The specs do not allow for a list of domains. This change works around this by comparing the Origin header to a list of configured domain in the array 'origin_domains'. It tests if the end of the domain matches either ".{domain}" or "//{domain}". Thus, if origins_domain="example.com, example.net", a request from Origin="http://www.example.com" would be allowed, as does a request from Origin="http://example.com". But Origin="http://my-example.com" is not allowed. In this case, no Access-Control-Allow-Origin header is set.
If both origin and origin_domains are configured, origin_domains take precedence over origin.

Full changelog

  • Changes to CORS plugin schema to have extra configuration for origin_domains[]
  • Changes to CORS plugin handler to test for multiple domain and set correct header
  • Changes to test script to test if the correct header is set when the origin domain matches
  • Changes to test script to test if the header remains empty when the origin domain does not match

Issues resolved

Fix #1043

@subnetmarco
Copy link
Member

subnetmarco commented Nov 16, 2016

Hello @danielvijge - can you please rebase on top of next. Tests should be working once you do that.

@danielvijge danielvijge force-pushed the feat/cors/multiple-origin-domains branch from 22eb10b to cc7be41 Compare November 16, 2016 15:34
@pklingem
Copy link

commenting to revive this PR.

@subnetmarco
Copy link
Member

@danielvijge I added some comments. Can you also rebase on top of next?

@thibaultcha
Copy link
Member

Replaced by #2203, thanks!

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

Successfully merging this pull request may close these issues.

4 participants