mirror of
https://github.com/meshtastic/firmware.git
synced 2026-09-15 15:59:40 -04:00
develop
22
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Garth Vander HouwenandThomas Göttgens
|
83198c1cbb |
fix(pki): reject a restored pre-2.8 low-entropy key at set time, explain the swap (#11686)
* fix(pki): reject a restored pre-2.8 low-entropy key at set time, explain the swap Restoring/setting a private key is a private-key change: the public key is *generated* from it. The low-entropy blacklist check in generateCryptoKeyPair runs against the stored public_key at entry, which is empty on a bare key restore — so a known pre-2.8 weak key derived from the provided private key was never caught at set time. It was only detected on the next boot (once the weak public key had been persisted and re-checked), which looks to the user like their saved key silently "did not stick", and their node number (== crc32(public_key)) had quietly changed too. - NodeDB::generateCryptoKeyPair: in the provided-private-key branch, re-check the *derived* public key against LOW_ENTROPY_HASHES. If it matches, replace it with a fresh secure keypair and set keyIsLowEntropy so the reason is surfaced. - AdminModule set-config(security): when the restore path regenerated a rejected low-entropy key, send a client warning at set time explaining the key can't be restored and the node number changed. Scoped to that branch so a stale flag from a boot-time regeneration can't fire on unrelated security sets. No protobuf changes; reuses the existing ClientNotification warning path. Signed-off-by: Garth Vander Houwen <garthvh@yahoo.com> * fix(pki): gate low-entropy restore warning on successful keygen generateCryptoKeyPair returns false on an unset LoRa region before resetting keyIsLowEntropy, so the set-time warning could fire on a stale flag. Capture the return value and require both. Shorten the rationale comments to two lines each. * fix(pki): clear key sizes when a restored private key derives nothing The provided-private-key branch sets private_key.size and public_key.size to 32 before regeneratePublicKey() runs. On failure it returned false with both sizes still set, and AdminModule persisted that pair; every later keygen then re-derived from the same dead key. Clear both on the failure path so the next keygen mints a fresh identity. Add test_admin_radio coverage for the set-time restore path: a derived low-entropy key warns and rotates, a stale keyIsLowEntropy flag with keygen blocked does not warn, and a failed derivation clears both sizes. * fix(pki): validate a restored public key that is itself blacklisted A restore supplying both private_key and public_key reached neither keygen branch, so a whole pre-2.8 low-entropy pair was accepted and persisted at set time and only caught on the next boot. Re-derive when the supplied public key is blacklisted, which routes it through the same rejection and warning as the bare-private-key restore. A non-blacklisted keypair import is unaffected. Install the test crypto stub through a helper and drop it in restoreAdminRadioGlobals(), so a failed assertion's longjmp cannot leak a freed engine into later tests. * fix(pki): only warn about a swapped key when one was actually swapped keyIsLowEntropy is set from the stored public key at function entry, so a restore whose supplied public key is blacklisted set it even when keygen merely re-derived the public key from a private key that was kept. The warning then claimed a new key had been generated and the node number changed, which was only half true. Gate it on the private key actually being replaced. * fix(pki): re-check a freshly minted keypair against the blacklist Both mint sites called crypto->generateKeyPair() once and trusted the result, so an entropy source still producing known-weak keys could persist another blacklisted identity. Route both through a helper that re-checks and retries a bounded number of times, then logs if it cannot do better. Pass the caller's own copy of the private key to generateCryptoKeyPair() instead of config.security.private_key.bytes, which aliased the memcpy destination inside it. * fix(pki): fail keygen when every replacement stays blacklisted generateBlacklistCheckedKeyPair() logged an error after exhausting its retries but left the compromised keypair in place and its callers marked the keygen successful, persisting exactly the identity the check exists to reject. Return a flag, clear both key sizes on exhaustion, and abort both callers so the next keygen starts clean. Match the declaration guard to the definition's, and derive the expected mint count in the retry test from the configured one. * refactor(pki): drop the keygen retry loop, fail on the first weak mint Retrying cannot help: an entropy source that lands on one of the twelve blacklisted keys is broken, and a second call to it produces the same result. With real entropy the odds are ~2^-250, so the loop never runs twice in practice either. Check once and fail, which is the same guarantee in a third of the code. * fix(pki): check the derived key on the stored-private-key path too factory_reset_config keeps the private key and clears the public one, so the entry check sees no stored key, reports "not low entropy" and takes the regenerate branch, which adopted whatever it derived. A preserved pre-2.8 key was therefore accepted for a whole boot cycle before the next boot caught it - the same silent revert this PR exists to remove. Hoist the post-derive blacklist check into a helper and use it on both derive paths. * fix(pki): clear key sizes when stored-private derivation fails too The stored-private-key path set public_key.size to 32 up front and left it there when regeneratePublicKey() failed, so config claimed a pair the node never got - the same defect already fixed on the provided-key path. Both paths now derive through one helper that clears on failure and vets the derived key, replacing the separate blacklist-replace helper. --------- Signed-off-by: Garth Vander Houwen <garthvh@yahoo.com> Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> |
||
|
Ben Meadors
|
7e9525ad83 |
feat(baseui): default US to LongTurbo on first region selection (#11637)
Selecting US in the BaseUI region chooser now installs LongTurbo instead of LongFast, but only for out-of-box setup: the outgoing region must be UNSET, so a later switch to US leaves whatever preset the node is running alone. Scoped to the menu on purpose. The US entry in regions[] keeps LongFast as its default preset, so preset repair, admin/phone writes and every other route onto US are unchanged. A build pinning USERPREFS_LORACONFIG_MODEM_PRESET, a preset already moved off the install default, or use_preset=false all outrank it. The decision is lifted into menuHandler::presetForRegionSelection() so it is reachable without a Screen, following toggleNodeMuted(). |
||
|
Ben Meadors
|
122ec0e9f4 |
Revert "feat(baseui): default US to LongTurbo on first region selection"
This reverts commit
|
||
|
Ben Meadors
|
dbba2b3f6c |
feat(baseui): default US to LongTurbo on first region selection
Selecting US in the BaseUI region chooser now installs LongTurbo instead of LongFast, but only for out-of-box setup: the outgoing region must be UNSET, so a later switch to US leaves whatever preset the node is running alone. Scoped to the menu on purpose. The US entry in regions[] keeps LongFast as its default preset, so preset repair, admin/phone writes and every other route onto US are unchanged. A build pinning USERPREFS_LORACONFIG_MODEM_PRESET, a preset already moved off the install default, or use_preset=false all outrank it. The decision is lifted into menuHandler::presetForRegionSelection() so it is reachable without a Screen, following toggleNodeMuted(). |
||
|
AustinandClaude Opus 5
|
514b476189 |
feat(admin): append the optional ham long_name to the call sign (#11612)
* feat(admin): append the optional ham long_name to the call sign HamParameters gained a long_name field (meshtastic/protobufs#941) that handleSetHamMode never read, so a client that sent one still ended up with a node named after the bare call sign. Join it behind the call sign with the "//" separator hams already use on the air: call_sign "N0CALL" plus long_name "Attic Heltec" becomes "N0CALL//Attic Heltec". An absent long_name keeps the previous call-sign-only name, which is what the on-device region picker still sends. Being cosmetic, long_name stays out of the whitespace-only rejection that guards call_sign and short_name: a blank one is dropped rather than costing the operator the whole licensing request over a stray space, which that path would report only as a LOG_WARN and so would be invisible from the app. The composed name is finished with clampLongName() rather than a bare sanitizeUtf8(), matching handleSetOwner and NodeDB: the proto caps the parts at 7 + 2 + 14 bytes, inside the 24-byte local budget, and clampLongName is the backstop if either cap moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(admin): enhance handleSetHamMode to return status for request validation --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Clive BlackledgeandClaude Opus 5
|
389559bddb |
fix(NodeDB): re-derive my_node_num when ensurePkiKeys() mints the identity keypair (#11426)
* fix(pki): re-derive NodeNum when setting a region mints the identity key
A node's mesh address is derived from its identity key:
my_node_num == crc32Buffer(config.security.public_key.bytes, 32)
NodeDB::createNewIdentity() is what establishes that, and NodeDB::
generateCryptoKeyPair() is the only thing that called it.
CryptoEngine::ensurePkiKeys() generates or re-derives the keypair and writes
security.public_key, security.private_key and user.public_key - but never
re-derives my_node_num. Boot-time keygen is suppressed while the LoRa region is
UNSET (generateCryptoKeyPair()'s regionBlocksKeygen guard), so on a fresh device
my_node_num is still the MAC-derived value from pickNewNodeNum(). The user then
sets the region - the stock onboarding flow - ensurePkiKeys() mints a key, and
the invariant is broken.
The node then signs its broadcasts (Router.cpp signs when !pki_encrypted &&
(owner.is_licensed || isBroadcast(p->to))). Every receiver runs
verifyFirstContactNodeInfo, fails crc32Buffer(user.public_key) != p->from, and
drops the NodeInfo. The node's identity beacons are invisible to the mesh.
Nothing reboots to repair it: AdminModule sets requiresReboot = false for LoRa
changes ("All LoRa radio changes apply live via configChanged observer") and
MenuHandler ends at service->reloadConfig(changes).
Four call sites reached ensurePkiKeys():
1. AdminModule set_config LORA, region first set (phone app - the common path)
2. MenuHandler applyLoraRegion (on-device region picker)
3. InkHUD MenuApplet applyLoRaRegion (schedules a reboot, so it
self-healed at next boot)
4. portduino wasm wasm_set_region
The reference implementation was already in the tree: the *licensed* branch of
call site 1, thirteen lines below the broken unlicensed one, calls
nodeDB->generateCryptoKeyPair() (which reaches createNewIdentity()) and widens
the persisted mask with SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE.
Rather than repeat that at four call sites, the key-mint is routed through one
chokepoint that owns both halves of the identity: NodeDB::ensurePkiIdentity()
calls crypto->ensurePkiKeys() and then createNewIdentity(). It lives in NodeDB
because createNewIdentity() operates on the devicestate/node-DB globals, which
CryptoEngine deliberately does not touch - ensurePkiKeys() takes the security
config and user by reference precisely so it stays free of that dependency, and
it is unit-tested against a standalone CryptoEngine.
ensurePkiIdentity() returns true only when my_node_num actually moved
(createNewIdentity() early-returns when the key is unchanged, so a repeat region
change does not disturb the self entry or force a needless flash write). Callers
use that to widen their save mask; my_node_num lives in devicestate and the self
row moves in the node DB, so both segments must be persisted or the fix would
revert at the next boot. SEGMENT_CONFIG, which carries the key itself, is
already unconditional on all four paths.
The InkHUD reboot is left as-is. It is now redundant for this invariant, but it
covers the rest of that menu's behaviour and a redundant reboot is not a bug.
Adds test_handleSetConfig_persistsUnlicensedFirstRegionIdentity, the unlicensed
twin of the existing licensed test, asserting both the segment mask and
my_node_num == crc32(public_key).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* style(NodeDB): trim identity-recovery comments and guard the WASM nodeDB deref
Two review asks, no behaviour change on any built target.
Copilot flagged the unguarded nodeDB deref in the WASM region setter; it is the
only ensurePkiIdentity() call site that did not check the pointer first.
The rest is comment length. AGENTS.md:83 caps code comments at two lines, and the
identity-recovery comments across the four call sites plus the NodeDB.h doc block
ran to four and six lines. The rationale they carried is in the commit messages
and the PR body, which is where AGENTS.md says it belongs.
The PR's own fix in AdminModule.cpp is deliberately untouched.
* fix(NodeDB): keep the identity move authoritative when the self record cannot be created
createNewIdentity() removes the old node entry and assigns myNodeInfo.my_node_num
before it tries to create the row for the new number. If getOrCreateMeshNode()
came back null it returned false, so the first-region callers left
SEGMENT_DEVICESTATE and SEGMENT_NODEDATABASE out of the save mask.
The number had already moved in RAM at that point, and the freshly minted key
goes to flash under SEGMENT_CONFIG regardless. The next boot therefore reloads
the old number alongside the new key, which is exactly the
crc32(public_key) != my_node_num break this path exists to prevent, reached
through the error branch instead of the happy one.
Rolling the number back is not an option either, since the key has already been
replaced by the time this runs. So the move is now reported as the fact it is and
the missing self record is logged separately; getOrCreateMeshNode() will recreate
that row on the next contact. Reachable when the self record is absent and the
table is full of protected nodes.
Reported by CodeRabbit on #11426.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Tom
|
a00675e00c | unset can have what it likes (#11496) | ||
|
Tom
|
de6b23190a |
Test suite rebuild (#11322)
* docs(nodedb): make the native node cap unambiguous The native node cap was stated in four places that disagreed, and the disagreement already caused a wrong diagnosis: a saturated 200-node database looked arithmetically impossible because the cap had been read as 248, computed from a header that does not apply on this platform. The real value is 198. On portduino MAX_NUM_NODES is not a compile-time constant at all - the variant defines it as `portduino_config.MaxNodes`, resolved at runtime, default 200 and settable per host with `General: MaxNodes`. variant.h is reached before mesh-pb-constants.h, so that header's ARCH_PORTDUINO branch never fires and its plausible-looking 250 is dead code. - #error-guard the dead branch rather than leave a wrong number where people grep. The guard found a real defect: seven translation units reach mesh-pb-constants.h without configuration.h (SerialConsole.cpp, StreamAPI.cpp, PacketAPI.cpp, ServerAPI.cpp, PiWebServer.cpp, ServiceEnvelope.cpp, MeshtasticOTA.cpp, and test/TestUtil.cpp), so each was compiling with a different MAX_NUM_NODES - and therefore a different PACKETHISTORY_MAX - than the rest of the build. Each now includes configuration.h first. It cannot be included from mesh-pb-constants.h itself: that reaches SerialConsole.h through DebugConfiguration.h and closes a cycle. - Name the bare 250 in getMaxNodesAllocatedSize() NODEDB_MIGRATION_LOAD_CEILING. It is a decode allowance for files written by larger-cap firmware, not a cap, and it read like one. - Fix docs/node_info_stores.md, which named the wrong source and a "10-250" range that is wrong for native, and the copilot-instructions tunables line that said "portduino 250". * test(harness): give each suite its own scratch HOME and report leftovers Native suites shared one directory. Every suite that constructs a NodeDB loads and saves ~/.portduino/default/prefs/ - nodes.proto, config.proto, channels.proto, module.proto, device.proto, warm.dat, transmit_history.dat - and nothing cleared it, so state leaked suite -> suite within a run and run -> every run after it. A test run could also rewrite a real meshtasticd node database on the same machine. Per-run isolation does not fix this: the leak is generated inside a single run, so the boundary has to be per suite. bin/pio-test-isolate.sh runs each suite in its own scratch $HOME, registered as test_testing_command for env:native and env:coverage so a bare `pio test` and CI get the same boundary, not just bin/run-tests.sh. It runs the binary unchanged and exits with its exit code, so PlatformIO's pass/fail is untouched. Overriding HOME here rather than around `pio` also sidesteps the blocker that a bare HOME= breaks pio's own ~/.platformio/penv/bin/pio lookup. Leftovers are reported as a second axis, PASS/FAIL x CLEAN/DIRTY, because an unintended write has no matching assertion by definition - nobody writes TEST_ASSERT for a save they do not know is happening. The harness asserts it from outside, so it applies to every suite without the author opting in. - Only the *set of changed paths* is asserted, never contents. Hashes answer the boolean "did this change?" and nothing more; content baselines over protobuf bytes would churn on every NodeInfoLite field added, which is how snapshot suites become noise. - Deliberate writes are declared in test/state-manifest.tsv - one central file, suite / flags / mandatory reason. run-tests.sh prints the opt-out count on every run. - Granularity follows the state flag, so the two ship together: per-test by default (TestUtil redefines RUN_TEST to checkpoint after each test, naming the exact test that dirtied things), suite boundary for state=per-suite, where carrying state across test cases is the declared behaviour. - A declared write that does NOT happen is reported as MISSING, not folded into DIRTY. It catches silently broken persistence; a warning for now, since some are conditional. - Graded AMBER, not RED. With isolation in place DIRTY means "undeclared", not "dangerous", and a check that lands red on day one gets switched off. Guard the guard, both halves: state_assert_empty() refuses to run a suite against a sandbox that is not empty (otherwise the after-diff measures against the wrong baseline and reports CLEAN while meaning nothing), and bin/test-state-check.sh drives the real wrapper with fixtures asserting CLEAN / CLEAN / DIRTY / MISSING plus both directions of the empty assertion. A checker that silently matches everything would otherwise pass forever. --write-manifest proposes entries for a human to paste and justify; it never applies them, and neither does CI. * test(harness): stop reporting Unity's exit code as a signal A native suite ends in exit(UNITY_END()), and UNITY_END() returns the failure count. PlatformIO's native runner reads that non-zero exit code as a POSIX signal number, so four failures print "Program received signal SIGILL", five print "SIGTRAP", and the suite is classified [ERRORED] rather than [FAILED]. There is no crash. The signal name tracks the failure count and nothing else - it moved SIGILL -> SIGTRAP when a diagnostic probe added a fifth failure - and it cost hours of hunting a memory bug that did not exist, on an env (native) that carries no sanitizer at all. It also explains the phantom extra test case in the totals: the runner adds a synthetic entry for the signal it thinks it saw. run-tests.sh now says so inline whenever a signal line appears, and the three agent-facing docs say it too. * test(admin): isolate NodeDB and globals per test setUp() did `if (!nodeDB) nodeDB = new NodeDB();` and never deleted it, so 83 of the 85 tests shared one never-reset database and never restored config, owner, devicestate or channelFile. The fixture that does restore them was opt-in and armed by exactly two tests. The setUp comment claiming the rest "set their own config/region state and are unaffected" was not true - the admin handlers under test write all four globals. Route every test through the fixture instead: setUp saves the globals and installs a fresh NodeDB, tearDown restores and deletes it. The two tests that armed it themselves no longer need to. All 85 pass, so nothing was silently relying on the shared state. It costs about 7% of the suite's runtime (a NodeDB construction is a loadFromDisk plus, with a region set, key generation) - worth paying to write the phase 3 tests against a clean fixture rather than 83 tests' residue. Also cap the per-test attribution in the run summary at five entries; the full list stays in the suite's sandbox. * test(fs): cover the bounded file-manifest walk getFiles() runs on every phone sync via STATE_SEND_FILEMANIFEST, and nothing asserted any of its bounding behaviour. It does execute unasserted from test_stream_api's handshakes, but the cap, the depth limit, the wasLimited paths, overlong-path rejection and capacity release were all unguarded. Eight tests, all describing what the code does today: today's code is already correct here, since #10778 landed the by-reference collectFiles(), the 64-entry cap, the strlcpy bounds and the swap-idiom release. They pass on arrival, which is the point - this is the baseline a later change has to leave alone. Two things they do not cover, and cannot: - Moving reserve() outside the __cpp_exceptions guard. Exceptions are on natively, so the #else branch is not compiled. The suite's job there is to prove that change alters nothing observable. - The file.name() null guard. No in-tree backend returns null; the guard is defensive. The manifest-release test pins the swap idiom rather than calling PhoneAPI's releaseFilesManifest(), which is file-local. It asserts capacity() == 0, not just size() == 0 - a size-only check passes on clear(), which is the bug #7924 shipped. Suite count 43 -> 44, recounted against the directories rather than copied. * test(admin): assert node-DB metadata saves skip the radio reload set_favorite_node, set_ignored_node and toggle_muted_node each persist a NodeInfoLite bit and nothing else. MeshService::reloadConfig() gates its region re-derivation and configChanged notification on saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS), so a SEGMENT_NODEDATABASE-only save already skips the live radio reconfigure. Pure characterization - all three pass on develop. Worth pinning because that reconfigure is the path implicated in the WisMesh Tag favourite-node crash, and develop asserts nothing about it: widening the saveWhat mask or reordering the check would currently go unnoticed. Ported from the config-save series along with ConfigChangedCounter (an Observer<void *> counting configChanged notifications, the only externally visible signal that the reload branch was taken) and TEST_NODE_NUM. They join the existing suite, so no suite-count change. * refactor(menu): extract the mute toggle into a named function The node menu's mute action was inline in a banner-callback lambda, and that lambda only ever runs via screen->showOverlayBanner() - which is why nothing in MenuHandler.cpp was reachable from a test. Lift the `selected == Mute` branch into menuHandler::toggleNodeMuted(uint32_t) and call it from the lambda. Behaviour-neutral by construction: same statements, same order, same bare saveToDisk(). The null check moves into the function, so the call site no longer needs its own lookup. Verified by the native build and suite; the byte-identical-image check on a headroom-constrained nRF52 board was not run locally - CI's firmware-size comment covers it. Three tests come with it, all describing today's behaviour: - the bit flips both ways and no configChanged fires (develop never calls reloadConfig on this path); - an unknown node is a no-op rather than a write; - and the segment mask. Flipping one NodeInfoLite bit currently rewrites all five segments via bare saveToDisk(). That is asserted deliberately, with the comment naming it as characterization of a known defect: a pending fix narrows it to SEGMENT_NODEDATABASE, and when it lands this assertion is expected to change, which makes the improvement visible in the diff instead of silent. saveToDisk() is not virtual, so the mask is observed through its effect - remove the five prefs files, toggle, and see which reappear. * docs(test): make every suite count a pointer to the canonical one test/native-suite-count is the registered total and is machine-checked against test/test_* on every full run and by the suite-count-check CI job. Every other statement of the count is a copy that drifts: copilot-instructions said 12, AGENTS.md said 19, and the real number is 44. Replace both literals with a pointer to the file, say explicitly that no document should state the count as a literal, and reframe the two suite listings as descriptions rather than inventories - they carry per-suite information the count does not, so they stay, but nothing should infer completeness from their length. Register the new FS suite in both. * test(harness): randomise suite order, reproducibly Landed last, deliberately. Randomising an order-dependent suite set does not find bugs so much as convert a silent pass into intermittent red, and the first instinct is to revert the randomisation rather than fix the coupling. Phases 1-2 removed the coupling; this keeps it removed. Both runners previously hid order dependence behind a fixed order that happened to differ between them, and neither order was chosen: CI's area rules put admin first, PlatformIO's local discovery is reverse alphabetical and put it last. CI was green by accident. - bin/run-tests.sh --shuffle / --seed <n>. The seed defaults to HEAD's short SHA: one order per commit, so a red is replayable and attributable to the diff instead of flaky, while the project keeps exploring orders. Printed at the start and carried into the RESULT line, so a verdict is replayable from that line alone; the full order is printed on failure, because for an order-dependent failure the order is the diagnostic. - The shuffle is a Fisher-Yates over a MINSTD generator rather than awk's rand(), whose sequence differs between gawk and mawk. A seed that does not reproduce the same order on another machine is not a seed. - Shuffling needs one `pio test -f <suite>` invocation per suite - PlatformIO orders by its own os.walk() over test/ and filters only select - which measures at about 4.7s per suite of extra startup. - CI shuffles its area order, seeded from GITHUB_SHA and printed with the command to replay it locally. Intra-area order stays PlatformIO's; controlling it there would mean per-suite invocations, which is a cost worth deciding separately. Also records the 16 measured entries in test/state-manifest.tsv, each with its reason, taken from a full run's --write-manifest output rather than guessed. * test(default): cover the region-throttle interval overload getConfiguredOrDefaultMsScaled(configured, default, nodes, TrafficType) is the overload every telemetry and position module actually calls, and nothing referenced TrafficType anywhere under test/. All four of its behaviours were unguarded: the no-region guard, the throttle <= 1 short-circuit, the multiply, and the 64-bit overflow clamp. The throttles are real, not hypothetical - EU_866 carries PROFILE_LITE, which sets both positionThrottle and telemetryThrottle to 10, so a change here moves broadcast spacing in that region by an order of magnitude. Each test pins numOnlineNodes at the congestion threshold and uses ROUTER, which never congestion-scales, so the coefficient is 1 and the throttle is the only variable. The overflow case needs a base above INT32_MAX/10, hence three days rather than one. * ci(test): keep pull-request suite order fixed, seed the rest Shuffling the area order on every run - including pull_request - would turn a contributor's PR red for an ordering they did not choose, which is how a randomisation gets reverted instead of the coupling being fixed. That is the exact dynamic the ordering work was sequenced last to avoid, and the previous commit walked straight into it. - pull_request keeps the fixed declared area order. - push and schedule shuffle, seeded from the commit SHA: deterministic per commit, printed, attributable, and never blocking someone else's PR. - A suite_order_seed input on workflow_call and workflow_dispatch overrides both, so a specific failing order can be replayed anywhere, including on a PR. The run log prints which mode it took, the resulting order, and the local command to replay it. * ci(test): satisfy CKV_GHA_7 and yamllint on the seed input The seed is reachable through workflow_call, which callers can pass programmatically. The workflow_dispatch copy tripped checkov's "workflow_dispatch inputs MUST be empty" rule, and suppressing it was not worth it: replaying a specific order is a local operation, and the run log already prints the exact bin/run-tests.sh command to do it. * style(menu): apply the node-ID format convention RadioInterface.cpp documents the rule: 0x%08x in logs, !%08x in user-facing display. MenuHandler held every remaining exception - seven logs printing bare %08X, and two display labels doing the same. Repo-wide there are now no bare %08X node IDs left in log calls. * ci(test): pass workflow inputs through env, not shell interpolation suite_order_seed and github.event_name were spliced into the run: script as ${{ }} text, so a value carrying shell metacharacters would execute as code on the runner rather than being read as data. semgrep (run-shell-injection) and zizmor (template-injection) both flag it. Both now arrive as environment variables and are read as "$VAR". * refactor(test): share the seeded shuffle between the harness and CI bin/run-tests.sh and test_native.yml each carried a byte-identical copy of the MINSTD Fisher-Yates awk. The workflow prints "replay locally: ./bin/run-tests.sh --shuffle --seed $seed" after a shuffled CI run, and that instruction is only true while the two agree - drift would be announced by a replay quietly reproducing a different order than the one that failed. Extract shuffle_suites() to bin/lib/shuffle.sh and source it from both. Permutations verified identical across seeds before and after the move. * fix(test): correct the shared-state MISSING check and summary join Three defects in the new harness: state_classify() matched declarations two different ways - state_path_declared() for "undeclared", a hand-rolled regex for "missing". Interpolating an entry into an ERE also let a metacharacter in a manifest name match a file that is not the declared one. Both directions now go through the one helper. `paste -sd'; '` does not join with "; ": with -s, paste cycles through a multi-character delimiter one character per join, so paths rendered as "a;b c;d e". Replaced with an awk join. test-state-check.sh ran on after a failed cd instead of stopping (SC2164). ./bin/test-state-check.sh: 6/6 fixtures pass, MISSING included. * fix(portduino): bound General.MaxNodes MaxNodes was validated only for <= 0. Any positive value, including a typo'd or pasted-in one, propagates to MAX_NUM_NODES and scales both the node DB and the nodes.proto decode ceiling - failing at boot with no obvious cause. The ceiling is a sanity bound, not a capability limit; raise it if a host genuinely needs more. * docs(nodedb): reconcile the capacity tables The property matrix omitted the ESP32-S3 100-node flash tier that the platform table above it lists, and neither mentioned that the WASM build overrides MaxNodes to 80 in wasm_config_apply(). * fix(nodedb): make mesh-pb-constants.h self-sufficient on portduino The ARCH_PORTDUINO #error assumed it was unreachable in a normal build. It is not: the vendored device-ui sources include this header without configuration.h, which broke both native-tft docker builds. Include configuration.h here instead, ahead of every compile-time default - variant.h overrides MAX_RX_TOPHONE as well as MAX_NUM_NODES, so placing it lower in the file just moves the divergence to a redefinition. The #error stays as a backstop for the case where that include genuinely stops providing the cap. Verified with the native env's own flags: a TU including only this header now compiles, normal-order use of both macros compiles, and NodeDB.cpp compiles. * fix(portduino): raise the MaxNodes ceiling to 16000 Marked artificial: nothing in the node DB fails at 16001. 16000 sits just under the 16384 (128 x 128) population where HopScalingModule saturates its sampling denominator and starts dropping nodes, so a host inside the bound still gets meaningful hop recommendations. * lint(trunk): advise on node IDs logged as bare %08x RadioInterface.cpp documents the convention - 0x%08x in logs, !%08x in display - but nothing enforced it, which is how the MenuHandler cluster drifted. 22 call sites in PacketHistory, NodeInfoModule and PositionModule are still off it. A trunk linter rather than a CI grep job, because trunk checks changed files: new violations get flagged without a 22-site cleanup landing in an unrelated PR. Modelled on the existing too-many-defined definition. Scoped to values it can tell are IDs - an ID-shaped argument (->num, .from, getNodeNum) or message text naming one. A 32-bit hex that is not an ID is out of scope, so the CRC32 logs in ethOTA.cpp are correctly ignored. Emits "note", trunk's only non-blocking level: "warning" and "info" both exit non-zero and would gate CI, which is not what a log-format nit deserves. The pre-existing sites are line-scoped in the allowlist, so a new bad call in those same files is still caught. * lint(trunk): stop exempting the known node-id-format sites The seeded allowlist made the rule green by declaring the backlog acceptable. Empty it instead, so the 22 pre-existing sites are reported and get cleaned up by whoever next edits those files. Costs nothing to do: the rule emits "note", so these are non-blocking either way. The allowlist stays for its real purpose - a value the linter misreads as an ID. * style: log node and packet IDs as 0x%08x Clears the 22 sites the node-id-format linter reports, so the rule starts from zero rather than from a backlog nobody can see - trunk suppresses pre-existing findings by default, so left alone these would not have surfaced on edit the way an empty allowlist implies. Format strings only; no argument or control flow changes. The !%08x user-facing display forms are deliberately untouched - that is the other half of the same convention. * test(harness): build once up front, so suite timings mean something run-tests.sh fused build and run in a single pio invocation, so whichever suite PlatformIO's directory walk reached first absorbed the entire src compile and reported it as its own duration. On a real run that made a 0.03s suite report 13m21s, and hid the build cost from every other number in the summary. Do what .github/workflows/test_native.yml already does: one --without-testing build pass, then run with --without-building. Measured on a full 44-suite run - the build is now a single reported figure and 968 test cases execute in 1.9s, with no suite above 0.084s. Build output goes to its own log rather than $LOG: the outcome regexes match "error:" and "[ERRORED]", so a compiler diagnostic sharing that file would read as a test failure. Both red paths now keep the log they quote from. $LOG and the build log are mktemps the EXIT trap removes, so the three grepped lines were previously all anyone ever saw - and the cause is usually further up than the first [FAILED]. * test(harness): keep the run log on every red path bin/pio-test-isolate.sh already keeps a failing or DIRTY suite's sandbox and log under .pio/test-state/<suite>/. What was missing is the cross-suite view: $LOG is a mktemp the EXIT trap deletes, so run-tests.sh quoted three grepped lines from a file that no longer existed by the time anyone looked. Preserve it as .pio/build/<env>/test-failure.log from both red paths - including "no success summary found", which said "see log" while preserving nothing, and which is exactly the case where the build died before any suite ran and so left no per-suite sandbox either. Cleared at the start of every run, so a green run cannot leave a red one's log lying around looking current. * fix(test): report the real failure count on a shuffled red A shuffled run is one `pio test` invocation per suite, all appending to the same log, so the log carries one PlatformIO "N test cases:" summary per suite. verdict_red() took `tail -1`, which reports whatever the LAST suite did: a failure in suite 3 printed a "0 failed" summary from suite 44 directly under "RED - failures detected:". Sum the summaries instead. A single summary line - every unshuffled run - is passed through verbatim, so the familiar output is byte-identical. The patterns are passed to the awk helper as strings rather than /regex/ literals: awk evaluates a regex literal in argument position as `$0 ~ /re/`, so the callee would receive 0 or 1 and silently sum garbage. * fix(test): do not emit an empty suite name for an empty shuffle `printf '%s\n' "$@"` with no arguments still writes one empty line, and both callers read shuffle_suites through mapfile, so an empty suite list arrived as a single suite named "". Return before the printf when there is nothing to shuffle. * test(harness): state and enforce the Linux host requirement The native harness is a Linux tool: bash 4+ (mapfile), GNU coreutils and GNU find (-printf, md5sum, -executable). Most of that predates this branch - mapfile and both find predicates are already on develop - but none of it was written down, so the requirement was there to be discovered rather than read. Refuse to start on a non-Linux uname instead of degrading. On a BSD userland this would not fail cleanly: it would mis-hash the sandbox and mis-read the suite list, and still print a verdict. A state check that silently measures the wrong thing is worse than one that declines to run. Carrying a per-host fallback was the alternative, and it buys a second code path that nothing in CI exercises. bin/test-native-docker.sh already exists for macOS and non-Linux hosts, and the native-macos PlatformIO env is a build target for meshtasticd, not a test host - the isolation wrapper is registered for env:native and env:coverage only. Documented in the script header, test/README.md, and both agent docs. * fix(test): terminate every suite with exit(UNITY_END()) Two sites across two suites ended on a bare UNITY_END(). That ends the reporting, not the suite: setup() returns, the runtime goes on calling loop(), and the process runs forever. PlatformIO does not notice - it reports a suite from its Unity output, not from process exit - so the suite passes, the run goes green, and the binary stays resident. Thirteen of them had accumulated on one dev box, the oldest 19 hours old. The costs are quiet by construction: - the per-suite sandbox is deleted underneath a live process, so its CLEAN/DIRTY verdict describes what the suite had written when the harness stopped looking, not what it left behind; - .gcda coverage and LeakSanitizer's report both flush from atexit handlers, so a suite that never exits contributes no coverage and gets no leak check; - each survivor pins its own deleted 94 MB binary, which du cannot see. One of the two is the #else of an architecture guard, which is the easiest one to get wrong - it looks like there is nothing to clean up. test_mqtt has a correct exit(UNITY_END()) in its live branch, so a "does this file call exit() anywhere" check passes the file whole. test_serial had two more. develop's serial-config validation rework restructured that suite - the architecture guard is gone and both remaining branches now exit correctly - so this commit no longer has anything to change there; bin/lint-unity-exit.sh, added later on this branch, is what keeps it that way. test/README.md gets a section on it, since the skeleton showing the right shape had not stopped this happening. * test(harness): detect and reap suites that outlive their run A suite that never exits was invisible: PlatformIO reports a suite from its Unity output, so the run stayed green while the binary kept running. Two checks, because they fail differently. Runtime, in bin/pio-test-isolate.sh: the sandbox $HOME is mktemp-unique per suite, so any process still holding it is a survivor of that suite. Matching on the environment rather than a remembered PID identifies one whatever its parentage - a fork, a grandchild, a process already reparented to init - none of which a $! comparison catches. Reaped before the after-fingerprint is taken, so that fingerprint measures a tree nobody is still writing to, and so a run cannot leave processes accumulating on the host. Recorded as a sixth summary column and graded AMBER: the tests did pass, but the CLEAN verdict and the coverage were measured under a false assumption. Author-time, as bin/lint-unity-exit.sh, wired into trunk at "note" like node-id-format: every UNITY_END() must be wrapped in exit(). The rule is per occurrence, and that is the point - a file-level "calls exit() somewhere" check passes test_serial and test_mqtt, which have a correct one in their live branch and a bare one in the #else. Running it over the tree turned up test_mqtt, which the file-level pass had missed. It allows `int rc = UNITY_END(); ...; exit(rc)`, used by test_packet_signing to restore globals between the summary and the exit. That is where the rule gives ground: capturing and never exiting would leak and is not flagged. Flagging a correct idiom would push someone to "fix" working code. bin/test-state-check.sh gains a survivor fixture, asserting the wrapper both reports and reaps - a detector that only reports leaves the host accumulating processes, which is half the harm. 8/8. * fix(lint): make the unity-exit scanner statement-aware The rule judged one physical line at a time, which reports two kinds of correct code as bare: /* a comment that happens to mention UNITY_END() */ <- interior lines were never stripped exit( UNITY_END()); <- exit( and the macro never met On a probe of both, two of three findings were wrong. This is a note-level rule whose whole job is advice, and bin/lint-node-id-format.sh already says why that matters: a false positive costs more than a miss. One that cries wolf gets ignored, and the real finding goes with it. Carry /* ... */ state across lines and accumulate logical statements before testing, with a 12-line cap so one unclosed call cannot swallow the rest of the file - the same structure lint-node-id-format.sh uses, so the two custom linters in bin/ work alike rather than each having its own idea. Verified both directions: the develop-era sources still produce the same four findings, the fixed tree produces none, and a probe covering block-comment interiors, wrapped exit(), line comments, return UNITY_END() and capture-then- exit reports only the genuinely bare calls - including a complete block comment followed by real bare code on the same line, which the state machine has to keep live. Reported by CodeRabbit on #11322. * fix(lint): tokenise instead of pattern-matching, and self-test it Second round of review findings on the same scanner, all confirmed by direct test before changing anything. Six defects, one root cause: layered regexes cannot tokenise C++. False positives (correct code reported): - UNITY_END() inside a string literal read as code False negatives (real leaks missed): - a string containing "/*" opened comment state and swallowed later lines - greedy .* removed everything between two block comments on one line, taking a bare call with it - myexit(UNITY_END()) matched the exit() exemption as a substring - x == UNITY_END() and total += UNITY_END() matched the assignment exemption Replaced with a character-level scan carrying comment state, and token-bounded exemptions: exit must be a whole identifier, and the capture form must be a plain `=`. Raw string literals are still not modelled - there are none under test/, and delimiter tracking for a case that does not occur would be untested code guarding untested code, so it is documented rather than guessed at. Also drops the `return UNITY_END()` exemption. It only terminates from main(), there is no main() under test/, and from a helper it just returns a count. bin/test-lint-unity-exit.sh pins all fifteen cases, every false positive and false negative found in review among them. The rule has been wrong twice in a way that looked fine by inspection; it needed a self-test more than it needed another careful reading. Two further findings in the same review: - bin/run-tests.sh dropped PASSTHRU in shuffled mode, so `--shuffle -vvv` built verbosely and then ran quietly. The shuffled loop now forwards EXTRA_ARGS, which is PASSTHRU minus the -f pair it supplies per suite. - bin/run-tests.sh did not guard `cd "$ROOT_DIR"`. And one that did not reproduce: the survivor fixture's glob does find the pid file (verified with the lookup instrumented - the earlier failure was an artifact of running the script from /tmp, where SCRIPT_DIR cannot resolve). The assertion was still weak, because an empty pid took the "not running" branch and passed vacuously. It now fails if the pid was never recorded, and finds the file by search rather than assuming a directory depth. Reported by CodeRabbit on #11322. * fix(lint): report each UNITY_END occurrence at its own location The self-test only asked "did the linter say anything", so it could not have caught a wrong line, a wrong column, or a missing second finding. Fixtures now assert the exact diagnostics as line:col, and the first run of that assertion found two real problems. The caret pointed at the wrong occurrence. For `exit(UNITY_END()); UNITY_END();` the verdict was right but the column was 17 - the wrapped call - because the scanner stripped terminating forms out of the whole statement and then reported the first occurrence it had seen. Two bare calls on one line reported once. Judged per occurrence now, by looking back through whitespace at what wraps it, so both the count and the caret are right. That also needed a position map from strip_noncode(): removing a comment or collapsing a literal shifts every later column, and counting occurrences in the raw line does not recover it either - TEST_MESSAGE("... UNITY_END() ..."); UNITY_END(); has two occurrences in the raw text and one in the code. Four of the expected columns I wrote by hand were also wrong, off by one. The linter was right in every case; the assertions were not. They are computed from the fixture text now rather than pasted from output, because a baseline accepted from the tool it is testing asserts nothing. 17 fixtures, including the two-on-one-line case from review and its mirror. Reported by CodeRabbit on #11322. |
||
|
Ben Meadors
|
ae16c052d5 |
Time out an abandoned admin edit transaction (#11254)
begin_edit_settings sets a bool that only the matching commit ever clears. If the commit never arrives -- the client dropped its link partway through a bulk config import, or went away entirely -- the transaction stays open indefinitely, and every later config write from any client is applied to RAM, acknowledged, and then never saved. The writes look successful and are visible in get_config, but the node reverts all of them at the next boot, and only a reboot clears it. Give the transaction a one minute idle timeout. Each deferred write restarts the clock, so the window bounds the gap between writes rather than the length of the edit; a bulk import sends them milliseconds apart. The next admin message after it lapses retires the transaction, persisting the segments it had deferred and flushing the warnings it was holding. The check runs after the auth gates and before the switch, so every case sees consistent state and the recovery's flash write happens in the main loop rather than a disconnect callback. It saves rather than rolls back because there is nothing to roll back to: each write already took effect in RAM and was acknowledged, so the transaction only ever deferred the save. That also makes an early expiry cheap -- the worst case for a client that really was still going is that its remaining writes are saved individually instead of batched. Closing on disconnect instead was tempting, but the reporter's captures show iOS dropping and reconnecting within half a second several times per session, which would abort imports that currently survive the blip. Also drops the file-scope hasOpenEditTransaction, shadowed by the class member at every use site and referenced nowhere else in the tree. Reported in #11245 |
||
|
|
1e982fa78c |
Sign plaintext packets in licensed mode (#10969)
* feat: sign licensed plaintext packets Preserve and publish identity keys in licensed mode so existing XEdDSA signatures can authenticate plaintext traffic. Sign licensed broadcasts and direct messages when they fit, while keeping PKI encryption disabled and normal routing behavior unchanged. * fix(security): persist licensed channel sanitation * fix(security): close licensed migration lifecycle gaps * fix(security): address licensed signing review feedback * test: restore signing globals from Unity teardown * fix(baseui): allow confirmed ham region selection * fix(baseui): guard region picker validation * style: trunk fmt --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Austin <vidplace7@gmail.com> |
||
|
Ben Meadors
|
5cf346311c |
fix: don't wipe admin keys when regenerating the keypair (#11088)
A client's "regenerate keys" action sends a blank SecurityConfig carrying only the new private key rather than the config it read from the device, so assigning it wholesale cleared admin_key, is_managed, serial_enabled, debug_log_api_enabled, admin_channel_enabled and packet_signature_policy. Losing the admin keys locks the owner out of remote admin with no recourse but a physical connection to the node. Detect that bare-rotation shape and swap in just the keypair, leaving the rest of the security config intact. Deliberately clearing admin keys still works through a SET that leaves the private key alone. Fixes #11073 |
||
|
|
5513c36757 |
Add helpful checks of channel names and PSK (#10792)
* added warnings from firmware for simple channel setting mistakes * more and better checks * one ping only * Drop literal 'AQ==' from blank-PSK channel warnings The base64 encoding of the default channel key isn't something a user should type in by hand; state the condition instead of a raw key value. --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Ben Meadors
|
c5355641d3 |
Add MEDIUM_TURBO modem preset (#10988)
* Protobufs * Wire up MEDIUM_TURBO modem preset MEDIUM_TURBO (500 kHz, SF9, CR 4/5) already existed in the protobuf enum but was never wired into firmware, so selecting it silently fell through to the LONG_FAST default and rendered an "Invalid" display name. Add its bw/sf/cr mapping (modemPresetToParams), display name (MediumTurbo/MedT), PRESETS_STD membership (standard regions only — 500 kHz does not fit EU868's 250 kHz band, so it stays out of PRESETS_EU_868 and is rejected/clamped there), and the MEDIUM SNR-grading bucket. Includes positive coverage in test_radio, EU868-reject + US-accept coverage in test_admin_radio and test_mesh_beacon, the STD preset count 9->10, an extended fuzz range, and the client-spec doc. * Address review feedback on MEDIUM_TURBO tests - test_mesh_beacon: assert has_mesh_beacon before checking the invalid preset was cleared, so the EU868-cleared test can't pass on a dropped message (matches the existing SHORT_TURBO test). - test_fuzz_packets: draw modem presets from _ModemPreset_ARRAYSIZE instead of a hard-coded 17 so the fuzz range tracks future enum additions automatically. |
||
|
Ben Meadors
|
515fe8b94f | Clamp bw for malformed use_preset false with empty bandwidth | ||
|
TomandClaude Fable 5
|
d846780a9b |
More fuzz tests and small fixes for the findings (#10864)
* first pass tests * more tests * Fix two crafted-admin-packet crashes found by the E5 fuzzer Both are reachable from an authorized admin (local from==0, admin channel, or PKC) - remote DoS: 1. SIGFPE in LoRa config validation. A set_config LoRaConfig with use_preset=false and bandwidth=0 makes freqSlotWidth 0, so numFreqSlots is 0 and `hash(name) % numFreqSlots` (RadioInterface.cpp) divides by zero. Guard the modulo; the existing channel_num check then rejects/ clamps the config. 2. Stack overflow in Channels::getKey. A SECONDARY channel at the primary slot with an empty PSK recursed into getKey(primaryIndex) forever. Skip the primary-key borrow when chIndex == primaryIndex. Re-enable the E5 admin fuzzer to hit both triggers again (use_preset both ways incl. bandwidth 0, plus the set_channel tag) as regression guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Correct fuzz-test invariants after the crash fixes - E5 admin fuzz: node eviction under a filling NodeDB is legitimate, so assert only the bounded-count invariant, not that a specific seed node survives 6000 mutating ops. - TMM blitz: scope off the nodeinfo direct-response send path (it needs a fully-wired MeshService/phone queue the fixture doesn't provide; the deterministic directResponse tests cover it). The crafted-nodenum rate/unknown/position cache stress is unchanged. clod helped too * realistic tests * test: dedup fuzz RNG into shared test/support/DeterministicRng.h The four in-tree fuzz suites (test_fuzz_decode, test_fuzz_packets, test_hop_scaling, test_traffic_management) each carried a byte-identical copy of the seeded 64-bit LCG (rngSeed/rngNext/rngByte/rngRange). Hoist it into one shared header so there is a single generator to reason about and no risk of the copies drifting. static inline keeps per-suite state per translation unit and avoids -Wunused-function for suites that don't use every helper. Also corrects a stale comment in test_traffic_management (the blitz's nodeinfo direct-response path is intentionally left off). No behavioral change: same constants, same per-suite seeds. clod helped too * test: fuzz uncovered ProtobufModule handlers and the MQTT downlink ingress Extend the in-tree fuzz coverage to packet sources that previously had none: - test_fuzz_packets E8/E9/E10: drive PositionModule, DeviceTelemetryModule and NeighborInfoModule at handleReceivedProtobuf directly (via using-shims, bypassing the ProtobufModule reply/send path so no router is needed). The fixture already stands up nodeDB/service/channels, and nodeStatus/powerStatus are auto-initialized in main.cpp, so no new globals are required. Adds a shared fuzzRxHeader() helper for crafting adversarial RX packet headers. - test_fuzz_decode: add meshtastic_KeyVerification to the decode table. The KeyVerification and StoreForward handler paths are documented as decode-level only, with the concrete reason each is intrinsic (private-state gating / PSRAM + self-pointer wiring), not a fixture gap. - test_mqtt: test_receiveFuzzServiceEnvelope blitzes the non-RF broker-push ingress (onReceiveProto) two ways - raw garbage bytes that must fail envelope decode cleanly, and a well-formed ServiceEnvelope wrapping a crafted inner MeshPacket over crafted channel_id/gateway_id - exercising the channel match, isFromUs, XEdDSA receive policy and perhapsDecode chain. Adds a deliverRaw() passthrough to MQTTUnitTest. All under the coverage env (ASan/LSan). No firmware/src changes. Full sweep GREEN 27/27, 544 cases. clod helped too * Harden LoRa/channel config against crafted admin messages; consolidate test helpers Production (review findings on the hot-fuzz crash fixes): - Clamp bandwidth at the source (clampBandwidthKHz) in checkOrClampConfigLora and applyModemConfig so numFreqSlots can never be 0 for any consumer; a bandwidth-0 set_config previously passed validation and re-armed the SIGFPE on the next applyModemConfig. - Guard applyModemConfig's hash % numFreqSlots (the validator's sibling modulo was fixed earlier but this one was still unguarded). - Enforce the primary-channel invariant in Channels::onConfigChanged: a config demoting every slot now re-promotes the stale SECONDARY slot (keeping its key) or restores the default channel if the slot is DISABLED, instead of leaving every getPrimaryIndex() reader on a non-primary slot. The getKey recursion guard stays as defense-in-depth. Tests: - New test/support/MockMeshService.h and AdminModuleTestShim.h replace four byte-identical mocks and three divergent admin shims (test_mqtt's capturing mock is genuinely different and stays). - DeterministicRng.h: add rngFill() (replaces 14 hand-rolled fill loops) and rngEdgeNodeNum() (unifies the three NodeNum boundary pools). - Extract fuzzChannelSettings() shared by the set_channel case and fuzzBeacon. - fuzzBeacon: the un-terminated branch now fills the whole buffer with non-NUL bytes so the strnlen bound is actually stressed (~50% of iterations, not ~4%). - E6 beacon fuzz: replace the TEST_ASSERT_TRUE(true) tautology with real invariants (handler never consumes; offers land in lastReceivedOffer keyed to the sender). - Trim the seven over-long comment blocks flagged against the 1-2 line rule; the FINDINGS trailer moves to this commit message (see production notes). Full native suite GREEN 27/27 under the coverage (ASan/LSan) env. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Clamp UTF-8 char length in the emote walkers; add test_fuzz_emotes A TEXT_MESSAGE payload is opaque protobuf bytes, so PB_VALIDATE_UTF8 never screens it - invalid UTF-8 and truncated multi-byte lead bytes reach the emote/width render path verbatim. EmoteRenderer's walkers advanced by utf8CharLen(lead) without clamping to the bytes actually remaining, so a truncated lead (e.g. a lone 0xF0, which claims 4 bytes) near the end of the buffer made getUtf8ChunkWidth's memcpy read past the string. ASan confirms a heap-buffer-overflow READ from measureStringWithEmotes. Add utf8CharLenClamped() and use it at every walk site (width measure, truncation cut-loop, and the draw-path text-run/chunk builders); the one already-guarded site (matchAtIgnoringModifiers) is unchanged. New test/test_fuzz_emotes drives measureStringWithEmotes and truncateToWidth over adversarial byte strings (biased to embed/end in truncated multi-byte leads) in exact-sized heap buffers so any over-read is a hard ASan fault. Its headless display uses a synthetic font (firstChar 0, fontData centered in a large buffer) so the stock OLEDDisplay::getStringWidth - which indexes the font jump table with a signed char and over-reads for any byte >= 0x80 - does not mask the finding. native-suite-count bumped 27 -> 28. Full native suite GREEN 28/28 under the coverage (ASan/LSan) env. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Keep emote width measurement in-bounds for non-ASCII bytes OLEDDisplay::getStringWidth (the utf8=false path EmoteRenderer uses on default builds) indexes the font jump table by (c - firstChar) with a signed char and no bounds check, so any byte outside printable ASCII - high bytes from UTF-8 text, but also a stray control byte like 0x0A - reads outside the font array. On-device this reads adjacent flash and returns a garbage width; under ASan the test_fuzz_emotes fuzzer flags it as a global-buffer-overflow, and it made the non-ASCII width measurement meaningless either way. The OLED driver is a pinned upstream dependency, so guard it firmware-side in EmoteRenderer's getStringWidth helper: measure a sanitized copy where any byte outside [0x20, 0x7E] counts as a '?' placeholder. Printable ASCII is unchanged and the UA/RU lookup path is untouched. test_fuzz_emotes now drives a real ArialMT font instead of the synthetic in-bounds font it needed before this fix, so the suite exercises the true production width path (utf8CharLen clamp + this sanitizer) end to end. The same fuzzer tripped the global-buffer-overflow before this change. Full native suite GREEN 28/28 under the coverage (ASan/LSan) env. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Tom
|
3becaf2d95 | emdashes begone (#10847) | ||
|
Ben Meadors
|
0488a46a3c |
fix: stop unexpected NodeNum regeneration from PKI key loss (#10808)
* fix: stop unexpected NodeNum regeneration from PKI key loss A node's NodeNum is derived from its PKI public key (my_node_num == crc32(public_key)), so it changes only when the keypair is regenerated -- which happens only when a keygen path runs while config.security.private_key.size != 32. Two paths could trigger that without the user intending a new identity: 1. Boot: loadFromDisk() collapsed every non-success loadProto() result for the config file into installDefaultConfig() (preserveKey=false), which memset()s config and wipes the private key. loadProto() distinguishes DECODE_FAILED (file present but undecodable/undecryptable) from OTHER_FAILURE (absent), so a transient/corrupt read silently minted a new identity. A new configDecodeFailed flag now gates the boot keygen and the boot config-save directly (not via region, so it survives the userprefs/region overrides that run later in loadFromDisk): identity is frozen, the unreadable file is left intact for a later clean boot to recover, and the node boots radio-silent (region UNSET, TX off). A genuinely-absent config still gets defaults + keygen. 2. AdminModule security SET: config.security = c.payload_variant.security wholesale-clobbered the keypair, then regenerated whenever the incoming private_key.size != 32. A client editing an unrelated security field without round-tripping the private key would re-mint identity. The existing keypair is now preserved when the incoming config omits it; first provisioning and key import are unchanged. Intentional reset goes through factory_reset. Native PlatformIO unit suite (Docker): 499/499 test cases pass. * test: cover security keypair preservation; clarify load-failure comment Addresses PR review feedback: - Add native unit tests (test_admin_radio) asserting handleSetConfig preserves the existing identity keypair when a security SET omits the private key, and applies a full supplied keypair (key import). Guards against reintroduced NodeNum/keypair regeneration. AdminModuleTestShim (now a friend) defers saves so the lightweight harness doesn't saveToDisk an uninitialized node database. - Clarify the non-DECODE_FAILED config-load comment: OTHER_FAILURE / NO_FILESYSTEM cover an absent or unopenable file, not just first boot. |
||
|
Tom
|
ab882c5619 |
EU regions merge (#10675)
* stronger together * validate 2.4ghz regions * less noise * you're right, and that shapens the analysis significantly * sassy rejoinder |
||
|
nomdetom
|
5d55353939 | Some fixes and tidies for testing both online and in unit_tests | ||
|
vidplace7andCopilot
|
178ae0a7f1 |
Move overrideSlot from RegionProfile to RegionInfo (override per-region)
Move default frequency (slot) override from RegionProfile to RegionInfo (set per-region). This is usually set to `0`, but will be especially useful for Ham modes where each region default must fit within a band plan. Co-authored-by: Copilot <copilot@github.com> |
||
|
TomandCopilot
|
5e69bc6c3f |
Enable Narrow and Lite regions for EU (#10120)
* Enable Lite and Narrow regions and introduce getEffectiveDutyCycle for Lite profiles * Add TrafficType enum and extend getConfiguredOrDefaultMsScaled to manage based on regionProfile settings * Refactor telemetry modules to include TrafficType in getConfiguredOrDefaultMsScaled calls * Update submodule protobufs to latest commit * Add support for new region presets and modem presets in menu options * Add new LoRa region codes and modem presets for EU bands * boof * Add modem presets for LITE and NARROW configurations * Update subproject commit reference in protobufs * Update protobufs * Refactor modem preset definitions to use macro for consistency and clarity * Refactor modem preset cases to use PRESET macro for consistency * fix: update LoRa region code for EU 868 narrowband configuration Co-authored-by: Copilot <copilot@github.com> * Fix test suite failure Co-authored-by: Copilot <copilot@github.com> * Add override slot override - for when one override isn't enough. Co-authored-by: Copilot <copilot@github.com> * address copilot comments --------- Co-authored-by: Copilot <copilot@github.com> |
||
|
TomandCopilot Autofix powered by AI
|
22d63fa69c |
Lora settings expansion and validation logic improvement (#9878)
* Enhance LoRa configuration with modem presets and validation logic * Rename bootstrapLoRaConfigFromPreset tests to validateModemConfig for clarity and consistency * additional tidy-ups to the validateModemConfig - still fundamentally broken at this point * Enhance region validation by adding numPresets to RegionInfo and implementing validateRegionConfig in RadioInterface * Add validation for modem configuration in applyModemConfig * Fix region unset handling and improve modem config validation in handleSetConfig * Refactor LoRa configuration validation methods and introduce clamping method for invalid settings * Update handleSetConfig to use fromOthers parameter to either correct or reject invalid settings * Fix some of the copilot review comments for LoRa configuration validation and clamping methods; add tests for region and preset handling * Redid the slot default checking and calculation. Should resolve the outstanding comments. * Add bandwidth calculation for LoRa modem preset fallback in clampConfigLora * Remove unused preset name variable in validateConfigLora and fix default frequency slot check in applyModemConfig * update tests for region handling * Got the synthetic colleague to add mock service for testing * Flash savings... hopefully * Refactor modem preset handling to use sentinel values and improve default preset retrieval * Refactor region handling to use profile structures for modem presets and channel calculations * added comments for clarity on parameters * Add shadow table tests and validateConfigLora enhancements for region presets * Add isFromUs tests for preset validation in AdminModule * Respond to copilot github review * address copilot comments * address null poointers * fix build errors * Fix the fix, undo the silly suggestions from synthetic reviewer. * we all float here * Fix include path for AdminModule in test_main.cpp * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * More suggestion fixes * admin module merge conflicts * admin module fixes from merge hell * fix: initialize default frequency slot and custom channel name; update LNA mode handling * save some bytes... * fix: simplify error logging for bandwidth checks in LoRa configuration * Update src/mesh/MeshRadio.h Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |