Merge PR #1793

crates/engine/src/cmd/color.rs

@@ -156,7 +156,7 @@ fn targeted(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn index(p: &Value, c: &str) -> Result<usize> {
-    Ok(f64_req(p, "index", c)? as usize)
+    super::index_req(p, "index", c)
 }
 
 pub fn specs() -> Vec<CommandSpec> {

crates/engine/src/cmd/develop.rs

@@ -5,7 +5,7 @@ use lightcraft_develop::{DevelopSettings, Preset, ProcessVersion, Section, Setti
 use lightcraft_geom::{CropGeometry, CropHandle, Point, Rect, crop_fit_angle, drag_crop};
 use serde_json::{Value, json};
 
-use super::{CommandSpec, always, bad, bool_or, cmd, f64_or, f64_req, has_active, has_clipboard, has_selection, ok, str_param};
+use super::{CommandSpec, always, bad, bool_or, cmd, f64_or, f64_req, has_active, has_clipboard, has_selection, index_req, ok, str_param};
 use crate::{Result, Session, media::SourceLevel};
 
 fn active(s: &Session, c: &str) -> Result<PhotoId> {
@@ -144,7 +144,7 @@ fn user_preset<'a>(s: &'a mut Session, p: &Value, c: &str) -> Result<&'a mut Pre
 /// Change version `p.index` of the active photo (`f` gets it and the current settings).
 fn edit_version(s: &mut Session, p: &Value, c: &str, label: &str, f: impl FnOnce(&mut Version, std::sync::Arc<DevelopSettings>)) -> Result<Value> {
     let id = active(s, c)?;
-    let i = f64_req(p, "index", c)? as usize;
+    let i = index_req(p, "index", c)?;
     let ph = s.catalog.photo(id).ok_or_else(|| bad(c, "no photo"))?;
     let mut versions = ph.versions.clone();
     let v = versions.get_mut(i).ok_or_else(|| bad(c, "no such version"))?;
@@ -1060,14 +1060,14 @@ pub fn specs() -> Vec<CommandSpec> {
         }),
         cmd!("version.restore", "Restore Version", [], None, "{index}", has_active, |s, p| {
             let id = active(s, "version.restore")?;
-            let i = f64_req(p, "index", "version.restore")? as usize;
+            let i = index_req(p, "index", "version.restore")?;
             let v = s.catalog.photo(id).and_then(|ph| ph.versions.get(i).cloned()).ok_or_else(|| bad("version.restore", "no such version"))?;
             s.set_develop(id, (*v.settings).clone(), &format!("Restore {}", v.name))?;
             ok()
         }),
         cmd!("version.delete", "Delete Version", [], None, "{index}", has_active, |s, p| {
             let id = active(s, "version.delete")?;
-            let i = f64_req(p, "index", "version.delete")? as usize;
+            let i = index_req(p, "index", "version.delete")?;
             let mut versions = s.catalog.photo(id).map(|ph| ph.versions.clone()).unwrap_or_default();
             if i >= versions.len() {
                 return Err(bad("version.delete", "no such version"));

crates/engine/src/cmd/masks.rs

@@ -927,7 +927,7 @@ pub fn specs() -> Vec<CommandSpec> {
             has_active,
             |s, p| {
                 let c = "redeye.catchlight";
-                let i = super::f64_req(p, "index", c)? as usize;
+                let i = super::index_req(p, "index", c)?;
                 let id = s.active().ok_or_else(|| bad(c, "no active photo"))?;
                 let mut dd = (*s.develop_of(id).unwrap_or_default()).clone();
                 let eye = dd.red_eye.get_mut(i).ok_or_else(|| bad(c, "no such eye"))?;
@@ -942,7 +942,7 @@ pub fn specs() -> Vec<CommandSpec> {
         ),
         cmd!("redeye.delete", "Delete Red Eye Correction", [], None, "{index}", has_active, |s, p| {
             let c = "redeye.delete";
-            let i = super::f64_req(p, "index", c)? as usize;
+            let i = super::index_req(p, "index", c)?;
             let id = s.active().ok_or_else(|| bad(c, "no active photo"))?;
             let mut dd = (*s.develop_of(id).unwrap_or_default()).clone();
             if i >= dd.red_eye.len() {

crates/engine/src/cmd/mod.rs

@@ -194,6 +194,15 @@ 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.
+pub(crate) fn index_req(p: &Value, key: &str, cmd: &str) -> Result<usize> {
+    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")));
+    }
+    Ok(i as usize)
+}
 pub(crate) fn f64_or(p: &Value, key: &str, d: f64) -> f64 {
     p.get(key).and_then(Value::as_f64).unwrap_or(d)
 }

crates/engine/src/tests_history_cursor.rs

@@ -98,3 +98,20 @@ fn the_cursor_survives_the_op_log_and_old_catalogs_load_at_the_end() {
     let old: lightcraft_catalog::Photo = serde_json::from_value(v).unwrap();
     assert_eq!((old.history_cursor, old.history_index()), (None, Some(n - 1)));
 }
+
+/// A negative `index` must be an error on every indexed command, not element 0 (`-1.0 as usize`
+/// saturates to 0): `history.restore` guards it, `version.*` (and the other `index` commands)
+/// must too. A caller sending `index: -1` used to delete or restore version 0 silently.
+#[test]
+fn negative_indices_are_refused() {
+    let mut s = Session::with_demo();
+    let id = s.active().unwrap();
+    let versions = |s: &Session| s.catalog.photo(id).unwrap().versions.len();
+    s.execute("version.create", &json!({"name": "A"})).unwrap();
+    assert_eq!(versions(&s), 1);
+    assert!(s.execute("version.delete", &json!({"index": -1})).is_err(), "must not delete version 0");
+    assert_eq!(versions(&s), 1, "nothing was deleted");
+    assert!(s.execute("version.restore", &json!({"index": -1})).is_err());
+    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");
+}