Merge pull request #3306 from csutcliff/pr/upload-406-response

fix(upload): send the 406 response for a rejected upload

CHANGELOG_LATEST.md

@@ -33,6 +33,8 @@ This release is based on the latest Espressif/Arduino core version 3. It brings
 ## Fixed
 
 - restart from the WebUI (Restart button, firmware/settings upload) only happened while the browser tab stayed open. If the tab was closed the restart was silently dropped after 5 minutes, leaving a new firmware uninstalled. EMS-ESP now restarts on its own if the WebUI hasn't picked up the pending restart within 10 seconds
+- a firmware upload from a script or `curl` was accepted (`md5_ok`) but EMS-ESP didn't restart into it: the client closing the connection after the upload was treated as an aborted upload, which cancelled the pending restart
+- EMS-ESP crashed and restarted when an upload was rejected, for example an empty or invalid MD5 file. The error code was kept in the request's `_tempObject`, which the web server frees as heap memory when the request ends
 - 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 `!`. Also 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)
@@ -58,6 +60,7 @@ This release is based on the latest Espressif/Arduino core version 3. It brings
 - Guru Meditation (cache error) on HybridCompile builds when the EMS or console UART ISR ran from flash during a SPI flash read (WiFi power-down, mDNS, NVS). The UART ISR and ringbuf ISR helpers are now in IRAM
 - compile errors against Arduino 3.3.12 from Tasmota/Arduino headers named `console.h`, `network.h` and `httpClient.h`. Those EMS-ESP headers are now `emsesp_console.h`, `emsesp_network.h` and `emsesp_http_client.h`
 - HTTP OTA from WSL/mDNS stalled on IPv6 and used tiny write chunks. Uploads now resolve IPv4 and stream in 64 KB blocks (`scripts/upload.py`, `scripts/upload_cli.py`)
+- a rejected upload (an invalid MD5 file, an unsupported file type) got no HTTP response: the connection was closed before the 406 was written, so `curl` reported "Empty reply from server". The 406 is now sent once the upload has been received
 
 ## Changed
 

src/ESP32React/UploadFileService.cpp

@@ -151,7 +151,7 @@ void UploadFileService::handleUpload(AsyncWebServerRequest * request, const Stri
 
 void UploadFileService::uploadComplete(AsyncWebServerRequest * request) {
     if (request->_tempObject) {
-        const int code = static_cast<int>(reinterpret_cast<intptr_t>(request->_tempObject));
+        const int code = *static_cast<int *>(request->_tempObject);
         if (!_response_sent && code >= 400) {
             request->send(request->beginResponse(code));
             _response_sent = true;
@@ -214,6 +214,10 @@ void UploadFileService::uploadComplete(AsyncWebServerRequest * request) {
             AsyncWebServerResponse * response = request->beginResponse(200);
             request->send(response);
         }
+        // the upload is done: clear the flags so handleDisconnect() doesn't treat the client closing the
+        // connection as an aborted upload and cancel the pending restart
+        _is_firmware   = false;
+        _is_filesystem = false;
         emsesp::EMSESP::system_.systemStatus(
             emsesp::SYSTEM_STATUS::SYSTEM_STATUS_PENDING_RESTART); // will be handled by the main loop. We use pending for the Web's SystemMonitor
         return;
@@ -237,22 +241,19 @@ void UploadFileService::handleError(AsyncWebServerRequest * request, int code) {
     if (request->_tempObject) {
         return;
     }
-    request->_tempObject = reinterpret_cast<void *>(static_cast<intptr_t>(code)); // stop later chunks from retrying the write
+    // stop later chunks from retrying the write. The request destructor frees _tempObject, so it must be heap memory
+    auto * error = static_cast<int *>(malloc(sizeof(int)));
+    if (error) {
+        *error = code;
+    }
+    request->_tempObject = error;
 
     emsesp::EMSESP::logger().info("Upload error: %d", code);
     emsesp::EMSESP::system_.uart_init(); // re-enable UART
     _ota.abort();
 
-    // 406: the client sent a file we won't accept. Close now so the browser fails fast.
-    // Other codes wait for uploadComplete() to send, so we don't destroy the request mid-stream.
-    if (code == 406) {
-        request->send(request->beginResponse(code));
-        _response_sent = true;
-        request->client()->close();
-        _is_firmware   = false;
-        _is_filesystem = false;
-        _is_recovery   = false;
-    }
+    // uploadComplete() sends the error code. The web server only writes a response once the whole body has
+    // arrived, so closing the connection here to fail fast would lose the response.
 
     // if we aborted a filesystem upload, remount LittleFS so the device keeps working
     if (_is_filesystem) {