Multi-platform http server - #646
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds a platform-neutral HTTP server module with routing, request parsing, response APIs, lifecycle control, chunked transfer, and integration tests. Development and web server services migrate from ESP-IDF HTTP APIs to the new server API. Related services and apps now compile and register across platforms. Web server settings use portable IP lookup and platform-specific default ports. File execution uses Merge Risk: 🟠 High · up to The portable HTTP services are not ready to merge: exposed management routes can trigger unintended actions, malformed uploads can exhaust storage, and uploaded dashboard content can execute in the server origin. Several cross-platform lifecycle and response behaviors also remain incorrect. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
Modules/http-module/source/server.cpp (1)
226-229: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTerminate a chunked response that the handler left open.
The fallback covers only the case where the handler sent nothing. If a handler calls
http_server_request_send_chunk_startand then returns without callinghttp_server_request_send_chunk_end,response_sentis already true, so no terminator is sent. The client receives a chunked body with no final "0\r\n\r\n" and reports a truncated response.Close the chunked stream here when it is still open.
♻️ Proposed change
if (!request.response_sent) { LOG_W(TAG, "Handler for %s did not send a response", matched->uri); http_server_request_send_error(&request, 500, "Handler did not send a response"); + } else if (request.chunked) { + LOG_W(TAG, "Handler for %s left a chunked response open", matched->uri); + http_server_request_send_chunk_end(&request); }Modules/http-module/tests/source/server_test.cpp (1)
27-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the file and function names in this comment.
The comment names
server_posix.cppandrecv_retry(). The implementation lives inModules/http-module/source/server.cpp, and the function isreceive_retry().♻️ Proposed change
- // The FreeRTOS POSIX port's tick signal can interrupt a blocking syscall on this thread, so - // every blocking call below retries on EINTR (matches server_posix.cpp's own recv_retry()). + // The FreeRTOS POSIX port's tick signal can interrupt a blocking syscall on this thread, so + // every blocking call below retries on EINTR (matches server.cpp's own receive_retry()).Tactility/Source/service/webserver/WebServerService.cpp (1)
1725-1729: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the 2-second delay on non-ESP platforms.
Line 1724 blocks the request task for 2 seconds so the response can flush before
esp_restart(). On non-ESP platforms no restart follows, so the delay only stalls the HTTP server task and delays the reply to the client. MovevTaskDelayinside theESP_PLATFORMbranch.♻️ Proposed change
- // Reboot after a short delay to allow response to be sent - vTaskDelay(pdMS_TO_TICKS(2000)); `#ifdef` ESP_PLATFORM + // Reboot after a short delay to allow response to be sent + vTaskDelay(pdMS_TO_TICKS(2000)); esp_restart(); `#else` LOG_W(TAG, "Reboot is not supported on this platform"); `#endif`
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: a15ce6b3-38a0-4e4b-ba64-b6bca864ae86
📒 Files selected for processing (24)
Modules/http-module/include/http/server.hModules/http-module/include/http/types.hModules/http-module/source/module.cppModules/http-module/source/server.cppModules/http-module/source/types.cppModules/http-module/tests/source/server_test.cppTactility/Include/Tactility/network/HttpServer.hTactility/Include/Tactility/network/HttpServerReq.hTactility/Include/Tactility/network/HttpdReq.hTactility/Include/Tactility/settings/WebServerSettings.hTactility/Private/Tactility/service/development/DevelopmentService.hTactility/Private/Tactility/service/development/DevelopmentSettings.hTactility/Private/Tactility/service/webserver/WebServerService.hTactility/Source/Tactility.cppTactility/Source/app/development/Development.cppTactility/Source/app/files/View.cppTactility/Source/app/terminal/Terminal.cppTactility/Source/app/webserversettings/WebServerSettings.cppTactility/Source/network/HttpServer.cppTactility/Source/network/HttpServerReq.cppTactility/Source/network/HttpdReq.cppTactility/Source/service/development/DevelopmentService.cppTactility/Source/service/development/DevelopmentSettings.cppTactility/Source/service/webserver/WebServerService.cpp
💤 Files with no reviewable changes (5)
- Tactility/Source/service/development/DevelopmentSettings.cpp
- Tactility/Source/network/HttpServer.cpp
- Tactility/Include/Tactility/network/HttpServer.h
- Tactility/Source/network/HttpdReq.cpp
- Tactility/Private/Tactility/service/development/DevelopmentSettings.h
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Modules/http-module/source/server.cpp (1)
451-451: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftApply a body-progress deadline.
Line 451 starts a new five-second wait for every body read. A client can keep
/app/installinreceiveFileby sending one byte before each timeout. The listener task then remains inhandle_connection, so it cannot accept other clients andhttp_server_stopwaits for the handler to finish.Carry a body deadline into
HttpServerRequest, or enforce a minimum body throughput and a maximum body duration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 1c950c29-6178-4755-bb2f-a5553a8b7cb0
📒 Files selected for processing (6)
Modules/http-module/source/server.cppModules/http-module/tests/source/server_test.cppTactility/Include/Tactility/network/HttpServerReq.hTactility/Source/network/HttpServerReq.cppTactility/Source/network/Url.cppTactility/Source/service/development/DevelopmentService.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- Modules/http-module/tests/source/server_test.cpp
- Tactility/Include/Tactility/network/HttpServerReq.h
- Tactility/Source/network/HttpServerReq.cpp
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Tactility/Source/service/webserver/WebServerService.cpp (4)
520-524: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop AP mode on every HTTP server startup failure.
If
startApMode()succeeds but allocation or startup fails, this branch frees the HTTP server and returns without callingstopApMode().stopServer()cannot clean up later becausehttpServeris null and returns at Line 543. Stop AP mode before returning from this failure path.Proposed fix
if (httpServer == nullptr || http_server_start(httpServer) != ERROR_NONE) { LOG_E(TAG, "Failed to start HTTP server on port %u", (unsigned)settings.webServerPort); http_server_free(httpServer); httpServer = nullptr; + stopApMode(); return false; }
1002-1002: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse boundary-aware URI matching in all dispatchers.
These
strncmpchecks accept arbitrary suffixes. For example,/admin/reboot-nowtriggers reboot and/api/apps/run-nowinvokes the app-run handler instead of returning 404. Replace the prefix checks with the existinguriMatcheshelper.Also applies to: 1015-1015, 1027-1034, 1052-1056, 1074-1074
1723-1726: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report a reboot on non-ESP platforms.
The handler sends
Rebooting...and blocks for two seconds before the non-ESP branch only logs that reboot is unsupported. Non-ESP clients receive a misleading success response. Send an unsupported-operation error on non-ESP platforms, and keep the delay and restart logic inside the ESP branch.
1790-1792: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winImplement the
default.htmlfallback.This branch only logs that
default.htmlshould be served. It does not updaterequestedPathordataPath, so the request still checks the missingdashboard.htmlpath and returns 404.Proposed fix
if (requestedPath == "/dashboard.html" && !file::isFile(dataPath.c_str())) { LOG_I(TAG, "dashboard.html not found, serving default.html"); + requestedPath = "/default.html"; + dataPath = std::string(file::MOUNT_POINT_SYSTEM) + "/app/WebServer" + requestedPath; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 88bca0cd-09fc-462d-861f-b0bb2f0ef341
📒 Files selected for processing (1)
Tactility/Source/service/webserver/WebServerService.cpp
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Tactility/Source/service/webserver/WebServerService.cpp (4)
1003-1003: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire exact or segment-boundary route checks.
strncmpaccepts any URI that starts with the route. For example,/admin/reboot-nowcan invoke reboot, and/api/apps/run-extra?id=...can invokeapp_start. Use an exact-path check for leaf endpoints. Allow only\0or?after the route.Also applies to: 1016-1016, 1028-1035, 1053-1057, 1075-1076
1728-1734: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn an unsupported response on non-ESP platforms.
The handler sends
"Rebooting..."and waits two seconds before the non-ESP branch logs that reboot is unsupported. Non-ESP clients receive a successful-looking response even though no reboot occurs.Move the response and delay inside
#ifdef ESP_PLATFORM. Return HTTP 501 on non-ESP platforms.
1432-1433: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDenial of Service (CWE-400): Uncontrolled Resource Consumption
Reachability: External · Exploitability: Moderate
Delete the temporary file when trailing multipart validation fails.
receiveFilecreatesfile_pathbeforereadAndDiscardOrSendErrorruns. This branch returns without deleting it, allowing repeated truncated uploads to fill the temporary filesystem.Proposed cleanup
if (!network::readAndDiscardOrSendError(request, boundary_and_newlines_after_file)) { + file::deleteFile(file_path); return ERROR_UNDEFINED; }
923-924: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winXSS (CWE-79): Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')
Reachability: External · Exploitability: Moderate
Do not serve user-writable files as active web content.
handleFsUploadwrites client-supplied content below/sdcard, including/sdcard/tactility/webserver.handleAssetsserves that directory and maps.html,.js, and.svgfiles to active MIME types. A client can upload content that executes in the dashboard origin when web-server authentication is disabled.Store uploads outside the served directory, allow only inert formats, or serve user files from an isolated origin with download disposition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 09f87cf8-8a3d-4899-a58f-28e511d3a9c1
📒 Files selected for processing (2)
Modules/http-module/source/server.cppTactility/Source/service/webserver/WebServerService.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Modules/http-module/source/server.cpp
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.
1 participant
http-moduleDevelopmentServiceworks on all platforms (including posix)