fix(webconfig): stop the CLI reading secrets, and enforce the setup password

Two findings from review, both real, both mine.

The CLI could read secrets the portal has never exposed. CommonCLI splits its
surface by CALLER, not by command: a serial caller (sender_timestamp 0, physical
access) reads secrets in plaintext, a remote one gets "******** (serial only)".
Its own comments say so — "Serial only (WiFi creds grant LAN access); remote
sees set/unset". execCommand passes 0, which is what makes `erase`, `stats-*`
and `set freq` reachable at all, and with it the terminal inherited the serial
console's plaintext answers for an HTTP request: `get prv.key` returned this
node's identity, `get wifi.pwd` the operator's network.

Worse in setup mode, which authenticates by proximity to an open AP — and `start
webconfig ap` can be run on an already-configured node, so the secrets are real
by then, not blank.

I had reasoned that the AP was the trust boundary either way because the wizard
can already rewrite these. That conflated two capabilities: replacing a WiFi
password does not reveal the current one, and replacing an identity does not
reveal the existing private key. /api/config has always masked these on read
(wcIsSecretKey); the CLI simply broke that rule. Now only the READ is masked —
the command surface stays whole — in CommonCLI's own words, keeping the
set/unset signal that is the useful part.

Onboarding could also skip the mandatory password. handleConfigPost refuses to
arm a reboot during initial setup without one; the CLI only warned in the
browser, which a pasted script or a direct POST ignores, so a node could reboot
onto the LAN still holding the factory credential. Same rule now applies at
POST. It is satisfied by a `password` command anywhere in the session rather
than only in the same request, so the natural two-step console flow still works
— the form batch always sends both together and never needed that memory.

wcIsSecretReadCommand lives in WebConfigKeys.h beside the rest of the secret
classification, pinned by three host tests: what must be masked, what must not,
and that only reads are touched. 17 keys + 24 batch tests pass; the audit checks
a masked read round-trips as masked.

scripts/webconfig_cli_audit.py

@@ -105,7 +105,10 @@ ROUND_TRIPS = [
     ("set mqtt.neighbors on", "get mqtt.neighbors", "on"),
     ("set path.hash.mode 2", "get path.hash.mode", "2"),
     ("set mqtt.iata den", "get mqtt.iata", "DEN"),
-    ("set guest.password hunter2", "get guest.password", "********"),
+    # Secret reads are masked back down for an HTTP caller, in CommonCLI's own
+    # words for a non-serial one (wcIsSecretReadCommand).
+    ("set guest.password hunter2", "get guest.password", "******** (serial only)"),
+    ("set wifi.pwd hunter2", "get wifi.pwd", "******** (serial only)"),
 ]
 
 

scripts/webconfig_mock_server.py

@@ -156,6 +156,7 @@ class State:
         self.session = None            # cookie token when logged in (LAN mode)
         self.batch = {"state": "idle"}
         self.cli = {"state": "idle"}   # deferred CLI sequence, see /api/cli
+        self.admin_pwd_set = False     # satisfies the initial-setup invariant
         self.scan_started = None
 
     # ---- auth -------------------------------------------------------------
@@ -467,6 +468,17 @@ def cli_unavailable(cmd):
 # Failure replies CommonCLI emits that do NOT start with "Err" — the shapes that
 # made a naive prefix test call them success. Mirrors
 # WebConfigBatch::cliReplyIsFailure.
+def cli_reads_secret(cmd):
+    """Commands that READ a secret. CommonCLI gates these on the caller being
+    the serial console; the portal is not, so the value is masked here the way
+    CommonCLI masks it for remote callers. Mirrors wcCliReadsSecret()."""
+    if not cmd.startswith("get "):
+        return False
+    key = cmd[4:].strip()
+    return key in ("prv.key", "guest.password", "alert.psk", "bridge.secret") \
+        or is_secret_key(key)
+
+
 def cli_reply_is_failure(reply):
     if not reply:
         return False
@@ -907,6 +919,15 @@ class Handler(BaseHTTPRequestHandler):
             why = cli_unavailable(c)
             if why:
                 return self._json(400, {"error": why})
