From d0db89bbcde37d64cb859134c9756f54eaf64ae9 Mon Sep 17 00:00:00 2001 From: mjgodat Date: Thu, 29 Oct 2026 11:33:51 -0700 Subject: [PATCH] fix(presets): unguard preset file include, commit on room/sensor admin login, avoid NAME_MAX - CommonCLI.cpp included RadioPresetFile.h only on nRF52/STM32; ESP32 and RP2040 builds could not see the file helpers. Include it unconditionally. - Room server and sensor now commit a pending remote preset load on a successful strict-admin login, like the repeater; drop the doc caveat. - Rename RadioPresetTable::NAME_MAX to MAX_NAME_LEN: newlib/ESP-IDF defines NAME_MAX as a macro. Host harnesses now predefine NAME_MAX so such collisions fail on host too. Co-Authored-By: Claude Opus 5.5 --- docs/cli_command_availability.md | 4 +-- docs/cli_commands.md | 4 +-- examples/simple_room_server/MyMesh.cpp | 2 ++ examples/simple_sensor/SensorMesh.cpp | 2 ++ src/helpers/CommonCLI.cpp | 2 +- src/helpers/RadioPresetCLI.cpp | 2 +- src/helpers/RadioPresets.h | 12 +++---- test/test_radio_presets.py | 46 ++++++++++++++++++++++++- test/test_room_login_integration.py | 8 ++++- test/test_room_login_replay_capacity.py | 5 +++ 10 files changed, 72 insertions(+), 15 deletions(-) diff --git a/docs/cli_command_availability.md b/docs/cli_command_availability.md index 0e29347bc..0cf32a2f1 100644 --- a/docs/cli_command_availability.md +++ b/docs/cli_command_availability.md @@ -171,7 +171,7 @@ should use command `0x42`. See [Companion radio binary protocol](companion_proto | Radio | [`get tx`; `set tx `](cli_commands.md#view-or-change-this-nodes-transmit-power) | Board TX-power limits apply; busy changes apply automatically | Yes | Yes | Yes | | Radio | [`tempradio ...`; `normalradio`](cli_commands.md#change-the-radio-parameters-for-a-set-duration) | Full parser | Yes | Yes | Yes | | Radio | [`get/set/del radioat`; `get/set/del tempradioat`](cli_commands.md#schedule-radio-parameter-changes) | Full parser | Yes | Yes | Yes | -| Radio | [`get preset [n]`; `add preset`; `load preset`; `del preset`](cli_commands.md#save-and-switch-named-radio-presets) | Admin only remotely; remote `load preset` reverts after 10 min unless a repeater admin logs in again | Yes | Yes | Yes | +| Radio | [`get preset [n]`; `add preset`; `load preset`; `del preset`](cli_commands.md#save-and-switch-named-radio-presets) | Admin only remotely; remote `load preset` reverts after 10 min unless an admin logs in again | Yes | Yes | Yes | | Radio | [`get freq`; `set freq `](cli_commands.md#view-or-change-this-nodes-frequency) | Local or authorized LoRa CLI | Yes | Yes | Yes | | Radio | [`get/set radio.rxgain`](cli_commands.md#view-or-change-this-nodes-rx-boosted-gain-mode-sx12xx-and-lr1110-v1141) | Supported radio | Feature | Feature | Feature | | Radio | [`get/set radio.fem.rxgain`](cli_commands.md#view-or-change-the-lora-fem-receive-path-gain-state-on-supported-boards) | Controllable LoRa FEM | Feature | Feature | Feature | @@ -349,7 +349,7 @@ should use command `0x42`. See [Companion radio binary protocol](companion_proto | Radio | [`get tx`; `set tx `](cli_commands.md#view-or-change-this-nodes-transmit-power) | Board TX-power limits apply; busy changes apply automatically | Yes | Yes | Yes | Yes | Yes | | Radio | [`tempradio ...`; `normalradio`](cli_commands.md#change-the-radio-parameters-for-a-set-duration) | Full parser | Yes | Yes | Yes | Yes | Yes | | Radio | [`get/set/del radioat`; `get/set/del tempradioat`](cli_commands.md#schedule-radio-parameter-changes) | Full parser | Yes | Yes | Yes | Yes | Yes | -| Radio | [`get preset [n]`; `add preset`; `load preset`; `del preset`](cli_commands.md#save-and-switch-named-radio-presets) | Admin only remotely; remote `load preset` reverts after 10 min unless a repeater admin logs in again | Yes | Yes | Yes | Yes | Yes | +| Radio | [`get preset [n]`; `add preset`; `load preset`; `del preset`](cli_commands.md#save-and-switch-named-radio-presets) | Admin only remotely; remote `load preset` reverts after 10 min unless an admin logs in again | Yes | Yes | Yes | Yes | Yes | | Radio | [`get freq`; `set freq `](cli_commands.md#view-or-change-this-nodes-frequency) | Local or authorized LoRa CLI | Yes | Yes | Yes | Yes | Yes | | Radio | [`get/set radio.rxgain`](cli_commands.md#view-or-change-this-nodes-rx-boosted-gain-mode-sx12xx-and-lr1110-v1141) | Supported radio | Feature | Feature | Feature | Feature | Feature | | Radio | [`get/set espnow.channel`](cli_commands.md#view-or-change-the-primary-esp-nowwifi-channel) | Primary ESP-NOW mesh radio | Feature | Feature | Feature | Feature | Feature | diff --git a/docs/cli_commands.md b/docs/cli_commands.md index e054de2ab..3cee51a5d 100644 --- a/docs/cli_commands.md +++ b/docs/cli_commands.md @@ -2043,9 +2043,7 @@ preset commands, and their logins do not confirm a pending load. While a load is pending, `get preset` starts with `pending, reverts in m;` and another remote load is refused. A remote load is also refused while a TempRadio session is active. If the revert itself fails, it retries every minute and at every -boot. Login confirmation is wired on repeaters; on room servers and sensors a -remote load always reverts after 10 minutes, so load from the local terminal -there. +boot. --- diff --git a/examples/simple_room_server/MyMesh.cpp b/examples/simple_room_server/MyMesh.cpp index cc75ce521..c9cb8a045 100644 --- a/examples/simple_room_server/MyMesh.cpp +++ b/examples/simple_room_server/MyMesh.cpp @@ -1961,6 +1961,8 @@ void MyMesh::onAnonDataRecv(mesh::Packet *packet, const uint8_t *secret, const m reply_data[7] = client->permissions; // NEW getRNG()->random(&reply_data[8], 4); // random blob to help packet-hash uniqueness reply_data[12] = FIRMWARE_VER_LEVEL; // New field + // A fresh strict-admin login keeps a remotely loaded radio preset. + if (client->isAdmin()) _cli.radioPresets().onAdminLogin(); next_push = futureMillis(PUSH_NOTIFY_DELAY_MILLIS); // delay next push, give RESPONSE packet time to arrive first diff --git a/examples/simple_sensor/SensorMesh.cpp b/examples/simple_sensor/SensorMesh.cpp index 76ebaa054..960e98107 100644 --- a/examples/simple_sensor/SensorMesh.cpp +++ b/examples/simple_sensor/SensorMesh.cpp @@ -650,6 +650,8 @@ uint8_t SensorMesh::handleLoginReq(const mesh::Identity& sender, const uint8_t* getRNG()->random(&reply_data[8], 4); // random blob to help packet-hash uniqueness reply_data[12] = FIRMWARE_VER_LEVEL; + // A fresh strict-admin login keeps a remotely loaded radio preset. + if (client->isAdmin()) _cli.radioPresets().onAdminLogin(); return 13; // reply length } diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 86be79652..d2cead884 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -62,10 +62,10 @@ #endif #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) #include "AtomicFileWriter.h" -#include "RadioPresetFile.h" #elif defined(ESP32_PLATFORM) || defined(RP2040_PLATFORM) #include "ContactFileTransaction.h" #endif +#include "RadioPresetFile.h" #include #include #include diff --git a/src/helpers/RadioPresetCLI.cpp b/src/helpers/RadioPresetCLI.cpp index 48e232f02..995b52135 100644 --- a/src/helpers/RadioPresetCLI.cpp +++ b/src/helpers/RadioPresetCLI.cpp @@ -143,7 +143,7 @@ bool RadioPresetCLI::handleGet(const char* args, char* reply, size_t cap) { // add preset , validated like "set radio". bool RadioPresetCLI::handleAdd(const char* args, char* reply, size_t cap) { - char name[RadioPresetTable::NAME_MAX + 2]; + char name[RadioPresetTable::MAX_NAME_LEN + 2]; const char* sp = strchr(args, ' '); if (!sp || (size_t)(sp - args) >= sizeof(name)) { cli::formatError(reply, cap, "usage: add preset "); diff --git a/src/helpers/RadioPresets.h b/src/helpers/RadioPresets.h index 37605ba17..6614c712a 100644 --- a/src/helpers/RadioPresets.h +++ b/src/helpers/RadioPresets.h @@ -32,7 +32,7 @@ struct RadioPresetPending { class RadioPresetTable { public: - static constexpr uint8_t SLOTS = 5, NAME_MAX = 12; + static constexpr uint8_t SLOTS = 5, MAX_NAME_LEN = 12; static constexpr size_t ENCODED_MAX = 160; RadioPresetTable() { clearAll(); } @@ -40,7 +40,7 @@ class RadioPresetTable { static bool validName(const char* name) { if (!name) return false; size_t n = strlen(name); - if (n == 0 || n > NAME_MAX) return false; + if (n == 0 || n > MAX_NAME_LEN) return false; for (size_t i = 0; i < n; i++) { unsigned char c = (unsigned char)name[i]; if (c <= ' ' || c == ',' || c >= 0x7F) return false; @@ -135,7 +135,7 @@ class RadioPresetTable { private: static constexpr uint8_t VERSION = 1; // Wire record: name (12, no terminator) + freq + bw + sf + cr + preamble. - static constexpr size_t RECORD_SIZE = NAME_MAX + 4 + 4 + 1 + 1 + 2; + static constexpr size_t RECORD_SIZE = MAX_NAME_LEN + 4 + 4 + 1 + 1 + 2; static constexpr size_t IMAGE_SIZE = 3 + SLOTS * RECORD_SIZE + 1 + 4 + RECORD_SIZE + 4; static_assert(IMAGE_SIZE <= ENCODED_MAX, "ENCODED_MAX too small for table + pending"); @@ -168,7 +168,7 @@ class RadioPresetTable { static size_t putRecord(uint8_t* p, const RadioPreset& r) { memset(p, 0, RECORD_SIZE); if (r.used) { - memcpy(p, r.name, NAME_MAX); + memcpy(p, r.name, MAX_NAME_LEN); putFloat(p + 12, r.freq); putFloat(p + 16, r.bw); p[20] = r.sf; @@ -181,8 +181,8 @@ class RadioPresetTable { static void getRecord(const uint8_t* p, RadioPreset& r) { memset(&r, 0, sizeof(r)); if (p[0] == 0) return; - memcpy(r.name, p, NAME_MAX); - r.name[NAME_MAX] = 0; + memcpy(r.name, p, MAX_NAME_LEN); + r.name[MAX_NAME_LEN] = 0; r.freq = getFloat(p + 12); r.bw = getFloat(p + 16); r.sf = p[20]; diff --git a/test/test_radio_presets.py b/test/test_radio_presets.py index 7ef050ee0..a50d70f3d 100644 --- a/test/test_radio_presets.py +++ b/test/test_radio_presets.py @@ -399,6 +399,10 @@ int main() { class RadioPresetsTest(unittest.TestCase): + # ESP-IDF/newlib defines NAME_MAX as a macro. Predefine it on host + # so any identifier collision with a platform macro fails here too. + PLATFORM_MACROS = ('-DNAME_MAX=255',) + def build_and_run(self, name, source, expected, extra_sources=()): compiler = shutil.which('g++') or shutil.which('clang++') if compiler is None: @@ -412,7 +416,7 @@ class RadioPresetsTest(unittest.TestCase): binary = work / (name + ('.exe' if os.name == 'nt' else '')) built = subprocess.run([ compiler, '-std=c++17', '-O1', '-g', '-Wall', '-Wextra', '-Werror', - *sanitize, + *sanitize, *self.PLATFORM_MACROS, '-I', str(ROOT / 'src'), '-I', str(ROOT / 'test'), '-isystem', str(ROOT / 'test' / 'mocks'), # Arduino.h mock; not -Werror clean str(cpp), *[str(ROOT / s) for s in extra_sources], '-o', str(binary), @@ -507,6 +511,28 @@ class RadioPresetWiringContractTest(unittest.TestCase): self.assertRegex(h, r'bool\s+saveRadioPresetFile\(FILESYSTEM\*\s*fs,\s*const uint8_t\*\s*buf,' r'\s*size_t\s+len\)') self.assertIn('AtomicFileWriter', h) + self.assertIn('ContactFileTransaction', h) + + def test_preset_file_include_is_not_platform_guarded(self): + # Every platform needs the helpers: the include must not sit inside a + # platform-specific #if/#elif/#else branch in CommonCLI.cpp. + stack = [] + found = False + for line in _read('src/helpers/CommonCLI.cpp').splitlines(): + t = line.strip() + if t.startswith('#if'): + stack.append(t) + elif t.startswith('#elif') and stack: + stack[-1] = stack[-1] + ' ' + t + elif t.startswith('#else') and stack: + stack[-1] = stack[-1] + ' #else' + elif t.startswith('#endif') and stack: + stack.pop() + elif t == '#include "RadioPresetFile.h"': + found = True + guarded = [c for c in stack if '_PLATFORM' in c] + self.assertEqual(guarded, [], 'RadioPresetFile.h include is platform-guarded') + self.assertTrue(found, 'CommonCLI.cpp must include RadioPresetFile.h') def test_repeater_login_commits_only_for_full_admin(self): cpp = _read('examples/simple_repeater/MyMesh.cpp') @@ -524,6 +550,24 @@ class RadioPresetWiringContractTest(unittest.TestCase): header = _read('examples/simple_repeater/MyMesh.h') self.assertRegex(header, r'bool\s+login_full_admin\s*=\s*false\s*;') + def test_room_server_and_sensor_login_commit_for_strict_admin(self): + room = _read('examples/simple_room_server/MyMesh.cpp') + anon = _function_body(room, 'void MyMesh::onAnonDataRecv(') + self.assertIsNotNone(anon) + ok = anon.find('reply_data[4] = RESP_SERVER_LOGIN_OK;') + commit = anon.find('if (client->isAdmin()) _cli.radioPresets().onAdminLogin();') + self.assertGreaterEqual(ok, 0) + self.assertGreater(commit, ok, 'room server must commit after a successful login') + self.assertEqual(room.count('onAdminLogin()'), 1) + sensor = _read('examples/simple_sensor/SensorMesh.cpp') + login = _function_body(sensor, 'uint8_t SensorMesh::handleLoginReq(') + self.assertIsNotNone(login) + self.assertRegex(login, r'RESP_SERVER_LOGIN_OK[\s\S]*if \(client->isAdmin\(\)\) ' + r'_cli\.radioPresets\(\)\.onAdminLogin\(\);\s*\n\s*return 13;') + self.assertEqual(sensor.count('onAdminLogin()'), 1) + docs = _read('docs/cli_commands.md') + self.assertNotIn('remote load always reverts', docs) + def test_manager_roles_cannot_use_presets(self): cpp = _read('examples/simple_repeater/MyMesh.cpp') for name in ('static bool isRegionMgrAllowed(', 'static bool isFilterMgrAllowed('): diff --git a/test/test_room_login_integration.py b/test/test_room_login_integration.py index 732060a7d..2ba62740a 100644 --- a/test/test_room_login_integration.py +++ b/test/test_room_login_integration.py @@ -159,10 +159,13 @@ struct MyMesh { void prepareReply(const void*) {} void armReply(const void*,bool) {} } inactive_morse_reply; + struct PresetCommits { unsigned admin_logins = 0; void onAdminLogin() { ++admin_logins; } }; struct CLI { NoMorseReply* reply; NoMorseReply& morseReply() { return *reply; } - } _cli{&inactive_morse_reply}; + PresetCommits presets; + PresetCommits& radioPresets() { return presets; } + } _cli{&inactive_morse_reply, {}}; FakeACL acl; mesh::Identity self_id{}; PostInfo retained_posts[MAX_UNSYNCED_POSTS]{}; @@ -402,6 +405,8 @@ static void authorizationCases() { assert(mesh.acl.storage.reads == (c.accepted ? 1U : 0U)); // Only the admin credential allocates a durable record for a new identity. assert(mesh.acl.storage.writes == (c.accepted && c.role == PERM_ACL_ADMIN ? 1U : 0U)); + // Only a successful strict-admin login commits a remotely loaded radio preset. + assert(mesh._cli.presets.admin_logins == (c.accepted && c.role == PERM_ACL_ADMIN ? 1U : 0U)); if (c.accepted) { assert(mesh.acl.client.permissions == c.role); checkReply(mesh, c.role); @@ -546,6 +551,7 @@ static void storageFailures() { full.acl.allocate_ok = false; login(full, "admin"); assert(!full.acl.known && full.acl.allocations == 1 && full.creations == 0); + assert(full._cli.presets.admin_logins == 0); // failed login commits nothing assert(full.acl.storage.found && full.acl.storage.ceiling == 160); login(full, "admin", 100); assert(full.acl.allocations == 1 && full.acl.storage.writes == 1); diff --git a/test/test_room_login_replay_capacity.py b/test/test_room_login_replay_capacity.py index e589ef1e5..b84181d64 100644 --- a/test/test_room_login_replay_capacity.py +++ b/test/test_room_login_replay_capacity.py @@ -52,6 +52,11 @@ struct MyMesh { unsigned long dirty_contacts_expiry = 0, next_push = 0; uint8_t contacts_save_failures = 0, reply_data[16] = {}; unsigned replies = 0; + struct PresetCommits { unsigned admin_logins = 0; void onAdminLogin() { ++admin_logins; } }; + struct Cli { + PresetCommits presets; + PresetCommits& radioPresets() { return presets; } + } _cli; mesh::Packet response; explicit MyMesh(FakeFilesystem* fs) : _fs(fs) { // Same order as MyMesh::begin().