Support CORS GET Requests #846
Conversation
|
Thanks for the contribution! This PR could be an effective solution to issue #501 and I'm sure many users will be happy with it ;) I would need some time to read the references you cite to learn more on CORS and also to discuss the PR with @kzangeli . Regarding testing for this PR, rather than unit test I'd suggest to use test harness (a kind of end-to-end automatic test framework we have for Orion), but don't worry about that too much: @kzangeli or me could manage that. |
| @@ -85,6 +87,15 @@ void restReply(ConnectionInfo* ciP, const std::string& answer) | |||
| MHD_add_response_header(response, "Content-Type", "application/xml"); | |||
| else if (ciP->outFormat == JSON) | |||
| MHD_add_response_header(response, "Content-Type", "application/json"); | |||
|
|
|||
| // If any origin is allowed the header is sent always with "any" as value | |||
| if(strcmp(allowedOrigin, "*") == 0) { | |||
There was a problem hiding this comment.
Please use this C++ style:
if (...)
{
...
}
else
{
...
}
|
thanks for the feedback. I have updated the PR to meet the indenting rules and I have improved the design of the condition check. |
|
+1! :) |
|
+2 |
Conflicts: src/app/contextBroker/contextBroker.cpp
|
The PR is now ready for review and final merge. LGTM from @kzangeli and @jmcanterafonseca are required. |
| @@ -246,6 +247,7 @@ int writeConcern; | |||
| #define HTTPSCERTFILE_DESC "certificate key file (for https)" | |||
| #define RUSH_DESC "rush host (IP:port)" | |||
| #define MULTISERVICE_DESC "service multi tenancy mode" | |||
| #define ALLOWED_ORIGIN_DESC "CORS allowed origin. use '__ALL' for any" | |||
There was a problem hiding this comment.
Allowing -corsOrigin '*' in the CLI is dangerous... If the user forgets to include * as literal, the shell will extend it with the contents of the current directory. @jmcanterafonseca do you see any problem in using an alias ("__ALL" could be changed for a better one if you want)? Of course, this precludes to use literally __ALL as domain origin is a name weird enough so nobody should be using it as real domain name :)
There was a problem hiding this comment.
yes, you are right. I don't have any particular opinion on what syntax to use for representing any URI, if you are ok with __ALL , I'm fine
thanks!
| @@ -1068,6 +1071,8 @@ void restInit | |||
| rushHost = _rushHost; | |||
| rushPort = _rushPort; | |||
|
|
|||
| strcpy(restAllowedOrigin, _allowedOrigin); | |||
There was a problem hiding this comment.
Use strncpy instead. The style guide script will complain ...
Wish we had the style guide check running ...
|
LGTM |
|
LGTM as well, thanks for the work! |
|
LGTM (final) |
…_less_new_1 Hardening/less memory less new 1
This PR is intended to enable cross-origin GET requests from Web Browsers to Orion. The advantage of supporting CORS is that Orion will be available directly from a Web Browser client without the need of intermmediate backends or proxies. More info on CORS can be found at
https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS
https://annevankesteren.nl/2015/02/same-origin-policy
For the time being, only the ability to perform GET requests have been implemented. For POST / PUT requests it would be needed to implement preflight requests through the OPTIONS method. But that would part of a subsequent PR.
I guess the PR would need some unit tests as well, but I am a bit lost on how to start with. If they are needed any hint on how to do that would be appreciated.