Auth consolidation across a few routes - #3
Open
xn101de wants to merge 6 commits into
Open
Conversation
These three routes are registered directly on the AsyncWebServer and call request->send(LittleFS, "/config.json", ...) without ever going through isAuthenticated() - unlike the static UI asset routes just above them, which all use sendGzipChunkedResponse(..., checkAuth, ...) with an explicit auth check. config.json holds the device's WiFi password, MQTT password, and (for consumers of this library) its own web-UI login password, "encrypted" with a key that's compiled into the (often open-source) firmware - so anyone who can fetch the file can recover the plaintext. With setAuthentication(true) enabled, the main UI correctly redirects to /login, but curl <device-ip>/config.json still returned the full file with no credentials at all. Enabling auth on a device that stores its own login password in this file arguably made the exposure worse. Fix: check isAuthenticated() at the top of all three handlers (redirect to /login for the two GET routes, 401 JSON for the POST upload, matching the response style /login itself already uses for a failed attempt), and check it again in the upload's chunk callback so an unauthenticated request can't write chunks to config.json even though the final response already reports 401. Verified by pointing a real downstream project (ESP_Buderus_KM271) at this branch via a local symlink dependency and rebuilding end to end: 0 errors, 0 warnings. Fixes dewenni#2
login() accepts the fixed pair esp/xxx alongside the configured credentials. That is convenient while bringing a board up, but it is compiled into release builds too, so on a device reachable from the LAN it stays available as a second way into an admin session - including on devices that have set a password precisely to avoid that. Dropping it leaves the configured username/password as the only route in, which also keeps it consistent with the isAuthenticated() checks in 02f3ac5. Anyone who wants the old shortcut during development can just set the configured credentials to esp/xxx.
The OTA firmware-upload endpoint had no auth check at all, on either the request handler or the chunked upload handler that actually calls Update.write(). Anyone on the LAN could flash arbitrary firmware without any credentials - the most severe of the auth gaps found so far, since this device monitors a live oil-fired boiler.
The /ws WebSocket upgrade was the only entry point into the app that never checked isAuthenticated() - the HTTP routes were gated but every control message (cfg_*, boiler commands, even cfg_auth_enable itself) goes through this socket instead. Reject the upgrade at the connection middleware, where the request (and its session cookie) is still available, so an unauthenticated client can never open a socket at all.
The upload handler opened /config.json with "w", truncating it on the very first chunk. If the browser tab was closed mid-upload, or the WiFi link dropped, the "final" callback never fired and the file was left as a truncated JSON prefix, with the static File handle never closed. For consumers that fall back to defaults when their config fails to parse, that turns a cancelled upload into a device that comes back up with no WiFi credentials - i.e. unreachable without physical access. Upload into /config.json.upload instead and only move it into place once the last chunk has arrived.
setCredentials() strncpy'd into char[32], so anything longer than 31 characters was silently truncated. Consumers that store the same credential in a wider field and check it themselves elsewhere - e.g. a telnet console sharing config.auth.password - then disagree with the web login about what the password is: the first 31 characters authenticate here while the full string is required there. That penalises exactly the user who deliberately chose a long passphrase, and it fails silently in both directions.
xn101de
pushed a commit
to xn101de/ESP_Buderus_KM271
that referenced
this pull request
Aug 19, 2026
The fork branch backing dewenni/EspWebUI#3 was rebased to reword one commit message, so d355006 is no longer reachable from its main. The tree content is byte-identical; 8475a72 is the new tip.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Small follow-up to #2 - while I was in this area I went through the
rest of the routes and made auth handling a bit more consistent:
configured one
as the other file-handling routes, on both the request and upload
handlers
Verified by rebuilding a downstream project (dewenni/ESP_Buderus_KM271)
against this branch - clean build, no warnings. Happy to adjust if any
of this was intentional.