ISSUE: 3030. CORS Expose Headers. #3043

McMutton · 2017-11-16T17:05:55Z

Access-Control-Expose-Headers is now sent in the response, allowing the client to access certain response headers.

With this PR, I took the liberty to add some improvements to our logic with respect to the CORS specification: We are no longer adding any CORS headers to the response if the client fails to add Origin to the request or the Origin value does not match the one allowed by Orion.

next release details added

fgalan · 2017-11-16T17:12:37Z

test/functionalTest/cases/0501_cors/max_age.test

@@ -41,6 +41,7 @@ HTTP/1.1 200 OK
 Content-Length: 0
 Access-Control-Max-Age: 600
 Access-Control-Allow-Headers: REGEX(.*)
+Access-Control-Expose-Headers: REGEX(.*)


As far as I see, the list is explicit (define CORS_EXPOSED_HEADERS FIWARE_CORRELATOR", "FIWARE_TOTAL_COUNT", "RESOURCE_LOCATION). Thus, I'd suggest to use the actual headers instead a generic regex, in order to make the test stronger.

I fact.. would it make sense to to the same for Access-Control-Allow-Headers, I mean, to pass from REGEX(.*) to the actual list of headers?

@fgalan, thank you for this comment. This made me realize I have skipped an important step in testing. Let me explain : )

My initial idea was to have a separate test file focusing on each CORS header and avoid failing of a specific test just because another CORS header has a wrong value.

For example, here in max_age.test we are focusing on the Access-Control-Max-Age header and this test should not fail in case there is something wrong with Access-Control-Allow-Headers. A specific test or a set of tests should be focusing on that header.

The reality is, there are no tests focusing on Access-Control-Allow-Headers or Access-Control-Expose-Headers. I'll go ahead and add them quickly to the set of CORS tests. Please check if the tests make sense as a whole then and we can discuss if we need any further improvements.

Let me check if I'm undersetanding your point before providing feedback

You mean having 3 .test files, each one focusing on each header, thus:

In the first .test Access-Control-Max-Age: has particular values but Access-Control-Allow-Headers and Access-Control-Expose-Headers use REGEX(.*)

In the second .test Access-Control-Allow-Headers: has particular values but Access-Control-Max-Age and Access-Control-Expose-Headers use REGEX(.*)

In the third .test Access-Control-Expose-Headers: has particular values but Access-Control-Max-Age and Access-Control-Allow-Headers use REGEX(.*)

Is this correct?

Yes that's the idea. We also have the Access-Control-Allow-Methods and Access-Control-Allow-Origin headers but they already have their own dedicated test files.

The general rule we use in the .test is try to be as specific as possible, that is, to avoid the usage of REGEX() (and specially, REGEX(.*)) as much as possible.

Having said that, I think that in this particular case it makes sense to do an exception to the general rule and proceed as you suggest. However, I'd suggest to use the following in the preamble of the involved .test in order to be clear:

This test is focused in header Access-Control-Max-Age. Thus, other CORS related headers use REGEX(.*) as value. However, note that those specific headers are tested with specific headers values in other .test files in this same directory

or something similar

Fixed in 3e6e50a

fgalan · 2017-11-16T17:22:13Z

src/lib/rest/restReply.cpp

@@ -179,16 +181,27 @@ void restReply(ConnectionInfo* ciP, const std::string& _answer)
      {
        MHD_add_response_header(response, ACCESS_CONTROL_ALLOW_ORIGIN, corsOrigin);
      }
