From 5f2a9b093d3277520bcf618da3b6e1bcdcfa8761 Mon Sep 17 00:00:00 2001 From: proddy Date: Wed, 10 Nov 2021 12:26:29 +0100 Subject: [PATCH] improve error handling, fix crash on empty /api URL --- src/command.cpp | 56 ++++++---------- src/command.h | 8 ++- src/test/test.cpp | 160 ++++++++++++++++++++++++---------------------- 3 files changed, 112 insertions(+), 112 deletions(-) diff --git a/src/command.cpp b/src/command.cpp index 2cf2b8739..db7cf51ce 100644 --- a/src/command.cpp +++ b/src/command.cpp @@ -35,9 +35,7 @@ uint8_t Command::process(const char * path, const bool is_admin, const JsonObjec p.parse(path); if (!p.paths().size()) { - output.clear(); - output["message"] = "invalid path"; // path is empty - return CommandRet::ERROR; + return message(CommandRet::ERROR, "invalid path", output); } // check first if it's from API, if so strip the "api/" @@ -50,16 +48,19 @@ uint8_t Command::process(const char * path, const bool is_admin, const JsonObjec strncpy(new_path, path, sizeof(new_path)); p.parse(new_path + Mqtt::base().length() + 1); // re-parse the stripped path } else { - // error - output.clear(); - output["message"] = "unrecognized path"; - return CommandRet::ERROR; + return message(CommandRet::ERROR, "unrecognized path", output); // error } } // Serial.println(p.path().c_str()); // dump paths, for debugging - size_t num_paths = p.paths().size(); // re-calculate new path + // re-calculate new path + // if there is only a path (URL) and no body then error! + size_t num_paths = p.paths().size(); + if (!num_paths && !input.size()) { + return message(CommandRet::ERROR, "missing command in path", output); + } + std::string cmd_s; int8_t id_n = -1; // default hc @@ -79,11 +80,8 @@ uint8_t Command::process(const char * path, const bool is_admin, const JsonObjec // validate the device, make sure it exists uint8_t device_type = EMSdevice::device_name_2_device_type(device_s); if (!device_has_commands(device_type)) { - output.clear(); - char error[100]; - snprintf(error, sizeof(error), "no device called %s", device_s); - output["message"] = error; - return CommandRet::NOT_FOUND; + LOG_DEBUG(F("Command failed: unknown device '%s'"), device_s); + return message(CommandRet::ERROR, "unknown device", output); } // the next value on the path should be the command @@ -117,9 +115,7 @@ uint8_t Command::process(const char * path, const bool is_admin, const JsonObjec command_p = "values"; } } else { - output.clear(); - output["message"] = "missing or bad command"; - return CommandRet::NOT_FOUND; + return message(CommandRet::NOT_FOUND, "missing or bad command", output); } } @@ -154,15 +150,10 @@ uint8_t Command::process(const char * path, const bool is_admin, const JsonObjec char data_str[10]; return_code = Command::call(device_type, command_p, Helpers::render_value(data_str, (float)data.as(), 2), is_admin, id_n, output); } else if (data.isNull()) { - // empty - return_code = Command::call(device_type, command_p, "", is_admin, id_n, output); + return_code = Command::call(device_type, command_p, "", is_admin, id_n, output); // empty } else { - // can't process - output.clear(); - output["message"] = "cannot parse command"; - return CommandRet::ERROR; + return message(CommandRet::ERROR, "cannot parse command", output); // can't process } - return return_code; } @@ -175,10 +166,10 @@ const std::string Command::return_code_string(const uint8_t return_code) { return read_flash_string(F("OK")); break; case CommandRet::NOT_FOUND: - return read_flash_string(F("Not found")); + return read_flash_string(F("Not Found")); break; case CommandRet::NOT_ALLOWED: - return read_flash_string(F("Not authorized")); + return read_flash_string(F("Not Authorized")); break; case CommandRet::FAIL: return read_flash_string(F("Failed")); @@ -289,18 +280,15 @@ uint8_t Command::call(const uint8_t device_type, const char * cmd, const char * // report error if call failed if (return_code != CommandRet::OK) { - output.clear(); - output["message"] = "callback function failed"; + return message(return_code, "callback function failed", output); } return return_code; } // we didn't find the command and its not an endpoint, report error - char error[100]; - snprintf(error, sizeof(error), "invalid command '%s'", cmd); - output["message"] = error; - return CommandRet::NOT_FOUND; + LOG_DEBUG(F("Command failed: invalid command '%s'"), cmd); + return message(CommandRet::NOT_FOUND, "invalid command", output); } // add a command to the list, which does not return json @@ -525,10 +513,8 @@ void Command::show_all(uuid::console::Shell & shell) { } } -/** - * Extract only the path component from the passed URI and normalized it. - * Ex. //one/two////three/// becomes /one/two/three - */ +// Extract only the path component from the passed URI and normalized it +// e.g. //one/two////three/// becomes /one/two/three std::string SUrlParser::path() { std::string s = "/"; // set up the beginning slash for (std::string & f : m_folders) { diff --git a/src/command.h b/src/command.h index edc62f838..3598684f9 100644 --- a/src/command.h +++ b/src/command.h @@ -139,7 +139,13 @@ class Command { private: static uuid::log::Logger logger_; - static std::vector cmdfunctions_; // list of commands + static std::vector cmdfunctions_; // the list of commands + + inline static uint8_t message(uint8_t error_code, const char * message, JsonObject & output) { + output.clear(); + output["message"] = (const char *)message; + return error_code; + } }; typedef std::unordered_map KeyValueMap_t; diff --git a/src/test/test.cpp b/src/test/test.cpp index 1e70ea8c7..3c3095847 100644 --- a/src/test/test.cpp +++ b/src/test/test.cpp @@ -486,96 +486,113 @@ void Test::run_test(uuid::console::Shell & shell, const std::string & cmd) { #if defined(EMSESP_STANDALONE) - AsyncWebServerRequest request2; - request2.method(HTTP_GET); + AsyncWebServerRequest requestX; + DynamicJsonDocument docX(2000); + JsonVariant jsonX; - // request2.url("/api/thermostat/seltemp"); - // EMSESP::webAPIService.webAPIService_get(&request2); - // return; + requestX.method(HTTP_GET); /* - request2.url("/api/thermostat/mode/auto"); - EMSESP::webAPIService.webAPIService_get(&request2); + requestX.url("/api"); // should fail + EMSESP::webAPIService.webAPIService_get(&requestX); return; */ /* - request2.url("/api/thermostat"); // check if defaults to info - EMSESP::webAPIService.webAPIService_get(&request2); - request2.url("/api/thermostat/info"); - EMSESP::webAPIService.webAPIService_get(&request2); - request2.url("/api/thermostat/values"); - EMSESP::webAPIService.webAPIService_get(&request2); - return; - - request2.url("/api/thermostat/mode"); - EMSESP::webAPIService.webAPIService_get(&request2); + requestX.url("/api/thermostat/seltemp"); + EMSESP::webAPIService.webAPIService_get(&requestX); return; */ /* - request2.url("/api/system"); // check if defaults to info - EMSESP::webAPIService.webAPIService_get(&request2); - emsesp::EMSESP::logger().notice("*"); - - request2.url("/api/system/info"); - EMSESP::webAPIService.webAPIService_get(&request2); - emsesp::EMSESP::logger().notice("*"); - - request2.url("/api/thermostat"); // check if defaults to values - EMSESP::webAPIService.webAPIService_get(&request2); - emsesp::EMSESP::logger().notice("*"); - - request2.url("/api/thermostat/info"); - EMSESP::webAPIService.webAPIService_get(&request2); - emsesp::EMSESP::logger().notice("*"); - - request2.url("/api/thermostat/seltemp"); - EMSESP::webAPIService.webAPIService_get(&request2); - return; - - */ - - /* - request2.url("/api/system/restart"); - EMSESP::webAPIService.webAPIService_get(&request2); + requestX.url("/api/thermostat/mode/auto"); + EMSESP::webAPIService.webAPIService_get(&requestX); return; */ /* - request2.url("/api/dallassensor/fdfd"); - EMSESP::webAPIService.webAPIService_get(&request2); + requestX.url("/api/thermostat"); // check if defaults to info + EMSESP::webAPIService.webAPIService_get(&requestX); + requestX.url("/api/thermostat/info"); + EMSESP::webAPIService.webAPIService_get(&requestX); + requestX.url("/api/thermostat/values"); + EMSESP::webAPIService.webAPIService_get(&requestX); + return; + + requestX.url("/api/thermostat/mode"); + EMSESP::webAPIService.webAPIService_get(&requestX); + return; + */ + + /* + requestX.url("/api/system"); // check if defaults to info + EMSESP::webAPIService.webAPIService_get(&requestX); + emsesp::EMSESP::logger().notice("*"); + + requestX.url("/api/system/info"); + EMSESP::webAPIService.webAPIService_get(&requestX); + emsesp::EMSESP::logger().notice("*"); + + requestX.url("/api/thermostat"); // check if defaults to values + EMSESP::webAPIService.webAPIService_get(&requestX); + emsesp::EMSESP::logger().notice("*"); + + requestX.url("/api/thermostat/info"); + EMSESP::webAPIService.webAPIService_get(&requestX); + emsesp::EMSESP::logger().notice("*"); + + requestX.url("/api/thermostat/seltemp"); + EMSESP::webAPIService.webAPIService_get(&requestX); + return; + */ + + /* + requestX.url("/api/system/restart"); + EMSESP::webAPIService.webAPIService_get(&requestX); + return; + */ + + /* + requestX.url("/api/dallassensor/xxxx"); + EMSESP::webAPIService.webAPIService_get(&requestX); emsesp::EMSESP::logger().notice("****"); - request2.url("/api/dallassensor/info"); - EMSESP::webAPIService.webAPIService_get(&request2); + requestX.url("/api/dallassensor/info"); + EMSESP::webAPIService.webAPIService_get(&requestX); + return; + */ + + /* + requestX.url("/api"); // should fail + EMSESP::webAPIService.webAPIService_get(&requestX); + + requestX.method(HTTP_POST); + + char dataX[] = "{\"device\":\"thermostat\", \"entity\":\"seltemp\",\"value\":13}"; + deserializeJson(docX, dataX); + jsonX = docX.as(); + requestX.url("/api"); + EMSESP::webAPIService.webAPIService_post(&requestX, jsonX); return; */ /* - AsyncWebServerRequest request2; - request2.method(HTTP_POST); - DynamicJsonDocument docX(2000); - JsonVariant jsonX; // char dataX[] = "{\"value\":\"0B 88 19 19 02\"}"; char dataX[] = "{\"name\":\"temp\",\"value\":11}"; deserializeJson(docX, dataX); jsonX = docX.as(); - // request2.url("/api/system/send"); - request2.url("/api/thermostat"); - EMSESP::webAPIService.webAPIService_post(&request2, jsonX); + // requestX.url("/api/system/send"); + requestX.url("/api/thermostat"); + EMSESP::webAPIService.webAPIService_post(&requestX, jsonX); return; */ /* - AsyncWebServerRequest request3; - request3.method(HTTP_POST); - DynamicJsonDocument docX(2000); - JsonVariant jsonX; char dataX[] = "{}"; deserializeJson(docX, dataX); jsonX = docX.as(); - request3.url("/api/thermostat/mode/auto"); // should fail - EMSESP::webAPIService.webAPIService_post(&request3, jsonX); + requestX.url("/api/thermostat/mode/auto"); // should fail + EMSESP::webAPIService.webAPIService_post(&requestX, jsonX); + return; */ // test command parse @@ -618,26 +635,17 @@ void Test::run_test(uuid::console::Shell & shell, const std::string & cmd) { EMSESP::mqtt_.incoming("ems-esp/thermostat_hc1", "22"); // HA only EMSESP::mqtt_.incoming("ems-esp/thermostat_hc1", "off"); // HA only EMSESP::mqtt_.incoming("ems-esp/system/send", "11 12 13"); + EMSESP::mqtt_.incoming("ems-esp/boiler/syspress"); // empty payload, sends reponse + EMSESP::mqtt_.incoming("ems-esp/thermostat/mode"); // empty payload, sends reponse + EMSESP::mqtt_.incoming("ems-esp/system/publish"); + EMSESP::mqtt_.incoming("ems-esp/thermostat/seltemp"); // empty payload, sends reponse // MQTT bad tests EMSESP::mqtt_.incoming("ems-esp/thermostate/mode", "auto"); // unknown device EMSESP::mqtt_.incoming("ems-esp/thermostat/modee", "auto"); // unknown command - EMSESP::mqtt_.incoming("ems-esp/thermostat/mode/auto", "auto"); // invalid + EMSESP::mqtt_.incoming("ems-esp/thermostat/mode/auto", "auto"); // invalid, not allowed - // various MQTT checks - /* - EMSESP::mqtt_.incoming("ems-esp/thermostat/mode"); // empty payload, sends reponse - EMSESP::mqtt_.incoming("ems-esp/boiler/syspress"); // empty payload, sends reponse - EMSESP::mqtt_.incoming("ems-esp/thermostat/mode", "auto"); // set mode - EMSESP::mqtt_.incoming("ems-esp/thermostat/mode"); // empty payload, sends reponse - EMSESP::mqtt_.incoming("ems-esp/system/send", "11 12 13"); - EMSESP::mqtt_.incoming("ems-esp/system/publish"); - EMSESP::mqtt_.incoming("ems-esp/thermostat/seltemp"); // empty payload, sends reponse - EMSESP::mqtt_.incoming("ems-esp/system/send", "11 12 13"); - return; - */ - - // check long base + // check extended MQTT base Mqtt::base("home/cellar/heating"); EMSESP::mqtt_.incoming("home/cellar/heating/thermostat/mode"); // empty payload, sends reponse @@ -683,7 +691,7 @@ void Test::run_test(uuid::console::Shell & shell, const std::string & cmd) { EMSESP::webAPIService.webAPIService_post(&request, json); // 3 - char data3[] = "{\"device\":\"thermostat\", \"name\":\"temp\",\"value\":13}"; + char data3[] = "{\"device\":\"thermostat\", \"name\":\"seltemp\",\"value\":13}"; deserializeJson(doc, data3); json = doc.as(); request.url("/api"); @@ -1135,7 +1143,7 @@ void Test::run_test(uuid::console::Shell & shell, const std::string & cmd) { EMSESP::mqtt_.incoming(thermostat_topic, "{\"cmd\":\"wwmode\",\"data\":\"auto\"}"); EMSESP::mqtt_.incoming(thermostat_topic, "{\"cmd\":\"control\",\"data\":\"1\"}"); // RC35 only, should error - EMSESP::mqtt_.incoming(thermostat_topic, "{\"cmd\":\"mode\",\"data\":\"poep\",\"id\":2}"); // invalid mode + EMSESP::mqtt_.incoming(thermostat_topic, "{\"cmd\":\"mode\",\"data\":\"typo\",\"id\":2}"); // invalid mode EMSESP::mqtt_.incoming(thermostat_topic, "{\"cmd\":\"mode\",\"data\":\"auto\",\"id\":2}"); EMSESP::mqtt_.incoming(thermostat_topic, "{\"cmd\":\"mode\",\"data\":\"auto\",\"hc\":2}"); // hc as number EMSESP::mqtt_.incoming(thermostat_topic, "{\"cmd\":\"temp\",\"data\":19.5,\"hc\":1}"); // data as number