From 4ed4ea1070f79825044e6c7255f62cd155b4970f Mon Sep 17 00:00:00 2001 From: Aymeric Rabot Date: Tue, 4 Aug 2026 13:44:25 -0400 Subject: [PATCH] fix(editor): accept plugin node kinds in the scene save API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Placing a plugin node (`trees:tree` from the first-party Nature pack) in a saved scene made every later autosave fail with 400: `apiGraphSchema` validated every node against the static `AnyNode` union, which cannot enumerate kinds a plugin registers at runtime. A node whose `type` is outside `AnyNode` is now validated the way `validate-build-json` already treats a kind it cannot resolve — as a foreign node, held to the `BaseNode` envelope plus core's `AssetUrl` allowlist applied to every URL-shaped string it carries. Membership is decided by "not in `AnyNode`", not by a namespace pattern: `plugin-authoring.md` requires plugin *ids* to look like `vendor:pack`, never kinds, and its worked example registers `kind: 'couch'`. Reusing `AssetUrl` keeps the Phase 3 posture intact on a branch that has to accept unknown fields. A scheme denylist would have to enumerate every hostile scheme; `AssetUrl` already enumerates the safe ones, so `//evil.example`, `ws:`, `gopher:`, `about:blank`, the instance-metadata endpoint, and control-character-obfuscated `java\tscript:` are all rejected, and `PASCAL_ALLOWED_ASSET_ORIGINS` keeps narrowing https origins on this path too. Only URL-shaped values are checked, so a plugin can still store prose in `name` / `metadata` exactly as builtin nodes do. The URL scan is depth- and visit-bounded. An unbounded walk over a deeply nested body throws `RangeError` past `safeParse`, which the route answers as a 500 where the contract is a 400 with issues. Co-authored-by: Kone Venkatesh Co-Authored-By: Claude Opus 5 --- apps/editor/lib/graph-schema.test.ts | 167 +++++++++++++++++++++++++++ apps/editor/lib/graph-schema.ts | 136 +++++++++++++++++++++- packages/core/src/schema/index.ts | 2 + 3 files changed, 299 insertions(+), 6 deletions(-) create mode 100644 apps/editor/lib/graph-schema.test.ts diff --git a/apps/editor/lib/graph-schema.test.ts b/apps/editor/lib/graph-schema.test.ts new file mode 100644 index 0000000000..deb9bba229 --- /dev/null +++ b/apps/editor/lib/graph-schema.test.ts @@ -0,0 +1,167 @@ +import { expect, test } from 'bun:test' +import { apiGraphSchema } from './graph-schema' + +function buildGraph(nodes: Record, rootNodeIds: string[] = []) { + return { nodes, rootNodeIds } +} + +const LEVEL_ID = 'level_a1b2c3d4e5f6g7h8' +const TREE_ID = 'tree_a1b2c3d4e5f6g7h8' + +const level = (children: string[] = []) => ({ + object: 'node', + id: LEVEL_ID, + type: 'level', + parentId: null, + children, + level: 0, +}) + +const pluginTree = (overrides: Record = {}) => ({ + object: 'node', + id: TREE_ID, + type: 'trees:tree', + parentId: LEVEL_ID, + position: [1, 0, 2], + rotation: 0, + ...overrides, +}) + +test('accepts a graph containing a plugin node kind', () => { + const graph = buildGraph({ [TREE_ID]: pluginTree() }, [LEVEL_ID]) + + expect(apiGraphSchema.safeParse(graph).success).toBe(true) +}) + +test('accepts a builtin container whose children include a plugin node id', () => { + const graph = buildGraph({ [LEVEL_ID]: level([TREE_ID]), [TREE_ID]: pluginTree() }, [LEVEL_ID]) + + expect(apiGraphSchema.safeParse(graph).success).toBe(true) +}) + +test('keeps plugin child ids in the parsed graph', () => { + const graph = buildGraph({ [LEVEL_ID]: level([TREE_ID]), [TREE_ID]: pluginTree() }, [LEVEL_ID]) + + const res = apiGraphSchema.safeParse(graph) + + expect(res.success).toBe(true) + expect((res.data?.nodes[LEVEL_ID] as { children: string[] }).children).toEqual([TREE_ID]) +}) + +test('preserves installedPlugins alongside a plugin node', () => { + const res = apiGraphSchema.safeParse({ + ...buildGraph({ [TREE_ID]: pluginTree() }, [LEVEL_ID]), + installedPlugins: ['pascal:trees'], + }) + + expect(res.success).toBe(true) + expect(res.data?.installedPlugins).toEqual(['pascal:trees']) +}) + +test('rejects a plugin node that fails the base envelope', () => { + const graph = buildGraph({ tree_bad: pluginTree({ id: 42 }) }) + + expect(apiGraphSchema.safeParse(graph).success).toBe(false) +}) + +// The `AssetUrl` allowlist is the whole Phase 3 posture: every scheme outside +// it is rejected, so this list does not have to be exhaustive to be sound. A +// denylist would — which is why one isn't used. `169.254.169.254` is the cloud +// instance-metadata endpoint, the canonical SSRF target. +test('rejects URL-shaped plugin fields outside the AssetUrl allowlist', () => { + for (const url of [ + 'javascript:alert(1)', + ' file:///etc/passwd', + 'data:text/html,', + 'http://169.254.169.254/latest/meta-data', + 'http://evil.example/beacon.png', + 'ws://evil.example/socket', + 'gopher://evil.example/x', + 'about:blank', + // C0 controls inside the scheme: a browser ignores them and navigates, so + // a prefix match on the raw string is not enough. + 'java\tscript:alert(1)', + '\u0000javascript:alert(1)', + // Scheme matching must be case-insensitive. + 'DATA:TEXT/HTML,', + ]) { + const graph = buildGraph({ [TREE_ID]: pluginTree({ config: { textures: [{ src: url }] } }) }) + + const res = apiGraphSchema.safeParse(graph) + + expect(res.success, `expected ${JSON.stringify(url)} to be rejected`).toBe(false) + expect(res.error?.issues[0]?.message).toBe('URL is not in the allowed scheme list') + } +}) + +test('accepts the asset URL forms core allows', () => { + for (const url of [ + 'data:image/png;base64,iVBORw0KGgo=', + 'https://cdn.example/tree.webp', + 'asset://tree-bark', + 'blob:https://editor.pascal.app/9f1c', + '/textures/bark.webp', + 'http://localhost:3000/textures/bark.webp', + ]) { + const graph = buildGraph({ [TREE_ID]: pluginTree({ thumbnail: url }) }) + + expect(apiGraphSchema.safeParse(graph).success, `expected ${url} to be accepted`).toBe(true) + } +}) + +// Prose that happens to start with a word and a colon is not a URL. A plugin +// may put arbitrary text in `name` / `metadata`, exactly as builtin nodes do — +// the allowlist applies to URL-shaped values, not to every string. +test('does not treat prose or drive paths as URLs', () => { + for (const text of [ + 'FTP: north bed', + 'note: see plan 3', + 'Data: unavailable', + 'C:\\Users\\me\\plan.png', + 'Oak tree', + ]) { + const graph = buildGraph({ [TREE_ID]: pluginTree({ name: text, metadata: { note: text } }) }) + + expect(apiGraphSchema.safeParse(graph).success, `expected ${text} to be accepted`).toBe(true) + } +}) + +// A recursive walk over untrusted JSON must not throw past `safeParse` — the +// route would answer 500 where the contract is a 400 with issues. +test('reports deeply nested plugin nodes as a validation issue, not a crash', () => { + let nested: unknown = 'leaf' + for (let i = 0; i < 100_000; i++) nested = [nested] + const graph = buildGraph({ [TREE_ID]: pluginTree({ nested }) }) + + const res = apiGraphSchema.safeParse(graph) + + expect(res.success).toBe(false) + expect(res.error?.issues[0]?.message).toBe('Node is too deeply nested to validate') +}) + +test('still rejects invalid builtin nodes', () => { + const graph = buildGraph({ + wall_bad: { object: 'node', id: 'wall_a1b2c3d4e5f6g7h8', type: 'wall' }, + }) + + expect(apiGraphSchema.safeParse(graph).success).toBe(false) +}) + +// Unnamespaced kinds are legitimate: `wiki/architecture/plugin-authoring.md` +// requires plugin *ids* to look like `vendor:pack`, never kinds, and its worked +// example registers `kind: 'couch'`. Membership is decided by "not in AnyNode", +// so such a node is validated as foreign rather than rejected outright. +test('treats an unnamespaced unknown type as a foreign node', () => { + const couch = { + object: 'node', + id: 'couch_a1b2c3d4e5f6g7h8', + type: 'couch', + parentId: LEVEL_ID, + } + + expect(apiGraphSchema.safeParse(buildGraph({ [couch.id]: couch })).success).toBe(true) + expect( + apiGraphSchema.safeParse(buildGraph({ [couch.id]: { ...couch, src: 'javascript:alert(1)' } })) + .success, + ).toBe(false) +}) diff --git a/apps/editor/lib/graph-schema.ts b/apps/editor/lib/graph-schema.ts index c94c724faa..7f5f6c5ceb 100644 --- a/apps/editor/lib/graph-schema.ts +++ b/apps/editor/lib/graph-schema.ts @@ -1,4 +1,4 @@ -import { AnyNode } from '@pascal-app/core/schema' +import { AnyNode, AssetUrl, BaseNode } from '@pascal-app/core/schema' import { z } from 'zod' /** @@ -11,7 +11,90 @@ import { z } from 'zod' * route can silently accept malicious URLs via the `graph` payload. * * Phase 8 P4 found the POST bypass; Phase 10 A2 found the PUT bypass. + * + * Nodes minted by plugins (`trees:tree`, plus anything a third-party pack + * registers) are outside `AnyNode`, and their schemas live in packages that + * pull in renderer/UI code a route handler must not import — the node registry + * is empty in this process regardless, since nothing registers kinds + * server-side. Such a node is validated the way `validate-build-json` treats a + * kind it cannot resolve: as a foreign node, held to the `BaseNode` envelope + * plus the same `AssetUrl` allowlist applied to every URL-shaped string it + * carries. That keeps the Phase 3 posture on a branch that must accept unknown + * fields without resorting to a denylist, which would have to enumerate every + * hostile scheme where `AssetUrl` already enumerates the safe ones. + */ + +const KNOWN_TYPES = new Set( + AnyNode.options.map((o) => o.shape.type.parse(undefined) as string), +) + +/** The envelope every persisted node satisfies, builtin or foreign. */ +const ForeignNodeEnvelope = BaseNode.extend({ + type: z.string().min(1), + children: z.array(z.string()).optional(), +}).loose() + +// A walk over attacker-shaped JSON needs both bounds: depth, so a nest of +// arrays can't overflow the stack (an exception thrown past `safeParse` is a +// 500, not the 400 the caller is owed), and a visit count, so a wide-but-flat +// body can't burn the request budget. +const MAX_SCAN_DEPTH = 48 +const MAX_SCAN_VALUES = 50_000 + +/** + * Whether a string is trying to be a URL, and so has to satisfy `AssetUrl`. + * + * C0 controls are stripped first: browsers ignore them inside a scheme, so + * `java\tscript:alert(1)` navigates while reading as free text to a naive + * prefix match. The scheme must be followed immediately by a non-space — that + * is what separates `ftp://host/x` from prose like `FTP: north bed`, which a + * plugin is entitled to store in a node name. The two-character minimum on the + * scheme leaves Windows drive paths (`C:\…`) as free text; every scheme worth + * rejecting is longer than one letter. */ +// biome-ignore lint/suspicious/noControlCharactersInRegex: stripping C0 controls is the point — browsers ignore them mid-scheme. +const CONTROL_CHARS = /[\u0000-\u001f\u007f]/g +const SCHEME = /^[a-z][a-z0-9+.-]+:[^\s]/i +function isUrlShaped(value: string): boolean { + const bare = value.replace(CONTROL_CHARS, '').trimStart() + return SCHEME.test(bare) || bare.startsWith('//') +} + +type ScanResult = { path: (string | number)[]; reason: 'url' | 'too-complex' } + +/** + * First URL-shaped string `AssetUrl` rejects, or a breach of the walk bounds. + * + * The node's own `type` is exempt: a namespaced kind (`trees:tree`) is + * scheme-shaped by construction, and the envelope has already validated it. + */ +function findRejectedUrl(root: unknown): ScanResult | null { + let budget = MAX_SCAN_VALUES + const walk = (value: unknown, path: (string | number)[], depth: number): ScanResult | null => { + if (depth > MAX_SCAN_DEPTH || budget-- <= 0) return { path, reason: 'too-complex' } + if (typeof value === 'string') { + if (isUrlShaped(value) && !AssetUrl.safeParse(value).success) return { path, reason: 'url' } + return null + } + if (Array.isArray(value)) { + for (let i = 0; i < value.length; i++) { + const hit = walk(value[i], [...path, i], depth + 1) + if (hit) return hit + } + return null + } + if (value && typeof value === 'object') { + for (const [key, child] of Object.entries(value)) { + if (depth === 0 && key === 'type') continue + const hit = walk(child, [...path, key], depth + 1) + if (hit) return hit + } + } + return null + } + return walk(root, [], 0) +} + export const apiGraphSchema = z .object({ nodes: z.record(z.string(), z.unknown()), @@ -20,16 +103,57 @@ export const apiGraphSchema = z installedPlugins: z.array(z.string().min(1)).optional(), }) .superRefine((value, ctx) => { + const addIssues = (nodeId: string, error: z.ZodError) => { + for (const issue of error.issues) { + ctx.addIssue({ + code: 'custom', + path: ['nodes', nodeId, ...issue.path], + message: issue.message, + }) + } + } + + // Ids of foreign nodes in this graph. Builtin container schemas name the + // child kinds they accept (`BuildingNode.children`, `RoofNode.children`), + // so a container holding a plugin child fails against `AnyNode` even + // though the relationship is legitimate. Those ids are dropped from a + // *copy* handed to `AnyNode`; the stored graph keeps them, and each + // foreign node is still validated on its own. + const foreignIds = new Set() + for (const [nodeId, node] of Object.entries(value.nodes)) { + const type = (node as { type?: unknown } | null)?.type + if (typeof type === 'string' && !KNOWN_TYPES.has(type)) foreignIds.add(nodeId) + } + for (const [nodeId, node] of Object.entries(value.nodes)) { - const res = AnyNode.safeParse(node) - if (!res.success) { - for (const issue of res.error.issues) { + const type = (node as { type?: unknown } | null)?.type + + if (typeof type === 'string' && !KNOWN_TYPES.has(type)) { + const res = ForeignNodeEnvelope.safeParse(node) + if (!res.success) { + addIssues(nodeId, res.error) + continue + } + const rejected = findRejectedUrl(node) + if (rejected) { ctx.addIssue({ code: 'custom', - path: ['nodes', nodeId, ...issue.path], - message: issue.message, + path: ['nodes', nodeId, ...rejected.path], + message: + rejected.reason === 'url' + ? 'URL is not in the allowed scheme list' + : 'Node is too deeply nested to validate', }) } + continue } + + const children = (node as { children?: unknown } | null)?.children + const candidate = + foreignIds.size > 0 && Array.isArray(children) + ? { ...(node as object), children: children.filter((c) => !foreignIds.has(c as string)) } + : node + const res = AnyNode.safeParse(candidate) + if (!res.success) addIssues(nodeId, res.error) } }) diff --git a/packages/core/src/schema/index.ts b/packages/core/src/schema/index.ts index 0f6edbdd29..d11bdec7d1 100644 --- a/packages/core/src/schema/index.ts +++ b/packages/core/src/schema/index.ts @@ -6,6 +6,8 @@ export { type SolarPanelPresetDims, SolarPanelPresetKey, } from '../solar-panel-presets' +// Asset URL allowlist +export { ALLOWED_ORIGINS_ENV, ALLOWED_SCHEMES, AssetUrl } from './asset-url' export { BaseNode, generateId, Material, nodeType, objectId } from './base' // Camera export { CameraSchema } from './camera'