-
Notifications
You must be signed in to change notification settings - Fork 0
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
Hardening/3109 url parse refactor #20
Changes from 2 commits
28c27c6
3c30f33
02d46a8
64983d1
7c82dd8
74c7a9c
124c24a
41ad1ac
eaa194d
a265c79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,19 +35,61 @@ | |
#include "common/string.h" | ||
#include "common/limits.h" | ||
#include "common/errorMessages.h" | ||
|
||
#include "alarmMgr/alarmMgr.h" | ||
#include "metricsMgr/metricsMgr.h" | ||
|
||
#include "ngsi/ParseData.h" | ||
#include "mongoBackend/mongoSubCache.h" | ||
#include "jsonParseV2/jsonRequestTreat.h" | ||
#include "parse/textParse.h" | ||
#include "serviceRoutines/badRequest.h" | ||
|
||
#include "rest/ConnectionInfo.h" | ||
#include "rest/OrionError.h" | ||
#include "rest/RestService.h" | ||
#include "rest/restReply.h" | ||
#include "rest/rest.h" | ||
#include "rest/uriParamNames.h" | ||
#include "mongoBackend/mongoSubCache.h" | ||
#include "rest/RestService.h" | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* service vectors - | ||
*/ | ||
static RestService* getServiceV = NULL; | ||
static RestService* putServiceV = NULL; | ||
static RestService* postServiceV = NULL; | ||
static RestService* patchServiceV = NULL; | ||
static RestService* deleteServiceV = NULL; | ||
static RestService* optionsServiceV = NULL; | ||
RestService* restBadVerbV = NULL; | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* serviceVectorsSet - only for unit tests | ||
*/ | ||
void serviceVectorsSet | ||
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. If only used in unit tests, then use 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. Sure 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. Actually, the function is used, by restInit() to set the vectors. NTC |
||
( | ||
RestService* _getServiceV, | ||
RestService* _putServiceV, | ||
RestService* _postServiceV, | ||
RestService* _patchServiceV, | ||
RestService* _deleteServiceV, | ||
RestService* _optionsServiceV, | ||
RestService* _restBadVerbV | ||
) | ||
{ | ||
getServiceV = _getServiceV; | ||
putServiceV = _putServiceV; | ||
postServiceV = _postServiceV; | ||
patchServiceV = _patchServiceV; | ||
deleteServiceV = _deleteServiceV; | ||
optionsServiceV = _optionsServiceV; | ||
restBadVerbV = _restBadVerbV; | ||
} | ||
|
||
|
||
|
||
|
@@ -445,7 +487,7 @@ static bool compErrorDetect | |
* | ||
* restService - | ||
*/ | ||
std::string restService(ConnectionInfo* ciP, RestService* serviceV) | ||
static std::string restService(ConnectionInfo* ciP, RestService* serviceV, const char* serviceVectorName) | ||
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. Where is the new parameter Maybe github diff is fooling me, but I cannot find any ocurrence in the diff page appart from this one... 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. Not easy to find this kind of things in the github jungle ...
If I'm not mistaken, I used the "serviceVectorName" fior debugging purposes. 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. The parameter is no longer used. 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. Fixed in 64983d1 |
||
{ | ||
std::vector<std::string> compV; | ||
int components; | ||
|
@@ -487,38 +529,36 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV) | |
// | ||
// Lookup the requested service | ||
// | ||
|
||
for (unsigned int ix = 0; serviceV[ix].treat != NULL; ++ix) | ||
{ | ||
if ((serviceV[ix].components != 0) && (serviceV[ix].components != components)) | ||
{ | ||
continue; | ||
} | ||
|
||
if ((ciP->method != serviceV[ix].verb) && (serviceV[ix].verb != "*")) | ||
{ | ||
continue; | ||
} | ||
|
||
strncpy(ciP->payloadWord, serviceV[ix].payloadWord.c_str(), sizeof(ciP->payloadWord)); | ||
bool match = true; | ||
for (int compNo = 0; compNo < components; ++compNo) | ||
{ | ||
if (serviceV[ix].compV[compNo] == "*") | ||
const char* component = serviceV[ix].compV[compNo].c_str(); | ||
|
||
if ((component[0] == '*') && (component[1] == 0)) | ||
{ | ||
continue; | ||
} | ||
|
||
if (ciP->apiVersion == V1) | ||
{ | ||
if (strcasecmp(serviceV[ix].compV[compNo].c_str(), compV[compNo].c_str()) != 0) | ||
if (strcasecmp(component, compV[compNo].c_str()) != 0) | ||
{ | ||
match = false; | ||
break; | ||
} | ||
} | ||
else | ||
{ | ||
if (strcmp(serviceV[ix].compV[compNo].c_str(), compV[compNo].c_str()) != 0) | ||
if (strcmp(component, compV[compNo].c_str()) != 0) | ||
{ | ||
match = false; | ||
break; | ||
|
@@ -531,7 +571,11 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV) | |
continue; | ||
} | ||
|
||
if ((ciP->payload != NULL) && (ciP->payloadSize != 0) && (ciP->payload[0] != 0) && (serviceV[ix].verb != "*")) | ||
|
||
// | ||
// If in restBadVerbV vector, no need to check the payload | ||
// | ||
if ((serviceV != restBadVerbV) && (ciP->payload != NULL) && (ciP->payloadSize != 0) && (ciP->payload[0] != 0)) | ||
{ | ||
std::string response; | ||
std::string spath = (ciP->servicePathV.size() > 0)? ciP->servicePathV[0] : ""; | ||
|
@@ -563,7 +607,7 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV) | |
} | ||
} | ||
|
||
LM_T(LmtService, ("Treating service %s %s", serviceV[ix].verb.c_str(), ciP->url.c_str())); // Sacred - used in 'heavyTest' | ||
LM_T(LmtService, ("Treating service %s %s", ciP->method.c_str(), ciP->url.c_str())); // Sacred - used in 'heavyTest' | ||
if (ciP->payloadSize == 0) | ||
{ | ||
ciP->inMimeType = NOMIMETYPE; | ||
|
@@ -610,12 +654,11 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV) | |
|
||
// | ||
// If we have gotten this far the Input is OK. | ||
// Except for all the badVerb/badRequest, etc. | ||
// A common factor for all these 'services' is that the verb is '*' | ||
// Except for all the badVerb/badRequest, in the restBadVerbV vector. | ||
// | ||
// So, the 'Bad Input' alarm is cleared for this client. | ||
// | ||
if (serviceV[ix].verb != "*") | ||
if (serviceV != restBadVerbV) | ||
{ | ||
alarmMgr.badInputReset(clientIp); | ||
} | ||
|
@@ -645,6 +688,18 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV) | |
return response; | ||
} | ||
|
||
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. Could you elaboratate on the purpose of the two if clauses, please? 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. Sure.
And, if the badVerb service vector is NULL, then:
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. Maybe a comment explaining a bit this recursive way of working would be useful (or maybe the comment is already there but I haven't seen). 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. Fixed in 74c7a9c |
||
if (restBadVerbV == NULL) | ||
{ | ||
std::vector<std::string> cV; | ||
|
||
return badRequest(ciP, 0, cV, NULL); | ||
} | ||
|
||
if (serviceV != restBadVerbV) | ||
{ | ||
return restService(ciP, restBadVerbV, "BAD VERB"); | ||
} | ||
|
||
std::string details = std::string("service '") + ciP->url + "' not recognized"; | ||
alarmMgr.badInput(clientIp, details); | ||
|
||
|
@@ -655,3 +710,20 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV) | |
compV.clear(); | ||
return answer; | ||
} | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* orionServe - | ||
*/ | ||
std::string orionServe(ConnectionInfo* ciP) | ||
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.
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. This function was static and called What the function does is to call The best name might be to override "Selector" doesn't do the job as the function both selects and executes ... "orionRestServe"? "orionRestExecute". I think "restService" is the best name, even though I can't say I like it too much ... 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. What about using a namespace, e.g. In fact, I think |
||
{ | ||
if ((ciP->verb == GET) && (getServiceV != NULL)) return restService(ciP, getServiceV, "GET"); | ||
else if ((ciP->verb == POST) && (postServiceV != NULL)) return restService(ciP, postServiceV, "POST"); | ||
else if ((ciP->verb == PUT) && (putServiceV != NULL)) return restService(ciP, putServiceV, "PUT"); | ||
else if ((ciP->verb == PATCH) && (patchServiceV != NULL)) return restService(ciP, patchServiceV, "PATCH"); | ||
else if ((ciP->verb == DELETE) && (deleteServiceV != NULL)) return restService(ciP, deleteServiceV, "DELETE"); | ||
else if ((ciP->verb == OPTIONS) && (optionsServiceV != NULL)) return restService(ciP, optionsServiceV, "OPTIONS"); | ||
else return restService(ciP, restBadVerbV, "BADVERB"); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,6 @@ typedef std::string (*RestServiceHandler)(ConnectionInfo* ciP, int compononts, s | |
typedef std::string (*RestTreat)(ConnectionInfo* ciP, int components, std::vector<std::string>& compV, ParseData* reqDataP); | ||
typedef struct RestService | ||
{ | ||
std::string verb; // The method of the service, as a plain string. ("*" matches ALL methods) | ||
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. You have take advantage of this PR to remove an long-time unused variable? Or the removal is related with the purpose of the PR? 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. Yes, this is the whole idea with the PR. 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. NTC |
||
RequestType request; // The type of the request | ||
int components; // Number of components in the URL path | ||
std::string compV[10]; // Vector of URL path components. E.g. { "v2", "entities" } | ||
|
@@ -61,14 +60,6 @@ typedef struct RestService | |
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* restService - | ||
*/ | ||
extern std::string restService(ConnectionInfo* ciP, RestService* serviceV); | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* payloadParse - | ||
|
@@ -91,4 +82,29 @@ extern std::string payloadParse | |
*/ | ||
extern std::string tenantCheck(const std::string& tenant); | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* serviceVectorsSet - only for unit tests | ||
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. It seems that at the end this function is not only used only for unit tests, as restInit() also uses it. Thus the "- only for unit tests" fragment should be removed. 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. (Same in the .ccp file) 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. Fixed in a265c79 |
||
*/ | ||
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. If only used in unit tests, then use 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. NTC |
||
extern void serviceVectorsSet | ||
( | ||
RestService* _getServiceV, | ||
RestService* _putServiceV, | ||
RestService* _postServiceV, | ||
RestService* _patchServiceV, | ||
RestService* _deleteServiceV, | ||
RestService* _optionsServiceV, | ||
RestService* _restBadVerbV | ||
); | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* orionServe - | ||
*/ | ||
extern std::string orionServe(ConnectionInfo* ciP); | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,7 +62,6 @@ | |
* | ||
* Globals | ||
*/ | ||
static RestService* restServiceV = NULL; | ||
static unsigned short port = 0; | ||
static RestServeFunction serveFunction = NULL; | ||
static char bindIp[MAX_LEN_IP] = "0.0.0.0"; | ||
|
@@ -555,17 +554,6 @@ static int httpHeaderGet(void* cbDataP, MHD_ValueKind kind, const char* ckey, co | |
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* serve - | ||
*/ | ||
static void serve(ConnectionInfo* ciP) | ||
{ | ||
restService(ciP, restServiceV); | ||
} | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* requestCompleted - | ||
|
@@ -1758,7 +1746,13 @@ static int restStart(IpVersion ipVersion, const char* httpsKey = NULL, const cha | |
*/ | ||
void restInit | ||
( | ||
RestService* _restServiceV, | ||
RestService* _getServiceV, | ||
RestService* _putServiceV, | ||
RestService* _postServiceV, | ||
RestService* _patchServiceV, | ||
RestService* _deleteServiceV, | ||
RestService* _optionsServiceV, | ||
RestService* _restBadVerbV, | ||
IpVersion _ipVersion, | ||
const char* _bindAddress, | ||
unsigned short _port, | ||
|
@@ -1779,10 +1773,11 @@ void restInit | |
const char* key = _httpsKey; | ||
const char* cert = _httpsCertificate; | ||
|
||
serviceVectorsSet(_getServiceV, _putServiceV, _postServiceV, _patchServiceV, _deleteServiceV, _optionsServiceV, _restBadVerbV); | ||
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. If only used in unit tests, then use However, as alternative, have you considered the possiblity of moving the initialization vectors to testInit.cpp and invoke restInit() from that code? 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. NTC |
||
|
||
port = _port; | ||
restServiceV = _restServiceV; | ||
ipVersionUsed = _ipVersion; | ||
serveFunction = (_serveFunction != NULL)? _serveFunction : serve; | ||
serveFunction = (_serveFunction != NULL)? _serveFunction : orionServe; | ||
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. I have looked for usages of restInit() and it seems that we neever use 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. This is old stuff from when orion was both ContextBroker and IotAgent and ConfiguratrionManager at the same time. 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. Ok. I'd suggest to mark it with 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. This has nothing to do with the current refactoring. 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. Name changed from 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. Next refactoring on my list is exactly this - move RestService vectors into rest lib. No FIXME needed ... Right? 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. Current situation is that there is an parameter, the one named But you have more neurons on this than me right now, so as you prefer... 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. Sure, I can fix that. Almost easier than putting it in the issue ... :) 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. Fixed in 124c24a (the variable serveFunction is removed as well - which makes us have to modify images ...) 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. Images modified in eaa194d |
||
multitenant = _multitenant; | ||
connMemory = _connectionMemory; | ||
maxConns = _maxConnections; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,14 +73,23 @@ extern bool multitenant; | |
extern bool corsEnabled; | ||
extern char corsOrigin[64]; | ||
extern int corsMaxAge; | ||
extern RestService* restBadVerbV; | ||
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. Why this addition to global variables? 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. I answered before. The vector for bad verbs is needed in RestService.cpp 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. NTC |
||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* RestServeFunction - | ||
*/ | ||
typedef void (*RestServeFunction)(ConnectionInfo* ciP); | ||
typedef std::string (*RestServeFunction)(ConnectionInfo* ciP); | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* serve - | ||
*/ | ||
extern std::string serve(ConnectionInfo* ciP); | ||
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. After the change from
to
Is the 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. The 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. Ok. Github diff is not always as smart as it should be with renamings... ;) 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. NTC |
||
|
||
|
||
|
||
|
@@ -90,7 +99,13 @@ typedef void (*RestServeFunction)(ConnectionInfo* ciP); | |
*/ | ||
extern void restInit | ||
( | ||
RestService* _restServiceV, | ||
RestService* _getServiceV, | ||
RestService* _putServiceV, | ||
RestService* _postServiceV, | ||
RestService* _patchServiceV, | ||
RestService* _deleteServiceV, | ||
RestService* _optionsServiceV, | ||
RestService* _noServiceV, | ||
IpVersion _ipVersion, | ||
const char* _bindAddress, | ||
unsigned short _port, | ||
|
@@ -133,4 +148,21 @@ extern void firstServicePath(const char* servicePath, char* servicePath0, int se | |
*/ | ||
extern bool isOriginAllowedForCORS(const std::string& requestOrigin); | ||
|
||
|
||
|
||
/* **************************************************************************** | ||
* | ||
* serviceVectorsSet - only for unit tests | ||
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. Repeated declaration? (in seems is also at RestService.h) 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. Fixed in a265c79 |
||
*/ | ||
extern void serviceVectorsSet | ||
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. If only used in unit tests, then use (Maybe I'm repeating myself too much... sorry :) 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. As mentioned, the comment is no longer valid. NTC |
||
( | ||
RestService* _getServiceV, | ||
RestService* _putServiceV, | ||
RestService* _postServiceV, | ||
RestService* _patchServiceV, | ||
RestService* _deleteServiceV, | ||
RestService* _optionsServiceV, | ||
RestService* _badVerbV | ||
); | ||
|
||
#endif |
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.
Why this one is not static? Why the name uses a different pattern (versus
badVerbServiceV
for instance)?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.
This vector is needed in another file (use grep to see which), and as it is not static, it needs a prefix.
As it is in the rest-library, I used the prefix "rest".
Perhaps
restBadVerbServiceV
would have been a better name. A bit long thoughThere 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.
In the next-coming refactoring, all RestService vectors will be moved to RestService.cpp or rest.cpp, and then perhaps all of them can be made static ...
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.
FIXME PR for this or NTC... As you prefer :)
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.
NTC (I am preparing a list of nextcoming PRs about this)