From f498ce39f3ab61c5d30ee1dcb418b3ab4d9c5652 Mon Sep 17 00:00:00 2001 From: Michael Friedrich Date: Mon, 28 Sep 2015 16:08:14 +0200 Subject: [PATCH] Improve api error handling refs #10194 --- lib/remote/actionshandler.cpp | 14 ++++----- lib/remote/apilistener-configsync.cpp | 12 ++------ lib/remote/configfileshandler.cpp | 41 ++++++++++++++------------- lib/remote/configfileshandler.hpp | 3 -- lib/remote/configpackageshandler.cpp | 13 ++++----- lib/remote/configstageshandler.cpp | 21 ++++++-------- lib/remote/createobjecthandler.cpp | 12 +++----- lib/remote/deleteobjecthandler.cpp | 13 +++------ lib/remote/httphandler.cpp | 4 +-- lib/remote/modifyobjecthandler.cpp | 13 ++++----- lib/remote/objectqueryhandler.cpp | 10 ++++--- lib/remote/statushandler.cpp | 13 ++++----- lib/remote/typequeryhandler.cpp | 12 ++++---- 13 files changed, 75 insertions(+), 106 deletions(-) diff --git a/lib/remote/actionshandler.cpp b/lib/remote/actionshandler.cpp index a9f580758..d7ac3b353 100644 --- a/lib/remote/actionshandler.cpp +++ b/lib/remote/actionshandler.cpp @@ -33,22 +33,18 @@ REGISTER_URLHANDLER("/v1/actions", ActionsHandler); bool ActionsHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - if (request.RequestMethod != "POST") { - HttpUtility::SendJsonError(response, 400, "Invalid request type. Must be POST."); - return true; - } + if (request.RequestUrl->GetPath().size() != 3) + return false; - if (request.RequestUrl->GetPath().size() < 3) { - HttpUtility::SendJsonError(response, 400, "Action is missing."); - return true; - } + if (request.RequestMethod != "POST") + return false; String actionName = request.RequestUrl->GetPath()[2]; ApiAction::Ptr action = ApiAction::GetByName(actionName); if (!action) { - HttpUtility::SendJsonError(response, 404, "Action '" + actionName + "' could not be found."); + HttpUtility::SendJsonError(response, 404, "Action '" + actionName + "' does not exist."); return true; } diff --git a/lib/remote/apilistener-configsync.cpp b/lib/remote/apilistener-configsync.cpp index e40fb2d18..3824b4cb4 100644 --- a/lib/remote/apilistener-configsync.cpp +++ b/lib/remote/apilistener-configsync.cpp @@ -44,10 +44,8 @@ void ApiListener::ConfigUpdateObjectHandler(const ConfigObject::Ptr& object, con { ApiListener::Ptr listener = ApiListener::GetInstance(); - if (!listener) { - Log(LogCritical, "ApiListener", "No instance available."); + if (!listener) return; - } if (object->IsActive()) { /* Sync object config */ @@ -66,10 +64,8 @@ Value ApiListener::ConfigUpdateObjectAPIHandler(const MessageOrigin::Ptr& origin /* check permissions */ ApiListener::Ptr listener = ApiListener::GetInstance(); - if (!listener) { - Log(LogCritical, "ApiListener", "No instance available."); + if (!listener) return Empty; - } if (!listener->GetAcceptConfig()) { Log(LogWarning, "ApiListener") @@ -174,10 +170,8 @@ Value ApiListener::ConfigDeleteObjectAPIHandler(const MessageOrigin::Ptr& origin /* check permissions */ ApiListener::Ptr listener = ApiListener::GetInstance(); - if (!listener) { - Log(LogCritical, "ApiListener", "No instance available."); + if (!listener) return Empty; - } if (!listener->GetAcceptConfig()) { Log(LogWarning, "ApiListener") diff --git a/lib/remote/configfileshandler.cpp b/lib/remote/configfileshandler.cpp index cda6a9628..5969e39fc 100644 --- a/lib/remote/configfileshandler.cpp +++ b/lib/remote/configfileshandler.cpp @@ -31,16 +31,9 @@ REGISTER_URLHANDLER("/v1/config/files", ConfigFilesHandler); bool ConfigFilesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - if (request.RequestMethod == "GET") - HandleGet(user, request, response); - else - HttpUtility::SendJsonError(response, 400, "Invalid request type. Must be GET."); + if (request.RequestMethod != "GET") + return false; - return true; -} - -void ConfigFilesHandler::HandleGet(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) -{ Dictionary::Ptr params = HttpUtility::FetchRequestParameters(request); const std::vector& urlPath = request.RequestUrl->GetPath(); @@ -61,21 +54,29 @@ void ConfigFilesHandler::HandleGet(const ApiUser::Ptr& user, HttpRequest& reques String packageName = HttpUtility::GetLastParameter(params, "package"); String stageName = HttpUtility::GetLastParameter(params, "stage"); - if (!ConfigPackageUtility::ValidateName(packageName)) - return HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist."); + if (!ConfigPackageUtility::ValidateName(packageName)) { + HttpUtility::SendJsonError(response, 400, "Invalid package name."); + return true; + } - if (!ConfigPackageUtility::ValidateName(stageName)) - return HttpUtility::SendJsonError(response, 404, "Stage is not valid or does not exist."); + if (!ConfigPackageUtility::ValidateName(stageName)) { + HttpUtility::SendJsonError(response, 400, "Invalid stage name."); + return true; + } String relativePath = HttpUtility::GetLastParameter(params, "path"); - if (ConfigPackageUtility::ContainsDotDot(relativePath)) - return HttpUtility::SendJsonError(response, 403, "Path contains '..' (not allowed)."); + if (ConfigPackageUtility::ContainsDotDot(relativePath)) { + HttpUtility::SendJsonError(response, 400, "Path contains '..' (not allowed)."); + return true; + } String path = ConfigPackageUtility::GetPackageDir() + "/" + packageName + "/" + stageName + "/" + relativePath; - if (!Utility::PathExists(path)) - return HttpUtility::SendJsonError(response, 404, "Path not found."); + if (!Utility::PathExists(path)) { + HttpUtility::SendJsonError(response, 404, "Path not found."); + return true; + } try { std::ifstream fp(path.CStr(), std::ifstream::in | std::ifstream::binary); @@ -86,9 +87,9 @@ void ConfigFilesHandler::HandleGet(const ApiUser::Ptr& user, HttpRequest& reques response.AddHeader("Content-Type", "application/octet-stream"); response.WriteBody(content.CStr(), content.GetLength()); } catch (const std::exception& ex) { - return HttpUtility::SendJsonError(response, 500, "Could not read file.", + HttpUtility::SendJsonError(response, 500, "Could not read file.", request.GetVerboseErrors() ? DiagnosticInformation(ex) : ""); } + + return true; } - - diff --git a/lib/remote/configfileshandler.hpp b/lib/remote/configfileshandler.hpp index 6c36a20b5..604a72d98 100644 --- a/lib/remote/configfileshandler.hpp +++ b/lib/remote/configfileshandler.hpp @@ -31,9 +31,6 @@ public: DECLARE_PTR_TYPEDEFS(ConfigFilesHandler); virtual bool HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) override; - -private: - void HandleGet(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response); }; } diff --git a/lib/remote/configpackageshandler.cpp b/lib/remote/configpackageshandler.cpp index 23a1fc579..d6906f639 100644 --- a/lib/remote/configpackageshandler.cpp +++ b/lib/remote/configpackageshandler.cpp @@ -30,11 +30,8 @@ REGISTER_URLHANDLER("/v1/config/packages", ConfigPackagesHandler); bool ConfigPackagesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - if (request.RequestUrl->GetPath().size() > 4) { - String path = boost::algorithm::join(request.RequestUrl->GetPath(), "/"); - HttpUtility::SendJsonError(response, 404, "The requested path is too long to match any config package requests"); - return true; - } + if (request.RequestUrl->GetPath().size() > 4) + return false; if (request.RequestMethod == "GET") HandleGet(user, request, response); @@ -43,7 +40,7 @@ bool ConfigPackagesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& else if (request.RequestMethod == "DELETE") HandleDelete(user, request, response); else - HttpUtility::SendJsonError(response, 400, "Invalid request type. Must be GET, POST or DELETE."); + return false; return true; } @@ -83,7 +80,7 @@ void ConfigPackagesHandler::HandlePost(const ApiUser::Ptr& user, HttpRequest& re String packageName = HttpUtility::GetLastParameter(params, "package"); if (!ConfigPackageUtility::ValidateName(packageName)) { - HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist."); + HttpUtility::SendJsonError(response, 400, "Invalid package name."); return; } @@ -121,7 +118,7 @@ void ConfigPackagesHandler::HandleDelete(const ApiUser::Ptr& user, HttpRequest& String packageName = HttpUtility::GetLastParameter(params, "package"); if (!ConfigPackageUtility::ValidateName(packageName)) { - HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist."); + HttpUtility::SendJsonError(response, 400, "Invalid package name."); return; } diff --git a/lib/remote/configstageshandler.cpp b/lib/remote/configstageshandler.cpp index 4a9d9e181..93f58fff0 100644 --- a/lib/remote/configstageshandler.cpp +++ b/lib/remote/configstageshandler.cpp @@ -32,11 +32,8 @@ REGISTER_URLHANDLER("/v1/config/stages", ConfigStagesHandler); bool ConfigStagesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - if (request.RequestUrl->GetPath().size() > 5) { - String path = boost::algorithm::join(request.RequestUrl->GetPath(), "/"); - HttpUtility::SendJsonError(response, 404, "The requested path is too long to match any config tag requests."); - return true; - } + if (request.RequestUrl->GetPath().size() > 5) + return false; if (request.RequestMethod == "GET") HandleGet(user, request, response); @@ -45,7 +42,7 @@ bool ConfigStagesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& r else if (request.RequestMethod == "DELETE") HandleDelete(user, request, response); else - HttpUtility::SendJsonError(response, 400, "Invalid request type. Must be GET, POST or DELETE."); + return false; return true; } @@ -66,10 +63,10 @@ void ConfigStagesHandler::HandleGet(const ApiUser::Ptr& user, HttpRequest& reque String stageName = HttpUtility::GetLastParameter(params, "stage"); if (!ConfigPackageUtility::ValidateName(packageName)) - return HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist."); + return HttpUtility::SendJsonError(response, 400, "Invalid package name."); if (!ConfigPackageUtility::ValidateName(stageName)) - return HttpUtility::SendJsonError(response, 404, "Stage is not valid or does not exist."); + return HttpUtility::SendJsonError(response, 400, "Invalid stage name."); Array::Ptr results = new Array(); @@ -104,7 +101,7 @@ void ConfigStagesHandler::HandlePost(const ApiUser::Ptr& user, HttpRequest& requ String packageName = HttpUtility::GetLastParameter(params, "package"); if (!ConfigPackageUtility::ValidateName(packageName)) - return HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist."); + return HttpUtility::SendJsonError(response, 400, "Invalid package name."); Dictionary::Ptr files = params->Get("files"); @@ -155,10 +152,10 @@ void ConfigStagesHandler::HandleDelete(const ApiUser::Ptr& user, HttpRequest& re String stageName = HttpUtility::GetLastParameter(params, "stage"); if (!ConfigPackageUtility::ValidateName(packageName)) - return HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist."); + return HttpUtility::SendJsonError(response, 400, "Invalid package name."); if (!ConfigPackageUtility::ValidateName(stageName)) - return HttpUtility::SendJsonError(response, 404, "Stage is not valid or does not exist."); + return HttpUtility::SendJsonError(response, 400, "Invalid stage name."); try { ConfigPackageUtility::DeleteStage(packageName, stageName); @@ -171,7 +168,7 @@ void ConfigStagesHandler::HandleDelete(const ApiUser::Ptr& user, HttpRequest& re Dictionary::Ptr result1 = new Dictionary(); result1->Set("code", 200); - result1->Set("status", "Stage deleted"); + result1->Set("status", "Stage deleted."); Array::Ptr results = new Array(); results->Add(result1); diff --git a/lib/remote/createobjecthandler.cpp b/lib/remote/createobjecthandler.cpp index 3e0770e39..b73b5f317 100644 --- a/lib/remote/createobjecthandler.cpp +++ b/lib/remote/createobjecthandler.cpp @@ -31,20 +31,16 @@ REGISTER_URLHANDLER("/v1/objects", CreateObjectHandler); bool CreateObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - if (request.RequestMethod != "PUT") { - /* there might be other request methods pending */ + if (request.RequestUrl->GetPath().size() != 4) return false; - } - if (request.RequestUrl->GetPath().size() < 4) { - HttpUtility::SendJsonError(response, 400, "Object name is missing."); - return true; - } + if (request.RequestMethod != "PUT") + return false; Type::Ptr type = FilterUtility::TypeFromPluralName(request.RequestUrl->GetPath()[2]); if (!type) { - HttpUtility::SendJsonError(response, 403, "Erroneous type was supplied."); + HttpUtility::SendJsonError(response, 400, "Invalid type specified."); return true; } diff --git a/lib/remote/deleteobjecthandler.cpp b/lib/remote/deleteobjecthandler.cpp index 8d4384672..10f30cd93 100644 --- a/lib/remote/deleteobjecthandler.cpp +++ b/lib/remote/deleteobjecthandler.cpp @@ -34,21 +34,16 @@ REGISTER_URLHANDLER("/v1/objects", DeleteObjectHandler); bool DeleteObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - if (request.RequestMethod != "DELETE") { - /* there might be other request methods pending */ + if (request.RequestUrl->GetPath().size() < 3 || request.RequestUrl->GetPath().size() > 4) return false; - } - if (request.RequestUrl->GetPath().size() < 3) { - String path = boost::algorithm::join(request.RequestUrl->GetPath(), "/"); - HttpUtility::SendJsonError(response, 404, "The requested path is too long to match any config tag requests."); - return true; - } + if (request.RequestMethod != "DELETE") + return false; Type::Ptr type = FilterUtility::TypeFromPluralName(request.RequestUrl->GetPath()[2]); if (!type) { - HttpUtility::SendJsonError(response, 400, "Erroneous type was supplied."); + HttpUtility::SendJsonError(response, 400, "Invalid type specified."); return true; } diff --git a/lib/remote/httphandler.cpp b/lib/remote/httphandler.cpp index 64da5d2ed..51e0ab477 100644 --- a/lib/remote/httphandler.cpp +++ b/lib/remote/httphandler.cpp @@ -103,8 +103,8 @@ void HttpHandler::ProcessRequest(const ApiUser::Ptr& user, HttpRequest& request, } if (!processed) { String path = boost::algorithm::join(request.RequestUrl->GetPath(), "/"); - HttpUtility::SendJsonError(response, 404, "The requested API '" + path + - "' could not be found. Please check it for common errors like spelling and consult the docs."); + HttpUtility::SendJsonError(response, 404, "The requested path '" + path + + "' could not be found."); return; } } diff --git a/lib/remote/modifyobjecthandler.cpp b/lib/remote/modifyobjecthandler.cpp index 12c8b343c..2c2ed5a8f 100644 --- a/lib/remote/modifyobjecthandler.cpp +++ b/lib/remote/modifyobjecthandler.cpp @@ -32,18 +32,18 @@ REGISTER_URLHANDLER("/v1/objects", ModifyObjectHandler); bool ModifyObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - if (request.RequestMethod != "POST") { - /* there might be other request methods pending */ + if (request.RequestUrl->GetPath().size() < 3 || request.RequestUrl->GetPath().size() > 4) return false; - } - if (request.RequestUrl->GetPath().size() < 3) + if (request.RequestMethod != "POST") return false; Type::Ptr type = FilterUtility::TypeFromPluralName(request.RequestUrl->GetPath()[1]); - if (!type) - return false; + if (!type) { + HttpUtility::SendJsonError(response, 400, "Invalid type specified."); + return true; + } QueryDescription qd; qd.Types.insert(type->GetName()); @@ -100,4 +100,3 @@ bool ModifyObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& r return true; } - diff --git a/lib/remote/objectqueryhandler.cpp b/lib/remote/objectqueryhandler.cpp index ae8276a19..7b1182f8a 100644 --- a/lib/remote/objectqueryhandler.cpp +++ b/lib/remote/objectqueryhandler.cpp @@ -32,16 +32,18 @@ REGISTER_URLHANDLER("/v1/objects", ObjectQueryHandler); bool ObjectQueryHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - if (request.RequestMethod != "GET") + if (request.RequestUrl->GetPath().size() < 3 || request.RequestUrl->GetPath().size() > 4) return false; - if (request.RequestUrl->GetPath().size() < 3) + if (request.RequestMethod != "GET") return false; Type::Ptr type = FilterUtility::TypeFromPluralName(request.RequestUrl->GetPath()[2]); - if (!type) - return false; + if (!type) { + HttpUtility::SendJsonError(response, 400, "Invalid type specified."); + return true; + } QueryDescription qd; qd.Types.insert(type->GetName()); diff --git a/lib/remote/statushandler.cpp b/lib/remote/statushandler.cpp index d49a69f7e..810acf61f 100644 --- a/lib/remote/statushandler.cpp +++ b/lib/remote/statushandler.cpp @@ -71,15 +71,11 @@ public: bool StatusHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - Dictionary::Ptr result = new Dictionary(); - - if (request.RequestMethod != "GET") { - response.SetStatus(400, "Bad request"); - result->Set("info", "Request must be type GET"); - HttpUtility::SendJsonBody(response, result); - return true; - } + if (request.RequestUrl->GetPath().size() > 3) + return false; + if (request.RequestMethod != "GET") + return false; QueryDescription qd; qd.Types.insert("Status"); @@ -97,6 +93,7 @@ bool StatusHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request Array::Ptr results = Array::FromVector(objs); + Dictionary::Ptr result = new Dictionary(); result->Set("results", results); response.SetStatus(200, "OK"); diff --git a/lib/remote/typequeryhandler.cpp b/lib/remote/typequeryhandler.cpp index 966323e81..c278177cb 100644 --- a/lib/remote/typequeryhandler.cpp +++ b/lib/remote/typequeryhandler.cpp @@ -77,14 +77,11 @@ public: bool TypeQueryHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) { - Dictionary::Ptr result = new Dictionary(); + if (request.RequestUrl->GetPath().size() > 3) + return false; - if (request.RequestMethod != "GET") { - response.SetStatus(400, "Bad request"); - result->Set("info", "Request must be type GET"); - HttpUtility::SendJsonBody(response, result); - return true; - } + if (request.RequestMethod != "GET") + return false; QueryDescription qd; qd.Types.insert("Type"); @@ -157,6 +154,7 @@ bool TypeQueryHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& requ } } + Dictionary::Ptr result = new Dictionary(); result->Set("results", results); response.SetStatus(200, "OK");