Hardening/3109 remove payload word part 2 #35

kzangeli · 2018-08-08T18:25:54Z

Removed an unnecessary lookup of rest service.

Split connectionTreat (MHD callback) in three functions

…d more stuff ...

…rent types of call from MHD

…hree parts (functions)

fgalan · 2018-08-08T19:24:04Z

To which step/stage of telefonicaid#3109 this PR corresponds? Step 1, stage 4 maybe (given the last implemented on was step 1, stage 3)?

fgalan · 2018-08-08T19:25:08Z

src/lib/common/string.cpp

@@ -162,7 +162,7 @@ bool getIPv6Port(const std::string& in, std::string& outIp, std::string& outPort
 *
 * stringSplit - 
 */
-int stringSplit(const std::string& in, char delimiter, std::vector<std::string>& outV)
+int stringSplit(const std::string& in, char delimiter, std::vector<std::string>& outV, bool skipLastComponentIfEmpty)


New parameter (skipLastComponentIfEmpty) is a bit obscure... An explantation (or example) in the function preamble would be great.

I thought the name was descriptive enough ...
It's about not understanding /xxx/ as two components, but one (skip last component if it is empty).
but sure, it's easy enough to comment this in the function header

Fixed in 4dfd0c0

fgalan · 2018-08-08T19:26:03Z

src/lib/jsonParse/jsonRequest.cpp

    std::string  errorReply;
    char         reqTypeV[STRING_SIZE_FOR_INT];

-    restErrorReplyGet(ciP, SccBadRequest, details, &errorReply);
+    restErrorReplyGet(ciP, SccBadRequest, "service not found", &errorReply);


Maybe "service not found" should be a #define? I remember we have a .h with the "magic strings" used in error messages.

Fixed in 4dfd0c0

kzangeli · 2018-08-08T19:26:41Z

this part was not even in the initial plan.
just something I "left" for later as it was far from trivial to implement all this, when I implemented the earlier search of the rest service.

About splitting the connectionTreat function in three separate functions ... that I believe might be an old issue

fgalan · 2018-08-08T19:27:48Z

src/lib/rest/RestService.cpp

@@ -69,7 +69,7 @@ RestService*                     restBadVerbV          = NULL;

 /* *****************************************************************************
 *
-* restServiceGet -
+* restServiceVectorGet -


This refactor is hard to follow... could you summarize the changes a little bit, pls?

Well, this very function only changes name.
the problem comes later, in the following function.

what is changed is that i no longer lookup the rest service in the rest Service vector, as that is already done, and saved in ciP->restserviceP.

the diff is really messy ... :(

fgalan · 2018-08-08T19:29:47Z

src/lib/rest/restServiceLookup.cpp

+    return restServiceLookup(ciP, badVerbP);
+  }
+
+  return &restBadVerbV[104];  // FIXME 02: 99.9% sure we never get here, but if ... Change the 104 service for a fixed one


From where the magic number 104 did come?

yeah, this is beyond ugly.
I am sure we never get there, as all possible errors are caught before.
That said, &restBadVerbV[104] is a pointer to the last restservice item in the badVerb vector, the one that takes care of any error, not yet found.

As the fixme says, should be changed for a fixed struct for this case.

fixme in place, not really in a hurry to implement it

Also, once the new way of getting the info for "bad verb" (end of the "really big" step 1 in issue telefonicaid#3109) this will go away

fgalan · 2018-08-08T19:30:27Z

test/functionalTest/cases/0876_entity_dates/entity_dates_overridden_by_user_subs.test

@@ -584,7 +584,7 @@ Fiware-Correlator: REGEX([0-9a-f\-]{36})
 =======================================
 POST http://localhost:REGEX(\d+)/notify
 Fiware-Servicepath: /
-Content-Length: 208
+Content-Length: REGEX((208|291))


Another weird change in Content-Length... This .test should be annnotated in the issue we created about it.

yes, this is one more for that strange thing that is happening ...

Note added in issue telefonicaid#3258

fgalan · 2018-08-08T19:30:39Z

test/unittests/CMakeLists.txt

@@ -203,6 +171,38 @@ SET (SOURCES
    rest/restReply_test.cpp
    rest/RestService_test.cpp
    rest/rest_test.cpp
+
+    # serviceRoutines/badVerbGetOnly_test.cpp


I undesrtand the test has been disabled maybe due to the effort it would take to adjust them. In the past you mentioned they should be removed... If so, let's do that in this PR, no need to wait any longer.

no, i decided to leave all those.
might do the effort to put all these unit tests back in, soon.
just not right now

fgalan · 2018-08-08T19:32:17Z

test/unittests/rest/RestService_test.cpp

@@ -136,7 +136,7 @@ TEST(RestService, payloadParse)
 *
 * noSuchService -
 */
-TEST(RestService, noSuchServiceAndNotFound)
+TEST(RestService, DISABLED_noSuchServiceAndNotFound)


Maybe the test should be enterily removed? Related with #35

sure, we can remove all of them, not that i care very much.
however ... is it really better to remove then than to keep them to perhaps some day (soon) incorporate again?
i decided to go conservative ...

Again, prefer to leave it.
some day we will reincorporate all these functests

So, NTC

kzangeli · 2018-08-08T19:36:52Z

And yes, we're still in step one. Stage 4 seems OK

fgalan · 2018-08-08T21:36:25Z

src/app/contextBroker/orionRestServices.cpp

@@ -510,7 +510,7 @@ static RestService badVerbV[] =
  { InvalidRequest,                                2, { "ngsi9",   "*"                                                                 }, badNgsi9Request           },
  { InvalidRequest,                                2, { "ngsi10",  "*"                                                                 }, badNgsi10Request          },
  { InvalidRequest,                                0, { "*", "*", "*", "*", "*", "*"                                                   }, badRequest                },
-  { InvalidRequest,                                0, {                                                                                }, NULL                      },


Does the refactor and changes done in this PR has any impact in Developers Manual?

No, we didn't get down to this level of details in the manual, fortunately ...

kzangeli added 8 commits August 1, 2018 18:06

Step 1 in splitting connectionTreat into yhree separate functions, an…

e725e77

…d more stuff ...

merged with origin

bea67d4

Fixed minor mistake

7436d12

Merge branch 'master' into hardening/3109_remove_payload_word_part_2

469d1cc

Split the function connectionTreat into different functions for diffe…

6097429

…rent types of call from MHD

Fixed a stupid mistake in last modification

12c2e64

Saving url component vector in ConnectionInfo

99c0356

removed an unnecessary lookup of service. Split connectiontrerat in t…

2dc1cee

…hree parts (functions)

fgalan reviewed Aug 8, 2018

View reviewed changes

kzangeli mentioned this pull request Aug 9, 2018

Strange payloads in notifications telefonicaid/fiware-orion#3258

Open

PR fixes

4dfd0c0

kzangeli merged commit b4b1018 into master Aug 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardening/3109 remove payload word part 2 #35

Hardening/3109 remove payload word part 2 #35

kzangeli commented Aug 8, 2018

fgalan commented Aug 8, 2018

fgalan Aug 8, 2018

kzangeli Aug 8, 2018

kzangeli Aug 9, 2018

fgalan Aug 8, 2018

kzangeli Aug 9, 2018

kzangeli commented Aug 8, 2018

fgalan Aug 8, 2018

kzangeli Aug 8, 2018

fgalan Aug 8, 2018

kzangeli Aug 8, 2018

kzangeli Aug 9, 2018 •

edited

Loading

fgalan Aug 8, 2018

kzangeli Aug 8, 2018

kzangeli Aug 9, 2018

fgalan Aug 8, 2018

fgalan Aug 8, 2018

kzangeli Aug 8, 2018

kzangeli Aug 9, 2018

fgalan Aug 8, 2018

kzangeli Aug 8, 2018

kzangeli Aug 9, 2018

kzangeli commented Aug 8, 2018

fgalan Aug 8, 2018

kzangeli Aug 9, 2018

Hardening/3109 remove payload word part 2 #35

Hardening/3109 remove payload word part 2 #35

Conversation

kzangeli commented Aug 8, 2018

fgalan commented Aug 8, 2018

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

kzangeli commented Aug 8, 2018

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

kzangeli Aug 9, 2018 • 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

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

kzangeli commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli Aug 9, 2018 •

edited

Loading