Issue #2097: reject fractional command indices

crates/engine/src/cmd/develop.rs

@@ -1103,11 +1103,8 @@ pub fn specs() -> Vec<CommandSpec> {
             |s, p| {
                 let c = "history.restore";
                 let id = active(s, c)?;
-                let i = f64_req(p, "index", c)?;
-                if !(i.is_finite() && i >= 0.0) {
-                    return Err(bad(c, "no such step"));
-                }
-                go_to_history_step(s, c, id, i as usize)
+                let i = index_req(p, "index", c)?;
+                go_to_history_step(s, c, id, i)
             }
         ),
         cmd!("history.previous", "Previous History Step", [], None, "{} — steps the History cursor back one step", has_history_before, |s, _| {

crates/engine/src/cmd/mod.rs

@@ -198,12 +198,18 @@ pub(crate) fn bad(cmd: &str, msg: impl Into<String>) -> EngineError {
 pub(crate) fn f64_req(p: &Value, key: &str, cmd: &str) -> Result<f64> {
     p.get(key).and_then(Value::as_f64).ok_or_else(|| bad(cmd, format!("missing number `{key}`")))
 }
-/// A non-negative integer parameter: `index: -1` must be an error, not element 0 (`-1.0 as usize`
-/// saturates to 0), just as `history.restore` already refuses a negative step index.
+/// A non-negative whole number that fits `usize`. Keep integer JSON exact; accept integral
+/// floating-point values for compatibility, without truncating or saturating their conversion.
 pub(crate) fn index_req(p: &Value, key: &str, cmd: &str) -> Result<usize> {
+    let invalid = || bad(cmd, format!("`{key}` must be a non-negative integer"));
+    if let Some(i) = p.get(key).and_then(Value::as_u64) {
+        return usize::try_from(i).map_err(|_| invalid());
+    }
     let i = f64_req(p, key, cmd)?;
-    if !(i.is_finite() && i >= 0.0) {
-        return Err(bad(cmd, format!("`{key}` must be a non-negative integer")));
+    // The exclusive bound is exactly representable, unlike usize::MAX on 64-bit targets.
+    let upper_bound = 2.0_f64.powi(usize::BITS as i32);
+    if !(i.is_finite() && i >= 0.0 && i.fract() == 0.0 && i < upper_bound) {
+        return Err(invalid());
     }
     Ok(i as usize)
 }
@@ -223,3 +229,29 @@ pub(crate) fn point(p: &Value, key: &str) -> Option<lightcraft_geom::Point> {
     let a = p.get(key)?.as_array()?;
     Some(lightcraft_geom::Point::new(a.first()?.as_f64()?, a.get(1)?.as_f64()?))
 }
+
+#[cfg(test)]
+mod tests {
+    use serde_json::json;
+
+    use super::index_req;
+
+    #[test]
+    fn command_indices_preserve_integer_precision_and_check_usize_bounds() {
+        for i in [0, 1, usize::MAX] {
+            assert_eq!(index_req(&json!({"index": i}), "index", "test").unwrap(), i);
+        }
+        #[cfg(target_pointer_width = "64")]
+        {
+            let i = 9_007_199_254_740_993usize;
+            assert_eq!(index_req(&json!({"index": i}), "index", "test").unwrap(), i, "integer JSON must not round through f64");
+        }
+        let upper_bound = 2.0_f64.powi(usize::BITS as i32);
+        assert!(index_req(&json!({"index": upper_bound}), "index", "test").is_err(), "2^usize::BITS must not saturate");
+        #[cfg(target_pointer_width = "32")]
+        {
+            assert!(index_req(&json!({"index": u64::from(u32::MAX) + 1}), "index", "test").is_err());
+            assert_eq!(index_req(&json!({"index": f64::from(u32::MAX)}), "index", "test").unwrap(), usize::MAX);
+        }
+    }
+}

crates/engine/src/tests_history_cursor.rs

@@ -115,3 +115,95 @@ fn negative_indices_are_refused() {
     assert!(s.execute("version.rename", &json!({"index": -1, "name": "B"})).is_err());
     assert_eq!(s.catalog.photo(id).unwrap().versions[0].name, "A", "version 0 was not renamed");
 }
+
+#[test]
+fn fractional_version_delete_preserves_the_version() {
+    let mut s = Session::with_demo();
+    s.execute("version.create", &json!({"name": "A"})).unwrap();
+    let before = s.catalog.to_snapshot();
+    let result = s.execute("version.delete", &json!({"index": 0.9}));
+    assert!(result.is_err(), "index 0.9 must be refused; got {result:?}; catalog unchanged: {}", s.catalog.to_snapshot() == before);
+    assert_eq!(s.catalog.to_snapshot(), before, "the version must remain unchanged");
+}
+
+/// Invalid command indices must leave objects, settings, History and undo unchanged.
+#[test]
+fn invalid_command_indices_leave_the_catalog_and_history_unchanged() {
+    let (mut s, _) = three_edits();
+    for name in ["A", "B"] {
+        s.execute("version.create", &json!({"name": name})).unwrap();
+    }
+    s.execute("pointColor.pick", &json!({"x": 0.5, "y": 0.08})).unwrap();
+    for x in [0.3, 0.6] {
+        s.execute("redeye.add", &json!({"center": [x, 0.4], "rx": 0.03, "ry": 0.02, "pet": true})).unwrap();
+    }
+    let before = s.catalog.to_snapshot();
+    let history = s.execute("history.list", &json!({})).unwrap();
+    let undo_len = s.undo.len();
+    // The integer beyond u64 is parsed as a float by serde_json; it must not saturate to usize::MAX.
+    let oversized: serde_json::Value = serde_json::from_str("18446744073709551616").unwrap();
+    for command in [
+        "version.delete",
+        "version.rename",
+        "version.update",
+        "version.restore",
+        "pointColor.delete",
+        "redeye.delete",
+        "redeye.catchlight",
+        "history.restore",
+    ] {
+        for index in [json!(0.5), json!(1.9), json!(-1), json!(-0.5), json!("0"), json!(null), json!(u64::MAX), oversized.clone()] {
+            let result = s.execute(command, &json!({"index": index, "name": "Changed"}));
+            assert!(result.is_err(), "{command} accepted {index}: {result:?}");
+            assert_eq!(s.catalog.to_snapshot(), before, "{command} changed the catalog for {index}");
+            assert_eq!(s.execute("history.list", &json!({})).unwrap(), history, "{command} moved the History cursor for {index}");
+            assert_eq!(s.undo.len(), undo_len, "{command} added an undo step for {index}");
+        }
+        assert!(s.execute(command, &json!({"name": "Changed"})).is_err(), "{command} accepted a missing index");
+        assert_eq!(s.catalog.to_snapshot(), before);
+    }
+}
+
+/// Whole floating-point JSON numbers remain valid for existing control and MCP callers.
+#[test]
+fn whole_command_indices_accept_integer_and_float_numbers() {
+    let (mut s, n) = three_edits();
+    let id = s.active().unwrap();
+    s.execute("history.restore", &json!({"index": (n - 3) as f64})).unwrap();
+    assert_eq!((exposure(&s), list(&mut s)), (1.0, (n, Some(n - 3))));
+    s.execute("history.restore", &json!({"index": n - 1})).unwrap();
+    assert_eq!(exposure(&s), 3.0);
+    for name in ["A", "B", "C"] {
+        s.execute("version.create", &json!({"name": name})).unwrap();
+    }
+    s.execute("version.rename", &json!({"index": 2.0, "name": "Third"})).unwrap();
+    assert_eq!(s.catalog.photo(id).unwrap().versions[2].name, "Third");
+    s.execute("develop.set", &json!({"control": "light.exposure", "value": -2.0})).unwrap();
+    s.execute("version.update", &json!({"index": 1.0})).unwrap();
+    assert_eq!(s.catalog.photo(id).unwrap().versions[1].settings.light.exposure, -2.0);
+    s.execute("version.restore", &json!({"index": 0.0})).unwrap();
+    assert_eq!(exposure(&s), 3.0);
+    s.execute("version.delete", &json!({"index": 1})).unwrap();
+    assert_eq!(s.catalog.photo(id).unwrap().versions.iter().map(|v| v.name.as_str()).collect::<Vec<_>>(), ["A", "Third"]);
+    s.execute("version.delete", &json!({"index": 1.0})).unwrap();
+    assert_eq!(s.catalog.photo(id).unwrap().versions.len(), 1);
+    s.execute("pointColor.pick", &json!({"x": 0.5, "y": 0.08})).unwrap();
+    s.execute("pointColor.delete", &json!({"index": 0.0})).unwrap();
+    assert!(s.develop_of(id).unwrap().point_colors.is_empty());
+    s.execute("redeye.add", &json!({"center": [0.3, 0.4], "rx": 0.03, "ry": 0.02, "pet": true})).unwrap();
+    s.execute("redeye.catchlight", &json!({"index": 0.0})).unwrap();
+    assert!(s.develop_of(id).unwrap().red_eye[0].catchlight.is_some());
+    s.execute("redeye.delete", &json!({"index": 0.0})).unwrap();
+    assert!(s.develop_of(id).unwrap().red_eye.is_empty());
+}
+
+#[test]
+fn fractional_history_restore_preserves_the_cursor() {
+    let (mut s, _) = three_edits();
+    let before = s.execute("history.list", &json!({})).unwrap();
+    let result = s.execute("history.restore", &json!({"index": 1.9}));
+    let after = s.execute("history.list", &json!({})).unwrap();
+    assert!(result.is_err(), "index 1.9 must be refused; got {result:?}; History unchanged: {}", before == after);
+    assert_eq!(after, before);
+    assert_eq!(exposure(&s), 3.0);
+}