-    }
+      // If there is no match, originAllowed flag is set to false
+      else {


Style: { in next line.

Fixed in 3e6e50a

fgalan · 2017-11-16T17:42:34Z

src/lib/rest/HttpHeaders.h

+*
+* CORS Exposed Headers -
+*/
+#define CORS_EXPOSED_HEADERS FIWARE_CORRELATOR", "FIWARE_TOTAL_COUNT", "RESOURCE_LOCATION


Should the three exposed headers be documented as part of Orion documentation? I mean, is this list something implementation-specific (so it should) or derived directly from CORS specification without ambiguity?

Well, these are the headers we put in Context Broker responses that the client could read. In the context of CORS, we need to have these headers listed in Access-Control-Expose-Headers in our responses so that the client can access and read them.

Since we are not limiting or extending this set for CORS, I would say documenting might not be crucial but won't hurt either : )

By the way, if I have missed any headers that we put in the responses and expect it to be client accessible please let me know.

Since we are not limiting or extending this set for CORS, I would say documenting might not be crucial but won't hurt either : )

I agree.

Could you write a little cors.md piece of documentation about this (and any other CORS specific implementation aspects to mention) in doc/manuals/user to be included in this PR, please? Don't worty too much about how the material fits in the overall manual, I'll deal with it once the PR gets merged.

Fixed in 3e6e50a

fgalan · 2017-11-16T17:43:22Z

CHANGES_NEXT_RELEASE

@@ -3,3 +3,4 @@
 - Fix: case-sensitive header duplication (e.g. "Content-Type" and "Content-type") in custom notifications (#2893)
 - Fix: bug in GTE and LTE operations in query filters (q/mq), both for GET operations and subscriptions (#2995)
 - Fix: Wrong "max one service-path allowed for subscriptions" in NGSIv2 subscription operation (#2948)
+- Fix: Add support for accessing response headers when making CORS requests (#3030)


Could this be considered part of "CORS Preflight Requests support for all NGSIv2 resources"? Or it is better to consider it a separate item of work?

Well, it is not directly related with Preflight requests but since we are touching those parts in this PR as well, I can change the line below

Add: CORS Preflight Requests support for all NGSIv2 resources, -corsMaxAge switch (#501)

to

Add: CORS Preflight Requests support for all NGSIv2 resources, -corsMaxAge switch, CORS exposed headers (#501)

Ok, with a small ammedment to include the number of the issue about exposed headers:

Add: CORS Preflight Requests support for all NGSIv2 resources, -corsMaxAge switch, CORS exposed headers (#501, #3030)

Fixed in 3e6e50a

kzangeli · 2017-11-16T18:24:15Z

LGTM

fgalan · 2017-11-17T08:38:36Z

src/lib/serviceRoutinesV2/optionsAllNotDelete.cpp

-  ciP->httpHeaderValue.push_back("GET, PATCH, POST, PUT, OPTIONS");
+  if ( (ciP->httpHeaders.origin != "") && ((strcmp(corsOrigin, "__ALL") == 0) || (strcmp(ciP->httpHeaders.origin.c_str(), corsOrigin) == 0)) )
+  {
+    ciP->httpHeader.push_back("Access-Control-Allow-Methods");


Should use ACCESS_CONTROL_ALLOW_METHODS.

(Similar cases along others serviceRoutinesV2/ files in this PR)

Fixed in 3e6e50a

fgalan · 2017-11-17T08:46:57Z

src/lib/serviceRoutinesV2/optionsPostOnly.cpp

@@ -45,8 +46,11 @@ std::string optionsPostOnly
  ParseData*                 parseDataP
 )
 {
-  ciP->httpHeader.push_back("Access-Control-Allow-Methods");
-  ciP->httpHeaderValue.push_back("POST, OPTIONS");
+  if ( (ciP->httpHeaders.origin != "") && ((strcmp(corsOrigin, "__ALL") == 0) || (strcmp(ciP->httpHeaders.origin.c_str(), corsOrigin) == 0)) )


This check is done many times... Maybe it should be moved to a common function.

Which file do you think should host the function? Is rest.cpp or RestService.cpp are good candidates? Or should we have another, CORS specific file?

rest.cpp (along with extern declaration in rest.h) sounds good.

Fixed in 3e6e50a

fgalan

Good work @McMutton! Only some minor comments added to the PR

fgalan · 2017-11-21T10:31:56Z

src/lib/serviceRoutinesV2/optionsAllNotDelete.cpp

@@ -45,8 +47,11 @@ std::string optionsAllNotDelete
  ParseData*                 parseDataP
 )
 {
-  ciP->httpHeader.push_back("Access-Control-Allow-Methods");
-  ciP->httpHeaderValue.push_back("GET, PATCH, POST, PUT, OPTIONS");
+  if ( isOriginAllowedForCORS(ciP->httpHeaders.origin) )


I think is should be

if (isOriginAllowedForCORS(ciP->httpHeaders.origin))

per M11 style rule

Also, after ( and before ), spaces MUST NOT be present.

(Seen in other part of the PR also)

Fixed in da358b6

fgalan · 2017-11-21T10:33:34Z

src/lib/rest/rest.cpp

+*/
+bool isOriginAllowedForCORS(std::string requestOrigin)
+{
+  return ( (requestOrigin != "") && ((strcmp(corsOrigin, "__ALL") == 0) || (strcmp(requestOrigin.c_str(), corsOrigin) == 0)) );


If think it should change: return ( ( .. ) ); -> return ( ) per M11 style rule

Also, after ( and before ), spaces MUST NOT be present.

Fixed in da358b6

fgalan · 2017-11-21T10:34:40Z

src/lib/rest/rest.cpp

+* isOriginAllowedForCORS - checks the Origin header of the request and returns
+* true if that Origin is allowed to make a CORS request
+*/
+bool isOriginAllowedForCORS(std::string requestOrigin)


It should be const std::string&, see https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/contribution_guidelines.md#programming-patterns:

if the function should not be able to modify the object (i.e. "read only"), then const references should be used, e.g. const BigFish& bf

Thanks for pointing out these mistakes @fgalan, I will fix them quickly.

They aren't big issues, but I think is good to fix them so you get familiar with style aspects ;)

Yep, we need mistakes to memorize style guidelines :)

Fixed in da358b6

Just to add some clarification here ...
This is a bit more than just 'style guide' (understanding style guide as a "the way I prefer to see the code, my taste").
This is about avoiding to copy an entire object on the stack for every call to the function, and instead just passing the object by reference.

The style guide says something like:

"An entire object should never ever be sent over the stack."
"If the object is allowed to be altered by the function, use a C pointer"
"If not, use a C++ reference"

The reasoning behind this rule is that the ampersand in the call:
funcCall(&v):
shows that the variable v may be altered, while
funcCall(v)
does not.

Correct, this is more than style. Note it is in the section about Programming Patterns of the Contribution Guidelines, not in the sections for Must/should Style Rules.

fgalan · 2017-11-21T11:31:37Z

LGTM. Thanks!

ISSUE: 3030. CORS Expose Headers.

daaef7e

next release details added

fgalan reviewed Nov 16, 2017

View reviewed changes

fgalan reviewed Nov 17, 2017

View reviewed changes

fgalan requested changes Nov 17, 2017

View reviewed changes

Suggested changes

3e6e50a

fgalan reviewed Nov 21, 2017

View reviewed changes

style fix

da358b6

fgalan approved these changes Nov 21, 2017

View reviewed changes

fgalan merged commit dfc83ad into telefonicaid:master Nov 21, 2017

fgalan mentioned this pull request Nov 21, 2017

Add support for accessing response headers when making CORS requests #3030

Closed

McMutton deleted the cors_expose_headers branch November 30, 2017 16:32

ISSUE: 3030. CORS Expose Headers. #3043

ISSUE: 3030. CORS Expose Headers. #3043

Conversation

McMutton commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McMutton Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

McMutton Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan commented Nov 21, 2017

fgalan Nov 20, 2017 •

edited

Loading

fgalan Nov 20, 2017 •

edited

Loading

fgalan Nov 20, 2017 •

edited

Loading

McMutton Nov 20, 2017 •

edited

Loading

fgalan Nov 17, 2017 •

edited

Loading

McMutton Nov 20, 2017 •

edited

Loading