feat(cors): support for the Timing-Allow-Origin header #9365
Conversation
|
Hi @skimdz86, thanks for your contribution, Is Timing-Allow-Origin some kind of CORS? |
apisix/plugins/cors.lua
Outdated
| if not conf.allow_origins then | ||
| if conf.timing_allow_origins or conf.timing_allow_origins_by_regex then | ||
| return false, "you can not set 'timing_allow_origin' " .. | ||
| "or 'timing_allow_origin_by_regex' " .. | ||
| "when 'allow_origins' is not set" | ||
| end | ||
| end |
There was a problem hiding this comment.
I have considered this place carefully, and I think that allow_origins and timing_allow_origins should be independent of each other, because it is possible that the upstream has a response header of Access-Control-Allow-Origin, which does not require the plugin to set it.
There was a problem hiding this comment.
Actually, I didn't consider the case in which the CORS are managed already by the upstream server; so I agree, this way we can add only the timing header in the api gateway in addition to the existing CORS headers, leaving freedom to the users in the configuration. So I'll remove this check, but I also have to change the logic in the header_filter function, to allow setting the timing even when allow_origins is not set in the plugin conf.
t/plugin/cors4.t
Outdated
| Access-Control-Allow-Headers: request-h | ||
| Access-Control-Expose-Headers: expose-h | ||
| Access-Control-Max-Age: 10 | ||
| Timing-Allow-Origin: http://testurl.domain.com |
There was a problem hiding this comment.
Need a blank at the end of file
…rd cors headers, to allow setting it alone
|
@monkeyDluffy6017 Updated the PR with your suggestions; since we decoupled the management of cors headers and timing header, I also split the set_cors_headers function in 2, adding the set_timing_headers function. |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. |
|
Hi @monkeyDluffy6017 , any news on this PR? Thanks |
|
@skimdz86 sorry for this delay, i have no idea if Timing-Allow-Origin belongs to CORS |
Ok, so what are the next steps in this case? |
| Timing-Allow-Origin: | ||
|
|
||
|
|
||
| === TEST 5: set route ( allow_origins same as timing_allow_origins ) |
There was a problem hiding this comment.
Three blanks are needed, please check the other place
apisix/plugins/cors.lua
Outdated
| ) | ||
| end | ||
| if conf.allow_origins ~= "*" then | ||
| core.response.add_header("Vary", "Origin") | ||
| end | ||
| if allow_origins then | ||
| if allow_origins then |
There was a problem hiding this comment.
remove redundant indents
apisix/plugins/cors.lua
Outdated
| @@ -202,9 +236,20 @@ local function set_cors_headers(conf, ctx) | |||
| if conf.allow_credential then | |||
| core.response.set_header("Access-Control-Allow-Credentials", true) | |||
| end | |||
|
|
|||
| if ctx.timing_allow_origin then | |||
| core.response.set_header("Timing-Allow-Origin", ctx.timing_allow_origin) | |||
There was a problem hiding this comment.
Why do you need this? you have done this in set_timing_headers
There was a problem hiding this comment.
ok, useless, I may have forgotten to remove it when I decoupled the logic of classic cors and timing headers. I'll remove it
docs/en/latest/plugins/cors.md
Outdated
| | timing_allow_origins | string | False | nil | Origin to allow to access the resource timing information. See [Timing-Allow-Origin](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Timing-Allow-Origin). Use the `scheme://host:port` format. For example, `https://somedomain.com:8081`. If you have multiple origins, use a `,` to list them. | | ||
| | timing_allow_origins_by_regex | array | False | nil | Regex to match with origin for enabling access to the resource timing information. For example, `[".*\.test.com"]` can match all subdomain of `test.com`. When set to specified range, only domains in this range will be allowed, no matter what `timing_allow_origins` is. | |
There was a problem hiding this comment.
I find that CORS and Timing-Allow-Origin are 2 completely separate functions, and if we put their configurations in one table, the complexity of the configuration items can be very confusing, for example the configuration item allow_methods only works for CORS, so how about split the table into two part, one for cors and the other for Timing-Allow-Origin
There was a problem hiding this comment.
ok seems a good idea, I'll fix it
|
I will approve this pr after you resolve the review comments |
monkeyDluffy6017
added
the
wait for update
…litting attribute table, code style fixes
@monkeyDluffy6017 PR updated |
monkeyDluffy6017
added
approved
and removed
wait for update
shreemaan-abhishek
dismissed stale reviews from monkeyDluffy6017 and themself
via
1884a15
monkeyDluffy6017
changed the title
Description
Implements feature described in issue #9307: added support to the Timing-Allow-Origin header in the CORS plugin.
The new header can be configured with a list of domains or a regex, just as the Access-Control-Allow-Origin.
The new header is returned only if the Access-Control-Allow-Origin is returned.
I added a new parameter to the 2 functions process_with_allow_origins and process_with_allow_origins_by_regex to make these functions "common" and now used both for calculating the Access-Control-Allow-Origin and the Timing-Allow-Origin header; the new "allow_origin_type" parameter was essential to differentiate the cache used to save the elaboration results for the 2 different headers.
Note: I updated also the markdown documentation (english version) adding the 2 new configuration fields, but unfortunately I don't speak Chinese, so any help for updating also the /docs/zh/latest/plugins/cors.md file will be appreciated.
Fixes #9307
Checklist