From 1fb4c4cfcdb8a9a682a7b59d0f03fb83765fd7e1 Mon Sep 17 00:00:00 2001 From: cjkas Date: Sun, 18 Oct 2026 14:34:18 +0200 Subject: [PATCH 1/2] fix: stop sending stored passwords and PIN back to clients The settings routes returned the WiFi passphrase, MQTT password and the login password and PIN in plain text, and the debug log printed them and the API key. Responses now carry hasPassphrase/hasPassword/hasPin flags, logs mask the values, and an omitted secret keeps the stored one. The UI loads secret fields empty and treats a blank field as "keep" while the SSID or username is unchanged. Clearing a secret without changing the SSID or username is listed in docs/KNOWN_ISSUES.md. Co-Authored-By: Claude Opus 5.5 --- data-src/index.html | 8 +-- data-src/index.js | 51 ++++++++++++++----- docs/KNOWN_ISSUES.md | 27 ++++++++++ src/ConfigSettings.cpp | 21 ++++---- src/Secrets.h | 25 +++++++++ src/entryPoints/Web.cpp | 1 - src/entryPoints/WebUiRoutes.cpp | 4 +- test/test_secrets/test_secrets.cpp | 81 ++++++++++++++++++++++++++++++ 8 files changed, 189 insertions(+), 29 deletions(-) create mode 100644 src/Secrets.h create mode 100644 test/test_secrets/test_secrets.cpp diff --git a/data-src/index.html b/data-src/index.html index 235f1bb..231be5a 100644 --- a/data-src/index.html +++ b/data-src/index.html @@ -207,11 +207,11 @@
- +
- +
@@ -305,7 +305,7 @@
- +
@@ -412,7 +412,7 @@
- +
diff --git a/data-src/index.js b/data-src/index.js index e435753..e67a839 100644 --- a/data-src/index.js +++ b/data-src/index.js @@ -1236,7 +1236,7 @@ class UIBinder { divCfg.style.display = 'none'; document.getElementById('icoConfig').className = 'icss-gear'; if (sockIsOpen) socket.send('leave:0'); - general.setSecurityConfig({ type: 0, username: '', password: '', pin: '', permissions: 0 }); + general.setSecurityConfig({ type: 0, username: '', permissions: 0 }); } toggleConfig() { if (this.isConfigOpen()) @@ -1617,20 +1617,22 @@ class General { } } setSecurityConfig(security) { + // The device never returns the stored password or PIN, only whether one is set. + this.savedSecurity = { type: security.type, username: security.username, hasPassword: makeBool(security.hasPassword), hasPin: makeBool(security.hasPin) }; // We need to transform the security object so that it can be set to the configuration. let obj = { security: { - type: security.type, username: security.username, password: security.password, + type: security.type, username: security.username, password: '', repeatpassword: '', permissions: { configOnly: makeBool(security.permissions & 0x01) }, - pin: { - d0: security.pin[0], - d1: security.pin[1], - d2: security.pin[2], - d3: security.pin[3] - } + pin: { d0: '', d1: '', d2: '', d3: '' } } }; - ui.toElement(document.getElementById('divSecurityOptions'), obj); + let pnl = document.getElementById('divSecurityOptions'); + ui.toElement(pnl, obj); + let keepHint = 'Saved (leave blank to keep)'; + pnl.querySelector('#fldPassword').placeholder = this.savedSecurity.hasPassword ? keepHint : 'Password'; + pnl.querySelector('#fldReenterPassword').placeholder = this.savedSecurity.hasPassword ? keepHint : 'Re-enter Password'; + pnl.querySelector('label[for="fldPinEntry"]').innerHTML = this.savedSecurity.hasPin ? 'Enter Pin (leave blank to keep)' : 'Enter Pin'; this.onSecurityTypeChanged(); } rebootDevice() { @@ -1674,9 +1676,17 @@ class General { sec.pin += security.pin[`d${i}`]; } sec.permissions |= security.permissions.configOnly ? 0x01 : 0x00; + // Blank secret fields keep the stored value while the type (and username) stay the same. + let saved = this.savedSecurity || {}; + let keepPin = security.type === 1 && sec.pin === '' && saved.hasPin && saved.type === 1; + let keepPassword = security.type === 2 && sec.password === '' && security.repeatpassword === '' && + saved.hasPassword && saved.type === 2 && sec.username === saved.username; + // Only send the secret for the selected type so the other one stays stored. + if (security.type !== 1 || keepPin) delete sec.pin; + if (security.type !== 2 || keepPassword) delete sec.password; let confirm = ''; console.log(sec); - if (security.type === 1) { // Pin Entry + if (security.type === 1 && !keepPin) { // Pin Entry // Make sure our pin is 4 digits. if (sec.pin.length !== 4) { ui.errorMessage('Invalid Pin').querySelector('.sub-message').innerHTML = 'Pins must be exactly 4 alpha-numeric values in length. Please enter a complete pin.'; @@ -1693,7 +1703,8 @@ class General { ui.errorMessage('Invalid Username').querySelector('.sub-message').innerHTML = 'The maximum username length is 32 characters.'; return; } - + } + if (security.type === 2 && !keepPassword) { // Password if (sec.password.length === 0) { ui.errorMessage('No Password Provided').querySelector('.sub-message').innerHTML = 'You must provide a password for password security.'; return; @@ -1803,6 +1814,11 @@ class Wifi { document.getElementById('cbHardwired').checked = settings.connType >= 2; document.getElementById('cbFallbackWireless').checked = settings.connType === 3; ui.toElement(pnl, settings); + // The device never returns the stored passphrase; leave the field blank to keep it. + this.savedSsid = settings.wifi.ssid; + let fldPassphrase = document.getElementById('fldPassphrase'); + fldPassphrase.value = ''; + fldPassphrase.placeholder = settings.wifi.hasPassphrase ? 'Saved (leave blank to keep)' : 'Passphrase'; /* if (settings.connType >= 2) { document.getElementById('divWiFiMode').style.display = 'none'; @@ -1850,7 +1866,7 @@ class Wifi { //document.getElementById('btnConnectWiFi').classList.remove('disabled'); console.log(aps); if (err) { - this.displayAPs({ connected: { name: '', passphrase: '' }, accessPoints: [] }); + this.displayAPs({ connected: { name: '' }, accessPoints: [] }); } else { this.displayAPs(aps); @@ -1955,6 +1971,7 @@ class Wifi { saveNetwork() { let pnl = document.getElementById('divNetAdapter'); let obj = ui.fromElement(pnl); + if (this.keepSavedPassphrase(obj.wifi.ssid, obj.wifi.passphrase)) delete obj.wifi.passphrase; obj.connType = obj.ethernet.hardwired ? (obj.ethernet.wirelessFallback ? 3 : 2) : 1; console.log(obj); if (obj.connType >= 2) { @@ -1991,6 +2008,8 @@ class Wifi { this.sendNetworkSettings(obj); } } + // A blank passphrase for the saved SSID keeps the stored one; for a new SSID it means an open network. + keepSavedPassphrase(ssid, passphrase) { return passphrase === '' && ssid === this.savedSsid; } sendNetworkSettings(obj) { putJSONSync('/setNetwork', obj, (err, response) => { if (err) { @@ -2014,6 +2033,7 @@ class Wifi { ui.errorMessage('Invalid Passphrase').querySelector('.sub-message').innerHTML = 'The maximum length of the passphrase is 64 characters.'; return; } + if (this.keepSavedPassphrase(obj.ssid, obj.passphrase)) delete obj.passphrase; let overlay = ui.waitMessage(document.getElementById('divNetAdapter')); @@ -4364,12 +4384,19 @@ class MQTT { else { console.log(settings); ui.toElement(document.getElementById('divMQTT'), { mqtt: settings }); + // The device never returns the stored password; leave the field blank to keep it. + this.savedUsername = settings.username; + let fldPassword = document.getElementById('fldMqttPassword'); + fldPassword.value = ''; + fldPassword.placeholder = settings.hasPassword ? 'Saved (leave blank to keep)' : 'Password'; document.getElementById('divDiscoveryTopic').style.display = settings.pubDisco ? '' : 'none'; } }); } connectMQTT() { let obj = ui.fromElement(document.getElementById('divMQTT')); + // A blank password keeps the stored one unless the username changed. + if (obj.mqtt.password === '' && obj.mqtt.username === this.savedUsername) delete obj.mqtt.password; console.log(obj); if (obj.mqtt.enabled) { if (typeof obj.mqtt.hostname !== 'string' || obj.mqtt.hostname.length === 0) { diff --git a/docs/KNOWN_ISSUES.md b/docs/KNOWN_ISSUES.md index f6516a1..bb8a968 100644 --- a/docs/KNOWN_ISSUES.md +++ b/docs/KNOWN_ISSUES.md @@ -105,3 +105,30 @@ It needs a group whose flags are out of date at the moment a client reads it, fo - [ADR-0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md): the loop task is the only writer of the shade model. - [Large websocket events are dropped](#large-websocket-events-are-dropped): the same `g_response` buffer. + +## A saved WiFi passphrase or MQTT password can't be cleared from the UI + +**Status:** open. A workaround exists (below). +**Introduced:** 2026-10-18, when the device stopped sending stored secrets back to clients. +**Location:** `keepSavedPassphrase` in `Wifi` and `connectMQTT` in `MQTT`, both in [data-src/index.js](../data-src/index.js). The firmware side is `applySecret` in [src/Secrets.h](../src/Secrets.h). + +### Summary + +The settings routes report only whether a secret is stored (`hasPassphrase`, `hasPassword`), so the UI loads every secret field empty. When the field is still blank on save, the UI treats it as "keep the saved value" and leaves the key out of the request, and `applySecret` keeps a value whose key is absent. Without that rule, saving any other setting on the page would wipe the secret. + +The cost: a blank field can't mean "no password" while the other half of the login stays the same. That is the SSID for WiFi and the username for MQTT. When it does change, a blank field is sent as an empty value and clears the secret, so switching to a different open network works. + +The login password and PIN aren't affected. Neither can be empty, and security is turned off with the type selector. + +### Concrete failure modes + +1. The network the device is on is changed to open on the router, keeping its SSID. Saving the network page with a blank passphrase keeps the old one. When a passphrase is set, `WiFi.begin` won't accept anything weaker than WPA2-PSK, so the device can't join the now-open network. +2. The MQTT broker stops requiring a password but keeps the username. The device keeps sending the old password. + +### Workaround + +Save once with a different SSID or MQTT username, then change it back and save again with the secret field blank. + +### Fix options + +- **(a) "Clear saved password" checkbox** next to the WiFi passphrase and MQTT password fields, shown only when a value is stored. Ticking it sends an explicit empty value, which `applySecret` already treats as "clear" (covered by `test_applySecret_empty_value_clears_stored`). This is a UI-only change. diff --git a/src/ConfigSettings.cpp b/src/ConfigSettings.cpp index b87cc38..68aaff3 100644 --- a/src/ConfigSettings.cpp +++ b/src/ConfigSettings.cpp @@ -5,6 +5,7 @@ #include #include "ConfigSettings.h" #include "Utils.h" +#include "Secrets.h" #include "esp_chip_info.h" #include "esp_log.h" @@ -301,7 +302,7 @@ bool MQTTSettings::toJSON(JsonObject &obj) { obj["hostname"] = this->hostname; obj["port"] = this->port; obj["username"] = this->username; - obj["password"] = this->password; + obj["hasPassword"] = hasSecret(this->password); obj["rootTopic"] = this->rootTopic; obj["discoTopic"] = this->discoTopic; return true; @@ -312,7 +313,7 @@ bool MQTTSettings::fromJSON(JsonObject &obj) { this->parseValueString(obj, "protocol", this->protocol, sizeof(this->protocol)); this->parseValueString(obj, "hostname", this->hostname, sizeof(this->hostname)); this->parseValueString(obj, "username", this->username, sizeof(this->username)); - this->parseValueString(obj, "password", this->password, sizeof(this->password)); + applySecret(obj["password"].as(), this->password, sizeof(this->password)); this->parseValueString(obj, "rootTopic", this->rootTopic, sizeof(this->rootTopic)); this->parseValueString(obj, "discoTopic", this->discoTopic, sizeof(this->discoTopic)); if(!obj["port"].isNull()) this->port = obj["port"]; @@ -478,16 +479,16 @@ bool SecuritySettings::begin() { bool SecuritySettings::fromJSON(JsonObject &obj) { if(!obj["type"].isNull()) this->type = static_cast(obj["type"].as()); this->parseValueString(obj, "username", this->username, sizeof(this->username)); - this->parseValueString(obj, "password", this->password, sizeof(this->password)); - this->parseValueString(obj, "pin", this->pin, sizeof(this->pin)); + applySecret(obj["password"].as(), this->password, sizeof(this->password)); + applySecret(obj["pin"].as(), this->pin, sizeof(this->pin)); if(!obj["permissions"].isNull()) this->permissions = obj["permissions"]; return true; } bool SecuritySettings::toJSON(JsonObject &obj) { obj["type"] = static_cast(this->type); obj["username"] = this->username; - obj["password"] = this->password; - obj["pin"] = this->pin; + obj["hasPassword"] = hasSecret(this->password); + obj["hasPin"] = hasSecret(this->pin); obj["permissions"] = this->permissions; return true; } @@ -516,7 +517,7 @@ bool SecuritySettings::load() { } void SecuritySettings::print() { ESP_LOGD(TAG, "SECURITY Type:%u Username:[%s] Password:[%s] Pin:[%s] Permissions:%u", - static_cast(this->type), this->username, this->password, this->pin, this->permissions); + static_cast(this->type), this->username, maskSecret(this->password), maskSecret(this->pin), this->permissions); } WifiSettings::WifiSettings() {} @@ -526,14 +527,14 @@ bool WifiSettings::begin() { } bool WifiSettings::fromJSON(JsonObject &obj) { this->parseValueString(obj, "ssid", this->ssid, sizeof(this->ssid)); - this->parseValueString(obj, "passphrase", this->passphrase, sizeof(this->passphrase)); + applySecret(obj["passphrase"].as(), this->passphrase, sizeof(this->passphrase)); if(!obj["roaming"].isNull()) this->roaming = obj["roaming"]; if(!obj["hidden"].isNull()) this->hidden = obj["hidden"]; return true; } bool WifiSettings::toJSON(JsonObject &obj) { obj["ssid"] = this->ssid; - obj["passphrase"] = this->passphrase; + obj["hasPassphrase"] = hasSecret(this->passphrase); obj["roaming"] = this->roaming; obj["hidden"] = this->hidden; return true; @@ -579,7 +580,7 @@ String WifiSettings::mapEncryptionType(int type) { return "Unknown"; } void WifiSettings::print() { - ESP_LOGD(TAG, "WIFI Settings SSID: [%s] PassPhrase: [%s]", this->ssid, this->passphrase); + ESP_LOGD(TAG, "WIFI Settings SSID: [%s] PassPhrase: [%s]", this->ssid, maskSecret(this->passphrase)); } void WifiSettings::printNetworks() { int n = WiFi.scanNetworks(false, false); diff --git a/src/Secrets.h b/src/Secrets.h new file mode 100644 index 0000000..53d1a08 --- /dev/null +++ b/src/Secrets.h @@ -0,0 +1,25 @@ +#ifndef secrets_h +#define secrets_h +#include + +// Stored secrets (WiFi passphrase, MQTT password, login password/PIN) are +// write-only: responses carry hasSecret() flags and logs carry maskSecret(). + +inline bool hasSecret(const char *s) { return s && s[0] != '\0'; } + +inline const char *maskSecret(const char *s) { return hasSecret(s) ? "" : ""; } + +// Updates dest from an incoming value, truncating to size. A null incoming +// value means the client left the field out, so the stored secret is kept. +// Pass obj["key"].as(), which is null when the key is absent. +// Returns true when dest changed. +inline bool applySecret(const char *incoming, char *dest, size_t size) { + if(!incoming || size == 0) return false; + size_t len = strlen(incoming); + if(len >= size) len = size - 1; + if(strncmp(dest, incoming, len) == 0 && dest[len] == '\0') return false; + memcpy(dest, incoming, len); + dest[len] = '\0'; + return true; +} +#endif diff --git a/src/entryPoints/Web.cpp b/src/entryPoints/Web.cpp index f84634f..f3e988d 100644 --- a/src/entryPoints/Web.cpp +++ b/src/entryPoints/Web.cpp @@ -125,7 +125,6 @@ bool Web::createAPIToken(const char *payload, char *token) { sprintf(str, "%02x", (int)hmacResult[i]); strcat(token, str); } - ESP_LOGD(TAG, "Hash: %s", token); return true; } bool Web::createAPIToken(const IPAddress ipAddress, char *token) { diff --git a/src/entryPoints/WebUiRoutes.cpp b/src/entryPoints/WebUiRoutes.cpp index e46176b..0e19c31 100644 --- a/src/entryPoints/WebUiRoutes.cpp +++ b/src/entryPoints/WebUiRoutes.cpp @@ -621,7 +621,6 @@ void registerWebUiRoutes(AsyncWebServer &server) { resp.beginObject(); resp.beginObject("connected"); resp.addElem("name", settings.WIFI.ssid); - resp.addElem("passphrase", settings.WIFI.passphrase); resp.addElem("strength", (int32_t)WiFi.RSSI()); resp.addElem("channel", (int32_t)WiFi.channel()); resp.endObject(); @@ -834,7 +833,8 @@ void registerWebUiRoutes(AsyncWebServer &server) { JsonObject obj = json.as(); ESP_LOGD(TAG, "Settings WIFI connection..."); String ssid = ""; - String passphrase = ""; + // Clients never receive the stored passphrase, so an omitted one keeps it. + String passphrase = settings.WIFI.passphrase; if(!obj["ssid"].isNull()) ssid = obj["ssid"].as(); if(!obj["passphrase"].isNull()) passphrase = obj["passphrase"].as(); bool reboot = false; diff --git a/test/test_secrets/test_secrets.cpp b/test/test_secrets/test_secrets.cpp new file mode 100644 index 0000000..970d0c9 --- /dev/null +++ b/test/test_secrets/test_secrets.cpp @@ -0,0 +1,81 @@ +#include +#include "Secrets.h" + +void setUp() {} +void tearDown() {} + +/********************************************************************* + * hasSecret / maskSecret + ********************************************************************/ +static void test_hasSecret_null_and_empty_are_unset() { + TEST_ASSERT_FALSE(hasSecret(nullptr)); + TEST_ASSERT_FALSE(hasSecret("")); +} +static void test_hasSecret_any_value_is_set() { + TEST_ASSERT_TRUE(hasSecret("x")); + TEST_ASSERT_TRUE(hasSecret(" ")); +} +static void test_maskSecret_never_returns_the_secret() { + TEST_ASSERT_EQUAL_STRING("", maskSecret("hunter2")); + TEST_ASSERT_EQUAL_STRING("", maskSecret("")); + TEST_ASSERT_EQUAL_STRING("", maskSecret(nullptr)); +} + +/********************************************************************* + * applySecret + ********************************************************************/ +static void test_applySecret_absent_keeps_stored_value() { + char dest[16] = "stored"; + TEST_ASSERT_FALSE(applySecret(nullptr, dest, sizeof(dest))); + TEST_ASSERT_EQUAL_STRING("stored", dest); +} +static void test_applySecret_new_value_replaces_stored() { + char dest[16] = "stored"; + TEST_ASSERT_TRUE(applySecret("fresh", dest, sizeof(dest))); + TEST_ASSERT_EQUAL_STRING("fresh", dest); +} +static void test_applySecret_same_value_is_no_change() { + char dest[16] = "stored"; + TEST_ASSERT_FALSE(applySecret("stored", dest, sizeof(dest))); + TEST_ASSERT_EQUAL_STRING("stored", dest); +} +static void test_applySecret_prefix_of_stored_is_a_change() { + char dest[16] = "stored"; + TEST_ASSERT_TRUE(applySecret("sto", dest, sizeof(dest))); + TEST_ASSERT_EQUAL_STRING("sto", dest); +} +// Sending an empty value explicitly clears it, e.g. an open WiFi network. +static void test_applySecret_empty_value_clears_stored() { + char dest[16] = "stored"; + TEST_ASSERT_TRUE(applySecret("", dest, sizeof(dest))); + TEST_ASSERT_EQUAL_STRING("", dest); +} +static void test_applySecret_truncates_and_terminates() { + char storage[8]; + memset(storage, 'X', sizeof(storage)); + storage[0] = '\0'; + TEST_ASSERT_TRUE(applySecret("abcdefgh", storage, 5)); + TEST_ASSERT_EQUAL_STRING("abcd", storage); + // Nothing past the buffer size is touched. + TEST_ASSERT_EQUAL_HEX8('X', storage[5]); +} +static void test_applySecret_truncated_same_value_is_no_change() { + char dest[5] = "abcd"; + TEST_ASSERT_FALSE(applySecret("abcdefgh", dest, sizeof(dest))); + TEST_ASSERT_EQUAL_STRING("abcd", dest); +} + +int main(int argc, char **argv) { + UNITY_BEGIN(); + RUN_TEST(test_hasSecret_null_and_empty_are_unset); + RUN_TEST(test_hasSecret_any_value_is_set); + RUN_TEST(test_maskSecret_never_returns_the_secret); + RUN_TEST(test_applySecret_absent_keeps_stored_value); + RUN_TEST(test_applySecret_new_value_replaces_stored); + RUN_TEST(test_applySecret_same_value_is_no_change); + RUN_TEST(test_applySecret_prefix_of_stored_is_a_change); + RUN_TEST(test_applySecret_empty_value_clears_stored); + RUN_TEST(test_applySecret_truncates_and_terminates); + RUN_TEST(test_applySecret_truncated_same_value_is_no_change); + return UNITY_END(); +} From 9383ebec2e9061d407ff48eaa232bf5506a4e62b Mon Sep 17 00:00:00 2001 From: cjkas Date: Sun, 18 Oct 2026 20:07:31 +0200 Subject: [PATCH 2/2] fix: keep stored secrets when an older UI sends them empty Firmware and UI files update separately, so an old index.js can run against firmware that no longer returns secrets. It sends "" for the field it can't fill in, which cleared the WiFi passphrase (reboot into AP mode) or the MQTT password. The firmware now decides: "" keeps the stored secret, unless the same request changes the SSID or MQTT username, then it clears it (open network). /setNetwork compares the settings after applying them, so a kept passphrase no longer triggers a reboot. The UI cache-buster moves to 3.0.14d. Co-Authored-By: Claude Opus 5.5 --- data-src/index.html | 8 ++++---- docs/KNOWN_ISSUES.md | 19 ++++++++++++----- src/ConfigSettings.cpp | 10 +++++++-- src/Secrets.h | 13 ++++++++---- src/entryPoints/WebUiRoutes.cpp | 28 ++++++++++++------------- test/test_secrets/test_secrets.cpp | 33 ++++++++++++++++++++++++++---- 6 files changed, 77 insertions(+), 34 deletions(-) diff --git a/data-src/index.html b/data-src/index.html index 231be5a..0d9c41c 100644 --- a/data-src/index.html +++ b/data-src/index.html @@ -8,9 +8,9 @@ - - - + + + @@ -114,7 +114,7 @@ rel="apple-touch-startup-image"> - +
diff --git a/docs/KNOWN_ISSUES.md b/docs/KNOWN_ISSUES.md index bb8a968..f083de3 100644 --- a/docs/KNOWN_ISSUES.md +++ b/docs/KNOWN_ISSUES.md @@ -110,13 +110,21 @@ It needs a group whose flags are out of date at the moment a client reads it, fo **Status:** open. A workaround exists (below). **Introduced:** 2026-10-18, when the device stopped sending stored secrets back to clients. -**Location:** `keepSavedPassphrase` in `Wifi` and `connectMQTT` in `MQTT`, both in [data-src/index.js](../data-src/index.js). The firmware side is `applySecret` in [src/Secrets.h](../src/Secrets.h). +**Location:** `applySecret` in [src/Secrets.h](../src/Secrets.h), called from `WifiSettings::fromJSON` and `MQTTSettings::fromJSON` in [src/ConfigSettings.cpp](../src/ConfigSettings.cpp) and from `/connectwifi`. The UI side is `keepSavedPassphrase` in `Wifi` and `connectMQTT` in `MQTT`, in [data-src/index.js](../data-src/index.js). ### Summary -The settings routes report only whether a secret is stored (`hasPassphrase`, `hasPassword`), so the UI loads every secret field empty. When the field is still blank on save, the UI treats it as "keep the saved value" and leaves the key out of the request, and `applySecret` keeps a value whose key is absent. Without that rule, saving any other setting on the page would wipe the secret. +The settings routes report only whether a secret is stored (`hasPassphrase`, `hasPassword`), so the UI loads every secret field empty. The firmware decides what an empty secret in a request means, so that every UI version is safe, including one older than this change, which sends `""` for the field it can no longer fill in: -The cost: a blank field can't mean "no password" while the other half of the login stays the same. That is the SSID for WiFi and the username for MQTT. When it does change, a blank field is sent as an empty value and clears the secret, so switching to a different open network works. +| Secret | Key absent | `""` | A value | +|---|---|---|---| +| WiFi passphrase | keep | keep, or clear if the same request changes the SSID | replace | +| MQTT password | keep | keep, or clear if the same request changes the username | replace | +| Login password, PIN | keep | keep | replace | + +Without "empty keeps", saving any other setting from an old UI would wipe the secret: for WiFi the device would reboot and drop into AP mode. + +The cost: an empty value can't mean "no password" while the SSID or username stays the same. Switching to a different open network works. The login password and PIN aren't affected. Neither can be empty, and security is turned off with the type selector. @@ -127,8 +135,9 @@ The login password and PIN aren't affected. Neither can be empty, and security i ### Workaround -Save once with a different SSID or MQTT username, then change it back and save again with the secret field blank. +- **MQTT:** save once with a different username, then change it back and save again with the password blank. +- **WiFi:** there's no safe workaround. Saving a different SSID makes the device reboot and try to join that network. ### Fix options -- **(a) "Clear saved password" checkbox** next to the WiFi passphrase and MQTT password fields, shown only when a value is stored. Ticking it sends an explicit empty value, which `applySecret` already treats as "clear" (covered by `test_applySecret_empty_value_clears_stored`). This is a UI-only change. +- **(a) "Clear saved password" checkbox** next to the WiFi passphrase and MQTT password fields, shown only when a value is stored. Since `""` means keep, ticking it sends an explicit flag (`clearPassphrase` / `clearPassword`), which `fromJSON` turns into `applySecret(..., identityChanged = true)`. diff --git a/src/ConfigSettings.cpp b/src/ConfigSettings.cpp index 68aaff3..c796caa 100644 --- a/src/ConfigSettings.cpp +++ b/src/ConfigSettings.cpp @@ -312,8 +312,11 @@ bool MQTTSettings::fromJSON(JsonObject &obj) { if(!obj["pubDisco"].isNull()) this->pubDisco = obj["pubDisco"]; this->parseValueString(obj, "protocol", this->protocol, sizeof(this->protocol)); this->parseValueString(obj, "hostname", this->hostname, sizeof(this->hostname)); + // An empty password clears the stored one only alongside a new username. + const char *username = obj["username"].as(); + bool newUsername = username && strcmp(username, this->username) != 0; this->parseValueString(obj, "username", this->username, sizeof(this->username)); - applySecret(obj["password"].as(), this->password, sizeof(this->password)); + applySecret(obj["password"].as(), this->password, sizeof(this->password), newUsername); this->parseValueString(obj, "rootTopic", this->rootTopic, sizeof(this->rootTopic)); this->parseValueString(obj, "discoTopic", this->discoTopic, sizeof(this->discoTopic)); if(!obj["port"].isNull()) this->port = obj["port"]; @@ -526,8 +529,11 @@ bool WifiSettings::begin() { return true; } bool WifiSettings::fromJSON(JsonObject &obj) { + // An empty passphrase clears the stored one only alongside a new SSID (an open network). + const char *ssid = obj["ssid"].as(); + bool newSsid = ssid && strcmp(ssid, this->ssid) != 0; this->parseValueString(obj, "ssid", this->ssid, sizeof(this->ssid)); - applySecret(obj["passphrase"].as(), this->passphrase, sizeof(this->passphrase)); + applySecret(obj["passphrase"].as(), this->passphrase, sizeof(this->passphrase), newSsid); if(!obj["roaming"].isNull()) this->roaming = obj["roaming"]; if(!obj["hidden"].isNull()) this->hidden = obj["hidden"]; return true; diff --git a/src/Secrets.h b/src/Secrets.h index 53d1a08..64937bf 100644 --- a/src/Secrets.h +++ b/src/Secrets.h @@ -9,12 +9,17 @@ inline bool hasSecret(const char *s) { return s && s[0] != '\0'; } inline const char *maskSecret(const char *s) { return hasSecret(s) ? "" : ""; } -// Updates dest from an incoming value, truncating to size. A null incoming -// value means the client left the field out, so the stored secret is kept. -// Pass obj["key"].as(), which is null when the key is absent. +// Updates dest from an incoming value, truncating to size. Pass +// obj["key"].as(), which is null when the key is absent. +// absent -> keep the stored secret. +// "" -> keep it too, unless identityChanged (a new SSID or username in +// the same request), then clear it. UIs older than write-only +// secrets send "" for a field the device no longer fills in. +// value -> replace it. // Returns true when dest changed. -inline bool applySecret(const char *incoming, char *dest, size_t size) { +inline bool applySecret(const char *incoming, char *dest, size_t size, bool identityChanged = false) { if(!incoming || size == 0) return false; + if(incoming[0] == '\0' && !identityChanged) return false; size_t len = strlen(incoming); if(len >= size) len = size - 1; if(strncmp(dest, incoming, len) == 0 && dest[len] == '\0') return false; diff --git a/src/entryPoints/WebUiRoutes.cpp b/src/entryPoints/WebUiRoutes.cpp index 0e19c31..de5dd67 100644 --- a/src/entryPoints/WebUiRoutes.cpp +++ b/src/entryPoints/WebUiRoutes.cpp @@ -6,6 +6,7 @@ #include "ConfigSettings.h" #include "ConfigFile.h" #include "Utils.h" +#include "Secrets.h" #include "SSDP.h" #include "Somfy.h" #include "WResp.h" @@ -781,16 +782,13 @@ void registerWebUiRoutes(AsyncWebServer &server) { } if(!obj["wifi"].isNull()) { JsonObject objWifi = obj["wifi"]; - if(settings.connType == conn_types_t::wifi) { - if(!objWifi["ssid"].isNull() && objWifi["ssid"].as().compareTo(settings.WIFI.ssid) != 0) { - if(WiFi.softAPgetStationNum() == 0) reboot = true; - } - if(!objWifi["passphrase"].isNull() && objWifi["passphrase"].as().compareTo(settings.WIFI.passphrase) != 0) { - if(WiFi.softAPgetStationNum() == 0) reboot = true; - } - } + // Compared after fromJSON, which decides whether an empty passphrase keeps the stored one. + WifiSettings before = settings.WIFI; settings.WIFI.fromJSON(objWifi); settings.WIFI.save(); + if(settings.connType == conn_types_t::wifi && WiFi.softAPgetStationNum() == 0 && + (strcmp(before.ssid, settings.WIFI.ssid) != 0 || strcmp(before.passphrase, settings.WIFI.passphrase) != 0)) + reboot = true; } if(!obj["ethernet"].isNull()) { JsonObject objEth = obj["ethernet"]; @@ -833,19 +831,19 @@ void registerWebUiRoutes(AsyncWebServer &server) { JsonObject obj = json.as(); ESP_LOGD(TAG, "Settings WIFI connection..."); String ssid = ""; - // Clients never receive the stored passphrase, so an omitted one keeps it. - String passphrase = settings.WIFI.passphrase; if(!obj["ssid"].isNull()) ssid = obj["ssid"].as(); - if(!obj["passphrase"].isNull()) passphrase = obj["passphrase"].as(); - bool reboot = false; - if(ssid.compareTo(settings.WIFI.ssid) != 0) reboot = true; - if(passphrase.compareTo(settings.WIFI.passphrase) != 0) reboot = true; + bool newSsid = ssid.compareTo(settings.WIFI.ssid) != 0; + // Clients never receive the stored passphrase: an omitted or empty one keeps it unless the SSID changes. + char passphrase[sizeof(settings.WIFI.passphrase)]; + SETCHARPROP(passphrase, settings.WIFI.passphrase, sizeof(passphrase)); + bool newPassphrase = applySecret(obj["passphrase"].as(), passphrase, sizeof(passphrase), newSsid); + bool reboot = newSsid || newPassphrase; if(!settings.WIFI.ssidExists(ssid.c_str()) && ssid.length() > 0) { request->send(400, _encoding_json, "{\"status\":\"ERROR\",\"desc\":\"WiFi Network Does not exist\"}"); } else { SETCHARPROP(settings.WIFI.ssid, ssid.c_str(), sizeof(settings.WIFI.ssid)); - SETCHARPROP(settings.WIFI.passphrase, passphrase.c_str(), sizeof(settings.WIFI.passphrase)); + SETCHARPROP(settings.WIFI.passphrase, passphrase, sizeof(settings.WIFI.passphrase)); settings.WIFI.save(); settings.WIFI.print(); request->send(201, _encoding_json, "{\"status\":\"OK\",\"desc\":\"Successfully set server connection\"}"); diff --git a/test/test_secrets/test_secrets.cpp b/test/test_secrets/test_secrets.cpp index 970d0c9..07fe321 100644 --- a/test/test_secrets/test_secrets.cpp +++ b/test/test_secrets/test_secrets.cpp @@ -44,12 +44,33 @@ static void test_applySecret_prefix_of_stored_is_a_change() { TEST_ASSERT_TRUE(applySecret("sto", dest, sizeof(dest))); TEST_ASSERT_EQUAL_STRING("sto", dest); } -// Sending an empty value explicitly clears it, e.g. an open WiFi network. -static void test_applySecret_empty_value_clears_stored() { +// An older UI sends "" for a field the device no longer fills in. +static void test_applySecret_empty_value_keeps_stored() { char dest[16] = "stored"; - TEST_ASSERT_TRUE(applySecret("", dest, sizeof(dest))); + TEST_ASSERT_FALSE(applySecret("", dest, sizeof(dest))); + TEST_ASSERT_EQUAL_STRING("stored", dest); +} +// A new SSID or username with an empty secret, e.g. an open WiFi network. +static void test_applySecret_empty_value_with_new_identity_clears_stored() { + char dest[16] = "stored"; + TEST_ASSERT_TRUE(applySecret("", dest, sizeof(dest), true)); TEST_ASSERT_EQUAL_STRING("", dest); } +static void test_applySecret_empty_value_with_new_identity_and_nothing_stored() { + char dest[16] = ""; + TEST_ASSERT_FALSE(applySecret("", dest, sizeof(dest), true)); + TEST_ASSERT_EQUAL_STRING("", dest); +} +static void test_applySecret_new_identity_still_takes_a_value() { + char dest[16] = "stored"; + TEST_ASSERT_TRUE(applySecret("fresh", dest, sizeof(dest), true)); + TEST_ASSERT_EQUAL_STRING("fresh", dest); +} +static void test_applySecret_absent_keeps_stored_even_with_new_identity() { + char dest[16] = "stored"; + TEST_ASSERT_FALSE(applySecret(nullptr, dest, sizeof(dest), true)); + TEST_ASSERT_EQUAL_STRING("stored", dest); +} static void test_applySecret_truncates_and_terminates() { char storage[8]; memset(storage, 'X', sizeof(storage)); @@ -74,7 +95,11 @@ int main(int argc, char **argv) { RUN_TEST(test_applySecret_new_value_replaces_stored); RUN_TEST(test_applySecret_same_value_is_no_change); RUN_TEST(test_applySecret_prefix_of_stored_is_a_change); - RUN_TEST(test_applySecret_empty_value_clears_stored); + RUN_TEST(test_applySecret_empty_value_keeps_stored); + RUN_TEST(test_applySecret_empty_value_with_new_identity_clears_stored); + RUN_TEST(test_applySecret_empty_value_with_new_identity_and_nothing_stored); + RUN_TEST(test_applySecret_new_identity_still_takes_a_value); + RUN_TEST(test_applySecret_absent_keeps_stored_even_with_new_identity); RUN_TEST(test_applySecret_truncates_and_terminates); RUN_TEST(test_applySecret_truncated_same_value_is_no_change); return UNITY_END();