Allow multiple origin domains for CORS #1973
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit references @danielvijge’s commit eurail@cc7be41 and has been rebased on top of next
Tieske
reviewed
Jan 12, 2017
| local function configure_origin(ngx, conf) | ||
| if conf.origin == nil then | ||
| local origin = ngx.req.get_headers()["origin"] or nil |
There was a problem hiding this comment.
The or nil does not make sense here, can be removed
| @@ -7,6 +7,7 @@ return { | |||
| methods = { type = "array", enum = { "HEAD", "GET", "POST", "PUT", "PATCH", "DELETE" } }, | |||
| max_age = { type = "number" }, | |||
| credentials = { type = "boolean", default = false }, | |||
| preflight_continue = { type = "boolean", default = false } | |||
| preflight_continue = { type = "boolean", default = false }, | |||
| origin_domains = { type = "array" } | |||
There was a problem hiding this comment.
I think this array requires validation of its contents (and accompanying tests)
There was a problem hiding this comment.
additionally, not knowing CORS in-depth, shouldn't origin and origin-domains be mutually exclusive?
added 2 commits
January 12, 2017 15:29
…l-allow-origin-response-header) there should be a way to define a list of origins on the Origin request header, but with a note stating "In practice the origin-list-or-null production is more constrained. Rather than allowing a space-separated list of origins, it is either a single origin or the string "null"."
|
I have added some comments on the original #1774 |
thibaultcha
requested changes
Mar 14, 2017
| @@ -181,5 +199,27 @@ describe("Plugin: cors (access)", function() | |||
| assert.is_nil(res.headers["Access-Control-Allow-Credentials"]) | |||
| assert.is_nil(res.headers["Access-Control-Max-Age"]) | |||
| end) | |||
| it("sets CORS orgin based on origin host", function() | |||
| local function configure_origin(ngx, conf) | ||
| local origin = ngx.req.get_headers()["origin"] or nil |
There was a problem hiding this comment.
let's only do this in the else branch a couple lines underneath. Also, no need for or nil.
| if conf.origin == nil then | ||
| ngx.header["Access-Control-Allow-Origin"] = "*" | ||
| else | ||
| ngx.header["Access-Control-Allow-Origin"] = conf.origin | ||
| ngx.header["Vary"] = "Origin" | ||
| if origin ~= nil and host_in_domain(origin, conf) then |
There was a problem hiding this comment.
if origin , no need for ~= nil
| @@ -7,12 +7,24 @@ CorsHandler.PRIORITY = 2000 | |||
|
|
|||
| local OPTIONS = "OPTIONS" | |||
|
|
|||
| local function host_in_domain(domain, conf) | |||
| for i, d in ipairs(conf.origin) do | |||
| if string.match(domain, '[%.|//]'..d..'$') then | |||
There was a problem hiding this comment.
Let's do a couple things here:
- use the PCRE API offered by
ngx.re: https://github.com/openresty/lua-nginx-module#ngxrematch - build the regex ahead of time instead of on every request
1 task
|
Replaced by #2203 with a new commit on top of yours which fixes the missing requirements. Thanks again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR references 1774 contributed by @danielvijge and has been rebased on top of next.
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
Issues resolved
Fix #1043