deploy follow fixes

BitCraftServer/packages/game/src/game/handlers/player/deployable_follow.rs

@@ -7,6 +7,11 @@ use crate::{
 };
 use spacetimedb::ReducerContext;
 
+// All coordinates (player position, deployable position, source coordinates, target coordinates) should be within this distance of each other for a follow to occur
+// This is a safety measure to prevent both client bugs and client cheating
+// Increase if too strict and preventing legitimate follows
+const MAX_PROXIMITY_DISTANCE: f32 = 100.0;
+
 #[spacetimedb::reducer]
 pub fn deployable_follow(ctx: &ReducerContext, request: PlayerDeployableMoveRequest) -> Result<(), String> {
     let actor_id = game_state::actor_id(&ctx, true)?;
@@ -19,21 +24,28 @@ pub fn deployable_follow(ctx: &ReducerContext, request: PlayerDeployableMoveRequ
         }
     }
 
-    let _ = unwrap_or_err!(
-        ctx.db.mobile_entity_state().entity_id().find(deployable_entity_id),
-        "~Deployable is no longer available"
-    );
-
     let origin = unwrap_or_err!(request.origin, "~Expected origin in move request");
     let dest = unwrap_or_err!(request.destination, "~Expected destination in move request");
     let source_coordinates: FloatHexTile = origin.into();
     let target_coordinates: FloatHexTile = dest.into();
 
+    // Both these functions panic if the entity is not found in mobile entity
+    let deployable_location = game_state_filters::coordinates_float(ctx, deployable_entity_id);
     let player_location = game_state_filters::coordinates_float(ctx, actor_id);
-    if target_coordinates.dimension != player_location.dimension || target_coordinates.distance_to(player_location) > 100.0 {
-        return Err("~Deployable destination is too far from its owner position".into());
-    }
 
+    // We now have 4 points to check:
+    let points_to_check = [
+        deployable_location, // Deployable location (from server)
+        player_location,     // Player location (from server)
+        source_coordinates,  // Source coordinates (from client)
+        target_coordinates,  // Target coordinates (from client)
+    ];
+
+    //ALL of these points should be in the same dimension and within MAX_PROXIMITY_DISTANCE of each other
+    //Otherwise, client is bugged or cheating
+    all_close_or_err(&points_to_check, MAX_PROXIMITY_DISTANCE, "~Invalid coordinates")?;
+
+    // Further validation checks
     let mut terrain_cache = TerrainChunkCache::empty();
     let terrain_start = unwrap_or_err!(
         terrain_cache.get_terrain_cell(ctx, &source_coordinates.parent_large_tile()),
@@ -43,9 +55,11 @@ pub fn deployable_follow(ctx: &ReducerContext, request: PlayerDeployableMoveRequ
         terrain_cache.get_terrain_cell(ctx, &target_coordinates.parent_large_tile()),
         "~You can't go here!",
     );
+    let deployable_location_terrain = unwrap_or_err!(
+        terrain_cache.get_terrain_cell(ctx, &deployable_location.parent_large_tile()),
+        "~You can't go here!",
+    );
 
-    // Validate Boat Move. Note that this for the short term, ideally at some point
-    // we will re-enable the above _validate_move that has more comprehensive checks
     let deployable = unwrap_or_err!(
         ctx.db.deployable_state().entity_id().find(&deployable_entity_id),
         "~Deployable not found!"
@@ -65,12 +79,20 @@ pub fn deployable_follow(ctx: &ReducerContext, request: PlayerDeployableMoveRequ
         "~Invalid deployable pathfinding info"
     );
 
+    // Only checking water depth at the client-provided coordinates is unsafe, we cannot trust the client
+    // At the very least, we should also check the water depth at the deployable's ACTUAL location
     let start_water_depth = terrain_start.water_depth() as i32;
     let target_water_depth = terrain_target.water_depth() as i32;
+    let deployable_location_water_depth = deployable_location_terrain.water_depth() as i32;
+
+    let start_ok = start_water_depth >= pathfinding.min_water_depth && start_water_depth <= pathfinding.max_water_depth;
+    let target_ok = target_water_depth >= pathfinding.min_water_depth && target_water_depth <= pathfinding.max_water_depth;
+    let deployable_location_ok = deployable_location_water_depth >= pathfinding.min_water_depth && deployable_location_water_depth <= pathfinding.max_water_depth;
 
-    if (start_water_depth < pathfinding.min_water_depth || start_water_depth > pathfinding.max_water_depth)
-        && (target_water_depth < pathfinding.min_water_depth || target_water_depth > pathfinding.max_water_depth)
-    {
+    // Previously, this checked if start AND target were invalid
+    // That would allow a deployable to move from land->water or water->land (in both cases, one endpoint is valid)
+    // We should instead ensure start, target, and deployable location are all valid
+    if !start_ok || !target_ok || !deployable_location_ok {
         return Err("~You can't go here!".into());
     }
 
@@ -96,3 +118,25 @@ pub fn deployable_follow(ctx: &ReducerContext, request: PlayerDeployableMoveRequ
     // update deployable location
     reducer_helpers::deployable_helpers::move_deployable(ctx, deployable_entity_id, origin, dest, request.timestamp, request.duration)
 }
+
+pub fn all_close_or_err(coords: &[FloatHexTile], max_dist: f32, msg: &str) -> Result<(), String> {
+    if coords.len() <= 1 {
+        return Ok(());
+    }
+
+    // Since currently distance_to() does not check dimensions, we need to check them manually
+    let dim = coords[0].dimension;
+    if coords.iter().any(|c| c.dimension != dim) {
+        return Err(msg.into());
+    }
+
+    for i in 0..coords.len() {
+        for j in (i + 1)..coords.len() {
+            if coords[i].distance_to(coords[j]) > max_dist {
+                return Err(msg.into());
+            }
+        }
+    }
+
+    Ok(())
+}