-
Notifications
You must be signed in to change notification settings - Fork 155
Issue/62 #74
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
Issue/62 #74
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| set $request_cors "${request_method}_${CORS_ENABLED}"; | ||
|
|
||
| if ($request_cors = "OPTIONS_1") { | ||
| add_header 'Access-Control-Allow-Origin' '${CORS_ALLOWED_ORIGIN}'; | ||
| add_header 'Access-Control-Allow-Methods' 'GET, POST, OPTIONS'; | ||
| # | ||
| # Custom headers and headers various browsers *should* be OK with but aren't | ||
| # | ||
| add_header 'Access-Control-Allow-Headers' 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range'; | ||
| # | ||
| # Tell client that this pre-flight info is valid for 20 days | ||
| # | ||
| add_header 'Access-Control-Max-Age' 1728000; | ||
| add_header 'Content-Type' 'text/plain; charset=utf-8'; | ||
| add_header 'Content-Length' 0; | ||
| return 204; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this is included in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| if ($request_cors = "GET_1") { | ||
| add_header 'Access-Control-Allow-Origin' '${CORS_ALLOWED_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; | ||
| } | ||
|
|
||
| if ($request_cors = "HEAD_1") { | ||
| add_header 'Access-Control-Allow-Origin' '${CORS_ALLOWED_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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious why |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(posting for visibility, not a change request) I had questions about whether it was necessary to fork the logic here just to exclude
OPTIONSsince it has general usage outside the context of CORS. However it seems that for S3 it's only really applicable to the CORS caseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.