From ea826b11cc5bec456c2b71e6bc6ea59a4f91af45 Mon Sep 17 00:00:00 2001 From: midasmatterhorn <248507632+midasmatterhorn@users.noreply.github.com> Date: Wed, 14 Oct 2026 22:53:04 +0300 Subject: [PATCH] engine: refuse negative indices instead of targeting element 0 `version.*`, `pointColor.delete` and `redeye.catchlight`/`redeye.delete` parsed `index` with `f64_req(..)? as usize`. Rust's float-to-int cast saturates, so `index: -1` became 0 and the bounds checks never saw a negative: a caller sending `index: -1` silently deleted, restored or renamed version 0 (or the first point-colour sample / red-eye correction) and got `ok` back. `history.restore` already guards this and `spot_index` uses `as_u64`. New `index_req` helper in `cmd/mod.rs` (the same finite, non-negative guard) is used by the five sites. Behaviour for valid indices is unchanged. Regression test `negative_indices_are_refused` creates a version, then asserts `version.delete/restore/rename` with `index: -1` error and version 0 is untouched (before the fix the first one deletes it). --- crates/engine/src/cmd/color.rs | 2 +- crates/engine/src/cmd/develop.rs | 8 ++++---- crates/engine/src/cmd/masks.rs | 4 ++-- crates/engine/src/cmd/mod.rs | 9 +++++++++ crates/engine/src/tests_history_cursor.rs | 17 +++++++++++++++++ 5 files changed, 33 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/color.rs b/crates/engine/src/cmd/color.rs index 55ea8b5ab..111159e7a 100644 --- a/crates/engine/src/cmd/color.rs +++ b/crates/engine/src/cmd/color.rs @@ -156,7 +156,7 @@ fn targeted(s: &mut Session, p: &Value) -> Result { } fn index(p: &Value, c: &str) -> Result { - Ok(f64_req(p, "index", c)? as usize) + super::index_req(p, "index", c) } pub fn specs() -> Vec { diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index 6f064b260..231316395 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/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 { @@ -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)) -> Result { 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 { }), 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")); diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index 25ff3f24d..8d5a6751e 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -921,7 +921,7 @@ pub fn specs() -> Vec { 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"))?; @@ -936,7 +936,7 @@ pub fn specs() -> Vec { ), 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() { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 4de460761..6092c5345 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -187,6 +187,15 @@ pub(crate) fn bad(cmd: &str, msg: impl Into) -> EngineError { pub(crate) fn f64_req(p: &Value, key: &str, cmd: &str) -> Result { 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 { + 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) } diff --git a/crates/engine/src/tests_history_cursor.rs b/crates/engine/src/tests_history_cursor.rs index 55c265403..ffe300795 100644 --- a/crates/engine/src/tests_history_cursor.rs +++ b/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"); +}