Issue/62 #74
Issue/62 #74
Conversation
| else | ||
| export LIMIT_METHODS_TO="GET HEAD" | ||
| export LIMIT_METHODS_TO_CSV="GET, HEAD" | ||
| fi |
There was a problem hiding this comment.
(posting for visibility, not a change request) I had questions about whether it was necessary to fork the logic here just to exclude OPTIONS since it has general usage outside the context of CORS. However it seems that for S3 it's only really applicable to the CORS case
There was a problem hiding this comment.
Yeah, it seems unnecessary because this approach is stemming from my security paranoia. I don't want to provide any paths to allow for proxying requests to the underlying S3 bucket beyond the narrow operations defined. As such, I'm going out of my way here to lock down the HTTP verbs to only GET/HEAD or in the case of CORS GET/HEAD/OPTIONS.
| add_header 'Content-Type' 'text/plain; charset=utf-8'; | ||
| add_header 'Content-Length' 0; | ||
| return 204; | ||
| } |
There was a problem hiding this comment.
Given that this is included in a location block, could/should this be rewritten using map to avoid the "if is evil in a location block" issue?
There was a problem hiding this comment.
For the OPTIONS if, it seems to fall under the category of "good ifs":
There are cases where you simply cannot avoid using an if, for example, if you need to test a variable which has no equivalent directive.
As for the other ifs, I believe they are ok due to there being no ifs after them. However, I have no idea how you would be able to do the same operation with map. Ideally, I wanted to conditionally template in the cors.conf include, but that seemed a bit too clunky. I guess I could conditional set an env var with the whole include statement in docker-entrypoint.sh.
There was a problem hiding this comment.
Yeah this certainly is the best for understandibility. I wouldn't add more moving parts if it's testing ok. I'm going to play around with making it work with map because I'm curious now but let's not let it hold up the PR.
| add_header 'Access-Control-Allow-Origin' '*' always; | ||
| add_header 'Access-Control-Allow-Methods' 'GET, POST, OPTIONS' always; | ||
| add_header 'Access-Control-Allow-Headers' 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range' always; | ||
| add_header 'Access-Control-Expose-Headers' 'Content-Length,Content-Range' always; |
There was a problem hiding this comment.
Curious why Access-Control-Expose-Headers is not sent in the OPTIONS response?
|
@worace I'll be waiting a week for your review/test and then we will merge |
Signed-off-by: Elijah Zupancic <e.zupancic@f5.com>
Fixes #62
Add support for CORS response headers.