case sensitivness in httpCustom notification

[cdupont]

A small bug, but that can bite (it did ;).
I tried to create a notification with Orion, using httpCustom and a Content-Type: application/json.
I used the encoding suggested at: http://fiware-orion.readthedocs.io/en/master/user/forbidden_characters/#custom-payload-special-treatment

curl localhost:1026/v2/subscriptions -s -S --header 'Content-Type: application/json' --header 'Accept: application/json' -d @- <<EOF
{
  "description": "",
  "subject": {
    "entities": [
      {
        "id": "WS_UPPA_Sensor2",
        "type": "SensingDevice"
      }
    ],
    "condition": {
      "attrs": [
        "SM1"
      ]
    }
  },
  "notification": {
    "httpCustom": {
      "url": "http://localhost:10839",
      "headers": {
         "Content-Type": "application/json"
      },
      "method": "POST",      
      "payload": "%7B%22src%22%3A%2200393806412092%22%2C%22dst%22%3A%2200393806412093%22%2C%22text%22%3A%22test%22%7D"
    },
    "attrs": [
      "SM1"
    ]
  },
  "expires": "2040-01-01T14:00:00.00Z",
  "throttling": 5
}
EOF

Listening on the notification port I get:

$ ncat -v -l 10839 
POST / HTTP/1.1
Host: localhost:10839
User-Agent: orion/1.7.0-next libcurl/7.47.0
Fiware-ServicePath: /UPPA/TESTS
Accept: application/json
Content-length: 61
Content-type: text/plain; charset=utf-8
Fiware-Correlator: e3c0e990-3c93-11e7-94a9-605718b19667
Ngsiv2-AttrsFormat: custom
Content-Type: application/json

You see that Content-Type is repeated twice, with different cases. This didn't work well with my provider.
If I put Content-type in the httpCustom headers, it works.
As per: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
field names should be case-insensitive.
Thanks!

[fgalan]

Good catch! :)

Some hints, hoping to help to find a solution...

The parameters used to send a custom notification are buit by the buildSenderParamsCustom(). This part if for HTTP headers:

    //
    // 5. HTTP Headers
    //
    for (std::map<std::string, std::string>::const_iterator it = httpInfo.headers.begin(); it != httpInfo.headers.end(); ++it)
    {
      std::string key   = it->first;
      std::string value = it->second;

      macroSubstitute(&key,   it->first, ce);
      macroSubstitute(&value, it->second, ce);

      if (key == "")
      {
        // To avoid empty header name
        continue;
      }

      headers[key] = value;
    }

The headers map ends as a field in the parameter object (of SenderThreadParams class):

params->extraHeaders     = headers;

Next, let's jump to the place where the parameter are used, i.e. startSenderThread(). This function calls httpRequestSend() which in secuence calls httpRequestSendWithCurl(), passing params->extraHeaders all the way down.

  // ----- Content-type
  std::string contentTypeString = std::string("Content-type: ") + content_type;
  httpHeaderAdd(&headers, "Content-type", contentTypeString, &outgoingMsgSize, extraHeaders, usedExtraHeaders);

The httpHeaderAdd() will try to add the "Content-type: " except if "Content-type" is in extraHeaders (local name for the former params->extraHeaders).

In httpHeadersAdd() we have:

  it = extraHeaders.find(headerName);
  if (it == extraHeaders.end())
  {
    <use what has been passed as function parameter>
  }
  else
  {
    <used what we have in extraHeaders>
  }

The problem would be due to the find() is done in a case sensitive way. Thus, the logic doesn't detect "Content-type" (in the function call) equal to "Content-Type" (in extraHeaders as a consecuence of custom parametrization).

Possible solutions ideas:

• Make that find() case insenstive
• Transform the custom header in lowercase before inserting in the map (i.e. at headers[key] = value; point). This problably need also to use lowercase in other parts of the httpRequestSendWithCurl() logic, but probably working always in lowercase will ease code.

[cdupont]

@fgalan Yes I think that converting everything to lowercase before the find() is easier.

[fgalan]

@arigliano , let's try with the easiest solution:

Transform the custom header in lowercase before inserting in the map (i.e. at headers[key] = value; point). This problably need also to use lowercase in other parts of the httpRequestSendWithCurl() logic, but probably working always in lowercase will ease code.

Some additional comments:

• Impact on documentation: none
• Impact on unit tests: none
• Impact on functional tests:
  • Existing .test should work without any expected change. If the modification break some existing test, please comment on this issue (in that case we would probably need to reevaluate the solution approach)
  • A new test mirroring the use case described by @cdupont in his original post should be added using (for instance) the directory cases/2893_case_insensitive_custom_notif_http_headers. You could start with some existng .test about NGSIv2 custom notificatins (hing: grep for "httpCustom" in the cases/ directory) and modify it accordingly.

Don't hesitate to ask if you have some doubt regarding implementation.

[arigliano]

In order to implement the proposed solution, following steps should be performed:

• As suggested, inserting a lower case transformation to key variable, before the line: headers[key] = value; point)

• In addition, in order to match lowercased header in the map against a default one (like "Content-Type) in the httpHeaderAdd function, and to produce notifications always with lowercase headers, following alternative solutions are suggested:

  • Change all the hardcoded strings, passed as input to httpHeaderAdd, to lowercase. (i.e httpHeaderAdd(&headers, "Content-type", contentTypeString, &outgoingMsgSize, extraHeaders, usedExtraHeaders);
  • At the beginning of httpHeaderAdd body, perfom a lowercase transformation to headerName (and maybe also to headerString), in order to lowercase any input string (as already done at: usedExtraHeaders[headerNameLowerCase.c_str()] = true;

[arigliano]

In any case, it could be better to modify the signature of httpHeaderAdd function, by replacing the input string headerString with headerValue, in order to avoid the redundance when passing first headerName and then the complete header (possible cause of errors), such as:
std::string contentTypeString = std::string("Content-type: ") + content_type; httpHeaderAdd(&headers, "Content-type", contntTypeString, &outgoingMsgSize, extraHeaders, usedExtraHeaders);
Then change a little bit its logic, by concatenating the headerName to construct the final header, with the value, either from the input ( new headerValue parameter) or the matched one from extraHeaders map.

[fgalan]

and to produce notifications always with lowercase headers

As far as I remember, HTTP headers are case insenstive by definition, so it would be a "legal" change from the point of view of backward compability. However, let's consider the potential impact in .test... all them are aligned with the current way of rendering headers (e.g. Content-Type, Fiware-Corrrelator, etc.).

[fgalan]

With regards to signature change, it makes sense to me. However, let's see how it looks like at PR time.

@kzangeli what do you think?

[kzangeli]

With regards to signature change, it makes sense to me too

[fgalan]

Fixed by PR #3024

[fgalan]

@cdupont , the issue has been fixed in master branch. As original author of the issue, it would be great if you could check now (maybe using the docker at dockerhub). If your check goes fine, the issue can be closed. If your check goes ok, new information can be provided. Thanks!

(We would keep the issue opened until the end of the 1.10.0 development cycle, e.g. around 0.5-1 months from now on. The issue will be closed if we don't have any news by this time, assuming it was fixed).

[cdupont]

Hi guys,
I don't know how to test locally, but I looked at the test at https://github.com/telefonicaid/fiware-orion/blob/a2acb52cbd2cea606ec6d18d0e2dc7413944055e/test/functionalTest/cases/2893_case_insensitive_custom_notif_http_headers/case_insensitive_custom_notif_http_headers.test
It seems to cover my case.
So feel free to close the issue! Thanks a lot for the hard work!

[fgalan]

Ok. Closing. Thx!