+        # Same invariant handleConfigPost enforces (see wcCliUnavailable's
+        # neighbour in WebConfigServer.cpp): first onboarding is committed by the
+        # reboot, and must not commit the factory password onto someone's LAN.
+        if (ST.setup_mode and ST.initial_setup and not ST.admin_pwd_set
+                and not any(c.startswith("password ") for c in cmds)
+                and (any(c.startswith(CLI_DEFERRED_REBOOT) for c in cmds)
+                     or any(c.startswith("set wifi.ssid ") for c in cmds))):
+            return self._json(400, {"error": "admin password required for initial setup — "
+                                             "run `password <new-password>` first"})
 
         with ST.lock:
             self._cli_advance(ST.cli)
@@ -936,6 +957,11 @@ class Handler(BaseHTTPRequestHandler):
                 _, reply = run_cli(ST.cfg, cmd)
                 if cmd.startswith("password "):
                     reply = "OK"      # never echo the new password back
+                    ST.admin_pwd_set = True
+                elif cli_reads_secret(cmd):
+                    val = reply[2:] if reply.startswith("> ") else reply
+                    reply = ("> (not set)" if val in ("", "(not set)")
+                             else "> ******** (serial only)")
             ok = not cli_reply_is_failure(reply)
             # Only writes gate the reboot, and only on the "OK" convention every
             # setter keeps (WebConfigBatch::cliReplyGatesReboot).

src/helpers/WebConfigKeys.h

@@ -82,6 +82,28 @@ static inline bool wcIsSecretKey(const char* key) {
   return false;
 }
 
+// CommonCLI answers a secret getter in plaintext only for the serial console
+// (sender_timestamp 0) and masks it for remote callers. The web CLI executes
+// with sender_timestamp 0 — that is what makes `erase`, `stats-*` and `set freq`
+// reachable — so it would otherwise inherit the serial console's plaintext
+// answers for an HTTP request. This says which `get` commands must be masked
+// back down, restoring the distinction for a caller not at the serial port.
+//
+// Writing these has always been possible from the portal; reading them never
+// was, because handleConfigGet masks them (wcIsSecretKey). The two are different
+// capabilities: replacing a WiFi password does not reveal the current one, and
+// replacing an identity does not reveal the existing private key.
+static inline bool wcIsSecretReadCommand(const char* cmd) {
+  if (strncmp(cmd, "get ", 4) != 0) return false;
+  const char* key = cmd + 4;
+  while (*key == ' ') key++;
+  if (strcmp(key, "prv.key") == 0) return true;         // this node's identity
+  if (strcmp(key, "guest.password") == 0) return true;
+  if (strcmp(key, "alert.psk") == 0) return true;
+  if (strcmp(key, "bridge.secret") == 0) return true;
+  return wcIsSecretKey(key);   // wifi.pwd, mqttN.password, mqttN.token
+}
+
 // Browser-generated request IDs are exactly eight random bytes encoded as
 // hexadecimal. Keeping the grammar deliberately small makes the ID safe to
 // echo in JSON/logs and prevents an empty or truncated ID from weakening the

src/helpers/esp32/WebConfigServer.cpp

