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

@@ -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)

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(() => {

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) {
+                const auto logic = to_logic(rhs);
-                }
+                if (logic >= 0) {
-                if (to_logic(rhs) >= 0) {
+                    stack.push_back(logic == 0 ? "1" : "0");
-                    stack.push_back(to_logic(rhs) == 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 <cond> ? <expr1> : <expr2>
 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 c1 = find_unquoted(expr_new, ':', q + 1);
-        auto q1 = expr_new.find_first_of('?', 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);
+            q1 = find_unquoted(expr_new, '?', q1 + 1);
-            c1 = expr_new.find_first_of(':', c1 + 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);

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<JsonObject>()) {
+            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<JsonObject>().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", "<br>\r\n");

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
+    // which computed the value before executing. Commands that compute their own argument are
-    // argument, so pre-computing it here would run it twice - pass it through raw.
+    // 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;
         }
     }

src/web/WebCommandService.h

@@ -85,6 +85,7 @@ class WebCommandService : public StatefulService<WebCommands> {
 
     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);
 

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);
 }