← Commit history

Merge PR #447: Reject unknown kicad_move_footprint arguments before native edits

Colby Knox ·317489025a ·2d ago ·parent 8309dc4
2 files changed +94
rust/crates/kicad-core/src/ipc.rs+3
@@ -1839,6 +1839,9 @@ pub(crate) fn place_footprint_proto(fp: &mut kicad_ipc_rs::FootprintItem, to: (i /// outline) then committed through `update_items` as ONE native Undo step. `dryRun` /// runs the checks and reports without touching the editor. pub fn move_footprint(ctx: &Ctx, args: &Value) -> RResult<Value> {+    // Fail unknown/typoed fields before even contacting the editor, not just+    // before committing. The same validator runs for file-based dry runs.+    placement::validate_move_arguments(args)?;     let s = connect(ctx, args)?;     let (text_before, board, revision) = s.snapshot()?;     check_revision(args, &revision)?;
rust/crates/kicad-core/src/placement.rs+91
@@ -566,6 +566,59 @@ fn normalize_deg(d: f64) -> f64 {     } } +/// Validate the *whole* move payload before resolving geometry or opening the live+/// editor. JSON getters otherwise silently treat typos such as `rot` as an omitted+/// `rotation`, which can move copper-connected parts without rotating their pads.+///+/// Includes the standard board-frame and IPC fields. Batch entries have their+/// own much smaller schema so typos inside `refs` fail the same way.+pub fn validate_move_arguments(args: &Value) -> Result<(), RoutingError> {+    const ROOT: &[&str] = &[+        "filePath", "socketPath", "expectedRevision", "ref", "x", "y",+        "rotation", "side", "refs", "dryRun", "allowOverlap",+        "allowOutside", "allowLocked", "save", "frame",+    ];+    const ENTRY: &[&str] = &["ref", "x", "y", "rotation", "side"];+    fn check(value: &Value, allowed: &[&str], context: &str) -> Result<(), RoutingError> {+        let map = value.as_object().ok_or_else(|| {+            RoutingError::new("invalid_arguments", format!("{context} must be a JSON object"))+                .with("mutated", json!(false))+        })?;+        for key in map.keys() {+            if !allowed.contains(&key.as_str()) {+                let suggested = if key == "rot" || key == "angle" {+                    Some("rotation")+                } else {+                    allowed.iter().find(|candidate| {+                        candidate.to_lowercase().starts_with(&key.to_lowercase())+                            || key.to_lowercase().starts_with(&candidate.to_lowercase())+                    }).copied()+                };+                let explanation = match suggested {+                    Some(want) => format!("Unknown {context} argument '{key}'; did you mean '{want}'?"),+                    None => format!("Unknown {context} argument '{key}'"),+                };+                let mut err = RoutingError::new("unknown_argument", explanation)+                    .with("argument", json!(key))+                    .with("allowedKeys", json!(allowed))+                    .with("mutated", json!(false));+                if let Some(want) = suggested {+                    err = err.with("suggestedKey", json!(want));+                }+                return Err(err);+            }+        }+        Ok(())+    }+    check(args, ROOT, "move")?;+    if let Some(Value::Array(entries)) = args.get("refs") {+        for (index, entry) in entries.iter().enumerate() {+            check(entry, ENTRY, &format!("refs[{index}]"))?;+        }+    }+    Ok(())+}+ /// `kicad_move_footprint`'s argument reading and checks. `ref` with `x`, `y`, `rotation?`, /// `side?`, or `refs: [{ref, x, y, rotation?, side?}, ...]` (1..256). Refuses an unknown /// or locked footprint, a side change (`side_change_not_supported`: a flip is separate@@ -573,6 +626,7 @@ fn normalize_deg(d: f64) -> f64 { /// courtyard outside the outline (`outside_outline`, unless `allowOutside`). Nothing is /// written here; the caller commits `plan.moves` as one undo step. pub fn plan_moves(board: &Board, args: &Value) -> Result<MovePlan, RoutingError> {+    validate_move_arguments(args)?;     let items: Vec<Value> = match args.get("refs") {         Some(Value::Array(a)) if !a.is_empty() && a.len() <= MAX_MOVES => a.clone(),         Some(_) => return Err(RoutingError::new("invalid_refs", format!("refs must list 1..{MAX_MOVES} moves, each {{ref, x, y, rotation?, side?}}"))),@@ -1295,6 +1349,43 @@ mod tests {         pcb::parse_pcb_text(&text, "placement.kicad_pcb").unwrap()     } +    #[test]+    fn move_arguments_reject_unknown_rotation_without_touching_placement() {+        let b = board(&[fp("Q2", 110.0, 110.0, 0.0, "F.Cu", RECT, "")]);+        for bad in [+            json!({"ref":"Q2","x":120,"y":110,"rot":270,"dryRun":true,"frame":"kicad"}),+            json!({"ref":"Q2","x":120,"y":110,"rot":270,"dryRun":false,"frame":"kicad"}),+            json!({"refs":[{"ref":"Q2","x":120,"y":110,"rot":270}],"dryRun":true}),+            json!({"refs":[{"ref":"Q2","x":120,"y":110,"rotation":180,"rott":90}]}),+            json!({"ref":"Q2","x":120,"y":110,"frame":"kicad","unexpected":true}),+        ] {+            let e = plan_moves(&b, &bad).unwrap_err();+            assert_eq!(e.code, "unknown_argument");+        }+        let original = placements(&b);+        assert_eq!(original[0].rotation, 0.0);+        assert_eq!(original[0].x, 110.0);+    }++    #[test]+    fn move_arguments_keep_documented_options_and_rotation() {+        let b = board(&[fp("Q2", 110.0, 110.0, 0.0, "F.Cu", RECT, "")]);+        let plan = plan_moves(&b, &json!({+            "filePath":"fixture.kicad_pcb","expectedRevision":"fixture-hash",+            "socketPath":"/tmp/ipc", "frame":"kicad",+            "ref":"Q2","x":120,"y":110,"rotation":180,+            "save":true,"dryRun":true,"allowOverlap":false,+            "allowOutside":false,"allowLocked":false+        })).unwrap();+        assert_eq!(plan.moves.len(), 1);+        assert_eq!(plan.moves[0].after.rotation, 180.0);+        let batch = plan_moves(&b, &json!({+            "refs":[{"ref":"Q2","x":120,"y":110,"rotation":270}],+            "frame":"standard","dryRun":true+        })).unwrap();+        assert_eq!(batch.moves[0].after.rotation, -90.0);+    }+     #[test]     fn courtyard_box_follows_rotation_and_falls_back_to_pads() {         let b = board(&[fp("U1", 110.0, 110.0, 0.0, "F.Cu", RECT, ""), fp("U2", 120.0, 110.0, 90.0, "F.Cu", RECT, ""), fp("R1", 130.0, 110.0, 0.0, "F.Cu", "", "")]);