From 84d67c0946a9469a2f3fb478a37cba63e05e419a Mon Sep 17 00:00:00 2001 From: proddy Date: Sun, 30 Aug 2026 14:42:42 +0200 Subject: [PATCH] dont run the shunting-yard twice on sendmail values. Sending literal text through `system/sendmail` logged a warning and dropped the computed body --- CHANGELOG_LATEST.md | 2 + .../src/app/settings/ApplicationSettings.tsx | 2 +- src/core/shuntingYard.cpp | 40 ++++++++++++++----- src/core/system.cpp | 38 +++++++++--------- src/web/WebCommandService.cpp | 15 +++++-- src/web/WebCommandService.h | 1 + test/test_api/test_shuntingYard.h | 30 ++++++++++++++ 7 files changed, 94 insertions(+), 34 deletions(-) diff --git a/CHANGELOG_LATEST.md b/CHANGELOG_LATEST.md index 41ca8ec4e..d2deaaf3f 100644 --- a/CHANGELOG_LATEST.md +++ b/CHANGELOG_LATEST.md @@ -16,6 +16,8 @@ This release is based on the latest Espressif/Arduino core version 3. It brings ## Fixed - shunting yard show json +- `system/sendmail` called from a Command or the Scheduler computed its value twice, which stripped the quotes from literal text and then failed on characters like `!` +- shunting yard treated a `?` inside a quoted string as a ternary, so text like `"too hot?"` returned nothing - memory leak when using different timezones for ems-esp and thermostat[#3184](https://github.com/emsesp/EMS-ESP32/issues/3184) - LED stayed off on a healthy system when "Disable LED" was unchecked - Ethernet MAC address changed with the new SDK, breaking DHCP reservations (currently disabled) diff --git a/interface/src/app/settings/ApplicationSettings.tsx b/interface/src/app/settings/ApplicationSettings.tsx index 15cd5770b..d9626d41b 100644 --- a/interface/src/app/settings/ApplicationSettings.tsx +++ b/interface/src/app/settings/ApplicationSettings.tsx @@ -177,7 +177,7 @@ const ApplicationSettings = () => { await sendAPI({ device: 'system', cmd: 'sendmail', - data: 'Email notification test successful!', + data: '"Email notification test successful!"', id: 0 }) .then(() => { diff --git a/src/core/shuntingYard.cpp b/src/core/shuntingYard.cpp index 202cf1e09..a60fc77a6 100644 --- a/src/core/shuntingYard.cpp +++ b/src/core/shuntingYard.cpp @@ -479,14 +479,14 @@ std::string calculate(const std::string & expr) { const auto rhs = stack.back(); stack.pop_back(); if (token.str[0] == '!') { - if (to_logic(rhs) < 0) { - } - if (to_logic(rhs) >= 0) { - stack.push_back(to_logic(rhs) == 0 ? "1" : "0"); + const auto logic = to_logic(rhs); + if (logic >= 0) { + stack.push_back(logic == 0 ? "1" : "0"); } else if (isnum(rhs)) { stack.push_back(std::stod(rhs) == 0 ? "1" : "0"); } else { - EMSESP::logger().warning("missing operator"); + // usually literal text that was left unquoted, so its '!' got read as a logical NOT + EMSESP::logger().warning("'!' needs a boolean or numeric operand, got '%s'. Literal text must be quoted", rhs.c_str()); return ""; } break; @@ -689,6 +689,24 @@ std::string calculate(const std::string & expr) { return result; } +// find the next occurrence of c at or after `from` that is not inside a quoted string, so that +// literal text like "too hot?" is not mistaken for a ternary. Always scans from the start of the +// string because the quote state at `from` depends on everything before it. +static size_t find_unquoted(const std::string & s, char c, size_t from = 0) { + bool in_single = false; + bool in_double = false; + for (size_t i = 0; i < s.length(); i++) { + if (!in_single && s[i] == '"') { + in_double = !in_double; + } else if (!in_double && s[i] == '\'') { + in_single = !in_single; + } else if (!in_single && !in_double && s[i] == c && i >= from) { + return i; + } + } + return std::string::npos; +} + // check for multiple instances of ? : std::string compute(const std::string & expr) { std::string expr_new = expr; @@ -772,14 +790,14 @@ std::string compute(const std::string & expr) { } // positions: q-questionmark, c-colon - auto q = expr_new.find_first_of('?'); + auto q = find_unquoted(expr_new, '?'); while (q != std::string::npos) { // find corresponding colon - auto c1 = expr_new.find_first_of(':', q + 1); - auto q1 = expr_new.find_first_of('?', q + 1); + auto c1 = find_unquoted(expr_new, ':', q + 1); + auto q1 = find_unquoted(expr_new, '?', q + 1); while (q1 < c1 && q1 != std::string::npos && c1 != std::string::npos) { - q1 = expr_new.find_first_of('?', q1 + 1); - c1 = expr_new.find_first_of(':', c1 + 1); + q1 = find_unquoted(expr_new, '?', q1 + 1); + c1 = find_unquoted(expr_new, ':', c1 + 1); } if (c1 == std::string::npos) { return ""; // error: missing colon @@ -810,7 +828,7 @@ std::string compute(const std::string & expr) { } else { return ""; // error } - q = expr_new.find_first_of('?'); // search next instance + q = find_unquoted(expr_new, '?'); // search next instance } return calculate(expr_new); diff --git a/src/core/system.cpp b/src/core/system.cpp index d9bd8e328..8a89ed4b8 100644 --- a/src/core/system.cpp +++ b/src/core/system.cpp @@ -154,6 +154,26 @@ bool System::command_sendmail(const char * value, const int8_t) { : " (plain)", value); + // Resolve the message before opening the SMTP session: compute() can do entity lookups and a + // blocking {url} fetch, which would otherwise leave the connection idle long enough to time out. + // The value is either a plain body or a JSON envelope overriding the configured subject/to/from. + JsonDocument doc(PSRAM_DOC); + String body = value; + if (body.length()) { + auto error = deserializeJson(doc, (const char *)value); + if (!error && doc.is()) { + subject = doc["subject"] | subject; + recp = doc["to"] | recp; + sender = doc["from"] | sender; + body = doc["body"] | body; + } + } + // keep the original body if the calculator returns nothing, so unquoted literal text still gets sent + std::string computed_body = compute(body.c_str()); + if (!computed_body.empty()) { + body = computed_body.c_str(); + } + bool success = false; #ifndef EMSESP_STANDALONE @@ -213,18 +233,6 @@ bool System::command_sendmail(const char * value, const int8_t) { return false; } } - JsonDocument doc(PSRAM_DOC); - String body = value; - if (body.length()) { - auto error = deserializeJson(doc, (const char *)value); - if (!error && doc.as().size() >= 0) { - subject = doc["subject"] | subject; - recp = doc["to"] | recp; - sender = doc["from"] | sender; - body = doc["body"] | body; - } - } - SMTPMessage & msg = smtp->getMessage(); msg.headers.add(rfc822_subject, subject); msg.headers.add(rfc822_from, sender); @@ -234,12 +242,6 @@ bool System::command_sendmail(const char * value, const int8_t) { // msg.headers.addCustom("Importance", PRIORITY); // msg.headers.addCustom("X-MSMail-Priority", PRIORITY); // msg.headers.addCustom("X-Priority", PRIORITY_NUM); - // run the body through the Shunting Yard calculator (entity substitution, expressions, optional {url} fetch) - // keep the original body if the calculator returns nothing - std::string computed_body = compute(body.c_str()); - if (!computed_body.empty()) { - body = computed_body.c_str(); - } msg.text.body(body); // bodyText.replace("\r\n", "
\r\n"); diff --git a/src/web/WebCommandService.cpp b/src/web/WebCommandService.cpp index 35350599d..646a56764 100644 --- a/src/web/WebCommandService.cpp +++ b/src/web/WebCommandService.cpp @@ -100,6 +100,13 @@ bool WebCommandService::isUrlCommand(const std::string & command) { return lower_url.starts_with("http://") || lower_url.starts_with("https://"); } +// true if the command runs the shunting-yard on its own argument. Those values must be passed +// through raw: a first pass strips the quotes from literal text, so the command's own pass then +// re-parses that text as an expression and chokes on characters like '!' or drops the spaces. +bool WebCommandService::computesOwnValue(const std::string & cmd) { + return cmd == "system/message" || cmd == "system/sendmail"; +} + // true if a value expression contains an embedded {"url":...} JSON snippet, which compute() // will resolve with a blocking HTTP request. Mirrors the scan compute() does in shuntingYard.cpp bool WebCommandService::valueContainsUrl(const std::string & value) { @@ -239,13 +246,13 @@ bool WebCommandService::executeCommand(const char * name, const std::string & co // run the value through the shunting-yard calculator so expressions like "custom/heatcnt + 1" // are resolved (entity references replaced by their values, then computed). Plain values pass // through unchanged. Applies to both URL and internal commands, like the old scheduler code - // which computed the value before executing. system/message runs the shunting-yard on its own - // argument, so pre-computing it here would run it twice - pass it through raw. + // which computed the value before executing. Commands that compute their own argument are + // skipped here so the value is only ever evaluated once. std::string computed_data = data; - if (!data.empty() && cmd != "system/message") { + if (!data.empty() && !computesOwnValue(cmd)) { computed_data = compute(data); if (computed_data.empty()) { - EMSESP::logger().warning("Command '%s': cannot compute value '%s'", name, data.c_str()); + EMSESP::logger().warning("Command '%s': cannot compute value '%s'. Literal text must be quoted", name, data.c_str()); return false; } } diff --git a/src/web/WebCommandService.h b/src/web/WebCommandService.h index 41ef54f67..c4a8cefc0 100644 --- a/src/web/WebCommandService.h +++ b/src/web/WebCommandService.h @@ -85,6 +85,7 @@ class WebCommandService : public StatefulService { static bool isUrlCommand(const std::string & command); // true if the command definition is a HTTP/URL command static bool valueContainsUrl(const std::string & value); // true if a value embeds a {"url":...} compute() will fetch + static bool computesOwnValue(const std::string & cmd); // true if the command runs the shunting-yard itself const CommandItem * find(const char * name); diff --git a/test/test_api/test_shuntingYard.h b/test/test_api/test_shuntingYard.h index cce84bc6f..902368d40 100644 --- a/test/test_api/test_shuntingYard.h +++ b/test/test_api/test_shuntingYard.h @@ -120,6 +120,31 @@ void shuntingYard_test28() { run_shuntingYard_test("", "-x"); } +// quoted text keeps its spaces and punctuation, unquoted text does not +void shuntingYard_test29() { + run_shuntingYard_test("email test!", "\"email test!\""); +} + +// a trailing '!' on unquoted text is read as a logical NOT and has no valid operand +void shuntingYard_test30() { + run_shuntingYard_test("", "email test!"); +} + +// computing an already computed value is lossy - callers must run compute() only once +void shuntingYard_test31() { + run_shuntingYard_test("", emsesp::compute("\"email test!\"")); +} + +// a '?' inside quotes is literal text, not a ternary +void shuntingYard_test32() { + run_shuntingYard_test("Is it hot?", "\"Is it hot?\""); +} + +// mixing quoted text with entity values keeps the punctuation of both parts +void shuntingYard_test33() { + run_shuntingYard_test("temp is 40 C!", "\"temp is \" + boiler/flowtempoffset + \" C!\""); +} + void run_shuntingYard_tests() { RUN_TEST(shuntingYard_test1); RUN_TEST(shuntingYard_test2); @@ -149,4 +174,9 @@ void run_shuntingYard_tests() { RUN_TEST(shuntingYard_test26); RUN_TEST(shuntingYard_test27); RUN_TEST(shuntingYard_test28); + RUN_TEST(shuntingYard_test29); + RUN_TEST(shuntingYard_test30); + RUN_TEST(shuntingYard_test31); + RUN_TEST(shuntingYard_test32); + RUN_TEST(shuntingYard_test33); }