collective
Browse / github.com/storytold/deckcraft / patch
Reject invalid slide index in file.export, file.render, slide.colorAt
Reject invalid slide index in file.export, file.render, slide.colorAt Found while testing `crates/engine/src/cmd/file.rs:581 (export); same pattern in render :539 and color_at :550` (no existing issue): file.export with an invalid (negative or non-integer) slide index exports the selected slide. ## Cause file.export, file.render and slide.colorAt read `slide` with `usize_param`, which returns None for a negative or non-integer value. That looked the same as an absent `slide`, so the commands used the selected slide and succeeded. ## Fix The three call sites now use `opt_usize_param`, so a present but invalid `slide` returns a `slide is not a valid index` error
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Silvertree2010 (imported from pull request #1410, open)
- Base
aaf6c1c38aa6c59beda8110bd014cc6c96c6ec74- Size
- +21 −4 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
e3f8719219577fde50e4ec6f
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd/file.rs crates/engine/src/tests.rs
@@ -7,7 +7,7 @@ use deckcraft_media::MediaInfo; use deckcraft_model::{MediaId, MediaItem, Presentation, Shape, ShapeKind, defaults}; use serde_json::{Value, json}; -use super::{CommandSpec, always, bad, base64_decode, base64_encode, bool_or, cmd, f64_or, has_doc, ok, str_param, usize_param};+use super::{CommandSpec, always, bad, base64_decode, base64_encode, bool_or, cmd, f64_or, has_doc, ok, opt_usize_param, str_param, usize_param}; use crate::{DocState, EngineError, Result, Session}; pub fn specs() -> Vec<CommandSpec> {@@ -536,7 +536,7 @@ fn picture_bytes(img: &deckcraft_render::Image, format: &str, scale: f64) -> Vec fn render(s: &mut Session, p: &Value) -> Result<Value> { let st = s.doc()?;- let i = usize_param(p, "slide").unwrap_or(st.selection.slide);+ let i = opt_usize_param(p, "slide", "file.render")?.unwrap_or(st.selection.slide); if i >= st.doc.slides.len() { return Err(bad("file.render", format!("no slide {i}"))); }@@ -547,7 +547,7 @@ fn render(s: &mut Session, p: &Value) -> Result<Value> { /// The colour the slide is drawn with at one point: what the Eyedropper takes. fn color_at(s: &mut Session, p: &Value) -> Result<Value> { let st = s.doc()?;- let i = usize_param(p, "slide").unwrap_or(st.selection.slide);+ let i = opt_usize_param(p, "slide", "slide.colorAt")?.unwrap_or(st.selection.slide); if i >= st.doc.slides.len() { return Err(bad("slide.colorAt", format!("no slide {i}"))); }@@ -578,7 +578,7 @@ fn export(s: &mut Session, p: &Value) -> Result<Value> { } match format.as_str() { "png" | "jpeg" | "jpg" => {- let slides: Vec<usize> = match usize_param(p, "slide") {+ let slides: Vec<usize> = match opt_usize_param(p, "slide", "file.export")? { Some(i) => vec![i], None if bool_or(p, "all", false) => (0..st.doc.slides.len()).collect(), None => vec![st.selection.slide],@@ -5646,3 +5646,20 @@ fn invalid_ids_array_never_falls_back_to_the_selection() { assert!(s.doc().unwrap().shape(deckcraft_model::ShapeId(a as u32)).is_some(), "shape deleted for {ids}"); } }++#[test]+fn invalid_slide_index_never_falls_back_to_the_selection_in_export_commands() {+ let mut s = session();+ let dir = std::env::temp_dir().join(format!("deckcraft-badslide-{}", std::process::id()));+ let path = dir.join("a.png");+ for slide in [json!(-1), json!(1.5), json!("two")] {+ let r = s.execute("file.export", &json!({"path": path.to_string_lossy(), "slide": slide, "scale": 0.1}));+ assert!(matches!(r, Err(crate::EngineError::BadParams { .. })), "export slide {slide} gave {r:?}");+ assert!(!path.exists(), "export wrote a file for {slide}");+ let r = s.execute("file.render", &json!({"slide": slide, "scale": 0.1}));+ assert!(matches!(r, Err(crate::EngineError::BadParams { .. })), "render slide {slide} gave {r:?}");+ let r = s.execute("slide.colorAt", &json!({"slide": slide, "x": 1, "y": 1, "scale": 0.1}));+ assert!(matches!(r, Err(crate::EngineError::BadParams { .. })), "colorAt slide {slide} gave {r:?}");+ }+ let _ = std::fs::remove_dir_all(&dir);+}