From 41303e48d55449ff9848b997469df21a3ce87b87 Mon Sep 17 00:00:00 2001 From: ewowi Date: Mon, 22 Jun 2026 09:39:19 +0200 Subject: [PATCH 01/10] Validate controls in the backend; drop the SET_DEVICE_MODEL Improv RPC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deviceModel is now set like any other catalog default — an APPLY_OP `set System.deviceModel` op (installer over serial) or POST /api/control (MoonDeck) — instead of a bespoke SET_DEVICE_MODEL Improv RPC. Its printable-ASCII rule moves onto the control as a per-control validator on ControlDescriptor, so every write path (HTTP, serial APPLY_OP, persistence load) runs the same check in one backend place. This is architecturally cleaner — any control that needs input rules declares them on itself — and it removes a whole vendor RPC + its device-side handler, buffers, and JS frame builder. SET_TX_POWER stays a dedicated RPC because it genuinely must land before the radio associates; deviceModel has no such ordering constraint, so it rides the generic transport (like deviceName already does). KPI: 16384lights | PC:365KB | tick:112/87/111/9/1/313/37/15/18/111/11us(FPS:8928/11494/9009/111111/1000000/3194/27027/66666/55555/9009/90909) | src:97(19660) | test:68(10232) | lizard:76w (No ESP32 tick line: this change touches only Improv provisioning + the validator hook + boot wiring — no render-path code — so the device tick is unchanged from the last committed ESP32 value, tick:5155us(FPS:193). ESP32 esp32 + esp32p4-eth both build clean under -Werror.) Core: - Control: ControlDescriptor gains an optional per-control validator (`bool (*validate)(const char*)`, Text/Password only). applyControlValue runs it on the incoming value BEFORE the write and returns Malformed on reject (prior value preserved, no partial write), so the check covers HTTP, APPLY_OP-over-serial, and persistence load in one place. addText takes an optional validator arg. - SystemModule: deviceModel's printable-ASCII rule (1..31, 0x20-0x7E, no NUL) is now that validator (validateDeviceModel), declared on the control via addText. Removed setDeviceModel(). - ImprovProvisioningModule: dropped the SET_DEVICE_MODEL pending buffer + per-tick poll + the deviceModelOut init args; deviceModel arrives via the APPLY_OP poll like any other control. systemModule wiring stays (GET_DEVICE_INFO device name). Platform: - platform_esp32_improv.cpp: removed the SET_DEVICE_MODEL (0xFE) handler, its dispatch case, and the deviceModelOut/deviceModelReady g_improv fields + init args. - platform.h + platform_desktop.cpp: improvProvisioningInit drops the deviceModelOut/deviceModelOutLen/deviceModelReady params. - main.cpp: comment-only — deviceModel injection now goes through the apply-core + validator, not a dedicated RPC. UI: - improv-frame.js: removed the IMPROV_CMD_SET_DEVICE_MODEL export. - install-orchestrator.js: removed encodeSetDeviceModelPayload + sendSetBoardFrame; pushDefaultsOverSerial sends only APPLY_OP ops now (the deviceModel name is one of the catalog `set` ops). The TX-power frame still precedes provisioning. Tests: - unit_Control_apply_absent_key: a per-control validator accepts valid input, rejects a raw control byte / empty value (Malformed), preserves the prior value on reject; a no-validator Text control accepts anything that fits. - unit_HttpServerModule_apply: a validated Text control (Tag) is enforced THROUGH the apply-core — valid value applies via both applySetControl (HTTP) and applyOp (APPLY_OP-over-serial), bad bytes / empty rejected with the prior value kept. Proves the serial path is guarded with no per-transport special-casing. Docs / CI: - CLAUDE.md: extended "Present tense only" to ban absence-narration ("no longer", "anymore", "formerly", "X was removed") — state what is, not what stopped being; decisions.md lessons exempt (the before/after contrast is the lesson). - ImprovProvisioningModule.md + SystemModule.md: deviceModel arrives via APPLY_OP set + the per-control validator; removed the SET_DEVICE_MODEL RPC from the wire contract. - install/README.md, index.html, landing/index.html: APPLY_OP-only wording; landing page links to the getting-started guide; installer credit line drops the inaccurate "secure handshake" phrasing. - backlog: removed the per-control-validator-hook item (shipped here). Co-Authored-By: Claude Opus 4.8 --- CLAUDE.md | 2 +- docs/backlog/backlog.md | 2 - docs/install/README.md | 2 +- docs/install/improv-frame.js | 5 - docs/install/index.html | 8 +- docs/install/install-orchestrator.js | 94 ++++------------ docs/landing/index.html | 3 + .../core/ImprovProvisioningModule.md | 7 +- docs/moonmodules/core/SystemModule.md | 2 +- src/core/Control.cpp | 12 +++ src/core/Control.h | 14 ++- src/core/ImprovProvisioningModule.h | 19 +--- src/core/SystemModule.h | 55 ++++------ src/main.cpp | 13 ++- src/platform/desktop/platform_desktop.cpp | 2 - src/platform/esp32/platform_esp32_improv.cpp | 101 +----------------- src/platform/platform.h | 19 ++-- .../core/unit_Control_apply_absent_key.cpp | 51 +++++++++ .../unit/core/unit_HttpServerModule_apply.cpp | 63 +++++++++++ 19 files changed, 213 insertions(+), 261 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3fbebe1..62415b8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,7 +21,7 @@ See `docs/architecture.md` for system design. This file contains only rules and - **Robust to any input.** A running device tolerates any sequence of UI actions or API calls: add, delete, replace, or reconfigure any module in any order, at any grid size, and it keeps running. Degraded or idle is acceptable; crashed is not. This robustness is a defining strongpoint of projectMM, and it's guarded by the test framework, not by hope: a discovered crash drives a new test that pins the fix (see the Hard Rule). Out of scope: power loss, malformed OTA, brown-out, and other physical/electrical faults the firmware can't intercept; this principle is about what the software accepts as input. - **No reboot to apply a configuration change.** Every setting takes effect live, on the next render tick — change a pin map, a strand length, an output protocol, a mic pin or rate, anything, on a running device and it just works. There is no init-once-at-boot step, and no *config* change requires a restart, which sets projectMM apart from most LED-controller firmware (where a pin or protocol change means a reboot). Like robustness, this is a defining strongpoint, and it falls out of the architecture for free rather than being hand-built per module: any control whose change reshapes derived state routes through the generic `onBuildState()` rebuild sweep, so drivers, the audio peripheral, effects, layouts, modifiers and network I/O all inherit it. When adding a feature, don't reach for a reboot/restart to apply config; make the change live. Full mechanism + rationale: [architecture.md § Live reconfiguration](docs/architecture.md#live-reconfiguration-every-change-applies-without-a-reboot). The one exception is what you'd expect: a *firmware* OTA flash swaps the binary and needs the usual power cycle — that's not a configuration change, and (like power loss and brown-out) it's the same physical-fault boundary the robustness principle draws. - **Domain-neutral core.** Separate core infrastructure from the light domain as much as practical. When mixing is necessary, use domain-neutral naming so the code stays open to future separation. -- **Present tense only.** Code, comments, and documentation describe the system as it is now. No changelogs, no roadmaps. History lives in git commits. Exceptions: `docs/backlog/` (forward-looking) and `docs/history/` (backward-looking). +- **Present tense only.** Code, comments, and documentation describe the system as it is now. No changelogs, no roadmaps. History lives in git commits. This bans not just future-tense ("will be", "planned") but **absence-narration**: phrases like "no longer", "anymore", "formerly", "used to", "X was removed", or "there's no longer a Y" describe a *change from a past state* a present-tense reader never saw — state what *is*, not what stopped being. (The test: "there is no MCLK pin" is a present-tense *property* — keep it; "there's no SET_DEVICE_MODEL RPC anymore" narrates a removal — cut it, just describe the path that exists.) Exceptions: `docs/backlog/` (forward-looking) and `docs/history/` (backward-looking) — and `decisions.md` lessons, which legitimately contrast before/after because the contrast *is* the lesson. ## Hard Rules diff --git a/docs/backlog/backlog.md b/docs/backlog/backlog.md index 30c9cfa..e260d24 100644 --- a/docs/backlog/backlog.md +++ b/docs/backlog/backlog.md @@ -292,8 +292,6 @@ Several `platform.h` APIs still use `(buf, len)` pairs where `std::span` would c Device-model injection over Improv shipped as **"Improv = REST over serial"** (the `APPLY_OP` vendor RPC pushes the whole `deviceModels.json` entry over serial during install; the device runs the same apply-core the HTTP REST API does, on WiFi *and* eth-only firmware). That subsumed the earlier multi-step "board injection + Improv as a general data injector" plan — the general injector *is* APPLY_OP. What remains: -**Open follow-up: per-control validator hook on `ControlDescriptor`.** `SystemModule::setDeviceModel()` validates ASCII-printable (rejecting control bytes, embedded NUL); the HTTP `POST /api/control` write path uses the generic `applyControlValue()` in `Control.cpp` which has no per-control validator and writes the raw bytes through. Acceptable today (HTTP-write callers source values from `deviceModels.json` which the project controls), but the right fix is a per-control validator hook on `ControlDescriptor` so any control can declare an inline validation function pointer. Worth doing when the next control with non-trivial input constraints lands, or when the threat model grows (an integration accepts arbitrary external input and POSTs it through). Sketch: `ControlDescriptor` grows a `bool (*validate)(const void*, size_t)` slot defaulting to nullptr; `applyControlValue` calls it before writing and returns `ApplyResult::Malformed` on false; `addText` / `addPassword` get an optional validator argument. Touches ~5 sites; no protocol change. - **Open follow-up: closed-loop APPLY_OP pacing (read-back ack + retry).** The installer paces APPLY_OP frames open-loop (`sendApplyOpFrame` waits a fixed ~120 ms between ops) rather than reading the device's ack back, because a Web Serial duplex read while the writer lock is held is awkward. The delay covers the worst-case single-buffer consume window with headroom, and each op is idempotent (a lost op re-applies cleanly on a re-flash), so this is robust today. The closed-loop upgrade — read the RPC response, retry once on error `0x82` (buffer busy) — removes the fixed delay (faster install) and makes op-loss impossible rather than improbable. Worth doing if a real install is ever observed dropping an op, or when the config push grows large enough that the cumulative fixed delay is noticeable. Touches only `install-orchestrator.js`. **Open follow-up: shared JS helpers across device-UI and web-installer.** `safeLocalGet` / `safeLocalSet` (3-line hostile-storage guards) are duplicated in `src/ui/install-picker.js` (device firmware, embedded as a C string via `embed_ui.cmake`) and `docs/install/devices.js` (web installer page, served from Pages). The two live in different build contexts so the shared extract isn't trivial — it'd need a new `src/ui/safe-storage.js` plus updates to: `embed_ui.cmake` (embed the new file), `ui_embedded.h` generator (new C array), HTTP server file routing (new path served), `release.yml` workflow staging, `preview_installer.py` staging. Five files for one 3-line helper is too much pre-merge. Worth doing when the next shared helper arrives — `relativeTime` and `formatBytes` are candidates. Two helpers earn the build-glue cost; one doesn't. diff --git a/docs/install/README.md b/docs/install/README.md index e581939..fb3fc5d 100644 --- a/docs/install/README.md +++ b/docs/install/README.md @@ -7,7 +7,7 @@ This directory holds the source for the **custom installer page** (driven by End users land here, pick a channel + device, click Install. The browser flashes the device over USB (Web Serial → ESP32), runs Improv-Serial provisioning, then pushes the picked device-model's whole config over the **same serial port** as REST -operations (**"Improv = REST over serial"**: SET_DEVICE_MODEL + APPLY_OP) — all from +operations (**"Improv = REST over serial"**: `APPLY_OP` frames, one per control/module) — all from the same orchestrator, no ESP Web Tools dependency. Pushing over serial (not HTTP) is what makes the deployed HTTPS installer work: a browser blocks an HTTPS page from POSTing to a plain-`http://` device (mixed-content), so the old HTTP fan-out + the diff --git a/docs/install/improv-frame.js b/docs/install/improv-frame.js index 409ca0d..9051df8 100644 --- a/docs/install/improv-frame.js +++ b/docs/install/improv-frame.js @@ -12,11 +12,6 @@ // [I][M][P][R][O][V][version=1][type][length][payload×length][checksum] // checksum = sum-mod-256 of the first 9+length bytes. -// SET_DEVICE_MODEL vendor RPC command ID. High end of the conventional 0x80-0xFE -// vendor extension range. Matches the device-side handler at -// src/platform/esp32/platform_esp32_improv.cpp. -export const IMPROV_CMD_SET_DEVICE_MODEL = 0xFE; - // SET_TX_POWER vendor RPC command ID — the pre-association TX-power cap for boards // whose LDO browns out at full power. Sent BEFORE provisioning so the very first // association runs capped. Matches the device-side handler. diff --git a/docs/install/index.html b/docs/install/index.html index fc13db4..65a70fc 100644 --- a/docs/install/index.html +++ b/docs/install/index.html @@ -692,7 +692,7 @@

projectMM Installer