diff --git a/desktop/wrapper/src/handle_desktop_wrapper_message.rs b/desktop/wrapper/src/handle_desktop_wrapper_message.rs index d17fca6038..2edbc015a9 100644 --- a/desktop/wrapper/src/handle_desktop_wrapper_message.rs +++ b/desktop/wrapper/src/handle_desktop_wrapper_message.rs @@ -31,10 +31,15 @@ pub(super) fn handle_desktop_wrapper_message(dispatcher: &mut DesktopWrapperMess SaveFileDialogContext::File { content } => { dispatcher.respond(DesktopFrontendMessage::WriteFile { path, content }); } - SaveFileDialogContext::MultipleFiles { files } => { - // Treat the chosen path as the folder name; strip any extension the user typed (e.g. "MyAnim.png" → "MyAnim"). + SaveFileDialogContext::MultipleFiles { files, expected_extension } => { + // Treat the chosen path as the folder name. Strip only the export's expected extension (e.g. ".png" for a + // PNG animation export) so that arbitrary dotted folder names like `v1.0` are preserved as-is, while a user + // who typed `MyAnim.png` still gets a `MyAnim/` folder rather than a `MyAnim.png/` folder. // The `WriteFile` handler creates parent directories if they don't exist, so the folder is materialized on first write. - let folder = path.with_extension(""); + let folder = match path.extension().and_then(|e| e.to_str()) { + Some(ext) if ext.eq_ignore_ascii_case(&expected_extension) => path.with_extension(""), + _ => path, + }; for (filename, content) in files { let file_path = folder.join(&filename); dispatcher.respond(DesktopFrontendMessage::WriteFile { path: file_path, content }); diff --git a/desktop/wrapper/src/intercept_frontend_message.rs b/desktop/wrapper/src/intercept_frontend_message.rs index d17e0bdc59..7ffaa8900c 100644 --- a/desktop/wrapper/src/intercept_frontend_message.rs +++ b/desktop/wrapper/src/intercept_frontend_message.rs @@ -71,10 +71,16 @@ pub(super) fn intercept_frontend_message(dispatcher: &mut DesktopWrapperMessageD // Materialize each frame to bytes; SVG strings are encoded as UTF-8. // Raster-needs-canvas-rasterize frames can't be encoded here without a Rust SVG rasterizer, // so fall through to the frontend zip path in that case. + // TODO: This fallback is inconsistent with the desktop folder-save flow for the rest of the animation + // export — desktop users will see a .zip download instead of a folder. Once SVG rasterization moves to + // Rust (resvg), this fallback can be removed and all frame paths can save into the chosen folder. let mut needs_frontend_rasterization = false; let mut materialized = Vec::with_capacity(frames.len()); + // Dynamic zero-pad width so files keep sorting in playback order beyond 9,999 frames. + let pad_width = frames.len().to_string().len().max(4); + let safe_base = graphite_editor::messages::frontend::utility_types::sanitize_filename_component(&name); for (index, frame) in frames.iter().enumerate() { - let filename = format!("{name}_{:04}.{extension}", index + 1); + let filename = format!("{safe_base}_{:0pad$}.{extension}", index + 1, pad = pad_width); let bytes = match frame { ExportAnimationFrame::Svg(svg) if extension == "svg" => svg.as_bytes().to_vec(), ExportAnimationFrame::Bytes(bytes) => bytes.to_vec(), @@ -100,10 +106,13 @@ pub(super) fn intercept_frontend_message(dispatcher: &mut DesktopWrapperMessageD // The dialog name is the folder the frames go into (analogous to the .zip on web). dispatcher.respond(DesktopFrontendMessage::SaveFileDialog { title: "Save Animation Frames Folder As".to_string(), - default_filename: name.clone(), + default_filename: safe_base, default_folder: folder, filters: Vec::new(), - context: SaveFileDialogContext::MultipleFiles { files: materialized }, + context: SaveFileDialogContext::MultipleFiles { + files: materialized, + expected_extension: extension, + }, }); } FrontendMessage::TriggerVisitLink { url } => { diff --git a/desktop/wrapper/src/messages.rs b/desktop/wrapper/src/messages.rs index 2e3cb82a11..26da75d832 100644 --- a/desktop/wrapper/src/messages.rs +++ b/desktop/wrapper/src/messages.rs @@ -118,11 +118,13 @@ pub enum SaveFileDialogContext { File { content: Vec, }, - /// Multiple files written into a folder whose path is the user-chosen path with any extension stripped - /// (e.g. picking `MyAnim.png` yields a `MyAnim/` folder). Each `(filename, content)` entry is written + /// Multiple files written into a folder whose path is the user-chosen path with the matching `expected_extension` + /// stripped (e.g. for a PNG animation export, picking `MyAnim.png` yields a `MyAnim/` folder, while picking `v1.0` + /// is preserved as `v1.0/` because `.0` isn't the expected extension). Each `(filename, content)` entry is written /// inside that folder, which is created if it doesn't exist. MultipleFiles { files: Vec<(String, Vec)>, + expected_extension: String, }, } diff --git a/editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs b/editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs index bbe451b503..12f464a037 100644 --- a/editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs +++ b/editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs @@ -38,10 +38,21 @@ impl Default for ExportDialogMessageHandler { } } +/// Upper bound on how many frames a single animation export will queue. Each frame is rendered then held +/// in memory until the whole batch ships to the frontend, so we cap to avoid pathological allocations from +/// large time ranges. Tuned for typical animation lengths (10k frames is ~5.5 min at 30 fps, ~2.8 min at 60 fps). +pub const ANIMATION_EXPORT_MAX_FRAMES: u32 = 10_000; + impl ExportDialogMessageHandler { fn total_frames(&self) -> u32 { + // Sanitize against non-finite values that could otherwise propagate into `Duration::from_secs_f64` and panic. + if !self.fps.is_finite() || self.fps <= 0. || !self.start_seconds.is_finite() || !self.end_seconds.is_finite() { + return 1; + } let duration = (self.end_seconds - self.start_seconds).max(0.); - ((duration * self.fps).round() as i64).max(1) as u32 + // `ceil` so a duration slightly longer than a frame multiple still gets the trailing frame. + let raw = (duration * self.fps).ceil() as i64; + raw.clamp(1, ANIMATION_EXPORT_MAX_FRAMES as i64) as u32 } } @@ -55,14 +66,19 @@ impl MessageHandler> for Exp ExportDialogMessage::ScaleFactor { factor } => self.scale_factor = factor, ExportDialogMessage::ExportBounds { bounds } => self.bounds = bounds, ExportDialogMessage::Animated { animated } => self.animated = animated, - ExportDialogMessage::Fps { fps } => self.fps = fps.max(0.001), + ExportDialogMessage::Fps { fps } => { + // Reject non-finite/non-positive values so we never feed NaN or infinity into duration math downstream. + self.fps = if fps.is_finite() && fps > 0. { fps } else { 0.001 }; + } ExportDialogMessage::StartSeconds { start } => { - self.start_seconds = start.max(0.); + self.start_seconds = if start.is_finite() { start.max(0.) } else { 0. }; if self.end_seconds < self.start_seconds { self.end_seconds = self.start_seconds; } } - ExportDialogMessage::EndSeconds { end } => self.end_seconds = end.max(self.start_seconds), + ExportDialogMessage::EndSeconds { end } => { + self.end_seconds = if end.is_finite() { end.max(self.start_seconds) } else { self.start_seconds }; + } ExportDialogMessage::Submit => { // Fall back to "All Artwork" if "Selection" was chosen but nothing is currently selected diff --git a/editor/src/messages/frontend/utility_types.rs b/editor/src/messages/frontend/utility_types.rs index cbb28641b5..0bfb1ac391 100644 --- a/editor/src/messages/frontend/utility_types.rs +++ b/editor/src/messages/frontend/utility_types.rs @@ -111,6 +111,23 @@ pub enum ExportAnimationFrame { Bytes(serde_bytes::ByteBuf), } +/// Replace characters that have special meaning in filesystem paths (or are otherwise unsafe in filenames) with `_`. +/// Used to neutralize user-controlled strings — document names, artboard names — before they become export filenames, +/// so a name like `..\evil` or `foo/bar` can't escape the export folder or create nested zip entries. +pub fn sanitize_filename_component(name: &str) -> String { + let cleaned: String = name + .chars() + .map(|c| match c { + '/' | '\\' | ':' | '*' | '?' | '<' | '>' | '|' | '"' | '\0' => '_', + c if c.is_control() => '_', + c => c, + }) + .collect(); + // Strip leading/trailing dots and whitespace; Windows treats trailing `.` / ` ` as hidden and `..` is the parent dir. + let trimmed = cleaned.trim().trim_matches('.').trim(); + if trimmed.is_empty() { "Untitled".to_string() } else { trimmed.to_string() } +} + #[cfg_attr(feature = "wasm", derive(tsify::Tsify), tsify(large_number_types_as_bigints))] #[derive(Clone, Debug, Default, Eq, PartialEq, serde::Serialize, serde::Deserialize)] pub struct EyedropperPreviewImage { diff --git a/editor/src/node_graph_executor.rs b/editor/src/node_graph_executor.rs index 5b59425655..042f49b02b 100644 --- a/editor/src/node_graph_executor.rs +++ b/editor/src/node_graph_executor.rs @@ -303,7 +303,17 @@ impl NodeGraphExecutor { .map_err(|e| e.to_string())?; if let Some(animation) = export_config.animation { - // Allocate an export ID and accumulator, then queue one execution per frame + // Defense-in-depth: the dialog already validates these, but reject non-finite/non-positive values here + // too so a corrupt message can't reach `Duration::from_secs_f64` (which panics on NaN/negative/huge). + if !animation.fps.is_finite() || animation.fps <= 0. || !animation.start_seconds.is_finite() { + return Err("Animation export rejected: fps and start time must be finite, with fps > 0".to_string()); + } + + // Allocate an export ID and accumulator, then queue one execution per frame. + // TODO: All encoded frames are held in `pending_animation_exports` until the last frame arrives, so peak + // memory grows with `total_frames * encoded_frame_size`. For long/high-resolution animations, this could + // exhaust memory. A bounded cap is enforced upstream in the export dialog (ANIMATION_EXPORT_MAX_FRAMES), + // but a true fix would stream frames out incrementally instead of accumulating. let export_id = self.next_animation_export_id; self.next_animation_export_id = self.next_animation_export_id.wrapping_add(1); @@ -325,7 +335,10 @@ impl NodeGraphExecutor { for frame_index in 0..animation.total_frames { let frame_seconds = animation.frame_time_seconds(frame_index); - let animation_time = Duration::from_secs_f64(frame_seconds.max(0.)); + // `Duration::from_secs_f64` panics on negative/NaN/huge values; clamp defensively (we've already + // validated `fps`/`start_seconds` above, but a far-future `start_seconds` could still overflow). + let safe_seconds = if frame_seconds.is_finite() { frame_seconds.clamp(0., 1e9) } else { 0. }; + let animation_time = Duration::from_secs_f64(safe_seconds); let timing = TimingInformation { time: frame_seconds, animation_time }; let frame_render_config = RenderConfig { time: timing, ..base_render_config }; @@ -382,6 +395,24 @@ impl NodeGraphExecutor { document.network_interface.update_click_targets(HashMap::new()); document.network_interface.update_outlines(HashMap::new()); document.network_interface.update_vector_modify(HashMap::new()); + + // If this failure belongs to an animation export, drop its accumulator so the partially + // rendered frames don't stay pinned in memory. Subsequent failed frames for the same + // export are then silently ignored by `process_animation_frame`. + // TODO: An export can also leak if it's interrupted by something *outside* this error + // path — e.g. the document is closed mid-export. A proper fix would route a + // cancellation through `pending_animation_exports`. Tracked separately. + let leaked_export_id = self + .futures + .iter() + .find(|(fid, _)| *fid == execution_id) + .and_then(|(_, ctx)| ctx.export_config.as_ref()) + .and_then(|cfg| cfg.animation_frame) + .map(|af| af.export_id); + if let Some(export_id) = leaked_export_id { + self.pending_animation_exports.remove(&export_id); + } + return Err(format!("Node graph evaluation failed:\n{e}")); } }; @@ -609,11 +640,19 @@ impl NodeGraphExecutor { TaggedValue::RenderOutput(RenderOutput { data: RenderOutputType::Buffer { data, width, height }, .. - }) if file_type != FileType::Svg => { - let encoded = encode_raster_buffer(file_type, data, width, height)?; - ExportAnimationFrame::Bytes(serde_bytes::ByteBuf::from(encoded)) + }) if file_type != FileType::Svg => match encode_raster_buffer(file_type, data, width, height) { + Ok(encoded) => ExportAnimationFrame::Bytes(serde_bytes::ByteBuf::from(encoded)), + Err(err) => { + // Drop the partial accumulator so its already-received frames don't leak. + self.pending_animation_exports.remove(&animation_frame.export_id); + return Err(err); + } + }, + other => { + // Drop the partial accumulator so its already-received frames don't leak. + self.pending_animation_exports.remove(&animation_frame.export_id); + return Err(format!("Incorrect render type for animation frame ({file_type:?}, {other})")); } - other => return Err(format!("Incorrect render type for animation frame ({file_type:?}, {other})")), }; let Some(accumulator) = self.pending_animation_exports.get_mut(&animation_frame.export_id) else { @@ -635,9 +674,11 @@ impl NodeGraphExecutor { // All frames received: drain the accumulator and emit a single message let accumulator = self.pending_animation_exports.remove(&animation_frame.export_id).expect("Accumulator was present"); - let base_name = match (accumulator.artboard_name, accumulator.artboard_count) { - (Some(artboard_name), count) if count > 1 => format!("{} - {}", accumulator.name, artboard_name), - _ => accumulator.name, + // Sanitize before the name reaches filesystem joins or zip entry names downstream. + let safe_doc_name = crate::messages::frontend::utility_types::sanitize_filename_component(&accumulator.name); + let base_name = match (accumulator.artboard_name.as_deref(), accumulator.artboard_count) { + (Some(artboard_name), count) if count > 1 => format!("{safe_doc_name} - {}", crate::messages::frontend::utility_types::sanitize_filename_component(artboard_name)), + _ => safe_doc_name, }; let frames: Vec<_> = accumulator .frames diff --git a/frontend/src/stores/portfolio.ts b/frontend/src/stores/portfolio.ts index 2d38a01a1c..afc96e8dce 100644 --- a/frontend/src/stores/portfolio.ts +++ b/frontend/src/stores/portfolio.ts @@ -140,28 +140,29 @@ export function createPortfolioStore(subscriptions: SubscriptionsRouter, editor: const backgroundColor = mime.endsWith("jpeg") ? "white" : undefined; const padWidth = Math.max(4, String(frames.length).length); - // Materialize each frame to bytes, rasterizing SVG via canvas when the destination format is raster + // Materialize each frame to bytes, rasterizing SVG via canvas when the destination format is raster. + // Any per-frame failure aborts the export rather than silently dropping frames, so the user never gets + // a zip with mismatched indices vs. the requested playback range. const entries: [string, Uint8Array][] = []; - for (let i = 0; i < frames.length; i++) { - const frame = frames[i]; - const filename = `${name}_${String(i + 1).padStart(padWidth, "0")}.${extension}`; + try { + for (let i = 0; i < frames.length; i++) { + const frame = frames[i]; + const filename = `${name}_${String(i + 1).padStart(padWidth, "0")}.${extension}`; - let bytes: Uint8Array; - if ("Bytes" in frame) { - bytes = frame.Bytes; - } else if (isRaster) { - let blob: Blob; - try { - blob = await rasterizeSVG(frame.Svg, size[0], size[1], mime, backgroundColor); - } catch { - // Skip frames that fail to rasterize (e.g. zero-sized) rather than aborting the whole export - continue; + let bytes: Uint8Array; + if ("Bytes" in frame) { + bytes = frame.Bytes; + } else if (isRaster) { + const blob = await rasterizeSVG(frame.Svg, size[0], size[1], mime, backgroundColor); + bytes = new Uint8Array(await blob.arrayBuffer()); + } else { + bytes = new TextEncoder().encode(frame.Svg); } - bytes = new Uint8Array(await blob.arrayBuffer()); - } else { - bytes = new TextEncoder().encode(frame.Svg); + entries.push([filename, bytes]); } - entries.push([filename, bytes]); + } catch (error) { + editor.errorDialog("Animation export failed", error instanceof Error ? error.message : String(error)); + return; } if (entries.length === 0) return; diff --git a/frontend/src/utility-functions/rasterization.ts b/frontend/src/utility-functions/rasterization.ts index 8a0807ac2d..4c96bd0f33 100644 --- a/frontend/src/utility-functions/rasterization.ts +++ b/frontend/src/utility-functions/rasterization.ts @@ -17,18 +17,23 @@ export async function rasterizeSVGCanvas(svg: string, width: number, height: num const svgBlob = new Blob([svg], { type: "image/svg+xml;charset=utf-8" }); const url = URL.createObjectURL(svgBlob); - // Load the Image from the URL and wait until it's done + // Load the Image from the URL and wait until it's done. Reject on error so callers don't hang forever + // if the SVG fails to decode (malformed markup, zero-sized viewport with no width/height attrs, etc.). const image = new Image(); image.src = url; - await new Promise((resolve) => { - image.onload = () => resolve(); - }); + try { + await new Promise((resolve, reject) => { + image.onload = () => resolve(); + image.onerror = () => reject(new Error("Failed to decode SVG for rasterization")); + }); - // Draw our SVG to the canvas - context?.drawImage(image, 0, 0, width, height); - - // Clean up the SVG blob URL (once the URL is revoked, the SVG blob data itself is garbage collected after `svgBlob` goes out of scope) - URL.revokeObjectURL(url); + // Draw our SVG to the canvas + context?.drawImage(image, 0, 0, width, height); + } finally { + // Always clean up the SVG blob URL (once the URL is revoked, the SVG blob data itself is garbage + // collected after `svgBlob` goes out of scope), even if loading or drawing threw. + URL.revokeObjectURL(url); + } return canvas; } diff --git a/frontend/wrapper/src/editor_wrapper.rs b/frontend/wrapper/src/editor_wrapper.rs index 9099d0869c..1904e1e38d 100644 --- a/frontend/wrapper/src/editor_wrapper.rs +++ b/frontend/wrapper/src/editor_wrapper.rs @@ -996,10 +996,13 @@ pub fn create_zip_from_files(entries: js_sys::Array) -> Result, JsValue> .ok_or_else(|| JsValue::from_str("createZipFromFiles: each entry must be a [filename, bytes] array"))?; let filename = pair.get(0).as_string().ok_or_else(|| JsValue::from_str("createZipFromFiles: filename must be a string"))?; + // Defense in depth: callers should already sanitize, but if a stray path separator leaks through, + // replace it here so the entry can't become a nested path inside the archive. + let safe_filename: String = filename.chars().map(|c| if c == '/' || c == '\\' { '_' } else { c }).collect(); let bytes = js_sys::Uint8Array::new(&pair.get(1)).to_vec(); writer - .start_file(filename, options) + .start_file(safe_filename, options) .map_err(|e| JsValue::from_str(&format!("createZipFromFiles: start_file failed: {e}")))?; writer.write_all(&bytes).map_err(|e| JsValue::from_str(&format!("createZipFromFiles: write_all failed: {e}")))?; }