From 5f17b48c8d4e6760d3f15b5586ab9fe5058e072e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 08:49:40 +0000 Subject: [PATCH] #2253: crop.aspect checks orientation before the current/toggle paths A bad `orientation` was refused only on the refit path: "current" and an unlocked "toggle" returned before the check, so {"aspect": "current", "orientation": "bogus"} succeeded silently, and a value that isn't a string fell back to the default. It is parsed first now, so every path refuses a wrong word, a number, a boolean or null, and nothing changes (CodeRabbit on PR #2280). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AbegtYaPuwqZ81pGfELsFK --- crates/engine/src/cmd/develop.rs | 15 ++++++++------- crates/engine/src/tests_crop_aspect.rs | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index 38bf7a3da..e4dc9e3fd 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -642,6 +642,14 @@ pub fn specs() -> Vec { |s, p| { let id = active(s, "crop.aspect")?; let (w, h) = crop_dims(s, id); + // checked first, whatever the aspect: a bad value is an error on every path + let turn = match p.get("orientation") { + Some(Value::String(o)) if o == "crop" => Turn::LikeCrop, + Some(Value::String(o)) if o == "given" => Turn::AsGiven, + Some(_) => return Err(bad("crop.aspect", "orientation is \"crop\" or \"given\"")), + None if matches!(p.get("aspect"), Some(Value::Array(_))) => Turn::AsGiven, + None => Turn::LikeCrop, + }; let locked = s.develop_of(id).is_some_and(|d| d.crop.aspect.is_some()); let lock_current = match p.get("aspect").and_then(Value::as_str) { Some("current") => true, @@ -656,13 +664,6 @@ pub fn specs() -> Vec { Ok(()) }); } - let turn = match p.get("orientation").and_then(Value::as_str) { - Some("crop") => Turn::LikeCrop, - Some("given") => Turn::AsGiven, - Some(_) => return Err(bad("crop.aspect", "orientation is \"crop\" or \"given\"")), - None if matches!(p.get("aspect"), Some(Value::Array(_))) => Turn::AsGiven, - None => Turn::LikeCrop, - }; let aspect = match p.get("aspect") { Some(Value::String(a)) if a == "toggle" => None, Some(Value::String(a)) if a == "free" => None, diff --git a/crates/engine/src/tests_crop_aspect.rs b/crates/engine/src/tests_crop_aspect.rs index 3cfee0c05..1b8698691 100644 --- a/crates/engine/src/tests_crop_aspect.rs +++ b/crates/engine/src/tests_crop_aspect.rs @@ -66,3 +66,21 @@ fn orientation_overrides_and_is_checked() { let e = s.execute("crop.aspect", &json!({"aspect": "2x3", "orientation": "sideways"})).unwrap_err(); assert!(e.to_string().contains("orientation"), "{e}"); } + +/// A bad `orientation` is refused on every path, the lock-as-is ones ("current", an unlocked +/// "toggle") included, and whether it is a wrong word or not a string at all; nothing changes. +#[test] +fn a_bad_orientation_is_refused_before_anything_is_locked() { + let mut s = landscape_photo(); + let before = s.develop_of(s.selection.active.unwrap()).unwrap().crop.clone(); + for aspect in [json!("current"), json!("toggle"), json!("2x3"), json!([2, 3]), json!("free")] { + for orientation in [json!("sideways"), json!(1), json!(true), json!(null)] { + let e = s.execute("crop.aspect", &json!({"aspect": aspect, "orientation": orientation})).unwrap_err(); + assert!(e.to_string().contains("orientation"), "{aspect} {orientation}: {e}"); + assert_eq!(s.develop_of(s.selection.active.unwrap()).unwrap().crop, before, "{aspect} {orientation}: untouched"); + } + } + // the valid words still lock as is + aspect(&mut s, json!({"aspect": "current", "orientation": "crop"})); + assert!(s.develop_of(s.selection.active.unwrap()).unwrap().crop.aspect.is_some()); +}