From 435d72a137ddf8059eb4f1e2ab01d912ea8ab4f6 Mon Sep 17 00:00:00 2001 From: forkwright Date: Sun, 16 Aug 2026 22:02:26 -0500 Subject: [PATCH 1/5] fix(kerykeion): attribute mesh-packet source to the sender, guard sentinel from values mesh_packet.from feeds the node DB unconditionally, so any node on the mesh can claim any NodeNum -- including the two values that are never a real originating node (0, and the 0xFFFF_FFFF broadcast address) -- and create or update that entry. Meshtastic carries no cryptographic sender binding at this layer in this proto subset (no signature, no relay_node field), so `from` remains the strongest identity signal available but not a verified fact. Wrap it in ClaimedNodeNum at the one place it turns into a node-DB attribution, so the conversion reads as a stated trust decision rather than a bare cast, and reject the two non-node sentinels before they reach the DB. node_came_online (router.rs) moves store-and-forward messages toward a NodeNum and was flagged as reachable from raw packet attribution; it is not -- verified by repo-wide grep, only MeshRouter's own tests call it. Documented the invariant so it stays that way. --- crates/kerykeion/src/collector.rs | 55 +++++++- .../src/collector_tests_attribution.rs | 124 ++++++++++++++++++ crates/kerykeion/src/router.rs | 9 ++ 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 crates/kerykeion/src/collector_tests_attribution.rs diff --git a/crates/kerykeion/src/collector.rs b/crates/kerykeion/src/collector.rs index d2df6bb..621e5d5 100644 --- a/crates/kerykeion/src/collector.rs +++ b/crates/kerykeion/src/collector.rs @@ -62,6 +62,40 @@ pub trait Collector: Send + Sync { ) -> impl std::future::Future> + Send; } +/// A node number as CLAIMED by a raw, over-the-air `MeshPacket.from` field. +/// +/// Meshtastic carries no cryptographic sender binding at this layer in this +/// proto subset (no signature, no `relay_node`) — any node holding the +/// channel key can set `from` to any value, including another node's number. +/// This wrapper keeps that fact visible at the one place a raw wire header +/// turns into a [`NodeNum`] used to CREATE or UPDATE a node-DB entry, so the +/// conversion reads as a stated trust decision rather than a bare cast that +/// looks like an established fact (#246). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct ClaimedNodeNum(NodeNum); + +impl ClaimedNodeNum { + /// Wraps a raw wire `from` value, rejecting the two values that are + /// never a real originating node: `0` (unset) and the broadcast address + /// `0xFFFF_FFFF`. A packet claiming either is dropped before it reaches + /// the node DB rather than creating or updating an entry under it. + fn from_wire(raw: u32) -> Option { + let candidate = NodeNum(raw); + (raw != 0 && !candidate.is_broadcast()).then_some(Self(candidate)) + } + + /// Accepts the claim as a [`NodeNum`] for node-DB attribution. + /// + /// Named explicitly rather than via `From`/`Into` so every call site + /// states, in its own name, that it is accepting an UNAUTHENTICATED + /// identity claim, not a verified fact — akroasis has no channel key or + /// out-of-band anchor to check `from` against at this layer, so this is + /// the strongest attribution available, not proof. + const fn accept_unauthenticated(self) -> NodeNum { + self.0 + } +} + /// Meshtastic mesh networking collector. /// /// Manages connections to one or more Meshtastic radios, receives mesh packets, @@ -152,8 +186,23 @@ impl MeshCollector { } /// Handles a received mesh packet by updating the node database. + /// + /// `mesh_packet.from` is the sender this layer actually received the + /// packet attributed to — the strongest identity signal available here — + /// but it is an unauthenticated claim, not a verified fact (see + /// [`ClaimedNodeNum`]). A packet claiming the non-node sentinels (`0` or + /// broadcast) is dropped before it can create or update a node-DB entry + /// (#246). async fn handle_mesh_packet(&self, mesh_packet: &crate::proto::MeshPacket) { - let node_num = NodeNum(mesh_packet.from); + let Some(node_num) = + ClaimedNodeNum::from_wire(mesh_packet.from).map(ClaimedNodeNum::accept_unauthenticated) + else { + tracing::trace!( + from = mesh_packet.from, + "ignoring mesh packet with sentinel `from` (unset or broadcast)" + ); + return; + }; let snr = if mesh_packet.rx_snr == 0.0 { None } else { @@ -675,3 +724,7 @@ where #[cfg(test)] #[path = "collector_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "collector_tests_attribution.rs"] +mod tests_attribution; diff --git a/crates/kerykeion/src/collector_tests_attribution.rs b/crates/kerykeion/src/collector_tests_attribution.rs new file mode 100644 index 0000000..bf71466 --- /dev/null +++ b/crates/kerykeion/src/collector_tests_attribution.rs @@ -0,0 +1,124 @@ +//! Tests for [`super`]'s unauthenticated-attribution guard (#246); split out +//! from `collector_tests.rs` rather than added to it, which is already at the +//! RUST/file-too-long 800-line threshold. + +use super::*; +use crate::config::{ConnectionConfig, MeshConfig, StoreForwardConfig, TopologyConfig}; + +fn make_config(connections: Vec) -> MeshConfig { + MeshConfig { + connections, + store_forward: StoreForwardConfig::default(), + topology: TopologyConfig::default(), + ..MeshConfig::default() + } +} + +#[test] +fn claimed_node_num_rejects_zero_and_broadcast() { + assert!( + ClaimedNodeNum::from_wire(0).is_none(), + "from == 0 must be rejected" + ); + assert!( + ClaimedNodeNum::from_wire(0xFFFF_FFFF).is_none(), + "from == broadcast must be rejected" + ); +} + +#[test] +fn claimed_node_num_accepts_a_real_value() { + // WHY: the falsifiable half of the sentinel-rejection test above -- + // without this, a guard that rejected EVERY `from` value (not just the + // two sentinels) would also pass it. + assert_eq!( + ClaimedNodeNum::from_wire(0xDEAD_BEEF).map(ClaimedNodeNum::accept_unauthenticated), + Some(crate::types::NodeNum(0xDEAD_BEEF)) + ); +} + +#[tokio::test] +async fn process_packet_ignores_zero_from_sentinel() { + // WHY(#246): pre-fix, `mesh_packet.from == 0` created a node-DB entry + // keyed on a value that is never a real node -- ANY node on the mesh + // could spoof `from: 0` and still land in the DB. + let c = MeshCollector::new(make_config(vec![])); + let pkt = FromRadio { + id: 1, + payload_variant: Some(from_radio::PayloadVariant::Packet( + crate::proto::MeshPacket { + from: 0, + to: 0xFFFF_FFFF, + rx_snr: 5.0, + hop_start: 3, + hop_limit: 1, + ..Default::default() + }, + )), + }; + + c.process_packet(&pkt).await; + + let db = c.node_db().lock().await; + assert!( + db.get(crate::types::NodeNum(0)).is_none(), + "from == 0 must never create a node-DB entry" + ); + assert!(db.is_empty(), "from == 0 must not touch the node DB at all"); +} + +#[tokio::test] +async fn process_packet_ignores_broadcast_from_sentinel() { + // WHY(#246): pre-fix, a spoofed `from == 0xFFFF_FFFF` (broadcast) would + // insert/update a node-DB entry keyed on the broadcast address, feeding + // a phantom entry into topology/discovery. + let c = MeshCollector::new(make_config(vec![])); + let pkt = FromRadio { + id: 1, + payload_variant: Some(from_radio::PayloadVariant::Packet( + crate::proto::MeshPacket { + from: 0xFFFF_FFFF, + to: 0x1234, + rx_snr: 5.0, + hop_start: 3, + hop_limit: 1, + ..Default::default() + }, + )), + }; + + c.process_packet(&pkt).await; + + let db = c.node_db().lock().await; + assert!( + db.get(crate::types::NodeNum(0xFFFF_FFFF)).is_none(), + "from == broadcast must never create a node-DB entry" + ); + assert!(db.is_empty()); +} + +#[tokio::test] +async fn process_packet_still_admits_a_real_node_num() { + // WHY: the falsifiable half of the two sentinel-rejection tests above -- + // without this, a guard that rejected EVERY packet (not just the + // sentinels) would also pass them. + let c = MeshCollector::new(make_config(vec![])); + let pkt = FromRadio { + id: 1, + payload_variant: Some(from_radio::PayloadVariant::Packet( + crate::proto::MeshPacket { + from: 0xDEAD, + to: 0xFFFF_FFFF, + rx_snr: 5.0, + hop_start: 3, + hop_limit: 1, + ..Default::default() + }, + )), + }; + + c.process_packet(&pkt).await; + + let db = c.node_db().lock().await; + assert!(db.get(crate::types::NodeNum(0xDEAD)).is_some()); +} diff --git a/crates/kerykeion/src/router.rs b/crates/kerykeion/src/router.rs index 2199ed1..aa4bf6e 100644 --- a/crates/kerykeion/src/router.rs +++ b/crates/kerykeion/src/router.rs @@ -252,6 +252,15 @@ impl MeshRouter { /// each `packet_bytes` back into the original [`MeshPacket`] so the /// re-enqueued message carries its real payload and header fields /// (`from`, `hop_limit`, `hop_start`, ...) rather than a synthetic shell. + /// + /// WARNING: `dest` drives WHERE a store-and-forward queue's contents get + /// re-sent, so callers must reach this ONLY from an authenticated + /// reachability/topology event, never from a raw per-packet `from` + /// attribution (an unauthenticated claim per #246) — that coupling would + /// let a spoofed `from` redirect a queued message toward an + /// attacker-named destination. No call site in this crate currently + /// drives this from packet receipt (verified: only `MeshRouter`'s own + /// tests call it); keep it that way. pub fn node_came_online(&mut self, dest: NodeNum) { let stored = self.store_forward.drain_for(dest); for msg in stored { From b57559020345eff87d5b72c35d79efe30d41b39c Mon Sep 17 00:00:00 2001 From: forkwright Date: Sun, 16 Aug 2026 22:02:45 -0500 Subject: [PATCH 2/5] fix(kerykeion): attribute NeighborInfo links to the packet sender, not the payload claim handle_neighborinfo derived the reporting node's identity entirely from the protobuf body field ni.node_id, never consulting the packet's actual transmitting sender. Any mesh node could broadcast a NEIGHBORINFO_APP packet claiming an arbitrary node_id and have the resulting topology links, and the located signal, attributed to that victim rather than to itself. Pass the packet's sender into handle_neighborinfo and require ni.node_id == from; on mismatch the report is dropped rather than silently reattributed to the sender, since a mismatch does not distinguish a forged claim from a payload describing a genuinely different node. Updated the two doc comments that cited NEIGHBORINFO as the reason event subjects can differ from the packet sender -- that case is now enforced equal by construction. TRACEROUTE's intermediate hops are the still-live example and now stand alone. --- crates/kerykeion/src/processor.rs | 42 +++++++++++++---- crates/kerykeion/src/processor_tests.rs | 62 +++++++++++++++++++++---- crates/kerykeion/src/signals.rs | 6 ++- 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/crates/kerykeion/src/processor.rs b/crates/kerykeion/src/processor.rs index 80470a0..aaecb51 100644 --- a/crates/kerykeion/src/processor.rs +++ b/crates/kerykeion/src/processor.rs @@ -117,7 +117,7 @@ impl PacketProcessor { self.handle_telemetry(from, &decoded.payload, &mut events); } p if p == portnum::NEIGHBORINFO_APP => { - self.handle_neighborinfo(&decoded.payload, &mut events); + self.handle_neighborinfo(from, &decoded.payload, &mut events); } p if p == portnum::TRACEROUTE_APP => { self.handle_traceroute(from, packet, &decoded.payload, &mut events); @@ -138,9 +138,10 @@ impl PacketProcessor { } // WHY: an event names its own subject, which is not always the packet - // sender. NEIGHBORINFO reports carry a reporter id from the payload, so - // locating every signal at `from` puts a relayed report at the relay - // rather than at the node the report is about. + // sender. TRACEROUTE reports name every hop along the discovered + // route, so locating every signal at `from` would put each + // intermediate hop's link at the packet's own sender instead of the + // hop it actually describes. for event in &events { let position = event .subject() @@ -366,7 +367,20 @@ impl PacketProcessor { } } - fn handle_neighborinfo(&mut self, payload: &[u8], events: &mut Vec) { + /// Handles a `NEIGHBORINFO_APP` payload, attributing the reported links + /// to `from` (the packet's actual sender) rather than the payload's own + /// `node_id` claim. + /// + /// The payload's `node_id` is a SECOND, independently forgeable identity + /// assertion embedded inside the packet body — Meshtastic gives no + /// channel-level guarantee it agrees with `from`. Trusting it + /// unconditionally lets any sender attribute fabricated neighbor links + /// to whichever victim node number it names, on that victim's behalf + /// (#207). When the two disagree the report is dropped rather than + /// silently reattributed to `from`: a mismatch does not distinguish a + /// forged claim from a payload describing a genuinely different node, so + /// neither identity can be trusted for this report. + fn handle_neighborinfo(&mut self, from: NodeNum, payload: &[u8], events: &mut Vec) { let ni = match NeighborInfo::decode(payload) { Ok(n) => n, Err(e) => { @@ -375,15 +389,23 @@ impl PacketProcessor { } }; - let reporter = NodeNum(ni.node_id); - self.topology.add_node(reporter); + let claimed = NodeNum(ni.node_id); + if claimed != from { + tracing::warn!( + claimed = claimed.0, + sender = from.0, + "NEIGHBORINFO payload node_id disagrees with packet sender; dropping report" + ); + return; + } + + self.topology.add_node(from); for neighbor in &ni.neighbors { let neighbor_num = NodeNum(neighbor.node_id); - self.topology - .update_link(reporter, neighbor_num, neighbor.snr); + self.topology.update_link(from, neighbor_num, neighbor.snr); events.push(MeshEvent::TopologyChange { - from: reporter, + from, to: neighbor_num, snr: neighbor.snr, }); diff --git a/crates/kerykeion/src/processor_tests.rs b/crates/kerykeion/src/processor_tests.rs index 5cc0177..e641a88 100644 --- a/crates/kerykeion/src/processor_tests.rs +++ b/crates/kerykeion/src/processor_tests.rs @@ -656,19 +656,24 @@ fn node_at(num: u32, latitude: f64, longitude: f64) -> MeshNode { } #[tokio::test] -async fn neighborinfo_signal_is_located_at_the_reporter_not_the_packet_sender() { - // WHY: NEIGHBORINFO carries its reporter id in the payload, so a relayed - // report describes links the relay is not an endpoint of. Locating every - // signal at the packet sender puts those links at the relay's coordinates. +async fn neighborinfo_spoofed_node_id_is_dropped_not_attributed_to_the_victim() { + // WHY(#207): NEIGHBORINFO's `node_id` is a claim embedded in the payload + // ITSELF, independent of `packet.from`. Pre-fix, node 0x1111 could + // broadcast a NEIGHBORINFO packet claiming `node_id: 0x2222` and have the + // resulting link/signal attributed to 0x2222 -- a victim who never sent + // anything -- rather than to 0x1111, the node that actually transmitted + // it. The fix requires `node_id == packet.from`; on mismatch the report + // is dropped, producing no event and no topology mutation under either + // identity. let (tx, mut rx) = broadcast::channel(64); let mut node_db = NodeDb::new(); node_db.set_my_node(NodeNum(0xAAAA)); - node_db.insert(node_at(0x1111, 10.0, 10.0)); // the relay that transmitted - node_db.insert(node_at(0x2222, 50.0, 60.0)); // the node the report is about + node_db.insert(node_at(0x1111, 10.0, 10.0)); // the actual sender + node_db.insert(node_at(0x2222, 50.0, 60.0)); // the claimed/victim node_id let mut proc = PacketProcessor::new(node_db, MeshTopology::new(), tx); let ni = NeighborInfo { - node_id: 0x2222, + node_id: 0x2222, // spoofed: does not match the packet's `from` last_sent_by_id: 0, node_broadcast_interval_secs: 0, neighbors: vec![Neighbor { @@ -681,7 +686,48 @@ async fn neighborinfo_signal_is_located_at_the_reporter_not_the_packet_sender() let packet = make_mesh_packet(0x1111, portnum::NEIGHBORINFO_APP, payload); let events = proc.process_mesh_packet(&packet); + + assert!( + events.is_empty(), + "a spoofed node_id must yield no events, got {events:?}" + ); + assert!( + !proc.topology().contains_node(NodeNum(0x2222)), + "the victim node must gain no topology entry from a report it never sent" + ); + assert!( + rx.try_recv().is_err(), + "no signal should be broadcast for a dropped report" + ); +} + +#[tokio::test] +async fn neighborinfo_agreeing_node_id_is_located_at_the_reporter() { + // WHY: the falsifiable half of the spoof-rejection test above -- without + // this, a handler that dropped EVERY NEIGHBORINFO report (not just + // mismatched ones) would also pass it. + let (tx, mut rx) = broadcast::channel(64); + let mut node_db = NodeDb::new(); + node_db.set_my_node(NodeNum(0xAAAA)); + node_db.insert(node_at(0x2222, 50.0, 60.0)); // sender == claimed node_id + let mut proc = PacketProcessor::new(node_db, MeshTopology::new(), tx); + + let ni = NeighborInfo { + node_id: 0x2222, // agrees with packet.from below + last_sent_by_id: 0, + node_broadcast_interval_secs: 0, + neighbors: vec![Neighbor { + node_id: 0x3333, + snr: 4.0, + }], + }; + let mut payload = Vec::new(); + ni.encode(&mut payload).unwrap(); + + let packet = make_mesh_packet(0x2222, portnum::NEIGHBORINFO_APP, payload); + let events = proc.process_mesh_packet(&packet); assert_eq!(events.len(), 1, "one neighbor should yield one event"); + assert!(proc.topology().contains_node(NodeNum(0x2222))); let signal = rx.recv().await.unwrap(); #[expect(clippy::expect_used, reason = "test-only")] @@ -691,7 +737,7 @@ async fn neighborinfo_signal_is_located_at_the_reporter_not_the_packet_sender() assert!( (coords.latitude - 50.0).abs() < f64::EPSILON && (coords.longitude - 60.0).abs() < f64::EPSILON, - "signal should be located at reporter 0x2222 (50, 60), got ({}, {})", + "signal should be located at 0x2222 (50, 60), got ({}, {})", coords.latitude, coords.longitude ); diff --git a/crates/kerykeion/src/signals.rs b/crates/kerykeion/src/signals.rs index 5e03e25..7e4ff63 100644 --- a/crates/kerykeion/src/signals.rs +++ b/crates/kerykeion/src/signals.rs @@ -89,8 +89,10 @@ impl MeshEvent { /// The node this event is about, when it names exactly one. /// /// This is the node whose position locates the emitted signal. It is not - /// the packet sender: `NEIGHBORINFO` carries a reporter id in its payload, - /// so a relayed report describes links the relay is not an endpoint of. + /// always the packet's transmitting sender: a `TRACEROUTE` report names + /// every hop along the discovered route, most of which are not the + /// sender. (`NEIGHBORINFO`'s reporter id is required to match the + /// sender to be admitted at all — see `PacketProcessor::handle_neighborinfo`.) /// /// Returns `None` for the partition events, which describe a set of nodes /// rather than one; their conversions carry no location either. From 6335e5776f6e5866001daef5199ce7d84b2685cc Mon Sep 17 00:00:00 2001 From: forkwright Date: Sun, 16 Aug 2026 22:03:05 -0500 Subject: [PATCH 3/5] fix(kerykeion): validate lat/lon at the MessageBuilder position boundary position_with_config converted caller-supplied f64 lat/lon straight to i32 via an as cast with no finiteness or range check. Rust saturates an out-of-range as-cast rather than panicking, so a NaN latitude (e.g. from a failed GPS read) silently became 0 and an infinite value silently became an i32 extreme, producing a plausible-looking wire coordinate instead of an error -- exactly the silent corruption the removed SAFETY comment claimed could not happen. Validate finiteness and Meshtastic's geographic range before the cast and return Error::InvalidPosition on failure. position and position_with_config now return Result; dropped their #[must_use] since Result already carries it at the type level (clippy::double_must_use under -D warnings, kanon#3473). --- crates/kerykeion/src/error.rs | 25 ++++++++++ crates/kerykeion/src/message.rs | 88 +++++++++++++++++++++++++++++---- 2 files changed, 104 insertions(+), 9 deletions(-) diff --git a/crates/kerykeion/src/error.rs b/crates/kerykeion/src/error.rs index 5929121..116e59d 100644 --- a/crates/kerykeion/src/error.rs +++ b/crates/kerykeion/src/error.rs @@ -233,6 +233,21 @@ pub enum Error { location: snafu::Location, }, + /// Position lat/lon is non-finite or outside Meshtastic's valid + /// geographic range, which the fixed-point wire conversion assumes holds. + #[snafu(display( + "invalid position: lat={lat}, lon={lon} (must be finite, lat ∈ [-90, 90], lon ∈ [-180, 180])" + ))] + InvalidPosition { + /// The rejected latitude in decimal degrees. + lat: f64, + /// The rejected longitude in decimal degrees. + lon: f64, + /// Source location for diagnostics. + #[snafu(implicit)] + location: snafu::Location, + }, + /// A store-forward snapshot has more distinct destinations than the /// running cap allows; loading it verbatim would silently disable the /// per-destination resource-exhaustion protection for every over-cap @@ -301,4 +316,14 @@ mod tests { }; assert!(err.to_string().contains("0xdeadbeef")); } + + #[test] + fn invalid_position_message() { + let err = Error::InvalidPosition { + lat: f64::NAN, + lon: 200.0, + location: snafu::location!(), + }; + assert!(err.to_string().contains("invalid position")); + } } diff --git a/crates/kerykeion/src/message.rs b/crates/kerykeion/src/message.rs index 410c50d..1259d8a 100644 --- a/crates/kerykeion/src/message.rs +++ b/crates/kerykeion/src/message.rs @@ -5,7 +5,7 @@ use rand_core::{OsRng, RngCore as _}; use crate::config::MessageConfig; use crate::crypto; -use crate::error::Error; +use crate::error::{Error, InvalidPositionSnafu}; use crate::proto::mesh_packet::Priority; use crate::proto::{AdminMessage, Data, MeshPacket, PortNum, Position, mesh_packet}; use crate::types::{ChannelIndex, MAX_HOP_LIMIT, NodeNum}; @@ -56,25 +56,53 @@ impl MessageBuilder { /// /// Latitude and longitude are in decimal degrees; they are converted to /// Meshtastic's `i32` representation (`value * 1e7`). - #[must_use] - pub fn position(dest: NodeNum, lat: f64, lon: f64) -> Self { + /// + /// # Errors + /// + /// Returns [`Error::InvalidPosition`] if `lat` or `lon` is not finite, or + /// outside `[-90, 90]` / `[-180, 180]` respectively (#247). + pub fn position(dest: NodeNum, lat: f64, lon: f64) -> Result { Self::position_with_config(dest, lat, lon, &MessageConfig::default()) } /// Build a position message with a caller-supplied [`MessageConfig`]. - #[must_use] - pub fn position_with_config(dest: NodeNum, lat: f64, lon: f64, config: &MessageConfig) -> Self { + /// + /// # Errors + /// + /// Returns [`Error::InvalidPosition`] if `lat` or `lon` is not finite, or + /// outside `[-90, 90]` / `[-180, 180]` respectively (#247). + pub fn position_with_config( + dest: NodeNum, + lat: f64, + lon: f64, + config: &MessageConfig, + ) -> Result { + // WHY: the `as i32` cast below never panics — Rust saturates NaN to 0 + // and ±Inf to the i32 extremes — so an unchecked cast turns a failed + // GPS read (NaN) or a caller bug (Inf, out-of-range degrees) into a + // plausible-looking wire coordinate instead of a build-time error. + // Validate BEFORE the cast so the invariant the cast relies on + // (finite, in-range degrees) is actually established, not merely + // asserted (#247). + if !lat.is_finite() + || !(-90.0..=90.0).contains(&lat) + || !lon.is_finite() + || !(-180.0..=180.0).contains(&lon) + { + return InvalidPositionSnafu { lat, lon }.fail(); + } + // WHY: Meshtastic firmware stores lat/lon as fixed-point i32 = degrees * 1e7. #[expect( clippy::as_conversions, reason = "f64→i32 via multiplication is the Meshtastic wire format convention" )] let pos = Position { - latitude_i: (lat * 1e7) as i32, // SAFETY: lat ∈ [-90, 90] so lat*1e7 ∈ [-9e8, 9e8] which fits i32 (±2.1e9) - longitude_i: (lon * 1e7) as i32, // SAFETY: lon ∈ [-180, 180] so lon*1e7 ∈ [-1.8e9, 1.8e9] which fits i32 + latitude_i: (lat * 1e7) as i32, // SAFETY: lat validated finite + ∈ [-90, 90] above, so lat*1e7 ∈ [-9e8, 9e8] which fits i32 (±2.1e9) + longitude_i: (lon * 1e7) as i32, // SAFETY: lon validated finite + ∈ [-180, 180] above, so lon*1e7 ∈ [-1.8e9, 1.8e9] which fits i32 ..Default::default() }; - Self { + Ok(Self { dest, portnum: PortNum::PositionApp, payload: pos.encode_to_vec(), @@ -82,7 +110,7 @@ impl MessageBuilder { want_ack: false, hop_limit: config.default_hop_limit.min(MAX_HOP_LIMIT), priority: Priority::Default, - } + }) } /// Build an admin message (`ADMIN_APP`) with the default hop limit. @@ -297,6 +325,7 @@ mod tests { fn position_message_encodes_lat_lon() { #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::position(DEST, 37.7749, -122.4194) + .unwrap() .build(FROM_NODE, &[]) .unwrap(); @@ -314,6 +343,47 @@ mod tests { ); } + #[test] + fn position_rejects_nan_latitude() { + // WHY(#247): pre-fix, `NaN as i32` saturates to 0 rather than + // panicking, so a failed GPS read silently produced a packet at + // (0, lon) instead of failing the build. + let result = MessageBuilder::position(DEST, f64::NAN, -122.4194); + assert!( + matches!(result, Err(Error::InvalidPosition { .. })), + "NaN latitude must be rejected, got {result:?}" + ); + } + + #[test] + fn position_rejects_infinite_longitude() { + let result = MessageBuilder::position(DEST, 37.7749, f64::INFINITY); + assert!( + matches!(result, Err(Error::InvalidPosition { .. })), + "infinite longitude must be rejected, got {result:?}" + ); + } + + #[test] + fn position_rejects_out_of_range_latitude() { + // WHY: distinguishes the range check from the finiteness check -- + // 91.0 is finite but not a valid latitude. + let result = MessageBuilder::position(DEST, 91.0, 0.0); + assert!( + matches!(result, Err(Error::InvalidPosition { .. })), + "out-of-range latitude must be rejected, got {result:?}" + ); + } + + #[test] + fn position_accepts_boundary_coordinates() { + // WHY: the falsifiable half of the range checks above -- without + // this, an inverted comparison (e.g. `>` instead of `>=`) that + // rejects everything would also pass the rejection tests. + assert!(MessageBuilder::position(DEST, 90.0, 180.0).is_ok()); + assert!(MessageBuilder::position(DEST, -90.0, -180.0).is_ok()); + } + #[test] fn admin_message_sets_reliable_priority() { let admin = AdminMessage { From a88d383fa159bcc88b45cb903314e29ed4c037eb Mon Sep 17 00:00:00 2001 From: forkwright Date: Sun, 16 Aug 2026 22:08:44 -0500 Subject: [PATCH 4/5] fix(kerykeion): derive Debug on MessageBuilder for the position() test assertions --- crates/kerykeion/src/message.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/kerykeion/src/message.rs b/crates/kerykeion/src/message.rs index 1259d8a..8d68517 100644 --- a/crates/kerykeion/src/message.rs +++ b/crates/kerykeion/src/message.rs @@ -21,6 +21,7 @@ use crate::types::{ChannelIndex, MAX_HOP_LIMIT, NodeNum}; /// .with_ack() /// .build(NodeNum(0xABCD), &[0x01])?; /// ``` +#[derive(Debug)] pub struct MessageBuilder { dest: NodeNum, portnum: PortNum, From c5fd7b7ec27236ceb2802613e924594d15a17d52 Mon Sep 17 00:00:00 2001 From: forkwright Date: Sun, 16 Aug 2026 22:13:23 -0500 Subject: [PATCH 5/5] fix(kerykeion): drop the node_db lock before trailing asserts in attribution tests --- .../src/collector_tests_attribution.rs | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/crates/kerykeion/src/collector_tests_attribution.rs b/crates/kerykeion/src/collector_tests_attribution.rs index bf71466..e30930b 100644 --- a/crates/kerykeion/src/collector_tests_attribution.rs +++ b/crates/kerykeion/src/collector_tests_attribution.rs @@ -60,11 +60,11 @@ async fn process_packet_ignores_zero_from_sentinel() { c.process_packet(&pkt).await; let db = c.node_db().lock().await; - assert!( - db.get(crate::types::NodeNum(0)).is_none(), - "from == 0 must never create a node-DB entry" - ); - assert!(db.is_empty(), "from == 0 must not touch the node DB at all"); + let has_entry = db.get(crate::types::NodeNum(0)).is_some(); + let db_is_empty = db.is_empty(); + drop(db); + assert!(!has_entry, "from == 0 must never create a node-DB entry"); + assert!(db_is_empty, "from == 0 must not touch the node DB at all"); } #[tokio::test] @@ -90,11 +90,14 @@ async fn process_packet_ignores_broadcast_from_sentinel() { c.process_packet(&pkt).await; let db = c.node_db().lock().await; + let has_entry = db.get(crate::types::NodeNum(0xFFFF_FFFF)).is_some(); + let db_is_empty = db.is_empty(); + drop(db); assert!( - db.get(crate::types::NodeNum(0xFFFF_FFFF)).is_none(), + !has_entry, "from == broadcast must never create a node-DB entry" ); - assert!(db.is_empty()); + assert!(db_is_empty); } #[tokio::test] @@ -120,5 +123,7 @@ async fn process_packet_still_admits_a_real_node_num() { c.process_packet(&pkt).await; let db = c.node_db().lock().await; - assert!(db.get(crate::types::NodeNum(0xDEAD)).is_some()); + let has_entry = db.get(crate::types::NodeNum(0xDEAD)).is_some(); + drop(db); + assert!(has_entry); }