From ac63c701e6f885ee147e3fce1d64a43871dc8fbc Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Sun, 17 May 2026 16:37:31 +0800 Subject: [PATCH] =?UTF-8?q?fix(figma):=20codex=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20hardening=20+=20ZIP=20entrypoint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the first codex review of the binary `.fig` parser. BLOCKs: - kiwi: 64-bit varint now uses Kiwi's terminal-byte rule (eight 7-bit groups then a final full-8-bit byte) — the old `& 127` mask on every byte corrupted u64 values above 2^56. - kiwi: an invalid schema definition kind (> 2) is now rejected instead of silently treated as a message. - kiwi: array decode rejects a length exceeding the buffer size — guards against a hostile zero-byte-element array spinning the decode loop billions of times. - zip_reader: aggregate 2 GiB decompression budget + 10k entry cap on top of the existing per-entry limit (zip-bomb defence). CONCERNs: - detect_kind now recognises the `PK\x03\x04` ZIP magic as Binary — the common Figma export form (`canvas.fig` + `images/` in a ZIP) was being rejected before container.rs could unwrap it. - resolve_style_references now also resolves style refs inside instance `symbolData.symbolOverrides` entries. - kiwi: enum field type codes are no longer resolved (unused; kiwi writes 0) so a stray code can't reject a valid schema. Plus a zip-wrapped end-to-end test + the misleading zstd test rename. op-figma 84 tests green (+1); clean build. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- crates/op-figma/src/binary_e2e_tests.rs | 47 +++++++++++++++++++++++++ crates/op-figma/src/container.rs | 8 ++--- crates/op-figma/src/kiwi.rs | 32 +++++++++++++---- crates/op-figma/src/lib.rs | 9 ++++- crates/op-figma/src/node_mapper.rs | 11 ++++++ crates/op-figma/src/zip_reader.rs | 12 +++++++ 6 files changed, 108 insertions(+), 11 deletions(-) diff --git a/crates/op-figma/src/binary_e2e_tests.rs b/crates/op-figma/src/binary_e2e_tests.rs index 58c2df9af..933c08eea 100644 --- a/crates/op-figma/src/binary_e2e_tests.rs +++ b/crates/op-figma/src/binary_e2e_tests.rs @@ -253,6 +253,53 @@ fn parses_a_full_binary_fig_into_a_document() { } } +/// Wrap a payload as `canvas.fig` in a minimal stored (uncompressed) +/// ZIP archive — the common Figma export form. +fn wrap_in_zip(canvas: &[u8]) -> Vec { + const LFH: u32 = 0x0403_4b50; + const CDFH: u32 = 0x0201_4b50; + const EOCD: u32 = 0x0605_4b50; + let name = b"canvas.fig"; + let mut z = Vec::new(); + let local_off = 0u32; + z.extend_from_slice(&LFH.to_le_bytes()); + z.extend_from_slice(&[20, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); + z.extend_from_slice(&(canvas.len() as u32).to_le_bytes()); + z.extend_from_slice(&(canvas.len() as u32).to_le_bytes()); + z.extend_from_slice(&(name.len() as u16).to_le_bytes()); + z.extend_from_slice(&[0, 0]); + z.extend_from_slice(name); + z.extend_from_slice(canvas); + let cd_off = z.len() as u32; + z.extend_from_slice(&CDFH.to_le_bytes()); + z.extend_from_slice(&[20, 0, 20, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); + z.extend_from_slice(&(canvas.len() as u32).to_le_bytes()); + z.extend_from_slice(&(canvas.len() as u32).to_le_bytes()); + z.extend_from_slice(&(name.len() as u16).to_le_bytes()); + z.extend_from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); + z.extend_from_slice(&local_off.to_le_bytes()); + z.extend_from_slice(name); + let cd_size = z.len() as u32 - cd_off; + z.extend_from_slice(&EOCD.to_le_bytes()); + z.extend_from_slice(&[0, 0, 0, 0, 1, 0, 1, 0]); + z.extend_from_slice(&cd_size.to_le_bytes()); + z.extend_from_slice(&cd_off.to_le_bytes()); + z.extend_from_slice(&[0, 0]); + z +} + +#[test] +fn parses_a_zip_wrapped_fig() { + let bare = build_fig(&build_schema(), &build_data()); + let zipped = wrap_in_zip(&bare); + // The ZIP form must be recognised + routed through the parser. + let import = parse_fig_binary(&zipped, "Zipped", FigLayoutMode::OpenPencil) + .expect("zip-wrapped .fig parses"); + let pages = import.document.pages.expect("has pages"); + assert_eq!(pages[0].name, "Page 1"); + assert_eq!(pages[0].children.len(), 1); +} + #[test] fn document_serializes_to_canonical_json() { let fig = build_fig(&build_schema(), &build_data()); diff --git a/crates/op-figma/src/container.rs b/crates/op-figma/src/container.rs index e8a47ceaf..ce79d6aab 100644 --- a/crates/op-figma/src/container.rs +++ b/crates/op-figma/src/container.rs @@ -193,10 +193,10 @@ mod tests { } #[test] - fn zstd_chunk_round_trips() { - // A zstd-framed chunk: build one with ruzstd's sibling encoder - // is not available, so assert the raw-deflate path here and - // trust zstd via the magic-detection unit below. + fn deflate_chunk_round_trips() { + // Chunks are deflate-compressed here (no zstd encoder in the + // dev-deps); the zstd decode path is covered by `ruzstd`'s own + // test suite and the magic-detection branch in decompress_chunk. let fig = make_fig_container(&[b"only-one"]); let result = fig_to_binary_parts(&fig).expect("parses"); assert_eq!(result.parts.len(), 1); diff --git a/crates/op-figma/src/kiwi.rs b/crates/op-figma/src/kiwi.rs index a67690fb2..fc76a038d 100644 --- a/crates/op-figma/src/kiwi.rs +++ b/crates/op-figma/src/kiwi.rs @@ -90,15 +90,22 @@ impl<'a> ByteBuffer<'a> { Ok(((v >> 1) as i32) ^ -((v & 1) as i32)) } - /// LEB128 unsigned 64-bit varint (max 10 bytes). + /// Kiwi 64-bit unsigned varint: eight 7-bit groups (56 bits) then a + /// final byte contributing all 8 bits — matching `kiwi-schema`. pub fn read_var_uint64(&mut self) -> Result { let mut value: u64 = 0; let mut shift: u32 = 0; loop { let byte = self.read_byte()?; - value |= ((byte & 127) as u64).wrapping_shl(shift); - shift += 7; - if byte & 128 == 0 || shift >= 70 { + if shift < 56 { + value |= ((byte & 127) as u64).wrapping_shl(shift); + shift += 7; + if byte & 128 == 0 { + break; + } + } else { + // Final byte — all 8 bits, no continuation. + value |= (byte as u64).wrapping_shl(shift); break; } } @@ -208,7 +215,8 @@ pub fn decode_binary_schema(bytes: &[u8]) -> Result { let kind = match bb.read_byte()? { 0 => DefKind::Enum, 1 => DefKind::Struct, - _ => DefKind::Message, + 2 => DefKind::Message, + other => return Err(KiwiError::BadType(format!("definition kind {other}"))), }; let field_count = bb.read_var_uint()?; let mut fields = Vec::with_capacity(field_count as usize); @@ -227,7 +235,12 @@ pub fn decode_binary_schema(bytes: &[u8]) -> Result { for (name, kind, fields) in raw { let mut resolved = Vec::with_capacity(fields.len()); for (field_name, type_code, is_array, value) in fields { - let type_name = if type_code < 0 { + // Enum fields carry only a name + ordinal; their encoded + // type code is unused (kiwi writes 0). Skip resolution so + // a stray code never rejects an otherwise-valid schema. + let type_name = if kind == DefKind::Enum { + String::new() + } else if type_code < 0 { let idx = !type_code; NATIVE_TYPES .get(idx as usize) @@ -413,6 +426,13 @@ impl<'a, 'b> Decoder<'a, 'b> { return Ok(FigValue::Bytes(self.bb.read_byte_array()?)); } let n = self.bb.read_var_uint()?; + // A real array cannot have more elements than the buffer + // has bytes — rejects a hostile length that would spin the + // decode loop billions of times (e.g. an array of a + // zero-byte struct type). + if n as usize > self.bb.data.len() { + return Err(KiwiError::OutOfBounds); + } let mut items = Vec::with_capacity(n.min(1 << 20) as usize); for _ in 0..n { items.push(self.decode_type(&field.type_name, depth + 1)?); diff --git a/crates/op-figma/src/lib.rs b/crates/op-figma/src/lib.rs index 4ed96ace9..dc5fc5441 100644 --- a/crates/op-figma/src/lib.rs +++ b/crates/op-figma/src/lib.rs @@ -56,10 +56,17 @@ pub enum FigFileKind { /// applies. Returns `Unknown` for empty input. pub fn detect_kind(bytes: &[u8]) -> FigFileKind { // Figma's binary container starts with the literal "fig-kiwi" — a - // 8-byte ASCII magic followed by the Zstd-compressed payload. + // 8-byte ASCII magic followed by the chunk payload. if bytes.len() >= 8 && &bytes[0..8] == b"fig-kiwi" { return FigFileKind::Binary; } + // The common export form wraps the container in a ZIP archive + // (`canvas.fig` + `images/*`); the ZIP local-file-header magic + // `PK\x03\x04` marks it. A non-fig ZIP simply fails later with a + // "no canvas.fig" error. + if bytes.len() >= 4 && bytes[0..4] == [0x50, 0x4b, 0x03, 0x04] { + return FigFileKind::Binary; + } // Clipboard JSON starts with `{` and contains the FIGMA_DOCUMENT // marker within the first KB. Cheap prefix check first. if let Some(first_non_ws) = bytes.iter().find(|b| !b.is_ascii_whitespace()) { diff --git a/crates/op-figma/src/node_mapper.rs b/crates/op-figma/src/node_mapper.rs index 868f540f4..71e3d4339 100644 --- a/crates/op-figma/src/node_mapper.rs +++ b/crates/op-figma/src/node_mapper.rs @@ -75,6 +75,17 @@ pub fn resolve_style_references(node_changes: &mut [FigValue]) { } for nc in node_changes.iter_mut() { resolve_on_node(nc, &style_map); + // Resolve style refs inside instance symbol overrides too. + if let Some(mut symbol_data) = nc.get("symbolData").cloned() { + if let Some(overrides) = symbol_data.get_array("symbolOverrides") { + let mut resolved: Vec = overrides.to_vec(); + for ov in &mut resolved { + resolve_on_node(ov, &style_map); + } + symbol_data.set("symbolOverrides", FigValue::Array(resolved)); + nc.set("symbolData", symbol_data); + } + } } } diff --git a/crates/op-figma/src/zip_reader.rs b/crates/op-figma/src/zip_reader.rs index 554a92ea0..b3e72c2be 100644 --- a/crates/op-figma/src/zip_reader.rs +++ b/crates/op-figma/src/zip_reader.rs @@ -38,6 +38,10 @@ const CDFH_SIG: u32 = 0x0201_4b50; const LFH_SIG: u32 = 0x0403_4b50; /// Per-entry decompressed ceiling — zip-bomb defence (512 MiB). const MAX_ENTRY_SIZE: usize = 512 * 1024 * 1024; +/// Aggregate decompressed ceiling across the whole archive (2 GiB). +const MAX_TOTAL_SIZE: usize = 2 * 1024 * 1024 * 1024; +/// Cap on the number of archive entries (many-tiny-files zip bomb). +const MAX_ENTRIES: usize = 10_000; fn u16_le(b: &[u8], off: usize) -> Option { b.get(off..off + 2) @@ -72,8 +76,12 @@ pub fn read_zip(buf: &[u8]) -> Result, ZipError> { let eocd = find_eocd(buf).ok_or(ZipError::NotZip)?; let total_entries = u16_le(buf, eocd + 10).ok_or(ZipError::Truncated)? as usize; let cd_offset = u32_le(buf, eocd + 16).ok_or(ZipError::Truncated)? as usize; + if total_entries > MAX_ENTRIES { + return Err(ZipError::TooLarge); + } let mut entries = Vec::with_capacity(total_entries); + let mut total_size = 0usize; let mut cursor = cd_offset; for _ in 0..total_entries { if u32_le(buf, cursor) != Some(CDFH_SIG) { @@ -93,6 +101,10 @@ pub fn read_zip(buf: &[u8]) -> Result, ZipError> { let name = String::from_utf8_lossy(name_bytes).into_owned(); let data = read_entry_data(buf, local_off, method, comp_size, uncomp_size)?; + total_size = total_size.saturating_add(data.len()); + if total_size > MAX_TOTAL_SIZE { + return Err(ZipError::TooLarge); + } entries.push(ZipEntry { name, data }); cursor += 46 + name_len + extra_len + comment_len;