@@ -79,6 +79,34 @@ static const char* wcCliUnavailable(const char* cmd) {
 static inline bool wcCliEchoesSecret(const char* cmd) {
   return strncmp(cmd, "password ", 9) == 0;
 }
+
+// CommonCLI splits its surface by CALLER, not by command: a serial caller
+// (sender_timestamp 0, physical access) reads secrets in plaintext, while a
+// remote one gets "******** (serial only)". Its own comments say so — "Serial
+// only (WiFi creds grant LAN access); remote sees set/unset".
+//
+// execCommand passes 0, which is what makes `erase`, `stats-*` and `set freq`
+// reachable from the terminal at all. Left alone, that also claims
+// physical-access trust for an HTTP request: `get prv.key` would hand this
+// node's identity to anyone associated with the open setup AP, and `get
+// wifi.pwd` would hand over the operator's network. Reading a secret and
+// writing one are not the same capability — the wizard has always been able to
+// REPLACE these; nothing in the portal could ever READ them, because
+// /api/config masks them (wcIsSecretKey).
+//
+// So the command surface stays whole and only the READ is masked, restoring the
+// distinction CommonCLI intended for a caller who is not at the serial port.
+// Which commands those are lives in WebConfigKeys.h (wcIsSecretReadCommand),
+// beside the rest of the secret classification and host-tested with it.
+
+// Keep the set/unset signal, which is the useful part and what CommonCLI itself
+// reports remotely; only the value goes. A getter answers "> value".
+static void wcMaskSecretReply(char* reply) {
+  const char* val = reply;
+  if (val[0] == '>' && val[1] == ' ') val += 2;
+  const bool unset = (val[0] == 0 || strcmp(val, "(not set)") == 0);
+  strcpy(reply, unset ? "> (not set)" : "> ******** (serial only)");
+}
 // Reply classification lives in WebConfigBatch.h with the rest of the decisions
 // (WebConfigBatch::cliReplyIsFailure / cliReplyGatesReboot / cliWriteSucceeded),
 // so the shapes CommonCLI actually emits are enumerated in one host-tested place.
@@ -388,7 +416,13 @@ void WebConfigServer::drainBatch(uint32_t now) {
       // reply, and replies are served to the client over the open setup AP.
       // Overwrite it: the command cannot fail, so there is nothing to report.
       // Config entries carry the key; a CLI entry is matched on the command.
-      if (wcIsAdminPasswordKey(e.key) || wcCliEchoesSecret(e.cmd)) strcpy(e.reply, "OK");
+      const bool set_admin_pwd = wcIsAdminPasswordKey(e.key) || wcCliEchoesSecret(e.cmd);
+      if (set_admin_pwd) strcpy(e.reply, "OK");
+      // Satisfies the initial-setup invariant for the rest of this session, so
+      // the operator can set the password and configure WiFi in separate steps
+      // (the form batch always sends them together and needs no such memory).
+      if (set_admin_pwd) _admin_pwd_set = true;
+      if (_batch_kind == BATCH_CLI && wcIsSecretReadCommand(e.cmd)) wcMaskSecretReply(e.reply);
       // What gates the reboot is narrower than "did anything fail": only a
       // write can leave the node in a config not worth rebooting into, and only
       // a write has a reply convention ("OK") solid enough to test. Diagnostics
@@ -1047,7 +1081,7 @@ void WebConfigServer::handleCliPost(AsyncWebServerRequest* req) {
   }
 
   int count = 0;
-  bool defer_reboot = false;
+  bool defer_reboot = false, seq_sets_pwd = false, seq_sets_ssid = false;
   for (JsonVariant v : cmds) {
     const char* raw = v.as<const char*>();
     if (!raw) continue;
@@ -1089,6 +1123,8 @@ void WebConfigServer::handleCliPost(AsyncWebServerRequest* req) {
     }
     e.key[0] = 0;                       // CLI entries have no config key
     if (wcIsDeferredReboot(e.cmd)) defer_reboot = true;
+    if (strncmp(e.cmd, "password ", 9) == 0) seq_sets_pwd = true;
+    if (strncmp(e.cmd, "set wifi.ssid ", 14) == 0) seq_sets_ssid = true;
     count++;
   }
   if (count == 0) {
@@ -1096,6 +1132,19 @@ void WebConfigServer::handleCliPost(AsyncWebServerRequest* req) {
     return;
   }
 
+  // The same invariant handleConfigPost enforces, and for the same reason: the
+  // reboot is what commits first onboarding, and a node that reboots onto the
+  // LAN still holding the factory password is a known credential on someone
+  // else's network. The terminal warned about this client-side, which is a
+  // reminder, not a rule — a pasted script or a direct POST ignored it.
+  if (_mode == MODE_SETUP && _initial_setup && !seq_sets_pwd && !_admin_pwd_set &&
+      (defer_reboot || seq_sets_ssid)) {
+    req->send(400, "application/json",
+              "{\"error\":\"admin password required for initial setup — "
+              "run `password <new-password>` first\"}");
+    return;
+  }
+
   _batch_kind = BATCH_CLI;
   _batch_count = count;
   _batch_next = 0;

src/helpers/esp32/WebConfigServer.h

@@ -136,6 +136,10 @@ private:
   bool _stopping = false;
   bool _was_setup_ap = false;
   bool _initial_setup = false;
+  // A `password` command has succeeded this session. Lets the CLI satisfy the
+  // initial-setup invariant across separate submissions; the form batch always
+  // sends the password with the rest, so it never needed the memory.
+  bool _admin_pwd_set = false;
   char _ap_ssid[33] = {0};
 
   // Currently attached session, also used by the display's setup-info poll.

test/test_webconfig_keys/test_webconfig_keys.cpp

@@ -119,6 +119,45 @@ TEST(WebConfigKeys, EverySecretKeyIsAlsoAllowed) {
   }
 }
 
+// ---- CLI secret reads ----------------------------------------------------
+// The web CLI runs commands with sender_timestamp 0, which is how CommonCLI
+// recognises the serial console and answers secret getters in plaintext. These
+// are the reads that must be masked back down for an HTTP caller.
+
+TEST(WebConfigKeys, MasksEverySecretReadTheCliCanReach) {
+  const char* masked[] = {
+    "get prv.key",           // this node's identity — the worst one to leak
+    "get wifi.pwd",          // grants the operator's LAN, not just the node
+    "get guest.password",
+    "get alert.psk",
+    "get bridge.secret",
+    "get mqtt1.password", "get mqtt1.token",
+    "get mqtt6.password", "get mqtt6.token",
+    "get  wifi.pwd",         // extra space after the verb
+  };
+  for (const char* c : masked) EXPECT_TRUE(wcIsSecretReadCommand(c)) << c;
+}
+
+TEST(WebConfigKeys, DoesNotMaskReadsThatCarryNoSecret) {
+  const char* plain[] = {
+    "get wifi.ssid", "get mqtt1.username", "get mqtt1.server", "get tx",
+    "get public.key",        // public half, safe to read
+    "get mqtt.owner",        // an owner's public key, not a credential
+  };
+  for (const char* c : plain) EXPECT_FALSE(wcIsSecretReadCommand(c)) << c;
+}
+
+TEST(WebConfigKeys, OnlyMasksReads) {
+  // Writing a secret has always been the portal's job and reveals nothing;
+  // only the read is restricted. Nor may a prefix be mistaken for a `get`.
+  EXPECT_FALSE(wcIsSecretReadCommand("set wifi.pwd hunter2"));
+  EXPECT_FALSE(wcIsSecretReadCommand("set prv.key aabb"));
+  EXPECT_FALSE(wcIsSecretReadCommand("password hunter2"));
+  EXPECT_FALSE(wcIsSecretReadCommand("getwifi.pwd"));
+  EXPECT_FALSE(wcIsSecretReadCommand("get"));
+  EXPECT_FALSE(wcIsSecretReadCommand(""));
+}
+
 // ---- request correlation -------------------------------------------------
 
 TEST(WebConfigKeys, AcceptsExactHexRequestIds) {

webui/index.html

@@ -1926,8 +1926,10 @@ function cliConfirm(cmds){
   // rebooting is what commits the node to normal operation.
   if(st.mode==="setup"&&!cli.pwd&&cmds.some(function(c){return /^(reboot|clkreboot)$/.test(c)})){
     var p=document.createElement("div");p.className="w";
-    p.textContent="⚠ No password command has run here. Set the admin password before rebooting, "+
-      "or the node keeps the factory one.";
+    // The node enforces this too (handleCliPost) — this is the earlier, kinder
+    // half of the same rule, so the operator finds out before sending.
+    p.textContent="⚠ Set the admin password first: run \"password <new-password>\". "+
+      "The node will refuse to finish setup on the factory one.";
     box.appendChild(p);
   }
   var btns=document.createElement("div");btns.className="btns";