aleleba/lightcraft
1
0
Fork 0
mirror of https://github.com/storytold/lightcraft.git synced 2026-10-29 20:48:33 -06:00
Code Issues Packages Projects Releases Wiki Activity

Merge PR #2101: Reject malformed mask and spot targets before mutation

Browse Source
This commit is contained in:
aiongg
2026-10-21 14:48:28 +00:00
parent df9d830189 005881ce94
commit 265b35a975
3 changed files with 278 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/engine/src/cmd/mask_effects.rs
+6 -9
View File
@@ -287,12 +287,7 @@ fn effect_index(s: &Session, name: &str, c: &str) -> Result<usize> {
/// The selected mask's adjustments on the active photo.
fn selected_mask(s: &Session, p: &Value, c: &str) -> Result<Mask> {
let id = s.active().ok_or_else(|| bad(c, "no active photo"))?;
let mid = p
.get("id")
.and_then(Value::as_u64)
.and_then(|v| u32::try_from(v).ok())
.or(s.active_mask)
.ok_or_else(|| bad(c, "no mask selected (give `id`)"))?;
let mid = super::masks::mask_id(p, s.active_mask, c)?;
let d = s.develop_of(id).unwrap_or_default();
d.masks.iter().find(|m| m.id == mid).cloned().ok_or_else(|| bad(c, format!("no mask {mid}")))
}
@@ -341,10 +336,12 @@ fn apply_effect(s: &mut Session, p: &Value) -> Result<Value> {
// the masks: `ids`, `all`, else `id` or the selected one
let ids: Vec<u32> = if p.get("all").and_then(Value::as_bool).unwrap_or(false) {
d.masks.iter().map(|m| m.id).collect()
} else if let Some(a) = p.get("ids").and_then(Value::as_array) {
a.iter().filter_map(Value::as_u64).filter_map(|v| u32::try_from(v).ok()).collect()
} else {
vec![selected_mask(s, p, C)?.id]
match p.get("ids") {
None | Some(Value::Null) => vec![selected_mask(s, p, C)?.id],
Some(Value::Array(a)) => a.iter().map(|v| super::masks::parse_mask_id(v, C)).collect::<Result<_>>()?,
Some(_) => return Err(bad(C, "`ids` must be a list of mask IDs")),
}
};
if ids.is_empty() {
return Err(bad(C, "no masks to apply the effect to"));
crates/engine/src/cmd/masks.rs
+33 -13
View File
@@ -199,8 +199,28 @@ fn connected_strokes(from: Point, last: &BrushStroke, to: BrushStroke) -> Vec<Br
out
}
fn mask_id(p: &Value, active: Option<u32>, c: &str) -> Result<u32> {
p.get("id").and_then(Value::as_u64).map(|v| v as u32).or(active).ok_or_else(|| bad(c, "no mask selected (give `id`)"))
pub(super) fn parse_mask_id(v: &Value, c: &str) -> Result<u32> {
v.as_u64().and_then(|v| u32::try_from(v).ok()).ok_or_else(|| bad(c, "`id` must be a non-negative integer that fits in u32"))
}
/// Omitted or null targets keep the command's default; malformed explicit targets never do.
fn optional_mask_id(p: &Value, c: &str) -> Result<Option<u32>> {
p.get("id").filter(|v| !v.is_null()).map(|v| parse_mask_id(v, c)).transpose()
}
pub(super) fn mask_id(p: &Value, active: Option<u32>, c: &str) -> Result<u32> {
optional_mask_id(p, c)?.or(active).ok_or_else(|| bad(c, "no mask selected (give `id`)"))
}
fn optional_index(p: &Value, key: &str, c: &str) -> Result<Option<usize>> {
p.get(key)
.filter(|v| !v.is_null())
.map(|v| {
v.as_u64()
.and_then(|v| usize::try_from(v).ok())
.ok_or_else(|| bad(c, format!("`{key}` must be a non-negative integer that fits in usize")))
})
.transpose()
}
fn find(masks: &mut [Mask], id: u32, c: &str) -> Result<usize> {
@@ -307,7 +327,7 @@ pub fn specs() -> Vec<CommandSpec> {
let connect = bool_or(p, "connect", false);
let mut connected = false;
let next = s.active().and_then(|id| s.develop_of(id)).map(|d| d.next_mask_id()).unwrap_or(1);
let want = p.get("id").and_then(Value::as_u64).map(|v| v as u32).or(s.active_mask);
let want = optional_mask_id(p, "mask.brushStroke")?.or(s.active_mask);
let adjust = super::mask_effects::new_mask_adjust(s, "brush");
let mut out = masks_edit(s, "mask.brushStroke", "Brush", |masks, active| {
let i = match want.and_then(|m| masks.iter().position(|x| x.id == m)) {
@@ -363,7 +383,7 @@ pub fn specs() -> Vec<CommandSpec> {
cmd!("mask.update", "Update Mask Shape", [], None, "{id?, component?: index (0), shape: MaskShape JSON}", has_active, |s, p| {
let shape: MaskShape =
serde_json::from_value(p.get("shape").cloned().unwrap_or_default()).map_err(|e| bad("mask.update", e.to_string()))?;
let comp = p.get("component").and_then(Value::as_u64).unwrap_or(0) as usize;
let comp = optional_index(p, "component", "mask.update")?.unwrap_or(0);
let mid = mask_id(p, s.active_mask, "mask.update")?;
masks_edit(s, "mask.update", "Edit Mask", |masks, _| {
let i = find(masks, mid, "mask.update")?;
@@ -381,6 +401,8 @@ pub fn specs() -> Vec<CommandSpec> {
has_active,
|s, p| {
let c = "mask.sampleColor";
let mid = mask_id(p, s.active_mask, c)?;
let comp = optional_index(p, "component", c)?;
let (x, y) = (super::f64_req(p, "x", c)?, super::f64_req(p, "y", c)?);
let add = p.get("add").and_then(Value::as_bool).unwrap_or(false);
let id = s.active().ok_or_else(|| bad(c, "no active photo"))?;
@@ -390,8 +412,6 @@ pub fn specs() -> Vec<CommandSpec> {
let req = lightcraft_pipeline::RenderRequest::fit(384, 384);
let lab = lightcraft_pipeline::color_range_sample(&src, &info, &d, &req, lightcraft_geom::Point::new(x, y))
.ok_or_else(|| bad(c, "the point is outside the photo"))?;
let mid = mask_id(p, s.active_mask, c)?;
let comp = p.get("component").and_then(Value::as_u64).map(|v| v as usize);
let mut out = Vec::new();
masks_edit(s, c, "Color Range", |masks, _| {
let i = find(masks, mid, c)?;
@@ -482,8 +502,8 @@ pub fn specs() -> Vec<CommandSpec> {
let mid = mask_id(p, s.active_mask, c)?;
let d = s.develop_of(id).unwrap_or_default();
let m = d.masks.iter().find(|m| m.id == mid).ok_or_else(|| bad(c, format!("no mask {mid}")))?;
let k = match p.get("component").and_then(Value::as_u64) {
Some(k) => k as usize,
let k = match optional_index(p, "component", c)? {
Some(k) => k,
None => m
.components
.iter()
@@ -604,7 +624,7 @@ pub fn specs() -> Vec<CommandSpec> {
}
),
cmd!("mask.select", "Select Mask", [], None, "{id|null}", has_active, |s, p| {
s.active_mask = p.get("id").and_then(Value::as_u64).map(|v| v as u32);
s.active_mask = optional_mask_id(p, "mask.select")?;
ok()
}),
cmd!("mask.delete", "Delete Mask", [], None, "{id?}", has_active, |s, p| {
@@ -684,7 +704,7 @@ pub fn specs() -> Vec<CommandSpec> {
|s, p| {
let c = "mask.component";
let mid = mask_id(p, s.active_mask, c)?;
let k = p.get("component").and_then(Value::as_u64).ok_or_else(|| bad(c, "missing component"))? as usize;
let k = optional_index(p, "component", c)?.ok_or_else(|| bad(c, "missing component"))?;
let action = str_param(p, "action").ok_or_else(|| bad(c, "missing action"))?.to_string();
let op: Option<MaskOp> = match p.get("op") {
Some(v) => Some(serde_json::from_value(v.clone()).map_err(|e| bad(c, e.to_string()))?),
@@ -879,8 +899,8 @@ pub fn specs() -> Vec<CommandSpec> {
),
cmd!("spot.select", "Select Spot", [], None, "{index|null}", has_active, |s, p| {
let n = spots_len(s);
s.active_spot = match p.get("index").and_then(Value::as_u64) {
Some(i) if (i as usize) < n => Some(i as usize),
s.active_spot = match optional_index(p, "index", "spot.select")? {
Some(i) if i < n => Some(i),
Some(_) => return Err(bad("spot.select", "no such spot")),
None => None,
};
@@ -1040,7 +1060,7 @@ fn spots_len(s: &Session) -> usize {
/// `index` from the params, else the selected spot.
fn spot_index(s: &Session, p: &Value, c: &str) -> Result<usize> {
let i = p.get("index").and_then(Value::as_u64).map(|v| v as usize).or(s.active_spot).ok_or_else(|| bad(c, "no spot selected (give `index`)"))?;
let i = optional_index(p, "index", c)?.or(s.active_spot).ok_or_else(|| bad(c, "no spot selected (give `index`)"))?;
if i >= spots_len(s) {
return Err(bad(c, "no such spot"));
}
crates/engine/src/tests_spots.rs
+239
View File
@@ -82,3 +82,242 @@ fn a_content_aware_spot_has_no_source() {
assert_eq!(serde_json::to_value(sp).unwrap()["mode"], "fill");
assert!(s.execute("spot.update", &json!({"mode": "nonsense"})).is_err());
}
fn target_session() -> Session {
let mut s = Session::with_demo();
s.execute("mask.add", &json!({"kind": "colorRange"})).unwrap();
s.execute("mask.add", &json!({"kind": "radial"})).unwrap();
s.execute("mask.addComponent", &json!({"kind": "colorRange"})).unwrap();
s.execute("mask.saveEffect", &json!({"id": 1, "name": "Existing"})).unwrap();
s.execute("mask.adjust", &json!({"values": {"exposure": 1.0}})).unwrap();
for x in [0.3, 0.7] {
s.execute("spot.add", &json!({"points": [[x, 0.5]], "source": [0.1, 0.0]})).unwrap();
}
// A rejected command must preserve a redo branch as well as existing undo steps.
s.execute("mask.rename", &json!({"name": "Temporary"})).unwrap();
s.execute("edit.undo", &json!({})).unwrap();
s
}
fn target_state(s: &Session) -> serde_json::Value {
let photo = s.catalog.photo(s.active().unwrap()).unwrap();
json!({
"develop": photo.develop,
"history": photo.history,
"selection": s.selection,
"activeMask": s.active_mask,
"activeSpot": s.active_spot,
"undo": format!("{:?}", s.undo),
"redo": format!("{:?}", s.redo),
"commits": s.commits(),
"effects": s.mask_effects,
"rememberedEffects": s.mask_tool_effects,
"journal": s.journal,
"revision": s.catalog.revision,
})
}
fn rejects_target(s: &mut Session, command: &str, params: &serde_json::Value) {
let before = target_state(s);
let result = s.execute(command, params);
let after = target_state(s);
assert!(result.is_err(), "{command} {params} returned {result:?}; state unchanged: {}", before == after);
assert_eq!(after, before, "{command} {params} changed edits, selection or history on rejection");
for field in ["id", "component", "index"] {
if let Some(value) = params.get(field)
&& !value.is_null()
&& value.as_u64().is_none()
{
assert!(
result.unwrap_err().to_string().contains(&format!("`{field}`")),
"{command} must reject the malformed {field} before doing other work"
);
break;
}
}
}
fn malformed_targets() -> Vec<serde_json::Value> {
vec![json!(-1), json!(0.5), json!("1"), json!(true), json!([]), json!({}), json!(u64::MAX)]
}
#[test]
fn malformed_mask_ids_are_rejected_without_changes() {
let mut s = target_session();
let commands = [
("mask.delete", json!({})),
("mask.rename", json!({"name": "Wrong mask"})),
("mask.invert", json!({})),
("mask.visible", json!({"visible": false})),
("mask.duplicate", json!({})),
("mask.move", json!({"to": 0})),
("mask.adjust", json!({"values": {"exposure": 1.0}})),
("mask.update", json!({"shape": {"kind": "brush", "strokes": []}})),
("mask.addComponent", json!({"kind": "brush"})),
("mask.component", json!({"component": 0, "action": "invert"})),
("mask.brushStroke", json!({"points": [[0.5, 0.5]]})),
("mask.sampleColor", json!({"x": 0.5, "y": 0.5})),
("mask.objectPoint", json!({"x": 0.5, "y": 0.5})),
("mask.refineDetail", json!({})),
];
for (command, mut params) in commands {
for target in malformed_targets().into_iter().chain([json!(4_294_967_297_u64)]) {
params["id"] = target;
rejects_target(&mut s, command, &params);
}
}
}
#[test]
fn malformed_mask_selection_is_rejected_without_changes() {
let mut s = target_session();
for target in malformed_targets().into_iter().filter(|v| !v.is_null()).chain([json!(4_294_967_297_u64)]) {
rejects_target(&mut s, "mask.select", &json!({"id": target}));
}
}
#[test]
fn overflowing_mask_id_does_not_wrap_to_an_existing_mask() {
let mut s = target_session();
rejects_target(&mut s, "mask.delete", &json!({"id": 4_294_967_297_u64}));
}
#[test]
fn malformed_mask_effect_targets_are_rejected_without_changes() {
let mut s = target_session();
for (command, mut params) in [
("mask.saveEffect", json!({"name": "Wrong effect"})),
("mask.updateEffect", json!({"name": "Existing"})),
("mask.applyEffect", json!({"name": "Existing"})),
] {
for target in malformed_targets().into_iter().chain([json!(4_294_967_297_u64)]) {
params["id"] = target;
rejects_target(&mut s, command, &params);
}
}
for target in malformed_targets().into_iter().chain([json!([1, -1]), json!([1, null]), json!([1, 4_294_967_297_u64])]) {
rejects_target(&mut s, "mask.applyEffect", &json!({"name": "Existing", "ids": target}));
}
}
#[test]
fn mask_effect_targets_preserve_explicit_defaults_and_lists() {
let mut s = target_session();
s.execute("mask.saveEffect", &json!({"id": 1, "name": "Explicit"})).unwrap();
assert_eq!(s.mask_effects.last().unwrap().adjust.exposure, 0.0);
for (name, target) in [("Omitted", json!({})), ("Null", json!({"id": null}))] {
let mut params = target;
params["name"] = json!(name);
s.execute("mask.saveEffect", &params).unwrap();
assert_eq!(s.mask_effects.last().unwrap().adjust.exposure, 1.0);
params["name"] = json!("Existing");
s.execute("mask.updateEffect", &params).unwrap();
assert_eq!(s.mask_effects[0].adjust.exposure, 1.0);
}
s.execute("mask.updateEffect", &json!({"id": 1, "name": "Existing"})).unwrap();
assert_eq!(s.mask_effects[0].adjust.exposure, 0.0);
for mut params in [json!({}), json!({"id": null}), json!({"ids": null})] {
s.execute("mask.adjust", &json!({"values": {"exposure": 1.0}})).unwrap();
params["name"] = json!("Existing");
s.execute("mask.applyEffect", &params).unwrap();
assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks[1].adjust.exposure, 0.0);
}
s.execute("mask.adjust", &json!({"id": 1, "values": {"exposure": 1.0}})).unwrap();
s.execute("mask.adjust", &json!({"id": 2, "values": {"exposure": 2.0}})).unwrap();
s.execute("mask.applyEffect", &json!({"name": "Existing", "ids": [1, 2]})).unwrap();
assert!(s.develop_of(s.active().unwrap()).unwrap().masks.iter().all(|m| m.adjust.exposure == 0.0));
rejects_target(&mut s, "mask.applyEffect", &json!({"name": "Existing", "ids": []}));
}
#[test]
fn malformed_mask_components_are_rejected_without_changes() {
let mut s = target_session();
for (command, mut params) in [
("mask.update", json!({"shape": {"kind": "brush", "strokes": []}})),
("mask.component", json!({"action": "invert"})),
("mask.sampleColor", json!({"x": 0.5, "y": 0.5})),
("mask.refineDetail", json!({})),
] {
for target in malformed_targets().into_iter().chain([json!(4_294_967_296_u64)]) {
params["component"] = target;
rejects_target(&mut s, command, &params);
}
}
}
#[test]
fn malformed_spot_indices_are_rejected_without_changes() {
let mut s = target_session();
for (command, mut params) in [("spot.delete", json!({})), ("spot.update", json!({"size": 0.1})), ("spot.refreshSource", json!({}))] {
for target in malformed_targets().into_iter().chain([json!(4_294_967_296_u64)]) {
params["index"] = target;
rejects_target(&mut s, command, &params);
}
}
}
#[test]
fn malformed_spot_selection_is_rejected_without_changes() {
let mut s = target_session();
for target in malformed_targets().into_iter().filter(|v| !v.is_null()).chain([json!(4_294_967_296_u64)]) {
rejects_target(&mut s, "spot.select", &json!({"index": target}));
}
}
#[test]
fn mask_and_spot_targets_preserve_explicit_defaults_and_selection_clearing() {
use lightcraft_develop::MaskShape;
let mut s = target_session();
for target in [json!({}), json!({"id": null})] {
let mut params = target;
params["name"] = json!("Selected mask");
s.execute("mask.rename", &params).unwrap();
assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks[1].name, "Selected mask");
}
s.execute("mask.rename", &json!({"id": 1, "name": "Explicit mask"})).unwrap();
assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks[0].name, "Explicit mask");
assert_eq!(s.active_mask, Some(2));
for params in [json!({"shape": {"kind": "brush", "strokes": []}}), json!({"component": null, "shape": {"kind": "brush", "strokes": []}})] {
s.execute("mask.update", &params).unwrap();
let d = s.develop_of(s.active().unwrap()).unwrap();
assert!(matches!(d.masks[1].components[0].shape, MaskShape::Brush { .. }));
assert!(matches!(d.masks[1].components[1].shape, MaskShape::ColorRange { .. }));
}
s.execute("mask.update", &json!({"id": 1, "component": 0, "shape": {"kind": "sky"}})).unwrap();
assert!(matches!(s.develop_of(s.active().unwrap()).unwrap().masks[0].components[0].shape, MaskShape::Sky));
for params in [json!({"points": [[0.2, 0.2]]}), json!({"id": null, "points": [[0.4, 0.4]]})] {
s.execute("mask.brushStroke", &params).unwrap();
}
assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks.len(), 2);
for params in [json!({"x": 0.5, "y": 0.5}), json!({"component": null, "x": 0.5, "y": 0.5})] {
s.execute("mask.sampleColor", &params).unwrap();
let d = s.develop_of(s.active().unwrap()).unwrap();
let MaskShape::ColorRange { samples, .. } = &d.masks[1].components[1].shape else { panic!("color range") };
assert_eq!(samples.len(), 1);
}
for params in [json!({"size": 0.04}), json!({"index": null, "size": 0.04})] {
s.execute("spot.update", &params).unwrap();
assert_eq!(spots(&s)[1].size, 0.04);
}
s.execute("spot.update", &json!({"index": 0, "size": 0.03})).unwrap();
assert_eq!(spots(&s)[0].size, 0.03);
assert_eq!(s.active_spot, Some(1));
rejects_target(&mut s, "mask.component", &json!({"component": null, "action": "delete"}));
for params in [json!({"id": null}), json!({})] {
s.execute("mask.select", &json!({"id": 1})).unwrap();
assert_eq!(s.active_mask, Some(1));
s.execute("mask.select", &params).unwrap();
assert_eq!(s.active_mask, None);
}
for params in [json!({"index": null}), json!({})] {
s.execute("spot.select", &json!({"index": 0})).unwrap();
assert_eq!(s.active_spot, Some(0));
s.execute("spot.select", &params).unwrap();
assert_eq!(s.active_spot, None);
}
rejects_target(&mut s, "mask.delete", &json!({}));
rejects_target(&mut s, "spot.delete", &json!({}));
// An omitted mask still creates a brush mask when no mask is selected.
s.execute("mask.brushStroke", &json!({"points": [[0.5, 0.5]]})).unwrap();
assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks.len(), 3);
}