From 2c057b0fc6027c87d890d533dc3610e248aea5d7 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 19 Sep 2026 21:08:54 -0700 Subject: [PATCH] fix(webconfig): publish the board probe safely and test the gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of the previous commit found two real problems. The routes go live in startLanMode()/startSetupMode(), one tick before probeBoardCommands() first runs, so a status request can land in between. That made _board_cmds a genuine cross-task race despite the comment claiming otherwise, and — worse — let /api/status answer "no board commands" before the board had been asked. The page reads status once, so a browser that loaded in that window hid working FEM controls for the whole session. The probe result is now published and read under _mux, and /api/status OMITS board_cmds until the probe has run, which is not the same as reporting an empty list. The page treats the absent field as "not known yet" and re-polls with backoff; the audit waits for it the same way, rather than auditing a board it only half knows. Probing from the start path instead would close the window, but Callbacks::execCommand is documented as loop-task-only and `start webconfig` is itself mid-handleCommand, so that trades a race for re-entrancy. The audit never executed fan.lo or fan.hi, which have no getter and so no round-trip: they were only ever matched as strings. They now run for real, including the four rejection paths, and the mock grew --probe-delay so the retry path is exercised rather than assumed. --- scripts/webconfig_cli_audit.py | 45 +++++++++++++++++++++++++-- scripts/webconfig_mock_server.py | 13 ++++++-- src/helpers/esp32/WebConfigServer.cpp | 37 ++++++++++++++++------ src/helpers/esp32/WebConfigServer.h | 5 +-- webui/index.html | 24 ++++++++++++-- 5 files changed, 106 insertions(+), 18 deletions(-) diff --git a/scripts/webconfig_cli_audit.py b/scripts/webconfig_cli_audit.py index 1b3767d4..40d7c0c0 100644 --- a/scripts/webconfig_cli_audit.py +++ b/scripts/webconfig_cli_audit.py @@ -12,6 +12,13 @@ that come back an error, so the two stay honest about each other. python3 scripts/webconfig_mock_server.py --port 8137 & python3 scripts/webconfig_cli_audit.py +Board-specific commands (Board::handleCommand) are offered only when the node +says it answers them, so one run exercises one shape of board. The mock claims +none by default, like a Heltec V3; drive the other shapes with --board-cmds: + + python3 scripts/webconfig_mock_server.py --port 8137 \ + --board-cmds radio.fem.rxgain,radio.fem.txgain,fan & + Exits non-zero if anything fails that is not in EXPECTED_FAILURES. Stdlib only. """ @@ -169,6 +176,21 @@ BOARD_ROUND_TRIPS = { "fan": [("set fan on", "get fan", "on 41.0C fan=on cd=0s")], } +# Board commands with no getter to read back, so a round-trip cannot reach them. +# Run in order, and drive the rejection path too: a set-only key that silently +# accepted anything would look identical to one that works. +# [(command, should_succeed, expected substring of the reply)] +BOARD_SET_PROBES = { + "fan": [ + ("set fan.lo 40", True, "OK"), + ("set fan.hi 70", True, "OK"), + ("set fan.lo 200", False, "0..100"), # out of range + ("set fan.lo 90", False, "< fan.hi"), # crosses the upper bound + ("set fan.hi 10", False, "> fan.lo"), # crosses the lower bound + ("set fan.hi 130", False, "<= 120"), # out of range + ], +} + class Client: def __init__(self, base): @@ -179,8 +201,17 @@ class Client: # rather than hardcoding a number that drifts when the slot is resized. status = json.load(self._open("/api/status")) self.max_cmds = status.get("max_cmds", 24) - # Board::handleCommand() commands this node probed for at startup. - self.board_cmds = [c for c in status.get("board_cmds", "").split(",") if c] + # Board::handleCommand() commands this node probed for at startup. The + # field is absent until the probe has run, which is NOT the same as an + # empty list, so wait for it rather than audit a board half-known. + for _ in range(20): + if "board_cmds" in status: + break + time.sleep(0.25) + status = json.load(self._open("/api/status")) + else: + sys.exit("node never reported board_cmds; it has not probed its board") + self.board_cmds = [c for c in status["board_cmds"].split(",") if c] def _open(self, path, data=None): headers = {"Content-Type": "application/json"} @@ -295,6 +326,16 @@ def main(): % (getc, got, want, setr["reply"])) failures.append((getc, got)) + # Set-only board commands, which no round-trip can reach. + probes = [p for gate in cli.board_cmds for p in BOARD_SET_PROBES.get(gate, [])] + if probes: + print("\nset-only board commands : %d" % len(probes)) + for (cmd, want_ok, want), (_, res) in zip(probes, cli.run([p[0] for p in probes])): + if res["ok"] == want_ok and want in res["reply"]: + continue + print(" FAIL %-30s %s" % (cmd, res["reply"])) + failures.append((cmd, res["reply"])) + # Commands the portal refuses must be refused clearly, not run and fudged. print("\nrefused with a reason : ", end="") refused = [] diff --git a/scripts/webconfig_mock_server.py b/scripts/webconfig_mock_server.py index 4a36e555..1c37955c 100644 --- a/scripts/webconfig_mock_server.py +++ b/scripts/webconfig_mock_server.py @@ -155,6 +155,7 @@ class State: # Board::handleCommand() commands this "board" answers. Empty by default: # the mock is a Heltec V3, which implements no such hook at all. self.board_cmds = [c for c in args.board_cmds.split(",") if c] + self.probe_delay = args.probe_delay self.cfg = default_config(args.setup) # latched at AP start, like WebConfigServer::_initial_setup self.initial_setup = args.setup and self.cfg["wifi"]["ssid"] == "" @@ -186,7 +187,7 @@ class State: return c def status_json(self, authed): - return { + out = { "mode": "setup" if self.setup_mode else "lan", "auth": authed, "needs_setup": self.cfg["wifi"]["ssid"] == "", @@ -198,9 +199,14 @@ class State: "role": "Repeater", "board": "Heltec V3 (mock)", "uptime_s": int(time.time() - self.start), "runtime_slots": 6, "max_slots": 6, "active_slots": self.active_slots, - "board_cmds": ",".join(self.board_cmds), "max_cmds": CLI_MAX_CMDS, } + # ABSENT, not empty, until the board has been probed: the node's routes + # go live a tick before probeBoardCommands() runs, and the page has to + # retry rather than latch "this board has no board commands". + if time.time() - self.start >= self.probe_delay: + out["board_cmds"] = ",".join(self.board_cmds) + return out # --------------------------------------------------------------------------- @@ -1119,6 +1125,9 @@ def main(): ap.add_argument("--port", type=int, default=8080) ap.add_argument("--setup", action="store_true", help="first-boot setup wizard mode") ap.add_argument("--active-slots", type=int, default=5, help="server slots to expose (2 or 5)") + ap.add_argument("--probe-delay", type=float, default=0.0, + help="seconds to withhold board_cmds from /api/status, simulating " + "the gap before the node has probed its board") ap.add_argument("--board-cmds", default="", help="comma list of Board::handleCommand() commands to answer, e.g. " "radio.fem.rxgain,radio.fem.txgain,fan (default: none, like a Heltec V3)") diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index e14088be..a898872b 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -477,6 +477,10 @@ void WebConfigServer::finalizeTeardown() { // not the hardware returns — starts with something else. // // Loop task only: execCommand() reaches the CLI, which the async task must not. +// The routes are already live by the time this first runs — createServer() +// happens in startLanMode()/startSetupMode(), one tick earlier — so the result +// is published under _mux and read the same way, and /api/status leaves the +// field out entirely until then rather than reporting a premature "nothing". void WebConfigServer::probeBoardCommands() { char cmd[48], reply[160]; uint8_t mask = 0; @@ -486,6 +490,7 @@ void WebConfigServer::probeBoardCommands() { _cb->execCommand(cmd, reply); if (reply[0] == '>') mask |= (uint8_t)(1 << i); } + WCLock lock(_mux); _board_cmds = mask; _board_cmds_probed = true; } @@ -796,17 +801,29 @@ void WebConfigServer::handleStatus(AsyncWebServerRequest* req) { doc["board"] = _board_name; // Board-specific CLI commands this board answers; the page hides the controls // for everything absent here rather than offering one that cannot work. - char board_cmds[80]; - size_t n = 0; - board_cmds[0] = 0; // the nothing-supported case - for (size_t i = 0; i < WC_BOARD_CMD_COUNT && n < sizeof(board_cmds) - 1; i++) { - if (!(_board_cmds & (1 << i))) continue; - int w = snprintf(&board_cmds[n], sizeof(board_cmds) - n, "%s%s", - n ? "," : "", WC_BOARD_CMDS[i]); - if (w < 0) break; - n += (size_t)w; // snprintf NUL-terminates; a truncating w stops the loop + // ABSENT until the probe has run, which is not the same as an empty list: the + // page must not latch "this board has none" from a status served in the gap + // between the routes going live and the first tick. + bool probed; + uint8_t board_mask; + { + WCLock lock(_mux); + probed = _board_cmds_probed; + board_mask = _board_cmds; + } + if (probed) { + char board_cmds[80]; + size_t n = 0; + board_cmds[0] = 0; // the nothing-supported case + for (size_t i = 0; i < WC_BOARD_CMD_COUNT && n < sizeof(board_cmds) - 1; i++) { + if (!(board_mask & (1 << i))) continue; + int w = snprintf(&board_cmds[n], sizeof(board_cmds) - n, "%s%s", + n ? "," : "", WC_BOARD_CMDS[i]); + if (w < 0) break; + n += (size_t)w; // snprintf NUL-terminates; a truncating w stops the loop + } + doc["board_cmds"] = board_cmds; } - doc["board_cmds"] = board_cmds; doc["uptime_s"] = millis() / 1000; doc["runtime_slots"] = RUNTIME_MQTT_SLOTS; doc["max_slots"] = MAX_MQTT_SLOTS; diff --git a/src/helpers/esp32/WebConfigServer.h b/src/helpers/esp32/WebConfigServer.h index b9083447..bdd3e272 100644 --- a/src/helpers/esp32/WebConfigServer.h +++ b/src/helpers/esp32/WebConfigServer.h @@ -201,8 +201,9 @@ private: char _stats_json[1024] = {0}; // Which Board::handleCommand() commands this board actually answers, as a - // bitmask over WC_BOARD_CMDS. Probed once per start on the loop task and then - // only read, so the async status handler needs no lock for it. + // bitmask over WC_BOARD_CMDS. Written once by the loop task in + // probeBoardCommands() and read by the async status handler, both under _mux: + // the routes are serving before the first tick, so the two really do race. uint8_t _board_cmds = 0; bool _board_cmds_probed = false; diff --git a/webui/index.html b/webui/index.html index c1c72e49..daac4f52 100644 --- a/webui/index.html +++ b/webui/index.html @@ -648,8 +648,9 @@ function boot(){ st.fw=s.fw||"";st.build=s.build_date||"";st.role=s.role||"";st.board=s.board||""; // Board-specific CLI commands this node answered for at startup. Controls // for anything absent stay hidden: on this board they are not commands. - st.boardCmds=(s.board_cmds||"").split(",").filter(Boolean); - applyBoardCmds(); + // The field is ABSENT, not empty, until the node has finished probing, and + // this page reads status once — so latch it only when it is really there. + setBoardCmds(s.board_cmds); $("#h-name").textContent=s.name||"MeshCore"; $("#h-sub").textContent=s.role+" · "+shortVer()+" · "+s.board; var b=$("#h-badge");b.classList.remove("hide"); @@ -1609,6 +1610,25 @@ var CLI_BOARD_KEYS=[ ["fan","fan.hi","Fan-on temperature in C {above fan.lo, max 120}",2] ]; function boardHas(gate){return st.boardCmds.indexOf(gate)>=0} +// An absent board_cmds means the node has not finished probing its board, which +// it does on the first tick after the routes go live — so in practice the very +// first retry finds it. Retry rather than read absent as "no board commands", +// which would hide working controls for the whole session. Backs off to ~15s +// total: generous against a one-tick gap, and bounded, since a node that never +// answers is one where hiding the controls is the right outcome anyway. +function setBoardCmds(v,tries){ + if(v===undefined||v===null){ + tries=tries||0; + if(tries<10)setTimeout(function(){ + api("/api/status").then(function(s){setBoardCmds(s.board_cmds,tries+1)}) + .catch(function(){}); + },Math.min(300*(tries+1),2000)); + return; + } + st.boardCmds=v.split(",").filter(Boolean); + cli.built=0; // the command table is keyed on this; force a rebuild + applyBoardCmds(); +} // Board-specific rows in the settings form. Hidden unless the node answered for // the command at startup, so the form never shows a control that cannot apply. function applyBoardCmds(){