From 90f2498bcf9d17d361f28f8910074652f62f320a Mon Sep 17 00:00:00 2001 From: Danila Poyarkov Date: Fri, 14 Aug 2026 20:38:46 +0300 Subject: [PATCH] fix(collab): validate synchronized node data (#517) * fix(collab): validate synchronized node data - Exclude renderer-only text picture caches from Yjs payloads - Normalize source metadata and geometry at the remote boundary - Cover malformed payloads and typed geometry round trips * fix(collab): reject malformed geometry fills * fix(collab): validate nested fill metadata * test(collab): preserve valid nested fill metadata --- CHANGELOG.md | 1 + src/app/collab/node-codec.ts | 193 +++++++++++++++++++++++++++ src/app/collab/yjs-sync.ts | 30 +---- tests/engine/collab/yjs-sync.test.ts | 112 ++++++++++++++-- 4 files changed, 301 insertions(+), 35 deletions(-) create mode 100644 src/app/collab/node-codec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 5de71abb9..6a798dc28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ ### Fixed +- Harden collaboration node synchronization against malformed remote source metadata and geometry while excluding derived text-renderer caches. - Transfer native `.fig` exports over binary Tauri IPC instead of JSON byte arrays, preventing large desktop saves from being truncated or exhausting WebView memory. (#484) - Keep unsaved source-less documents recoverable after their editor tab is closed, matching Figma's retained offline-change behavior. - Decode zstd-compressed FIG containers, reject invalid compressed payloads, and preserve exact fixture byte ranges. (#397) diff --git a/src/app/collab/node-codec.ts b/src/app/collab/node-codec.ts new file mode 100644 index 000000000..a1c8559b6 --- /dev/null +++ b/src/app/collab/node-codec.ts @@ -0,0 +1,193 @@ +import type { + Fill, + FillType, + GeometryPath, + SceneNode, + SourceMetadata +} from '@open-pencil/scene-graph' +import { copyFills } from '@open-pencil/scene-graph/copy' +import { createDefaultSourceMetadata } from '@open-pencil/scene-graph/node-defaults' +import type { Matrix, Vector } from '@open-pencil/scene-graph/primitives' + +const DERIVED_NODE_FIELDS = new Set(['textPicture']) +const FILL_TYPES = new Set([ + 'SOLID', + 'GRADIENT_LINEAR', + 'GRADIENT_RADIAL', + 'GRADIENT_ANGULAR', + 'GRADIENT_DIAMOND', + 'IMAGE', + 'VIDEO', + 'PATTERN', + 'NOISE', + 'CUSTOM' +]) + +type YjsNodeLike = { + entries(): IterableIterator<[string, unknown]> +} + +export function encodeNodeForYjs(node: SceneNode): Record { + const encoded: Record = {} + for (const [key, value] of Object.entries(node)) { + if (DERIVED_NODE_FIELDS.has(key as keyof SceneNode)) continue + encoded[key] = structuredClone(value) + } + return encoded +} + +export function syncEncodedNodeToYMap( + node: SceneNode, + ynode: { delete(key: string): void; set(key: string, value: unknown): void } +): void { + for (const key of DERIVED_NODE_FIELDS) ynode.delete(key) + for (const [key, value] of Object.entries(encodeNodeForYjs(node))) ynode.set(key, value) +} + +export function decodeNodeFromYjs(ynode: YjsNodeLike): Partial { + const props: Record = {} + for (const [key, value] of ynode.entries()) { + if (DERIVED_NODE_FIELDS.has(key as keyof SceneNode)) continue + props[key] = structuredClone(value) + } + + props.source = normalizeSourceMetadata(props.source) + if ('fillGeometry' in props) props.fillGeometry = normalizeGeometryPaths(props.fillGeometry) + if ('strokeGeometry' in props) props.strokeGeometry = normalizeGeometryPaths(props.strokeGeometry) + props.textPicture = null + return props as Partial +} + +export function normalizeSourceMetadata(source: unknown): SourceMetadata { + const defaults = createDefaultSourceMetadata() + if (!isRecord(source)) return defaults + + const fig = isRecord(source.fig) ? source.fig : {} + return { + format: source.format === 'fig' ? 'fig' : null, + id: stringOrNull(source.id), + orderKey: stringOrNull(source.orderKey), + editedFields: stringArray(source.editedFields), + fig: { + rawSize: normalizeVector(fig.rawSize), + rawTransform: normalizeMatrix(fig.rawTransform), + rawNodeFields: isRecord(fig.rawNodeFields) ? structuredClone(fig.rawNodeFields) : {}, + layout: isRecord(fig.layout) + ? (structuredClone(fig.layout) as SourceMetadata['fig']['layout']) + : null, + symbolOverrides: arrayOrEmpty(fig.symbolOverrides), + componentPropAssignments: arrayOrEmpty(fig.componentPropAssignments), + derivedSymbolData: arrayOrEmpty(fig.derivedSymbolData), + derivedSymbolDataLayoutVersion: numberOrNull(fig.derivedSymbolDataLayoutVersion), + uniformScaleFactor: numberOrNull(fig.uniformScaleFactor) + } + } +} + +function normalizeGeometryPaths(value: unknown): GeometryPath[] { + if (!Array.isArray(value)) return [] + const paths: GeometryPath[] = [] + for (const item of value) { + if (!isRecord(item) || !(item.commandsBlob instanceof Uint8Array)) continue + const path: GeometryPath = { + windingRule: item.windingRule === 'EVENODD' ? 'EVENODD' : 'NONZERO', + commandsBlob: new Uint8Array(item.commandsBlob) + } + const fills = normalizeFills(item.fills) + if (fills.length > 0) path.fills = copyFills(fills) + if (typeof item.fillStyleId === 'string') path.fillStyleId = item.fillStyleId + paths.push(path) + } + return paths +} + +function normalizeVector(value: unknown): Vector | null { + if (!isRecord(value) || !isFiniteNumber(value.x) || !isFiniteNumber(value.y)) return null + return { x: value.x, y: value.y } +} + +function normalizeMatrix(value: unknown): Matrix | null { + if (!isRecord(value)) return null + const entries = [value.m00, value.m01, value.m02, value.m10, value.m11, value.m12] + if (!entries.every((item) => isFiniteNumber(item))) return null + return { + m00: value.m00 as number, + m01: value.m01 as number, + m02: value.m02 as number, + m10: value.m10 as number, + m11: value.m11 as number, + m12: value.m12 as number + } +} + +function normalizeFills(value: unknown): Fill[] { + return Array.isArray(value) ? value.filter(isFill) : [] +} + +function isFill(value: unknown): value is Fill { + return ( + isRecord(value) && + typeof value.type === 'string' && + FILL_TYPES.has(value.type as FillType) && + isColor(value.color) && + isFiniteNumber(value.opacity) && + typeof value.visible === 'boolean' && + isOptionalGradientStops(value.gradientStops) && + isOptionalMatrix(value.gradientTransform) && + isOptionalMatrix(value.imageTransform) && + isOptionalVector(value.patternSpacing) && + isOptionalVector(value.noiseSize) + ) +} + +function isOptionalGradientStops(value: unknown): boolean { + return ( + value === undefined || + (Array.isArray(value) && + value.every((stop) => isRecord(stop) && isFiniteNumber(stop.position) && isColor(stop.color))) + ) +} + +function isOptionalMatrix(value: unknown): boolean { + return value === undefined || normalizeMatrix(value) !== null +} + +function isOptionalVector(value: unknown): boolean { + return value === undefined || normalizeVector(value) !== null +} + +function isColor(value: unknown): boolean { + return ( + isRecord(value) && + isFiniteNumber(value.r) && + isFiniteNumber(value.g) && + isFiniteNumber(value.b) && + isFiniteNumber(value.a) + ) +} + +function isFiniteNumber(value: unknown): value is number { + return typeof value === 'number' && Number.isFinite(value) +} + +function arrayOrEmpty(value: unknown): unknown[] { + return Array.isArray(value) ? structuredClone(value) : [] +} + +function stringArray(value: unknown): string[] { + return Array.isArray(value) + ? value.filter((item): item is string => typeof item === 'string') + : [] +} + +function stringOrNull(value: unknown): string | null { + return typeof value === 'string' ? value : null +} + +function numberOrNull(value: unknown): number | null { + return typeof value === 'number' && Number.isFinite(value) ? value : null +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} diff --git a/src/app/collab/yjs-sync.ts b/src/app/collab/yjs-sync.ts index 805969bd3..83f022fe7 100644 --- a/src/app/collab/yjs-sync.ts +++ b/src/app/collab/yjs-sync.ts @@ -1,7 +1,6 @@ import * as Y from 'yjs' -import type { SceneNode } from '@open-pencil/scene-graph' - +import { decodeNodeFromYjs, syncEncodedNodeToYMap } from '@/app/collab/node-codec' import type { EditorStore } from '@/app/editor/active-store' type YNodes = Y.Map> @@ -37,21 +36,6 @@ function logCollabSyncError(context: string, error: unknown) { console.error(`[Collab] ${context}:`, error) } -// Clone across the graph/Yjs boundary to avoid shared mutable nested data. -export function syncNodePropsToYMap(node: SceneNode, ynode: Y.Map) { - for (const [key, value] of Object.entries(node)) { - ynode.set(key, structuredClone(value)) - } -} - -export function yNodeToProps(ynode: Y.Map): Record { - const props: Record = {} - for (const [key, value] of ynode.entries()) { - props[key] = structuredClone(value) - } - return props -} - export function bindCollabGraphEvents({ store, getYdoc, @@ -156,7 +140,7 @@ export function createYjsGraphSync({ ynode = new Y.Map() ynodes.set(nodeId, ynode) } - syncNodePropsToYMap(node, ynode) + syncEncodedNodeToYMap(node, ynode) if (localYimages) { for (const fill of node.fills) { @@ -189,7 +173,7 @@ export function createYjsGraphSync({ ynode = new Y.Map() ynodes.set(node.id, ynode) } - syncNodePropsToYMap(node, ynode) + syncEncodedNodeToYMap(node, ynode) } }) if (localYimages) { @@ -244,20 +228,20 @@ export function createYjsGraphSync({ function applyYnodeToGraph(nodeId: string, ynode: Y.Map) { const store = getStore() const existing = store.graph.getNode(nodeId) - const props = yNodeToProps(ynode) + const props = decodeNodeFromYjs(ynode) const parentId = typeof props.parentId === 'string' ? props.parentId : null if (existing) { - store.graph.updateNode(nodeId, props as Partial) + store.graph.updateNode(nodeId, props) if (parentId === null) store.graph.rootId = nodeId ensureCurrentPageExists(store) return } - const type = props.type as SceneNode['type'] | undefined + const type = props.type if (!type) return // Parent childIds may arrive before or after the child node. - store.graph.createNodeWithId(nodeId, type, parentId, props as Partial) + store.graph.createNodeWithId(nodeId, type, parentId, props) if (parentId === null) store.graph.rootId = nodeId ensureCurrentPageExists(store) } diff --git a/tests/engine/collab/yjs-sync.test.ts b/tests/engine/collab/yjs-sync.test.ts index 7036c3d55..4041eb08f 100644 --- a/tests/engine/collab/yjs-sync.test.ts +++ b/tests/engine/collab/yjs-sync.test.ts @@ -5,13 +5,10 @@ import * as Y from 'yjs' import type { Fill, GeometryPath, SceneNode } from '@open-pencil/scene-graph' import { SceneGraph } from '@open-pencil/scene-graph' import { nodeVisualBounds } from '@open-pencil/scene-graph/geometry' +import { createDefaultSourceMetadata } from '@open-pencil/scene-graph/node-defaults' -import { - createYjsGraphSync, - registerYjsObservers, - syncNodePropsToYMap, - yNodeToProps -} from '@/app/collab/yjs-sync' +import { decodeNodeFromYjs, syncEncodedNodeToYMap } from '@/app/collab/node-codec' +import { createYjsGraphSync, registerYjsObservers } from '@/app/collab/yjs-sync' import { createEditorStore } from '@/app/editor/session' import { expectDefined, getNodeOrThrow } from '#tests/helpers/assert' @@ -19,7 +16,7 @@ import { connectYDocs } from '#tests/helpers/yjs' // Test copy of the private apply path. function applyYnodeToGraph(peer: SceneGraph, nodeId: string, ynode: Y.Map) { - const props = yNodeToProps(ynode) + const props = decodeNodeFromYjs(ynode) if (peer.getNode(nodeId)) { peer.updateNode(nodeId, props as Partial) return @@ -38,7 +35,7 @@ function seedHostIntoYjs(host: SceneGraph): Y.Map> { for (const node of host.getAllNodes()) { const ynode = new Y.Map() ynodes.set(node.id, ynode) - syncNodePropsToYMap(node, ynode) + syncEncodedNodeToYMap(node, ynode) } }) return ynodes @@ -149,6 +146,97 @@ describe('collab yjs-sync', () => { expect(page.childIds).toContain('remote-id') }) + test('excludes derived text pictures from collaboration payloads', () => { + const graph = new SceneGraph() + const page = firstPage(graph) + const text = graph.createNode('TEXT', page.id, { + text: 'Shared text', + textPicture: new Uint8Array([4, 5, 6]) + }) + const doc = new Y.Doc() + const ynode = new Y.Map() + doc.getMap>('nodes').set(text.id, ynode) + + syncEncodedNodeToYMap(text, ynode) + + expect(ynode.has('textPicture')).toBe(false) + ynode.set('textPicture', new Uint8Array([9])) + expect(decodeNodeFromYjs(ynode).textPicture).toBeNull() + }) + + test('normalizes malformed source metadata and geometry at the remote boundary', () => { + const doc = new Y.Doc() + const ynode = new Y.Map() + doc.getMap>('nodes').set('remote', ynode) + ynode.set('source', { format: 'fig', fig: { rawNodeFields: 'invalid' } }) + ynode.set('fillGeometry', [ + { + windingRule: 'EVENODD', + commandsBlob: new Uint8Array([0]), + fills: [ + null, + 'invalid', + { + type: 'GRADIENT_LINEAR', + color: { r: 0, g: 0, b: 0, a: 1 }, + opacity: 1, + visible: true, + gradientStops: 'invalid' + }, + { + type: 'NOISE', + color: { r: 0, g: 0, b: 0, a: 1 }, + opacity: 1, + visible: true, + noiseSize: 'invalid' + }, + { + type: 'GRADIENT_LINEAR', + color: { r: 0, g: 0, b: 0, a: 1 }, + opacity: 0.8, + visible: true, + gradientStops: [ + { color: { r: 1, g: 0, b: 0, a: 1 }, position: 0 }, + { color: { r: 0, g: 0, b: 1, a: 1 }, position: 1 } + ], + gradientTransform: { m00: 1, m01: 0, m02: 0, m10: 0, m11: 1, m12: 0 } + }, + { + type: 'SOLID', + color: { r: 1, g: 0, b: 0, a: 1 }, + opacity: 1, + visible: true + } + ] + }, + { windingRule: 'NONZERO', commandsBlob: 'invalid' } + ]) + ynode.set('strokeGeometry', 'invalid') + + const props = decodeNodeFromYjs(ynode) + const source = props.source as SceneNode['source'] + const fillGeometry = props.fillGeometry as GeometryPath[] + + expect(source).toEqual({ + ...createDefaultSourceMetadata(), + format: 'fig' + }) + expect(fillGeometry).toHaveLength(1) + expect(fillGeometry[0]?.commandsBlob).toBeInstanceOf(Uint8Array) + expect(fillGeometry[0]?.fills).toHaveLength(2) + expect(fillGeometry[0]?.fills?.[0]).toMatchObject({ + type: 'GRADIENT_LINEAR', + opacity: 0.8, + gradientStops: [ + { color: { r: 1, g: 0, b: 0, a: 1 }, position: 0 }, + { color: { r: 0, g: 0, b: 1, a: 1 }, position: 1 } + ], + gradientTransform: { m00: 1, m01: 0, m02: 0, m10: 0, m11: 1, m12: 0 } + }) + expect(fillGeometry[0]?.fills?.[1]?.type).toBe('SOLID') + expect(props.strokeGeometry).toEqual([]) + }) + test('binary geometry fields round-trip as Uint8Array, not strings', () => { const host = new SceneGraph() const page = firstPage(host) @@ -163,9 +251,9 @@ describe('collab yjs-sync', () => { const doc = new Y.Doc() const ynode = new Y.Map() doc.getMap>('nodes').set(ellipse.id, ynode) - syncNodePropsToYMap(ellipse, ynode) + syncEncodedNodeToYMap(ellipse, ynode) blob[0] = 99 - const props = yNodeToProps(ynode) + const props = decodeNodeFromYjs(ynode) expect(typeof ynode.get('fillGeometry')).not.toBe('string') const decoded = props.fillGeometry as GeometryPath[] @@ -207,11 +295,11 @@ describe('collab yjs-sync', () => { doc.transact(() => { const pageYnode = new Y.Map() ynodes.set(hostPage.id, pageYnode) - syncNodePropsToYMap({ ...hostPage, childIds: [] } as SceneNode, pageYnode) + syncEncodedNodeToYMap({ ...hostPage, childIds: [] } as SceneNode, pageYnode) const rectYnode = new Y.Map() ynodes.set(rect.id, rectYnode) - syncNodePropsToYMap(rect, rectYnode) + syncEncodedNodeToYMap(rect, rectYnode) }) const peer = new SceneGraph()