Code review fixes

Author: Keavon

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 });

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 } => {

desktop/wrapper/src/messages.rs

@@ -118,11 +118,13 @@ pub enum SaveFileDialogContext {
 	File {
 		content: Vec<u8>,
 	},
-	/// 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<u8>)>,
+		expected_extension: String,
 	},
 }
 

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<ExportDialogMessage, ExportDialogMessageContext<'_>> 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

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 {

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

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}`;
-
-			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;
+		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) {
+					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;

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<void>((resolve) => {
-		image.onload = () => resolve();
-	});
-
-	// 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);
+	try {
+		await new Promise<void>((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);
+	} 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;
 }

frontend/wrapper/src/editor_wrapper.rs

@@ -996,10 +996,13 @@ pub fn create_zip_from_files(entries: js_sys::Array) -> Result<Vec<u8>, 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}")))?;
 	}