fix(kerykeion): attribute mesh source to the verified sender, not the packet's claim

crates/kerykeion/src/collector.rs

@@ -62,6 +62,40 @@ pub trait Collector: Send + Sync {
     ) -> impl std::future::Future<Output = Result<(), Error>> + 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<Self> {
+        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;

crates/kerykeion/src/collector_tests_attribution.rs

@@ -0,0 +1,129 @@
+//! 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<ConnectionConfig>) -> 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;
+    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]
+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;
+    let has_entry = db.get(crate::types::NodeNum(0xFFFF_FFFF)).is_some();
+    let db_is_empty = db.is_empty();
+    drop(db);
+    assert!(
+        !has_entry,
+        "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;
+    let has_entry = db.get(crate::types::NodeNum(0xDEAD)).is_some();
+    drop(db);
+    assert!(has_entry);
+}

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"));
+    }
 }

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};
@@ -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,
@@ -56,33 +57,61 @@ 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, Error> {
         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<Self, Error> {
+        // 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(),
             channel: ChannelIndex(0),
             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 +326,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 +344,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 {