mirror of
https://github.com/agessaman/MeshCore.git
synced 2026-08-27 22:34:14 +00:00
observer-firmware-dev
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 agessaman
|
8abe26ba7b |
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. |
||
|
agessaman
|
c831e599ec |
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.
|
||
|
agessaman
|
d532e4ea86 |
fix(webconfig): correct reply classification and the missing MyMesh commands
Two things hardware turned up.
The whole terminal came back red. The endpoint decided a command had succeeded
by testing its reply for an "OK" prefix — the convention the config batch relies
on, and a safe one there because every allowlisted setter uses it. The CLI
reaches the whole surface, where success has no single shape: setters answer
"OK...", getters answer "> value", `erase` answers "File system erase: OK". Only
failure is uniform ("Err", "ERR:", "Error:"), so that is what the CLI now tests
for.
Colour was the visible half. The other half was worse: _batch_all_ok went false
the moment a sequence contained a `get`, so a script ending in `reboot` was told
some commands had failed and the reboot was withheld.
Replies are green now and red means the node said no, which is what red should
have meant all along. The "> " a getter prefixes its value with is dropped for
display — on the serial console it sets the value apart, here it collides with
the prompt glyph that means "you typed this". The mock emits that marker too;
had it done so from the start, this would have shown up before the flash.
Second: discover.neighbors and discover.scopes did not autocomplete, because
MyMesh::handleCommand intercepts a few commands before delegating to CommonCLI
and the table was built by reading CommonCLI alone. setperm, `get acl` and
`shutdown` were missing for the same reason.
The audit could not have caught that: it drove every command the table offered
and checked the mock answered, which only finds gaps in one direction. It now
also reads the command literals the firmware dispatches on — across CommonCLI,
CommonCLI_Observer and MyMesh — and fails on any the table does not offer. That
check found `shutdown` immediately.
122 commands, all answered, none missing. 22 batch + 14 keys tests pass.
|
||
|
agessaman
|
d7109c185c |
feat(webconfig): implement /api/cli on the device
The terminal has been driving the mock since it was built. This is the firmware
side, so it works on hardware.
Same 202 + reqid + poll contract as a config save, for the same reason:
CommonCLI touches prefs, the radio and the filesystem, none of which may be
reached from the async_tcp task. Commands go into the deferred slot and tick()
drains them on the loop task. Unlike a save this is not allowlisted — reaching
what the serial console reaches is the point, and execCommand() already passes
sender_timestamp 0, so the terminal gets exactly the serial console's
privilege. Authentication is the boundary, as it is there.
The CLI shares the config batch's slot rather than owning a second MAX_BATCH
array: both drain on the loop task, both are single-slot, and a duplicate would
cost ~8 KB of permanently resident RAM. Sharing also makes a save and a CLI run
mutually exclusive, which they must be. Each reader checks the kind, so neither
can serve the other's results.
Three things the mock could not have taught us:
- Board::reboot() does not return, so a drained `reboot` would take the node
down before the client read a single result. It is answered rather than
executed, and the batch arms the existing deferred-reboot path once the
results have been read — withheld if any command failed, exactly as a save
withholds one. clkreboot/poweroff/ota update do real work on the way down
and cannot be faked, so they still drop the connection; the UI warns first.
- `password <new>` echoes the new password in its reply. The config path
already scrubbed that by key; a CLI entry has no key, so it is matched on
the command. CLI commands are also kept out of the serial log entirely —
the browser session and the serial console are different audiences.
- MAX_BATCH is 24, not the 64 the page assumed. It is reported as
status.max_cmds instead of hardcoded, so the cap cannot drift.
Results stream and page (kCliResultPage = 8), and "done" means the client has
been handed every result, not merely that execution finished — otherwise a
client that stops polling at "done" loses the last page. Commands are never
echoed back: they may carry a secret, and the client matches by index.
New decisions live in WebConfigBatch.h with the rest, covered by three host
tests. Builds clean for heltec_v4_repeater_observer_mqtt; 22 batch + 14 keys
tests pass; the CLI audit reports 119/119 against the updated mock.
|
||
|
agessaman
|
b72b02f55b |
fix(webconfig): make the mock answer the whole CLI surface
`get radio.fem.rxgain` returned "unknown config key" from the mock, which reads
as the terminal offering a command that does not exist. It does exist: CommonCLI
implements get and set for it, gated at runtime by Board::canControlLoRaFemLna()
rather than compiled out, so the command is present in every build and the board
answers for itself — "Error: unsupported" where there is no front-end module.
Auditing the whole table found 31 of 70 config keys unanswered, all the ones no
portal form drives: alert.*, bridge.*, owner.info, path.hash.mode, dutycycle and
the rest. Plus 14 verbs (gps, powersaving, sensor, region, clock sync) with no
handler at all. They now live in a "cli" section of the mock config, typed
through the existing lookup tables and stripped from /api/config, which does not
carry them.
Two real bugs behind that:
- the `set` path gated on whether a key was *readable*, so write-only and
computed keys (prv.key, dutycycle, radio.fem.rxgain) were rejected as
unknown. apply_set now owns that decision alone.
- apply_set accepted anything it did not recognise and replied OK. That
leniency is what let the gap hide: a CLI `set` on an unknown key looked
like it worked. It is strict now — verified against every key in
WC_ALLOWED_SET_KEYS so the form batch is unaffected.
Also mqtt.neighbors / mqtt.neighbors.interval, which the MQTT tab binds but the
mock's config never carried, so that toggle could not round-trip.
webconfig_cli_audit.py keeps the two honest: it drives every command the
autocomplete table offers through /api/cli and fails on anything unanswered.
119 commands, all answered.
|