fix(kerykeion): bound the wire fields that were trusted to bound themselves

crates/kerykeion/src/collector.rs

@@ -118,16 +118,11 @@ impl MeshCollector {
     }
 
     /// Computes hop count FROM packet hop fields.
-    #[expect(
-        clippy::cast_possible_truncation,
-        reason = "hop VALUES are bounded by MAX_HOP_LIMIT (7) in Meshtastic firmware"
-    )]
-    const fn compute_hop_count(hop_start: u32, hop_limit: u32) -> Option<u8> {
-        if hop_start > 0 && hop_limit <= hop_start {
-            Some((hop_start - hop_limit) as u8) // SAFETY: hop_start >= hop_limit is checked by caller; difference fits u8
-        } else {
-            None
-        }
+    ///
+    /// Delegates to [`crate::types::hop_count_from_wire`] so the bound lives in
+    /// one place; see there for why the fields cannot be trusted to hold it.
+    fn compute_hop_count(hop_start: u32, hop_limit: u32) -> Option<u8> {
+        crate::types::hop_count_from_wire(hop_start, hop_limit)
     }
 
     /// Processes a single `FromRadio` message, updating the node database.

crates/kerykeion/src/processor.rs

@@ -190,15 +190,7 @@ impl PacketProcessor {
             Some(packet.rx_snr)
         };
 
-        let hop_count = if packet.hop_start > 0 {
-            #[expect(
-                clippy::cast_possible_truncation,
-                reason = "hop VALUES are bounded by MAX_HOP_LIMIT (7)"
-            )]
-            Some((packet.hop_start.saturating_sub(packet.hop_limit)) as u8) // SAFETY: saturating_sub result is bounded by hop_start (u8 domain)
-        } else {
-            None
-        };
+        let hop_count = crate::types::hop_count_from_wire(packet.hop_start, packet.hop_limit);
 
         // WHY: UPDATE or CREATE the node record with latest packet metadata.
         let mut node = self.node_db.get(from).cloned().unwrap_or(MeshNode {
@@ -302,6 +294,21 @@ impl PacketProcessor {
         // WHY: Meshtastic encodes lat/lon as integer degrees × 1e7.
         let lat = f64::from(pos_proto.latitude_i) * 1e-7;
         let lon = f64::from(pos_proto.longitude_i) * 1e-7;
+
+        // WHY(#229): `latitude_i` is an i32 straight off the radio, so its full
+        // range scales to ±214.7 degrees — a neighbour that is hostile or simply
+        // wrong can place a node off the planet, and the value was persisted
+        // unchecked. `Coordinates::new` already owns this rule for the whole
+        // fleet, so it is reused rather than restated; it also rejects NaN,
+        // which a bare range comparison would silently admit.
+        if let Err(error) = koinon::Coordinates::new(lat, lon, None) {
+            tracing::warn!(
+                from = from.0,
+                %error,
+                "discarding POSITION_APP payload with out-of-range coordinates"
+            );
+            return;
+        }
         let alt = if pos_proto.altitude != 0 {
             Some(pos_proto.altitude)
         } else {

crates/kerykeion/src/processor_tests.rs

@@ -91,6 +91,71 @@ fn process_position_updates_node_and_emits_event() {
     );
 }
 
+/// WHY(#229): `latitude_i` is an i32 straight off the radio, so its extremes
+/// scale to ±214.7 degrees. A neighbour that is hostile or simply wrong could
+/// place a node off the planet and the value was stored unchecked, from where it
+/// reaches the topology and signal paths.
+#[test]
+fn a_position_outside_the_planet_is_discarded() {
+    for (label, latitude_i, longitude_i) in [
+        ("latitude past the pole", 900_000_001, 0),
+        ("latitude at the i32 extreme", i32::MAX, 0),
+        ("longitude past the meridian", 0, 1_800_000_001),
+        ("longitude at the i32 minimum", 0, i32::MIN),
+    ] {
+        let mut proc = make_processor();
+        let pos = crate::proto::Position {
+            latitude_i,
+            longitude_i,
+            altitude: 0,
+            time: 1_700_000_000,
+        };
+        let mut payload = Vec::new();
+        pos.encode(&mut payload).unwrap();
+
+        let events =
+            proc.process_mesh_packet(&make_mesh_packet(0xBEEF, portnum::POSITION_APP, payload));
+
+        assert!(
+            !events
+                .iter()
+                .any(|e| matches!(e, MeshEvent::PositionUpdate { .. })),
+            "{label}: an out-of-range position must not emit an update"
+        );
+        assert!(
+            proc.node_db()
+                .get(NodeNum(0xBEEF))
+                .is_none_or(|node| node.position.is_none()),
+            "{label}: an out-of-range position must not be stored"
+        );
+    }
+}
+
+/// Anti-vacuity for the case above, distinct from the existing happy path: a
+/// position exactly at the limits is valid and must survive.
+#[test]
+fn a_position_at_the_coordinate_limits_is_kept() {
+    let mut proc = make_processor();
+    let pos = crate::proto::Position {
+        latitude_i: 900_000_000,     // +90.0
+        longitude_i: -1_800_000_000, // -180.0
+        altitude: 0,
+        time: 1_700_000_000,
+    };
+    let mut payload = Vec::new();
+    pos.encode(&mut payload).unwrap();
+
+    let events =
+        proc.process_mesh_packet(&make_mesh_packet(0xBEEF, portnum::POSITION_APP, payload));
+
+    assert!(
+        events
+            .iter()
+            .any(|e| matches!(e, MeshEvent::PositionUpdate { .. })),
+        "the limits themselves are valid coordinates"
+    );
+}
+
 #[test]
 fn process_telemetry_updates_metrics() {
     let mut proc = make_processor();

crates/kerykeion/src/types.rs

@@ -185,6 +185,28 @@ pub const MAX_CHANNELS: u8 = 8;
 /// Maximum hop limit for a mesh packet.
 pub const MAX_HOP_LIMIT: u8 = 7;
 
+/// Hop count derived from a packet's wire hop fields, or `None` when they do
+/// not describe a hop count this protocol can produce.
+///
+/// WHY(#229) this is bounded rather than cast: both fields are `u32` read
+/// straight off the radio. The call sites justified an unchecked `as u8` on the
+/// grounds that Meshtastic firmware bounds them by [`MAX_HOP_LIMIT`] — true of
+/// firmware, and not true of a neighbour that is hostile or merely wrong. A
+/// `hop_start` of 1000 truncated to 232, so the claimed bound produced a value
+/// outside it.
+///
+/// `hop_limit` above `hop_start` is rejected rather than saturated: the
+/// difference is meaningless, and reporting zero hops for it invents a
+/// measurement the packet did not carry.
+pub(crate) fn hop_count_from_wire(hop_start: u32, hop_limit: u32) -> Option<u8> {
+    if hop_start == 0 || hop_limit > hop_start {
+        return None;
+    }
+    u8::try_from(hop_start - hop_limit)
+        .ok()
+        .filter(|&hops| hops <= MAX_HOP_LIMIT)
+}
+
 /// Maximum protobuf payload size enforced by Meshtastic firmware.
 pub const MAX_PACKET_SIZE: usize = 512;
 
@@ -257,6 +279,45 @@ mod tests {
         assert_eq!(MAX_HOP_LIMIT, 7);
     }
 
+    /// Anti-vacuity: ordinary packets must still yield a hop count, or the
+    /// rejection cases below would pass against a function returning None for
+    /// everything.
+    #[test]
+    fn ordinary_hop_fields_produce_a_count() {
+        assert_eq!(hop_count_from_wire(7, 7), Some(0));
+        assert_eq!(hop_count_from_wire(7, 4), Some(3));
+        assert_eq!(hop_count_from_wire(3, 0), Some(3));
+        assert_eq!(
+            hop_count_from_wire(u32::from(MAX_HOP_LIMIT), 0),
+            Some(MAX_HOP_LIMIT)
+        );
+    }
+
+    /// WHY(#229): the call sites cast these straight to u8 on the grounds that
+    /// firmware bounds them. A hostile neighbour is not running that firmware,
+    /// and 1000 truncates to 232 — a value outside the very bound the cast
+    /// claimed to rely on.
+    #[test]
+    fn wire_hop_fields_beyond_the_protocol_bound_are_rejected() {
+        assert_eq!(hop_count_from_wire(1000, 0), None, "would truncate to 232");
+        assert_eq!(hop_count_from_wire(u32::MAX, 0), None);
+        assert_eq!(hop_count_from_wire(256, 0), None, "would truncate to 0");
+        assert_eq!(
+            hop_count_from_wire(u32::from(MAX_HOP_LIMIT) + 1, 0),
+            None,
+            "one hop past the protocol maximum is still past it"
+        );
+    }
+
+    /// A `hop_limit` above `hop_start` describes no journey. Saturating it to zero
+    /// would report a measurement the packet never carried.
+    #[test]
+    fn an_inverted_hop_pair_is_rejected_rather_than_saturated() {
+        assert_eq!(hop_count_from_wire(3, 9), None);
+        assert_eq!(hop_count_from_wire(0, 0), None);
+        assert_eq!(hop_count_from_wire(0, 5), None);
+    }
+
     // ── Property tests ──────────────────────────────────────────────────────
 
     proptest::proptest! {