Webserver addition and TactilityC symbols #451
Conversation
📝 WalkthroughWalkthroughAdds a complete web server subsystem: WebServerService (extensive HTTP handlers for assets, /fs, /api, /admin), WebServerInit (Wi‑Fi/AP initialization), AssetVersion (Data/SD version I/O and sync), and persistent WebServerSettings with an LVGL Web Server Settings app. Adds dashboard and default HTML assets plus version.json and Documentation/webserver.md. Registers manifests and exposes setWebServerEnabled. Introduces three SystemEvent values, changes HttpServer::start() to return bool, implements many endpoints (sysinfo, apps, wifi, screenshot, file ops), and includes startup, sync, auth, and build/symbol-export adjustments. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/network/HttpServer.cpp (1)
26-35: Propagate URI handler registration failures to the caller.
startInternal()returnstrueeven when one or more URI handlers fail to register. Sincehttpd_register_uri_handleris per-call and not atomic, handler registration failures leave the server running in a partially broken state with missing endpoints. The caller cannot distinguish between complete success and partial failure.Consider tracking registration failures and returning
false(and optionally stopping the server) so callers can react appropriately.🔧 Suggested fix
- for (std::vector<httpd_uri_t>::reference handler : handlers) { - if (httpd_register_uri_handler(server, &handler) != ESP_OK) { - LOGGER.error("Failed to register URI handler: {}", handler.uri); - } - } - - LOGGER.info("Started on port {}", config.server_port); - - return true; + bool allRegistered = true; + for (std::vector<httpd_uri_t>::reference handler : handlers) { + if (httpd_register_uri_handler(server, &handler) != ESP_OK) { + LOGGER.error("Failed to register URI handler: {}", handler.uri); + allRegistered = false; + } + } + if (!allRegistered) { + httpd_stop(server); + server = nullptr; + return false; + } + + LOGGER.info("Started on port {}", config.server_port); + return true;
🤖 Fix all issues with AI agents
In `@Data/data/service/webserver/settings.properties`:
- Around line 10-23: The default credentials in the settings file are insecure:
update the AP and HTTP auth defaults so they are not predictable by (1) removing
hardcoded defaults for apPassword, webServerUsername, and webServerPassword and
instead generate or require device-unique values (e.g., derived from device
serial or a random per-device token) or leave them blank/disabled until user
config; (2) ensure webServerAuthEnabled and AP-enabled flow
(webServerEnabled/apSsid) do not default to enabled when credentials are blank;
and (3) adjust initialization logic that reads apPassword, webServerAuthEnabled,
webServerUsername, and webServerPassword so it enforces user setup or
auto-generates secure values at first boot rather than using fixed strings.
In `@Data/data/webserver/dashboard.html`:
- Around line 313-323: The CSS currently targets `#status` but the HTML uses
`#files-status` and `#apps-status` and the JS overwrites element.className, so
rename or broaden the selectors and stop clobbering classes: update the CSS
selectors to target `#files-status`, `#apps-status` (or better, use a reusable
.status class and style .status, .status.show, .status.error, etc.) and change
the JS that sets the element's class (where it currently assigns className) to
use classList.add/remove/toggle for 'show' and the state classes
('error'/'success'/'info') so styles and show/hide behavior apply correctly to
files-status and apps-status.
- Around line 767-781: updateBreadcrumb currently injects raw path segments into
inline onclick strings which can be exploited for XSS; replace the
string-concatenated HTML building with DOM APIs: create anchor/span/strong
elements using document.createElement, set text content with escapeHtml or
textContent, set href="#" and attach click handlers via element.addEventListener
that call navigateTo with a safely constructed path (e.g., use the cumulative
path variable or encodeURI on it), and finally append these nodes to
breadcrumbEl instead of assigning innerHTML; update references to
updateBreadcrumb, breadcrumbEl, navigateTo and escapeHtml accordingly.
- Around line 1100-1105: The installAppFile function is using the multipart
field name 'elf' which conflicts with the documented API expecting 'file';
update the FormData append call in installAppFile to use 'file' as the field
name (i.e., replace formData.append('elf', file, file.name) with
formData.append('file', file, file.name)) so the client matches the webserver.md
contract.
In `@Documentation/webserver.md`:
- Around line 27-29: The markdown code fences in Documentation/webserver.md lack
language identifiers; update each fenced block (e.g., the URL block containing
"http://<device-ip>/" and the directory listing blocks showing
"/data/webserver/" and "/sdcard/.tactility/webserver/") to include a language
tag such as "text" (i.e., change ``` to ```text) so markdownlint MD040 is
satisfied and rendering improves.
In `@Tactility/Source/app/webserversettings/WebServerSettings.cpp`:
- Around line 325-341: The dispatched lambda currently calls
settings::webserver::save(copy) and ignores its return value; update the
getMainDispatcher().dispatch lambda to check the save result from
settings::webserver::save(copy), log a failure via LOGGER.error with context if
it fails, and avoid calling
kernel::publishSystemEvent(kernel::SystemEvent::WebServerSettingsChanged) or
performing WiFi/webserver changes (service::webserver::setWebServerEnabled) when
persistence fails; keep successful-path behavior unchanged (log info and perform
actions) when save returns success.
In `@Tactility/Source/network/HttpServer.cpp`:
- Around line 39-44: The current shutdown logic nulls the server handle
unconditionally even when httpd_stop(server) fails; update the block in
HttpServer.cpp so that you only set server = nullptr when httpd_stop(server)
returns ESP_OK, leaving the handle intact on failure (and keep the LOGGER.warn
call), so that isStarted() and subsequent start()/cleanup logic reflect the true
state of the running server; reference the httpd_stop call and the server member
used by isStarted()/start() to implement this conditional nulling and consider
returning/propagating the stop error if needed.
In `@Tactility/Source/service/webserver/AssetVersion.cpp`:
- Around line 189-238: copyDirectory currently overwrites dstPath in place
causing clients to see partial files; modify the file-copy branch in
copyDirectory (where getLock is used and srcFile/dstFile are opened) to write to
a temporary path (e.g., dstPath + ".tmp.<pid/tid/random>"), flush and fsync the
temp file, close it, then atomically rename(tempPath, dstPath) to replace the
destination; ensure you hold the same lock (file::getLock(srcPath)) across the
sequence, handle and log errors via LOGGER, clean up the temp file on failures,
and only unlock after rename (or cleanup) so partial files are never visible if
rename fails.
In `@Tactility/Source/service/webserver/WebServerService.cpp`:
- Around line 1325-1402: The handler currently concatenates request->uri
directly into filesystem paths, allowing path traversal and failing on
cache-busting queries; in handleAssets(httpd_req_t* request) first strip the
query/fragment (truncate at '?' or '#'), URL-decode percent-escapes, normalize
the path by collapsing "." and ".." segments (remove redundant slashes, resolve
".." safely) and ensure it begins with '/' and contains no ".." after
normalization; if normalization fails or traversal is detected return a 400/403
and log; then use the sanitized path (instead of request->uri / requestedPath)
when building dataPath and sdPath, when calling file::isFile, file::getLock,
fopen, and getContentType so assets are resolved safely and cache‑busting
queries no longer break lookup.
- Around line 953-1028: In handleApiAppsInstall: validate and bound the computed
file_size before using it and sanitize the client filename from
content_disposition_map to prevent path traversal; specifically, ensure
file_size = content_left - boundary_and_newlines_after_file.length() is not
negative/underflowing and clamp it to a reasonable MAX_UPLOAD_SIZE, and also
verify content_left is sufficient for that size; sanitize filename_entry->second
by stripping any "../", leading "/" or path separators and enforcing a max
filename length (reject empty or invalid names) before calling std::format to
build file_path; if validation fails, send HTTP 400 and return ESP_FAIL; keep
using receiveFile, getTempPath, file::findOrCreateDirectory and file::deleteFile
after these checks.
- Around line 653-712: The dispatchers handleAdminPost, handleApiGet,
handleApiPost, and handleApiPut currently route sensitive endpoints without
enforcing WebServerSettings::webServerAuthEnabled; add an early auth check in
each of these functions that (1) if WebServerSettings::webServerAuthEnabled is
true, validates the HTTP Basic credentials on the httpd_req_t* request (reuse
the service's existing auth routine or add a small helper like
validateRequestAuth(request)), (2) if validation fails, log and send
HTTPD_401_UNAUTHORIZED (or httpd_resp_send_err with 401) and return ESP_FAIL to
stop processing; apply this for the sensitive routes listed (/admin/sync,
/admin/reboot, /api/apps/install, /api/apps/uninstall, /api/apps/run and any
/fs/* handlers routed through these dispatchers). Ensure the auth check is
performed before delegating to handlers (e.g., before calling handleSync,
handleReboot, handleApiAppsInstall, handleApiAppsUninstall, handleApiAppsRun).
In `@Tactility/Source/settings/WebServerSettings.cpp`:
- Around line 72-86: The numeric parsing must be made robust: for
settings.apChannel (from ap_channel) and settings.webServerPort (from
webserver_port) wrap parsing in a try/catch and validate/clamp ranges before
casting to uint8_t/uint16_t; on parse failure or out-of-range values fall back
to the current defaults (channel = 1, port = 80). Use safe unsigned parsing
(e.g., std::stoul or check isdigit) or catch
std::invalid_argument/std::out_of_range from std::stoi, then clamp the channel
to a sensible range (e.g., 1–13) and the port to 1–65535 before assigning to
settings.apChannel and settings.webServerPort, leaving the existing behavior for
settings.webServerEnabled and string defaults untouched.
🧹 Nitpick comments (2)
Tactility/Source/app/webserversettings/WebServerSettings.cpp (1)
89-101: Avoid blocking the UI/event loop during asset sync.
syncAssets()runs file I/O on the main dispatcher, which can stall UI and watchdog‑sensitive work. Consider offloading to a background worker/task and updating the UI with a “syncing” state.Tactility/Source/service/webserver/WebServerService.cpp (1)
630-651: Tighten dispatcher URI matching to avoid prefix collisions.
strncmpwith route length will treat/fs/listingas/fs/list. A boundary‑aware matcher avoids accidental matches and future route conflicts.♻️ Suggested boundary-aware matcher
+static bool uriMatches(const char* uri, const char* route) { + const size_t n = strlen(route); + return strncmp(uri, route, n) == 0 && (uri[n] == '\0' || uri[n] == '?' || uri[n] == '/'); +} esp_err_t WebServerService::handleFsGenericGet(httpd_req_t* request) { const char* uri = request->uri; - if (strncmp(uri, "/fs/list", strlen("/fs/list")) == 0) return handleFsList(request); - if (strncmp(uri, "/fs/download", strlen("/fs/download")) == 0) return handleFsDownload(request); - if (strncmp(uri, "/fs/tree", strlen("/fs/tree")) == 0) return handleFsTree(request); + if (uriMatches(uri, "/fs/list")) return handleFsList(request); + if (uriMatches(uri, "/fs/download")) return handleFsDownload(request); + if (uriMatches(uri, "/fs/tree")) return handleFsTree(request); LOGGER.warn("GET {} - not found in fs generic dispatcher", uri); httpd_resp_send_err(request, HTTPD_404_NOT_FOUND, "not found"); return ESP_FAIL; } // Generic POST dispatcher for /fs/* URIs esp_err_t WebServerService::handleFsGenericPost(httpd_req_t* request) { const char* uri = request->uri; - if (strncmp(uri, "/fs/mkdir", strlen("/fs/mkdir")) == 0) return handleFsMkdir(request); - if (strncmp(uri, "/fs/delete", strlen("/fs/delete")) == 0) return handleFsDelete(request); - if (strncmp(uri, "/fs/rename", strlen("/fs/rename")) == 0) return handleFsRename(request); - if (strncmp(uri, "/fs/upload", strlen("/fs/upload")) == 0) return handleFsUpload(request); + if (uriMatches(uri, "/fs/mkdir")) return handleFsMkdir(request); + if (uriMatches(uri, "/fs/delete")) return handleFsDelete(request); + if (uriMatches(uri, "/fs/rename")) return handleFsRename(request); + if (uriMatches(uri, "/fs/upload")) return handleFsUpload(request); LOGGER.warn("POST {} - not found in fs generic dispatcher", uri); httpd_resp_send_err(request, HTTPD_404_NOT_FOUND, "not found"); return ESP_FAIL; }
| # Access Point Mode Settings (create own WiFi network) | ||
| # apSsid will be auto-generated as Tactility-XXXX if empty | ||
| apSsid= | ||
| apPassword=tactility | ||
| apChannel=1 | ||
|
|
||
| # Web Server Settings | ||
| webServerEnabled=0 | ||
| webServerPort=80 | ||
|
|
||
| # HTTP Basic Authentication (optional) | ||
| webServerAuthEnabled=0 | ||
| webServerUsername=admin | ||
| webServerPassword=admin |
There was a problem hiding this comment.
Avoid predictable default WiFi/AP and HTTP auth credentials.
Line 13 (apPassword=tactility) and Lines 22–23 (admin/admin) are predictable. If users enable AP or Basic Auth without changing them, the device is exposed. Prefer device-specific random defaults or require explicit user configuration before enabling.
🔧 Suggested safer defaults
- apPassword=tactility
+ # Leave empty to auto-generate a strong password on first boot
+ apPassword=
- webServerUsername=admin
- webServerPassword=admin
+ # Leave empty to require explicit user configuration or auto-generate
+ webServerUsername=
+ webServerPassword=📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Access Point Mode Settings (create own WiFi network) | |
| # apSsid will be auto-generated as Tactility-XXXX if empty | |
| apSsid= | |
| apPassword=tactility | |
| apChannel=1 | |
| # Web Server Settings | |
| webServerEnabled=0 | |
| webServerPort=80 | |
| # HTTP Basic Authentication (optional) | |
| webServerAuthEnabled=0 | |
| webServerUsername=admin | |
| webServerPassword=admin | |
| # Access Point Mode Settings (create own WiFi network) | |
| # apSsid will be auto-generated as Tactility-XXXX if empty | |
| apSsid= | |
| # Leave empty to auto-generate a strong password on first boot | |
| apPassword= | |
| apChannel=1 | |
| # Web Server Settings | |
| webServerEnabled=0 | |
| webServerPort=80 | |
| # HTTP Basic Authentication (optional) | |
| webServerAuthEnabled=0 | |
| # Leave empty to require explicit user configuration or auto-generate | |
| webServerUsername= | |
| webServerPassword= |
🤖 Prompt for AI Agents
In `@Data/data/service/webserver/settings.properties` around lines 10 - 23, The
default credentials in the settings file are insecure: update the AP and HTTP
auth defaults so they are not predictable by (1) removing hardcoded defaults for
apPassword, webServerUsername, and webServerPassword and instead generate or
require device-unique values (e.g., derived from device serial or a random
per-device token) or leave them blank/disabled until user config; (2) ensure
webServerAuthEnabled and AP-enabled flow (webServerEnabled/apSsid) do not
default to enabled when credentials are blank; and (3) adjust initialization
logic that reads apPassword, webServerAuthEnabled, webServerUsername, and
webServerPassword so it enforces user setup or auto-generates secure values at
first boot rather than using fixed strings.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Tactility/Source/service/webserver/WebServerService.cpp`:
- Around line 1220-1222: The error string sent when isRootMountPoint(norm) is
true is truncated; update the httpd_resp_send_err call in WebServerService.cpp
(the if block that checks isRootMountPoint(norm)) to send the full message
"cannot delete mount point" instead of "cannot delete mount " so the client
receives the complete error text.
- Around line 124-133: The onStop implementation in WebServerService currently
never unsubscribes settingsEventSubscription, so move or add logic in
WebServerService::onStop to unsubscribe the WebServerSettingsChanged
subscription before tearing down the service; locate the
settingsEventSubscription member and call the appropriate unsubscribe/unregister
method (or event system unsubscribe function) to remove the callback, then
clear/reset settingsEventSubscription (and only afterwards null
g_webServerInstance and proceed with existing teardown like setEnabled(false)
and statusbarIcon removal).
- Around line 1086-1093: The loop that builds screenshot_path using save_path
and checks file::isFile can silently end on the last index (9999) and overwrite
an existing file, and screenshots are never cleaned up; update the allocation
logic in WebServerService.cpp so that the for-loop (which sets screenshot_path)
detects when no free slot is found and returns or fails early instead of
reusing/overwriting the 9999 name, and add a configurable cleanup policy or
rotate-old-files routine (or document manual cleanup) to remove/limit stored
screenshots; specifically modify the block that constructs screenshot_path and
uses file::isFile to either throw/return an error when the range is exhausted or
pick a safe alternative (e.g., timestamped filename or rotation), and implement
or wire in a cleanup/retention function to prune old screenshots.
- Around line 72-75: Protect concurrent access to
g_cachedSettings/g_settingsCached using the declared g_settingsMutex: in the
event subscription callback that updates g_cachedSettings and sets
g_settingsCached, acquire the mutex before modifying and release afterwards;
likewise, wrap all reads of g_cachedSettings and checks of g_settingsCached
performed by HTTP handlers in the same mutex (use a scoped lock/lock_guard
around reads and writes) so all access to g_cachedSettings and g_settingsCached
is synchronized via g_settingsMutex.
♻️ Duplicate comments (3)
Tactility/Source/service/webserver/WebServerService.cpp (3)
653-712: Authentication concerns for these dispatchers were previously raised. The issue about missing HTTP Basic Auth validation for sensitive endpoints (/admin/sync, /admin/reboot, /api/apps/install, /api/apps/uninstall, /api/apps/run) remains unaddressed.
953-1028: The multipart size validation and filename sanitization concerns were previously raised and remain unaddressed.
1325-1402: The path traversal and query string handling concerns were previously raised. Additionally noted:Content-Type determined from original URI, not resolved path.
When
dashboard.htmlfalls back todefault.html(lines 1334-1338), the content type is still derived from the originaluri(line 1342), not the actual file being served. This could cause incorrect MIME types in edge cases.🔧 Suggested fix
if (strcmp(uri, "/dashboard.html") == 0 && !file::isFile(dataPath.c_str())) { // Dashboard doesn't exist, try default.html dataPath = "/data/webserver/default.html"; + requestedPath = "/default.html"; // Update for correct content-type LOGGER.info("dashboard.html not found, serving default.html"); } // Try to serve from Data partition first if (file::isFile(dataPath.c_str())) { - httpd_resp_set_type(request, getContentType(uri)); + httpd_resp_set_type(request, getContentType(requestedPath));
🧹 Nitpick comments (1)
Tactility/Source/service/webserver/WebServerService.cpp (1)
158-159: Settings loaded twice - once inonStartand again instartServer.
startServer()callssettings::webserver::loadOrGetDefault()(line 159) despite settings being cached ing_cachedSettingsduringonStart. This causes redundant SD card reads. Consider using the cached settings unless fresh values are required.♻️ Suggested optimization
bool WebServerService::startServer() { - auto settings = settings::webserver::loadOrGetDefault(); + // Use cached settings to avoid redundant SD card reads + auto settings = g_cachedSettings;
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Tactility/Source/app/webserversettings/WebServerSettings.cpp`:
- Around line 41-52: In onWifiModeChanged, remove the line that sets
WebServerSettingsApp::webServerEnabledChanged inside the dispatched lambda so
changing wsSettings.wifiMode (and calling app->updateUrlDisplay()) only marks
wifiSettingsChanged and updated; locate the lambda in onWifiModeChanged (uses
getMainDispatcher().dispatch, app, index) and delete the
app->webServerEnabledChanged = true statement leaving app->wsSettings.wifiMode,
app->updated, app->wifiSettingsChanged, and app->updateUrlDisplay() intact.
In `@Tactility/Source/service/webserver/WebServerService.cpp`:
- Around line 1491-1500: The handler WebServerService::handleReboot is declared
to return esp_err_t but lacks a return; append a return ESP_OK; at the end of
WebServerService::handleReboot (after the esp_restart() call) to satisfy the
function signature and silence compiler warnings while keeping behavior
unchanged; locate the function by name (WebServerService::handleReboot) and add
the return statement immediately before the function's closing brace.
- Around line 1528-1529: The Content-Type is being set from the original uri
(httpd_resp_set_type(request, getContentType(uri))) instead of the actual file
served when you fall back (e.g., dashboard.html -> default.html or SD fallback);
update the response Content-Type to use the resolved filesystem path/served
filename (the variable used to open the file, e.g., path or resolvedPath) and
apply the same change in the SD fallback branch so both branches call
httpd_resp_set_type(request, getContentType(resolvedPath)) (or equivalent)
rather than getContentType(uri).
♻️ Duplicate comments (10)
Tactility/Source/service/webserver/AssetVersion.cpp (1)
204-293: Atomic copy pattern correctly implemented - previous concern addressed.The file copy now uses a temp file with random suffix, followed by atomic
rename(). This prevents serving partially-copied assets. Good handling of:
- Temp file cleanup on failure
fflush()+fsync()before rename- Lock held across the entire operation
Tactility/Source/app/webserversettings/WebServerSettings.cpp (1)
330-349: Save failure handling now implemented - previous concern addressed.The return value of
settings::webserver::save(copy)is now checked and a warning is logged on failure. The system event is still published even on save failure, which could be intentional to update the running state even if persistence fails.Tactility/Source/service/webserver/WebServerService.cpp (8)
194-209: Mutex now properly used for settings cache access - previous concern addressed.The settings cache is now protected by
g_settingsMutexduring initial load (lines 194-199) and in the event subscription callback (lines 204-207). This addresses the previous concern about concurrent access.
223-238: Event unsubscription now implemented in onStop - previous concern addressed.The
settingsEventSubscriptionis properly unsubscribed before cleanup (lines 226-229), preventing callbacks to invalid state after service stop.
779-794: Auth checks now implemented for admin endpoints - previous concern addressed.The
handleAdminPostdispatcher now validates authentication before routing tohandleSyncorhandleReboot.
1145-1170: Multipart payload validation and filename sanitization implemented - previous concern addressed.The implementation now:
- Validates payload size with
MAX_APP_UPLOAD_SIZE(20MB)- Checks for underflow:
content_left <= boundary_and_newlines_after_file.length()- Sanitizes filename using
file::getLastPathSegmentand rejects paths with..,/, or\
1258-1271: Screenshot slot exhaustion now handled - previous concern addressed.The loop now tracks
found_slotand returns an appropriate error (500) when all 9999 slots are exhausted.
1399-1401: Truncated error message fixed - previous concern addressed.The message now correctly reads "cannot delete mount point".
1502-1516: Asset URI now properly sanitized - previous concern addressed.The implementation now:
- Strips query strings before path processing (line 1509-1511)
- Normalizes the path (line 1512)
- Rejects path traversal attempts (line 1513-1516)
263-270: Settings accessed without mutex lock in startServer.While settings are copied locally at lines 265-270, HTTP handlers access
g_cachedSettingsdirectly (e.g., line 110, 167-168) without acquiring the mutex. This could cause data races if settings change during request processing.🔒 Suggested fix: copy settings under lock in validateRequestAuth
static esp_err_t validateRequestAuth(httpd_req_t* request, bool& authPassed) { authPassed = false; - if (!g_cachedSettings.webServerAuthEnabled) { + // Copy settings under lock to avoid races + settings::webserver::WebServerSettings localSettings; + { + auto lock = g_settingsMutex.asScopedLock(); + lock.lock(); + localSettings = g_cachedSettings; + } + + if (!localSettings.webServerAuthEnabled) { authPassed = true; return ESP_OK; // Auth disabled, allow request } // ... use localSettings.webServerUsername and localSettings.webServerPassword
🧹 Nitpick comments (4)
Documentation/webserver.md (1)
60-68: Consider documenting thesyncAssets()behavior difference.The documentation at line 66 states "If Data version > SD version: Copy Data -> SD (backup customizations)", but based on the implementation in
AssetVersion.cpp(lines 377-381), this copy is actually deferred during boot to avoid watchdog timeout. The "Sync Scenarios" section (lines 355-358) correctly documents this deferral, but this brief summary in thesyncAssets()docblock could be misleading.Suggested clarification
Consider updating the brief logic summary (around line 66) to note the deferral:
* Logic: * - If Data is empty: Copy from SD card (recovery mode) * - If SD version > Data version: Copy SD -> Data (firmware update) -- If Data version > SD version: Copy Data -> SD (backup customizations) +- If Data version > SD version: Deferred to runtime (backup customizations)Data/data/webserver/dashboard.html (1)
736-747: Consider adding a comment for the empty catch block.The empty catch block at line 744 is intentional since the device will disconnect during reboot, but a brief comment would improve clarity for future maintainers.
Suggested clarification
try { await fetch('/admin/reboot', { method: 'POST' }); - } catch (e) { } + } catch (e) { /* Expected: device disconnects during reboot */ }Tactility/Source/app/webserversettings/WebServerSettings.cpp (2)
89-107: Blocking sync operation on UI thread may cause unresponsiveness.
syncAssets()can perform directory copies (potentially slow on SD card). Dispatching togetMainDispatcher()still runs on the main thread. Consider running the sync on a background task or at minimum showing a spinner/progress indicator.
148-157: Force Station mode workaround should have a TODO or tracking issue.The comment mentions AP mode "might work one day" but silently forces Station mode. Consider adding a TODO comment or disabling the AP option in the dropdown entirely to avoid user confusion.
💡 Consider disabling the AP option in UI
Since AP mode is forced to Station, the dropdown should reflect this limitation more clearly, perhaps by only showing "Station" as an option (which is currently done at line 180) rather than silently overriding the saved preference.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Tactility/Source/service/webserver/WebServerService.cpp`:
- Around line 1509-1526: The fallback for dashboard.html uses the raw uri
(strcmp(uri, "/dashboard.html") == 0) so requests with cache‑busting queries
(e.g. "/dashboard.html?v=1") don't match; change that comparison to use the
normalized requestedPath (which already strips the query and is normalized) —
replace the strcmp check with a check against requestedPath (e.g. requestedPath
== "/dashboard.html") before calling file::isFile and setting dataPath to
"/data/webserver/default.html" (keep LOGGER.info and the rest unchanged).
- Around line 685-739: The handleFsUpload upload loop treats any negative
httpd_req_recv return as fatal; change it to detect HTTPD_SOCK_ERR_TIMEOUT (−3)
and retry the recv instead of aborting (e.g., continue/loop with a small retry
counter or backoff), while still treating other negative/error codes as fatal;
update the loop in WebServerService::handleFsUpload around the httpd_req_recv
call to only fail on non-timeout errors and decrement/advance remaining/received
only on successful reads, and apply the same timeout-retry logic to the
analogous functions receiveByteArray, receiveTextUntil, and receiveChunk in
HttpdReq.cpp.
♻️ Duplicate comments (2)
Tactility/Source/app/webserversettings/WebServerSettings.cpp (1)
329-347: Skip events/toggles if settings persistence fails.If the save fails, publishing
WebServerSettingsChangedand toggling the web server can leave runtime state out of sync with persisted settings. Consider aborting the downstream actions on failure.🐛 Proposed fix (short-circuit on failed save)
getMainDispatcher().dispatch([copy, wifiChanged, webServerChanged]{ // Save to flash (fast, low memory pressure) - if (!settings::webserver::save(copy)) { - LOGGER.warn("Failed to persist WebServer settings; changes may be lost on reboot"); - } + if (!settings::webserver::save(copy)) { + LOGGER.error("Failed to persist WebServer settings; changes may be lost on reboot"); + return; + } // Publish event immediately after save so WebServer cache refreshes BEFORE requests arrive kernel::publishSystemEvent(kernel::SystemEvent::WebServerSettingsChanged);Tactility/Source/service/webserver/WebServerService.cpp (1)
104-171: Protect cached settings reads in auth validation.
validateRequestAuth()readsg_cachedSettingswithout the mutex while the settings callback updates it, which is a data race onstd::stringmembers. Copy settings under lock before comparisons.🔒 Proposed fix (copy under lock)
static esp_err_t validateRequestAuth(httpd_req_t* request, bool& authPassed) { authPassed = false; - if (!g_cachedSettings.webServerAuthEnabled) { + settings::webserver::WebServerSettings settings; + { + auto lock = g_settingsMutex.asScopedLock(); + lock.lock(); + settings = g_cachedSettings; + } + + if (!settings.webServerAuthEnabled) { authPassed = true; return ESP_OK; // Auth disabled, allow request } @@ - bool usernameMatch = secureCompare(username, g_cachedSettings.webServerUsername); - bool passwordMatch = secureCompare(password, g_cachedSettings.webServerPassword); + bool usernameMatch = secureCompare(username, settings.webServerUsername); + bool passwordMatch = secureCompare(password, settings.webServerPassword);
🧹 Nitpick comments (1)
Tactility/Source/app/webserversettings/WebServerSettings.cpp (1)
88-106: Avoid running asset sync on the UI dispatcher.
syncAssets()can be heavy I/O; running it inside the main dispatcher risks UI stalls. Consider offloading the sync to a worker/task and only updating the button state on the UI thread.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/network/HttpdReq.cpp (1)
79-115: Fix malloc/delete[] mismatch inreceiveByteArray.The buffer is allocated with
mallocat line 81 but returned asstd::unique_ptr<char[]>at line 115, which will calldelete[]on destruction. This is undefined behavior. Usenew (std::nothrow)to allocate, which properly matchesunique_ptr<char[]>'sdelete[]semantics while avoiding exceptions.Proposed fix
- // We have to use malloc() because make_unique() throws an exception - // and we don't have exceptions enabled in the compiler settings - auto* buffer = static_cast<char*>(malloc(length)); - if (buffer == nullptr) { + // Use nothrow new to avoid exceptions while keeping delete[] semantics. + auto buffer = std::unique_ptr<char[]>(new (std::nothrow) char[length]); + if (!buffer) { LOGGER.error(LOG_MESSAGE_ALLOC_FAILED_FMT, length); return nullptr; } @@ - int bytes_received = httpd_req_recv(request, buffer + bytesRead, read_size); + int bytes_received = httpd_req_recv(request, buffer.get() + bytesRead, read_size); @@ - free(buffer); - return nullptr; + return nullptr; @@ - free(buffer); - return nullptr; + return nullptr; @@ - return std::unique_ptr<char[]>(buffer); + return buffer;
🤖 Fix all issues with AI agents
In `@Tactility/Source/service/webserver/WebServerService.cpp`:
- Line 1301: Replace the manual tick conversion inside the lvgl::lock call with
the FreeRTOS macro pdMS_TO_TICKS to ensure correct ms-to-tick conversion across
configurations; locate the lvgl::lock(100 / portTICK_PERIOD_MS) call in
WebServerService.cpp and change the argument to use pdMS_TO_TICKS(100) so the
lock timeout uses the proper tick conversion.
- Around line 107-176: validateRequestAuth reads g_cachedSettings without
locking, causing a data race; fix by taking g_settingsMutex at the top of
validateRequestAuth and copying g_cachedSettings into a local variable (like the
pattern used in startServer()), then release the mutex and use that local
settings copy for all checks (webServerAuthEnabled, webServerUsername,
webServerPassword) instead of accessing g_cachedSettings directly; ensure all
references in validateRequestAuth (including the initial auth-enabled check and
the username/password comparisons) use the local settings variable.
🧹 Nitpick comments (1)
Tactility/Source/service/webserver/WebServerService.cpp (1)
520-538: Consider adding explicit escape sequences for\band\f.The
escapeJsonfunction handles common escapes but relies on\uXXXXencoding for backspace (\b) and form feed (\f). While technically valid JSON, explicit escape sequences are more readable and conventional.♻️ Optional improvement
for (char c : s) { switch (c) { case '"': o << "\\\""; break; case '\\': o << "\\\\"; break; + case '\b': o << "\\b"; break; + case '\f': o << "\\f"; break; case '\n': o << "\\n"; break; case '\r': o << "\\r"; break; case '\t': o << "\\t"; break;
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Tactility/Source/service/webserver/WebServerService.cpp`:
- Around line 729-734: Move the upload size check to before the parent-directory
creation: check request->content_len against MAX_UPLOAD_SIZE first, call
httpd_resp_send_err(..., "file too large") and return ESP_FAIL immediately if it
exceeds the limit, and only call file::findOrCreateParentDirectory(norm, 0755)
after the size check passes; update the code paths around
file::findOrCreateParentDirectory, request->content_len, MAX_UPLOAD_SIZE,
httpd_resp_send_err, and the return ESP_FAIL to reflect this reorder.
- Around line 174-181: There is a duplicate declaration and copy of settings
that redeclares settings::webserver::WebServerSettings settings and re-locks
g_settingsMutex; remove the entire redundant block (the second declaration, the
scoped lock acquisition using g_settingsMutex.asScopedLock(), lock.lock(), and
the assignment from g_cachedSettings) so the function reuses the
earlier-populated settings variable (from lines where settings was first copied)
and avoids redeclaration/duplicate locking with g_cachedSettings.
- Around line 1635-1642: setWebServerEnabled currently checks
g_webServerInstance and then dereferences it without synchronization, allowing a
TOCTOU if onStop can null it; fix by atomically reading g_webServerInstance into
a local pointer (or acquiring the appropriate service mutex) and then performing
the null check and calling setEnabled on that local copy, or alternatively
change g_webServerInstance to std::atomic<WebServerService*> and use load()
before use; reference setWebServerEnabled, g_webServerInstance and onStop when
updating the implementation.
- Around line 227-234: Read of g_cachedSettings.webServerEnabled must be done
while holding the settings mutex to avoid a race with the settings-changed
callback; modify the WebServerService code so you acquire the same mutex used to
protect g_cachedSettings before checking webServerEnabled (e.g., use the
existing settings mutex / lock_guard around the read), then log and call
setEnabled(...) while still under that lock or immediately after reading the
protected value into a local variable to ensure consistency; specifically update
the block that reads g_cachedSettings.webServerEnabled in WebServerService.cpp
to perform the read under the settings lock.
🧹 Nitpick comments (1)
Tactility/Source/service/webserver/WebServerService.cpp (1)
1571-1572: Consider using a bounded timeout for file locks.
lock->lock(portMAX_DELAY)blocks indefinitely. While this is safe in normal operation, a bounded timeout (e.g., 5 seconds) would prevent indefinite blocking if something goes wrong with the filesystem.♻️ Optional improvement
- lock->lock(portMAX_DELAY); + if (!lock->lock(pdMS_TO_TICKS(5000))) { + LOGGER.error("Timeout acquiring file lock for {}", dataPath); + httpd_resp_send_err(request, HTTPD_500_INTERNAL_SERVER_ERROR, "file lock timeout"); + return ESP_FAIL; + }
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
This PR adds a full-featured web server subsystem. It delivers:
/fs), API routes (/api/*), and admin functions (/admin/*).This enhances Tactility with browser-accessible device management and monitoring capabilities, laying the groundwork for richer remote interaction.
📊 Dashboard Features
✅ System Information
📁 File Management
/fs/tree,/fs/list)📦 Apps Management
/api/apps/*)📶 Wi-Fi & Network
/api/wifiendpoints📸 Screenshots
/api/screenshotendpoint)⚙️ Admin & Utility Actions
🔐 Authentication & Config UI