diff --git a/scripts/webconfig_cli_audit.py b/scripts/webconfig_cli_audit.py index 1bca892f..4868c705 100644 --- a/scripts/webconfig_cli_audit.py +++ b/scripts/webconfig_cli_audit.py @@ -64,7 +64,33 @@ def table(): return gets + plain -# set -> get pairs, checking a value survives the round trip. +# Where top-level commands are implemented. MyMesh handles a few before +# delegating to CommonCLI, which is exactly how discover.* stayed missing from +# the table for so long: grepping CommonCLI alone does not see them. +COMMAND_SOURCES = [ + "src/helpers/CommonCLI.cpp", + "src/helpers/CommonCLI_Observer.cpp", + "examples/simple_repeater/MyMesh.cpp", +] + +# Firmware commands the table deliberately does not offer. +NOT_OFFERED = { + "tls.bundletest", # TLS debugging, not an operator command +} + + +def firmware_commands(): + """Top-level command literals the firmware dispatches on.""" + found = set() + for rel in COMMAND_SOURCES: + path = os.path.join(HERE, "..", rel) + try: + src = open(path, encoding="utf-8").read() + except OSError: + continue + for lit in re.findall(r'(?:mem|str)n?cmp\(\s*command\s*,\s*"([^"]+)"', src): + found.add(lit.strip()) + return found - NOT_OFFERED ROUND_TRIPS = [ ("set radio.watchdog 30", "get radio.watchdog", "30"), ("set dutycycle 25", "get dutycycle", "25.0"), @@ -153,15 +179,28 @@ def main(): print(" FAIL %-30s %s" % (cmd, reply)) failures += unexpected + # The reverse direction: a command the firmware implements but the table + # never offers is invisible to the check above, because the check only ever + # drives what the table already knows about. + offered = " ".join(cmds) + " " + " ".join( + re.findall(r'\["([^"]+)","', open(INDEX_HTML, encoding="utf-8").read())) + missing = sorted(c for c in firmware_commands() if c not in offered) + print("\nfirmware commands not in the table: %d" % len(missing)) + for c in missing: + print(" MISSING %s" % c) + failures += [(c, "not offered by autocomplete") for c in missing] + results = cli.run([c for probe in ROUND_TRIPS for c in probe[:2]]) print("\nround-trips : %d" % len(ROUND_TRIPS)) for i, (setc, getc, want) in enumerate(ROUND_TRIPS): setr, getr = results[i * 2][1], results[i * 2 + 1][1] - if setr["ok"] and getr["reply"] == want: + # `get` answers "> value"; compare the value, as the terminal displays it + got = re.sub(r"^>\s?", "", getr["reply"]) + if setr["ok"] and got == want: continue print(" FAIL %-30s got %r, wanted %r (set: %s)" - % (getc, getr["reply"], want, setr["reply"])) - failures.append((getc, getr["reply"])) + % (getc, got, want, setr["reply"])) + failures.append((getc, got)) print("\n%s" % ("FAILED: %d" % len(failures) if failures else "all clear")) return 1 if failures else 0 diff --git a/scripts/webconfig_mock_server.py b/scripts/webconfig_mock_server.py index 103204bd..e85852c9 100644 --- a/scripts/webconfig_mock_server.py +++ b/scripts/webconfig_mock_server.py @@ -449,6 +449,7 @@ GETTERS = { "%2d. %s%s" % (i + 1, n, "" if nd == "none" else " (needs %s)" % nd) for i, (n, nd) in enumerate(PRESETS)), "role": lambda c: "Repeater", + "acl": lambda c: "a1b2c3d4e5f60718 perms 3\n1122334455667788 perms 1", # not its own pref: the CLI derives it from airtime_factor both ways "dutycycle": lambda c: "%.1f" % (100.0 / (c["radio"]["af"] + 1)), "mqtt.config.valid": lambda c: ( @@ -475,6 +476,18 @@ def cli_mqtt_status(cfg): def cli_get(cfg, key): + """Reply to `get `. + + CommonCLI::handleGetCmd answers `> value` — the marker sets the value apart + on the serial console. Reproduced here because it is load-bearing: a reply + that starts with "> " does not start with "OK", which is what made the + firmware's first cut mark every getter a failure. + """ + ok, val = _cli_get_value(cfg, key) + return (ok, "> " + val) if ok else (ok, val) + + +def _cli_get_value(cfg, key): if key in GETTERS: val = GETTERS[key](cfg) return (True, val) if val is not None else (False, "Error: unsupported") @@ -536,6 +549,16 @@ def run_cli(cfg, line): if cmd == "neighbors": return True, ("d4e5f60718 -71 dBm snr 9.5 2m ago\n" "1122334455 -94 dBm snr 2.0 14m ago") + # Handled by MyMesh::handleCommand before it delegates to CommonCLI. + if cmd == "discover.neighbors": + return True, "OK - Discover sent" + if cmd == "discover.scopes": + return True, "OK - scopes queued (18s discovery remaining)" + if cmd.startswith("setperm "): + parts = cmd[8:].split() + if len(parts) != 2 or not _hex64(parts[0]): + return False, "Err - bad params" + return True, "OK" if cmd == "clock sync": return True, "OK - clock set: %s UTC" % time.strftime("%H:%M - %d/%m/%Y", time.gmtime()) if cmd == "region": diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index 92ab5e89..3eb0fb4e 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -43,6 +43,16 @@ static inline bool wcIsDeferredReboot(const char* cmd) { static inline bool wcCliEchoesSecret(const char* cmd) { return strncmp(cmd, "password ", 9) == 0; } +// Success has no single shape across the CLI: setters answer "OK...", getters +// answer "> value", and a few answer free-form ("File system erase: OK"). Only +// FAILURE is uniform — an "Err"/"ERR:"/"Error:" prefix. So the CLI must test for +// the failure prefix, where the config batch can test for "OK" because every +// allowlisted setter uses it. Testing for "OK" here marked every `get` a +// failure, which turned the terminal red and withheld requested reboots. +static inline bool wcCliReplyIsOk(const char* r) { + return !((r[0] == 'E' || r[0] == 'e') && (r[1] == 'R' || r[1] == 'r') && + (r[2] == 'R' || r[2] == 'r')); +} // Constant-time-ish comparison so login timing doesn't leak a prefix match. static bool fixedTimeEquals(const char* a, const char* b, size_t max_len) { @@ -349,10 +359,11 @@ void WebConfigServer::drainBatch(uint32_t now) { // 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"); - // Success convention across every allowlisted setter is an "OK" prefix - // (the UI relies on the same test); anything else is a rejection. - _batch_all_ok = WebConfigBatch::nextAllOk(_batch_all_ok, - strncmp(e.reply, "OK", 2) == 0); + // A config batch is all allowlisted setters, which uniformly answer "OK"; + // the CLI reaches the whole surface, where only failure has a fixed shape. + _batch_all_ok = WebConfigBatch::nextAllOk( + _batch_all_ok, _batch_kind == BATCH_CLI ? wcCliReplyIsOk(e.reply) + : strncmp(e.reply, "OK", 2) == 0); } _batch_last_cmd = millis(); // Config entries are named by their (non-secret) key. A CLI command is @@ -1114,7 +1125,7 @@ void WebConfigServer::handleCliResult(AsyncWebServerRequest* req) { JsonObject r = results.createNestedObject(); // The command is deliberately NOT echoed: it may hold a password or token, // and the client already has the sequence it sent. It matches by index. - r["ok"] = strncmp(_batch[i].reply, "OK", 2) == 0; + r["ok"] = wcCliReplyIsOk(_batch[i].reply); r["reply"] = (const char*)_batch[i].reply; } if (final_read) { diff --git a/webui/index.html b/webui/index.html index 6d8e87ae..7d4a1107 100644 --- a/webui/index.html +++ b/webui/index.html @@ -150,7 +150,9 @@ body.tab-cli{padding-bottom:0} .term-out>div{white-space:pre-wrap;word-break:break-word} .term-out .cmd{color:#fff} .term-out .cmd:before{content:"> ";color:var(--tgrn)} -.term-out .rep{color:#a9b7c6} +/* A reply is green because it succeeded; red is reserved for a node that said + no. Anything else reads every `get` as a failure. */ +.term-out .rep{color:#7ee787} .term-out .err{color:var(--terr)} .term-out .sys{color:var(--tdim)} .term-out .gap{height:.55em} @@ -1444,6 +1446,11 @@ var CLI_VERBS=[ ["memory","Heap and PSRAM free/min/largest block"], ["neighbors","Nodes heard recently, with RSSI and age"], ["neighbor.remove ","Drop one neighbour {64-hex-char-key}"], + // Handled by MyMesh before it delegates to CommonCLI — a whole second + // command surface the table missed until discover.* turned up absent. + ["discover.neighbors","Ask neighbours to identify themselves"], + ["discover.scopes","Collect neighbour scopes (needs the neighbors build)"], + ["setperm ","Set a node's ACL permissions {64-hex-char-key} {int8}"], ["advert","Send an advert now (flooded)"], ["advert.zerohop","Send an advert neighbours will not repeat"], ["tempradio ","Try radio params without saving {freq,bw,sf,cr}"], @@ -1480,6 +1487,7 @@ var CLI_VERBS=[ ["reboot","Restart the node"], ["clkreboot","Restart the node, preserving the clock"], ["poweroff","Power the node off"], + ["shutdown","Power the node off (same as poweroff)"], ["erase","Erase the filesystem — settings and identity"] ]; /* [key, description, mode, values] @@ -1490,6 +1498,7 @@ var CLI_KEYS=[ ["lat","Advert latitude",0], ["lon","Advert longitude",0], ["public.key","This node's public key",1], + ["acl","Access control list (per-node permissions)",1], ["prv.key","Restore an identity {64-hex-char-key}",2], ["role","Node role",1], ["radio","Radio parameters {freq,bw,sf,cr}",0], @@ -1975,9 +1984,14 @@ function cliPoll(reqid,cmds,from,status,errs,idles){ // The node never echoes the command back — it may hold a password or a // token, and we already have the sequence we sent. Match by index. cliEcho("cmd",cmds[from+i]||""); - // `ok` is advisory; the node's own OK/Error convention is authoritative - var ok=(x.ok!=null)?x.ok:!/^\s*(err|error)\b/i.test(x.reply||""); - if(x.reply)cliEcho(ok?"rep":"err",x.reply); + // `ok` is advisory; the node's own convention is authoritative, and only + // failure has a fixed shape there ("Err"/"ERR:"/"Error:"). + var ok=(x.ok!=null)?x.ok:!/^\s*err/i.test(x.reply||""); + // Getters answer "> value" — that leading marker is the serial console's + // way of setting a value apart, and here it collides with the prompt glyph + // that means "you typed this". Drop it; the colour already says "reply". + var reply=(x.reply||"").replace(/^>\s?/,""); + if(reply)cliEcho(ok?"rep":"err",reply); }); from+=(r.results||[]).length; // "running" also covers "finished, but more results are still to be paged