fix(webconfig): tighten CLI failure detection, reboot deferral and refusals
Five findings from review, all confirmed against the source.
Failure classification (P2). Testing replies for an "Err" prefix passed five
other shapes off as success: "Unknown command", "unknown config: x", "??: x",
"Can't find GPS", "(ERR: clock cannot go backwards)" and "File system erase:
Err". They rendered green, and worse, left _batch_all_ok true — so a queued
reboot went ahead after commands that had failed, defeating the gate entirely.
Rather than lengthen one guess, the two questions are now asked separately,
each erring safe:
- colour asks "does this look like a failure", against every shape CommonCLI
actually emits, enumerated in WebConfigBatch.h and pinned by a host test
that uses the literal strings. Getting this wrong is cosmetic.
- the reboot gate asks something narrower and answerable: "did every setting
I asked for take". Only `set`/`password` gate it, and only on the "OK"
prefix every setter keeps. Diagnostics no longer gate a reboot at all, so a
harmless `memory` cannot strand one and no guess is made about "> value".
Reboot deferral (P2). CommonCLI dispatches on a six-byte prefix, so `reboot
now` and `rebooted` reach Board::reboot() too. Matching exactly meant those
variants skipped both the confirmation and the deferral and took the node down
mid-drain — the precise failure deferral exists to prevent. Both sides now
anchor the way the firmware dispatches, and the UI's risk matcher with them.
Three commands the portal cannot honestly serve are refused at POST with a
reason, and dropped from autocomplete, instead of running and lying:
- `start ota` builds a second AsyncWebServer on port 80 with no bind check
and answers "Started" regardless; the portal already holds that port, so it
could only leak the allocation and inhibit sleep.
- `clock sync` takes its time from the caller's timestamp, which a web
request has none of, so CommonCLI always rejected it. `time <epoch>` works
and remains offered.
- bare `log` and `get acl` write their real output to Serial and hand back a
stub the terminal showed as success; `log` also streams a whole file from
the loop task, stalling the mesh and radio while it does.
The mock now emits the same failure shapes it used to fake as successes, so
these are reproducible off-hardware. 24 batch + 14 keys tests pass; audit
reports 119/119 answered, 0 missing, 4/4 refused with a reason.
This commit is contained in:
1 parent
0f4c87118a
commit
944caebed7
6 files changed
+284
-47
No files matched your search
@@ -73,9 +73,15 @@ COMMAND_SOURCES = [
|
||||
"examples/simple_repeater/MyMesh.cpp",
|
||||
]
|
||||
|
||||
# Firmware commands the table deliberately does not offer.
|
||||
# Firmware commands the table deliberately does not offer. Everything below
|
||||
# except tls.bundletest is also rejected by /api/cli (wcCliUnavailable), so the
|
||||
# portal never pretends to run something it cannot.
|
||||
NOT_OFFERED = {
|
||||
"tls.bundletest", # TLS debugging, not an operator command
|
||||
"tls.bundletest", # TLS debugging, not an operator command
|
||||
"start ota", # binds port 80, which the portal is already using
|
||||
"clock sync", # takes its time from the caller; a web request has none
|
||||
"log", # streams to Serial and stalls the radio ("log start" is offered)
|
||||
"get acl", # streams to Serial, returns nothing
|
||||
}
|
||||
|
||||
|
||||
@@ -202,6 +208,22 @@ def main():
|
||||
% (getc, got, want, setr["reply"]))
|
||||
failures.append((getc, got))
|
||||
|
||||
# Commands the portal refuses must be refused clearly, not run and fudged.
|
||||
print("\nrefused with a reason : ", end="")
|
||||
refused = []
|
||||
for cmd in sorted(NOT_OFFERED - {"tls.bundletest"}):
|
||||
try:
|
||||
cli._sequence([cmd])
|
||||
refused.append((cmd, "was accepted, expected a 400"))
|
||||
except urllib.error.HTTPError as e:
|
||||
body = json.load(e) if e.code == 400 else {}
|
||||
if e.code != 400 or not body.get("error"):
|
||||
refused.append((cmd, "HTTP %d, expected 400 with a reason" % e.code))
|
||||
print("%d/%d" % (len(NOT_OFFERED) - 1 - len(refused), len(NOT_OFFERED) - 1))
|
||||
for cmd, why in refused:
|
||||
print(" FAIL %-30s %s" % (cmd, why))
|
||||
failures += refused
|
||||
|
||||
print("\n%s" % ("FAILED: %d" % len(failures) if failures else "all clear"))
|
||||
return 1 if failures else 0
|
||||
|
||||
|
||||
@@ -333,7 +333,8 @@ def apply_set(cfg, key, val):
|
||||
# Strict fallthrough: this function is the single authority on what can be
|
||||
# set, for the batch and the CLI alike. Accepting unknown keys here once hid
|
||||
# the fact that the CLI could not reach `dutycycle` or `radio.fem.rxgain`.
|
||||
return False, "Error: unknown config key '%s'" % key
|
||||
# Verbatim shape from CommonCLI::handleSetCmd's fallthrough.
|
||||
return False, "unknown config: %s" % key
|
||||
|
||||
|
||||
# Payload-type names accepted alongside the decimal form. Mirrors
|
||||
@@ -432,7 +433,38 @@ CLI_RESULT_PAGE = 8 # WebConfigBatch::kCliResultPage
|
||||
CLI_CMD_SECS = 0.25 # simulated per-command execution time
|
||||
# Board::reboot() does not return, so the firmware answers `reboot` itself and
|
||||
# arms the deferred reboot once results have been read (see wcIsDeferredReboot).
|
||||
CLI_DEFERRED_REBOOT = "reboot"
|
||||
CLI_DEFERRED_REBOOT = "reboot" # matched as a PREFIX, like CommonCLI does
|
||||
|
||||
# Commands the CLI reaches but the portal cannot honestly serve; rejected at
|
||||
# POST. Mirrors wcCliUnavailable() in WebConfigServer.cpp.
|
||||
CLI_UNAVAILABLE = [
|
||||
("start ota", True, "start ota needs port 80, which this portal is using. "
|
||||
"Run it from the serial console, or use `ota update`."),
|
||||
("clock sync", True, "clock sync takes its time from the caller, which a web request "
|
||||
"has no way to supply. Use `time <epoch-seconds>` instead."),
|
||||
("log", False, "log writes the packet log to the serial console, not here, and "
|
||||
"blocks the radio while it does. Use `log start` / `log stop`."),
|
||||
("get acl", False, "get acl writes to the serial console, not here."),
|
||||
]
|
||||
|
||||
|
||||
def cli_unavailable(cmd):
|
||||
for token, is_prefix, why in CLI_UNAVAILABLE:
|
||||
if cmd.startswith(token) if is_prefix else cmd == token:
|
||||
return why
|
||||
return None
|
||||
|
||||
|
||||
# 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_reply_is_failure(reply):
|
||||
if not reply:
|
||||
return False
|
||||
if reply.startswith(("Err", "ERR", "err", "(ERR", "Unknown command",
|
||||
"unknown config", "??", "Can't find")):
|
||||
return True
|
||||
return ": Err" in reply
|
||||
|
||||
# Commands the device answers but that have no config-key equivalent.
|
||||
GETTERS = {
|
||||
@@ -497,7 +529,7 @@ def _cli_get_value(cfg, key):
|
||||
return True, SENTINEL if cli_read_key(cfg, key) else "(not set)"
|
||||
val = cli_read_key(cfg, key)
|
||||
if val is None:
|
||||
return False, "Error: unknown config key '%s'" % key
|
||||
return False, "??: %s" % key # CommonCLI::handleGetCmd fallthrough
|
||||
return True, str(val)
|
||||
|
||||
|
||||
@@ -542,7 +574,7 @@ def run_cli(cfg, line):
|
||||
if cmd in ("poweroff", "shutdown"):
|
||||
return True, "OK - powering off"
|
||||
if cmd == "erase":
|
||||
return True, "OK - filesystem erased, rebooting"
|
||||
return True, "File system erase: OK"
|
||||
if cmd == "memory":
|
||||
return True, ("heap free: 142000\nheap min: 118000\n"
|
||||
"largest block: 96000\npsram free: 3980000")
|
||||
@@ -559,8 +591,10 @@ def run_cli(cfg, line):
|
||||
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.startswith("clock sync"):
|
||||
# Rejected at POST, but modelled anyway: over the web the caller's
|
||||
# timestamp is 0, so CommonCLI always takes this branch.
|
||||
return False, "(ERR: clock cannot go backwards)"
|
||||
if cmd == "region":
|
||||
return True, "US915"
|
||||
if cmd == "sensor list":
|
||||
@@ -628,7 +662,7 @@ def run_cli(cfg, line):
|
||||
# key is *readable* rejected write-only and computed ones (`dutycycle`,
|
||||
# `prv.key`, `radio.fem.rxgain`).
|
||||
return apply_set(cfg, key, val.strip())
|
||||
return False, "Error: unknown command '%s'" % cmd[:40]
|
||||
return False, "Unknown command"
|
||||
|
||||
|
||||
def valid_reqid(reqid):
|
||||
@@ -859,6 +893,9 @@ class Handler(BaseHTTPRequestHandler):
|
||||
for c in cmds:
|
||||
if len(c) > BATCH_CMD_SIZE - 1:
|
||||
return self._json(400, {"error": "command too long"})
|
||||
why = cli_unavailable(c)
|
||||
if why:
|
||||
return self._json(400, {"error": why})
|
||||
|
||||
with ST.lock:
|
||||
self._cli_advance(ST.cli)
|
||||
@@ -868,7 +905,8 @@ class Handler(BaseHTTPRequestHandler):
|
||||
if ST.cli.get("state") == "running":
|
||||
return self._json(409, {"error": "busy", "reqid": ST.cli.get("reqid", "")})
|
||||
ST.cli = {"state": "running", "reqid": reqid, "cmds": cmds, "results": [],
|
||||
"all_ok": True, "reboot": CLI_DEFERRED_REBOOT in cmds,
|
||||
"all_ok": True,
|
||||
"reboot": any(c.startswith(CLI_DEFERRED_REBOOT) for c in cmds),
|
||||
"next_at": time.time() + CLI_CMD_SECS}
|
||||
return self._json(202, {"state": "running", "reqid": reqid, "total": len(cmds)})
|
||||
|
||||
@@ -881,13 +919,17 @@ class Handler(BaseHTTPRequestHandler):
|
||||
while (job.get("state") == "running" and len(job["results"]) < len(job["cmds"])
|
||||
and now >= job["next_at"]):
|
||||
cmd = job["cmds"][len(job["results"])]
|
||||
if cmd == CLI_DEFERRED_REBOOT:
|
||||
ok, reply = True, "OK - reboot queued"
|
||||
if cmd.startswith(CLI_DEFERRED_REBOOT):
|
||||
reply = "OK - reboot queued"
|
||||
else:
|
||||
ok, reply = run_cli(ST.cfg, cmd)
|
||||
_, reply = run_cli(ST.cfg, cmd)
|
||||
if cmd.startswith("password "):
|
||||
reply = "OK" # never echo the new password back
|
||||
job["all_ok"] = job.get("all_ok", True) and ok
|
||||
ok = not cli_reply_is_failure(reply)
|
||||
# Only writes gate the reboot, and only on the "OK" convention every
|
||||
# setter keeps (WebConfigBatch::cliReplyGatesReboot).
|
||||
if cmd.startswith(("set ", "password ")):
|
||||
job["all_ok"] = job.get("all_ok", True) and reply.startswith("OK")
|
||||
# The command is NOT echoed: it may carry a password or token, and
|
||||
# the client matches results to its own sequence by index.
|
||||
job["results"].append({"ok": ok, "reply": reply})
|
||||
|
||||
Reference in new issue
Block a user