From 5ce919daa638a15e0361e0342d571bb40c0d3bbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Fri, 28 Aug 2026 19:42:31 +0200 Subject: [PATCH] findings group W: pins/presence bridge never receives non-finite coords (W-1/W-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - comments.ts pushPins: finite filter; presence-kicad: finiteCursor() drops non-finite peer cursors. - collab_presence_core.h: numOr() — a present JSON null no longer throws type_error.302 across embind in setPins/setRemote/setRemoteCursors (takes effect on next wasm build). - comments-nonfinite.test.ts: sink repro (boot + throttled live push). - pcbjam-shared → ff8835c (finite wire schemas, resolveAnchor guard, encoded route segments). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CwF7w1pkSTh5Z8jQyiXmHm --- wasm/bindings/collab_presence_core.h | 18 ++++- web/pcbjam-shared | 2 +- .../wasm/collab/comments-nonfinite.test.ts | 77 +++++++++++++++++++ web/standalone/src/wasm/collab/comments.ts | 6 +- .../src/wasm/collab/presence-kicad.ts | 8 +- 5 files changed, 105 insertions(+), 6 deletions(-) create mode 100644 web/standalone/src/wasm/collab/comments-nonfinite.test.ts diff --git a/wasm/bindings/collab_presence_core.h b/wasm/bindings/collab_presence_core.h index 0cf7b93..5a3102b 100644 --- a/wasm/bindings/collab_presence_core.h +++ b/wasm/bindings/collab_presence_core.h @@ -77,6 +77,18 @@ inline long long nowMs() .count(); } +/** Findings W-1/W-4: `json::value( key, default )` only defaults on a MISSING + * key — a present `null` (what JSON.stringify makes of ±Infinity/NaN) throws + * nlohmann type_error.302 across embind. Read numbers permissively. */ +inline double numOr( const nlohmann::json& aObj, const char* aKey, double aDefault ) +{ + if( !aObj.is_object() ) + return aDefault; + + auto it = aObj.find( aKey ); + return it != aObj.end() && it->is_number() ? it->get() : aDefault; +} + inline KIGFX::COLOR4D parsePeerColor( const std::string& aHex ) { if( aHex.size() == 7 && aHex[0] == '#' ) @@ -591,7 +603,7 @@ struct CORE if( p.contains( "cursor" ) && p["cursor"].is_object() ) { peer.hasCursor = true; - peer.cursor = VECTOR2D( p["cursor"].value( "x", 0.0 ), p["cursor"].value( "y", 0.0 ) ); + peer.cursor = VECTOR2D( numOr( p["cursor"], "x", 0.0 ), numOr( p["cursor"], "y", 0.0 ) ); } for( const json& u : p.value( "selection", json::array() ) ) @@ -678,7 +690,7 @@ struct CORE if( c.contains( "cursor" ) && c["cursor"].is_object() ) { peer.hasCursor = true; - peer.cursor = VECTOR2D( c["cursor"].value( "x", 0.0 ), c["cursor"].value( "y", 0.0 ) ); + peer.cursor = VECTOR2D( numOr( c["cursor"], "x", 0.0 ), numOr( c["cursor"], "y", 0.0 ) ); } else { @@ -710,7 +722,7 @@ struct CORE PIN pin; pin.id = p.value( "id", "" ); pin.name = p.value( "name", "" ); - pin.pos = VECTOR2D( p.value( "x", 0.0 ), p.value( "y", 0.0 ) ); + pin.pos = VECTOR2D( numOr( p, "x", 0.0 ), numOr( p, "y", 0.0 ) ); pin.color = parsePeerColor( p.value( "color", "" ) ); pin.resolved = p.value( "resolved", false ); pin.unread = p.value( "unread", false ); diff --git a/web/pcbjam-shared b/web/pcbjam-shared index 5014383..ff8835c 160000 --- a/web/pcbjam-shared +++ b/web/pcbjam-shared @@ -1 +1 @@ -Subproject commit 50143831109cd59e32db0e2c62c0e1f2320dd005 +Subproject commit ff8835ccc173b5ff9b35ade95440793244ef27b4 diff --git a/web/standalone/src/wasm/collab/comments-nonfinite.test.ts b/web/standalone/src/wasm/collab/comments-nonfinite.test.ts new file mode 100644 index 0000000..22c5322 --- /dev/null +++ b/web/standalone/src/wasm/collab/comments-nonfinite.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, it } from "vitest"; +import * as Y from "yjs"; +import { createThread, setThreadAnchor } from "@pcbjam/shared"; +import { createComments, type CommentPinsModule } from "./comments"; + +// repro for W-1 (findings group W / security audit v3 #11): the pins bridge +// must never hand the wasm a `null` coordinate. JSON.stringify turns +// Infinity/NaN into null; the C++ consumer (collab_presence_core.h setPins, +// nlohmann `.value("x", 0.0)`) throws type_error.302 on a PRESENT null — the +// throw crosses embind as a JS exception at the pushPins call site (boot path +// = attachCollabAndPresence, live path = the throttled timer). +function stubMod(): CommentPinsModule & { calls: string[] } { + const calls: string[] = []; + return { + calls, + kicadCollabSetPins(json) { + calls.push(json); + // Mirror the C++ behaviour: a present null coordinate throws. + const parsed = JSON.parse(json) as { pins: Array<{ x: unknown; y: unknown }> }; + for (const p of parsed.pins) { + if (typeof p.x !== "number" || typeof p.y !== "number") { + throw new Error("type_error.302 type must be number, but is null"); + } + } + }, + kicadCollabSetViewport() {}, + kicadCollabGetViewport: () => JSON.stringify({ scale: 1, cx: 0, cy: 0, w: 100, h: 100 }), + }; +} + +describe("W-1 comment pins bridge with non-finite anchors", () => { + it("boot push does not throw and never serializes null coordinates", () => { + const doc = new Y.Doc(); + const good = createThread(doc, { anchor: { pos: { x: 10, y: 20 } }, author: "a", body: "ok", now: 1 }); + const bad = createThread(doc, { anchor: { pos: { x: 1, y: 1 } }, author: "b", body: "bad", now: 2 }); + // NOTE: NaN is already rejected by z.number() (thread dropped whole); Infinity + // passes z.number() — that is the live hole. + setThreadAnchor(doc, bad, { pos: { x: Infinity, y: -Infinity } }); + + const mod = stubMod(); + let ctl: ReturnType | undefined; + expect(() => { + ctl = createComments({ doc, mod, user: { id: "me" }, tool: "pcbnew" }); + }).not.toThrow(); + + expect(mod.calls.length).toBeGreaterThan(0); + const last = JSON.parse(mod.calls.at(-1)!) as { pins: Array<{ id: string; x: number; y: number }> }; + // The healthy thread still gets its pin; the poisoned one is dropped (or + // clamped) — either way no null reaches the wasm. + expect(last.pins.some((p) => p.id === good)).toBe(true); + for (const p of last.pins) { + expect(Number.isFinite(p.x) && Number.isFinite(p.y)).toBe(true); + } + ctl?.destroy(); + }); + + it("live push (poison arrives after bind) does not throw out of the timer", async () => { + const doc = new Y.Doc(); + const t = createThread(doc, { anchor: { pos: { x: 10, y: 20 } }, author: "a", body: "ok", now: 1 }); + const mod = stubMod(); + const ctl = createComments({ doc, mod, user: { id: "me" }, tool: "pcbnew" }); + const before = mod.calls.length; + + const unhandled: unknown[] = []; + const onErr = (e: unknown) => unhandled.push(e); + process.on("uncaughtException", onErr); + try { + setThreadAnchor(doc, t, { pos: { x: Infinity, y: 0 } }); + await new Promise((r) => setTimeout(r, 80)); // > PUSH_THROTTLE_MS + } finally { + process.off("uncaughtException", onErr); + } + expect(unhandled).toEqual([]); + expect(mod.calls.length).toBeGreaterThan(before); + ctl.destroy(); + }); +}); diff --git a/web/standalone/src/wasm/collab/comments.ts b/web/standalone/src/wasm/collab/comments.ts index 51259a6..26699b7 100644 --- a/web/standalone/src/wasm/collab/comments.ts +++ b/web/standalone/src/wasm/collab/comments.ts @@ -163,7 +163,11 @@ export function createComments(opts: { pins: !visible ? [] : cache - .filter((t) => !t.resolved) + // Findings W-1: never hand the wasm a non-finite coordinate + // (JSON null → nlohmann type_error across embind). + .filter( + (t) => !t.resolved && Number.isFinite(t.world.x) && Number.isFinite(t.world.y), + ) .map((t) => ({ id: t.id, // Author name rides along so the tuner's palette override can diff --git a/web/standalone/src/wasm/collab/presence-kicad.ts b/web/standalone/src/wasm/collab/presence-kicad.ts index 2f7c5bb..66d7303 100644 --- a/web/standalone/src/wasm/collab/presence-kicad.ts +++ b/web/standalone/src/wasm/collab/presence-kicad.ts @@ -238,7 +238,7 @@ export function bindKicadPresence(opts: { id: p.user.id, name: p.user.name, color: p.user.color, - cursor: p.cursor, + cursor: finiteCursor(p.cursor), selection: p.selection, })), }; @@ -325,3 +325,9 @@ export function bindKicadPresence(opts: { }, }; } + +/** Findings W-4: a non-finite peer cursor would serialize as JSON null and + * throw inside kicadCollabSetRemote / SetRemoteCursors — treat it as absent. */ +function finiteCursor(c: { x: number; y: number } | null | undefined): { x: number; y: number } | null { + return c && Number.isFinite(c.x) && Number.isFinite(c.y) ? c : null; +}