From 6911033c1a8d28e86102a3325c5876ed7a95590f Mon Sep 17 00:00:00 2001 From: ruinivist Date: Sun, 31 May 2026 11:47:20 +0000 Subject: [PATCH] fix: prevent stale drawing saves --- src/App.tsx | 124 +++++++++++++++++++++++++++++++++++------ src/api.test.ts | 86 ++++++++++++++++++++++++++++ src/api.ts | 59 ++++++++++++++------ src/db.test.ts | 65 ++++++++++++++++++++- src/db.ts | 89 +++++++++++++++++++++++------ src/mcp-server.test.ts | 11 +++- src/mcp-server.ts | 12 +++- src/shared.ts | 1 + 8 files changed, 391 insertions(+), 56 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index c4f2eef..9a8a62a 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -9,6 +9,17 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { DEFAULT_TITLE, type DrawingMeta, type ScenePayload } from "./shared"; type DrawingResponse = DrawingMeta & ScenePayload; +type ApiErrorBody = { ok: false; error?: string; drawing?: DrawingMeta }; +type UpdateResponse = { ok: true; drawing: DrawingMeta }; + +class RequestError extends Error { + constructor( + readonly status: number, + readonly body: ApiErrorBody, + ) { + super(body.error ?? `Request failed: ${status}`); + } +} const EXCALIDRAW_THEME_TOKEN_MAP = [ ["--island-bg-color", "--app-island-bg"], @@ -70,14 +81,18 @@ async function requestJson(url: string, init?: RequestInit): Promise { }, }); - const body = (await response.json()) as T & { error?: string }; + const body = (await response.json()) as T & ApiErrorBody; if (!response.ok) { - throw new Error(body.error ?? `Request failed: ${response.status}`); + throw new RequestError(response.status, body); } return body; } +function isConflictError(error: unknown): error is RequestError & { body: ApiErrorBody & { drawing: DrawingMeta } } { + return error instanceof RequestError && error.status === 409 && error.body.drawing !== undefined; +} + function DrawerIcon() { return (
{ const nextScene = sceneFromEditor(elements, appState, files); diff --git a/src/api.test.ts b/src/api.test.ts index a1a1ccd..e9b9ab1 100644 --- a/src/api.test.ts +++ b/src/api.test.ts @@ -26,6 +26,7 @@ describe("api", () => { const created = await api.createDrawing().json(); expect(created.appState.theme).toBe("dark"); + expect(created.revision).toBe(0); cleanup(); }); @@ -70,6 +71,7 @@ describe("api", () => { elements: [{ id: "shape" }], appState: {}, files: {}, + expectedRevision: created.revision, }), }), { params: { id: created.id } }, @@ -77,6 +79,90 @@ describe("api", () => { ); expect(response.status).toBe(200); + const body = await response.json(); + expect(body.drawing.revision).toBe(1); + cleanup(); + }); + + test("updates scene and title with a matching expected revision", async () => { + const { api, cleanup } = withApi(); + const created = await api.createDrawing().json(); + + const response = await api.updateDrawing( + Object.assign( + new Request(`http://local/api/drawings/${created.id}`, { + method: "PUT", + body: JSON.stringify({ + title: "Synced", + elements: [{ id: "shape" }], + appState: {}, + files: {}, + expectedRevision: 0, + }), + }), + { params: { id: created.id } }, + ), + ); + + expect(response.status).toBe(200); + const body = await response.json(); + expect(body.drawing.title).toBe("Synced"); + expect(body.drawing.revision).toBe(1); + cleanup(); + }); + + test("returns 409 for stale expected revisions without overwriting", async () => { + const { api, cleanup } = withApi(); + const created = await api.createDrawing().json(); + + await api.updateDrawing( + Object.assign( + new Request(`http://local/api/drawings/${created.id}`, { + method: "PUT", + body: JSON.stringify({ + elements: [{ id: "server" }], + appState: {}, + files: {}, + expectedRevision: 0, + }), + }), + { params: { id: created.id } }, + ), + ); + + const conflict = await api.updateDrawing( + Object.assign( + new Request(`http://local/api/drawings/${created.id}`, { + method: "PUT", + body: JSON.stringify({ + elements: [{ id: "stale" }], + appState: {}, + files: {}, + expectedRevision: 0, + }), + }), + { params: { id: created.id } }, + ), + ); + + expect(conflict.status).toBe(409); + const body = await conflict.json(); + expect(body).toEqual({ + ok: false, + error: "Drawing changed externally", + drawing: { + id: created.id, + title: created.title, + createdAt: body.drawing.createdAt, + updatedAt: body.drawing.updatedAt, + revision: 1, + }, + }); + + const stored = await api.getDrawing( + Object.assign(new Request(`http://local/api/drawings/${created.id}`), { params: { id: created.id } }), + ).json(); + expect(stored.elements).toEqual([{ id: "server" }]); cleanup(); }); }); diff --git a/src/api.ts b/src/api.ts index 89428f8..1efe1ca 100644 --- a/src/api.ts +++ b/src/api.ts @@ -1,4 +1,5 @@ import { type DrawingStore } from "./db"; +import { type DrawingMeta } from "./shared"; import { HttpError, normalizeTitle, parseJsonBody, parseSceneText } from "./scene"; type RouteRequest = Request & { @@ -22,6 +23,29 @@ function errorResponse(error: unknown): Response { return json({ ok: false, error: "Internal server error" }, { status: 500 }); } +function toMeta(drawing: DrawingMeta): DrawingMeta { + return { + id: drawing.id, + title: drawing.title, + createdAt: drawing.createdAt, + updatedAt: drawing.updatedAt, + revision: drawing.revision, + }; +} + +function parseExpectedRevision(raw: Record): number | undefined { + if (!Object.hasOwn(raw, "expectedRevision")) { + return undefined; + } + + const value = raw.expectedRevision; + if (typeof value !== "number" || !Number.isSafeInteger(value) || value < 0) { + throw new HttpError(400, "expectedRevision must be a non-negative integer"); + } + + return value; +} + export function createApi(store: DrawingStore) { return { health() { @@ -36,10 +60,7 @@ export function createApi(store: DrawingStore) { const drawing = store.createDrawing(); return json( { - id: drawing.id, - title: drawing.title, - createdAt: drawing.createdAt, - updatedAt: drawing.updatedAt, + ...toMeta(drawing), ...drawing.scene, }, { status: 201 }, @@ -55,10 +76,7 @@ export function createApi(store: DrawingStore) { } return json({ - id: drawing.id, - title: drawing.title, - createdAt: drawing.createdAt, - updatedAt: drawing.updatedAt, + ...toMeta(drawing), ...drawing.scene, }); }, @@ -77,29 +95,38 @@ export function createApi(store: DrawingStore) { Object.hasOwn(raw, "elements") || Object.hasOwn(raw, "appState") || Object.hasOwn(raw, "files"); + const expectedRevision = parseExpectedRevision(raw); if (!hasTitle && !hasScene) { throw new HttpError(400, "Nothing to update"); } const scene = hasScene ? parseSceneText(text) : undefined; - const updated = store.updateDrawing(id, { + const result = store.updateDrawing(id, { scene, title: hasTitle ? normalizeTitle(raw.title) : undefined, + expectedRevision, }); - if (!updated) { + if (!result.ok && result.reason === "not_found") { return json({ ok: false, error: "Drawing not found" }, { status: 404 }); } + if (!result.ok && result.reason === "conflict") { + return json( + { + ok: false, + error: "Drawing changed externally", + drawing: toMeta(result.drawing), + }, + { status: 409 }, + ); + } + + const updated = result.drawing; return json({ ok: true, - drawing: { - id: updated.id, - title: updated.title, - createdAt: updated.createdAt, - updatedAt: updated.updatedAt, - }, + drawing: toMeta(updated), }); } catch (error) { return errorResponse(error); diff --git a/src/db.test.ts b/src/db.test.ts index 8c52d38..52f3951 100644 --- a/src/db.test.ts +++ b/src/db.test.ts @@ -1,4 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; +import { Database } from "bun:sqlite"; import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -27,8 +28,10 @@ describe("drawing store", () => { const created = store.createDrawing(); expect(created.title).toBe("Untitled"); + expect(created.revision).toBe(0); expect(created.scene.appState.theme).toBe("dark"); expect(store.listDrawings()).toHaveLength(1); + expect(store.listDrawings()[0]?.revision).toBe(0); const updated = store.updateDrawing(created.id, { title: "Flow", @@ -39,12 +42,70 @@ describe("drawing store", () => { }, }); - expect(updated?.title).toBe("Flow"); - expect(updated?.scene.elements).toHaveLength(1); + expect(updated.ok).toBe(true); + expect(updated.ok ? updated.drawing.title : null).toBe("Flow"); + expect(updated.ok ? updated.drawing.revision : null).toBe(1); + expect(updated.ok ? updated.drawing.scene.elements : []).toHaveLength(1); const removed = store.deleteDrawing(created.id); expect(removed.deleted).toBe(true); expect(store.listDrawings()).toHaveLength(1); expect(removed.next?.id).not.toBe(created.id); }); + + test("rejects stale expected revisions without overwriting the scene", () => { + const store = createTempStore(); + const created = store.createDrawing(); + + const first = store.updateDrawing(created.id, { + expectedRevision: 0, + scene: { + elements: [{ id: "first" }], + appState: {}, + files: {}, + }, + }); + + expect(first.ok).toBe(true); + + const stale = store.updateDrawing(created.id, { + expectedRevision: 0, + scene: { + elements: [{ id: "stale" }], + appState: {}, + files: {}, + }, + }); + + expect(stale.ok).toBe(false); + expect(stale.ok ? null : stale.reason).toBe("conflict"); + expect(store.getDrawing(created.id)?.revision).toBe(1); + expect(store.getDrawing(created.id)?.scene.elements).toEqual([{ id: "first" }]); + }); + + test("migrates existing databases to include revision", () => { + const dir = mkdtempSync(join(tmpdir(), "excali-")); + cleanup.push(dir); + const databasePath = join(dir, "test.sqlite"); + const db = new Database(databasePath, { create: true, strict: true }); + db.exec(` + CREATE TABLE drawings ( + id TEXT PRIMARY KEY, + title TEXT NOT NULL, + data TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP + ); + + INSERT INTO drawings (id, title, data) + VALUES ('legacy', 'Legacy', '{"elements":[],"appState":{},"files":{}}'); + `); + db.close(false); + + const store = createDrawingStore(databasePath); + expect(store.getDrawing("legacy")?.revision).toBe(0); + + const updated = store.updateDrawing("legacy", { title: "Migrated" }); + expect(updated.ok ? updated.drawing.revision : null).toBe(1); + }); }); diff --git a/src/db.ts b/src/db.ts index 987dd01..af9a9e2 100644 --- a/src/db.ts +++ b/src/db.ts @@ -11,10 +11,16 @@ type DrawingRow = { data: string; createdAt: string; updatedAt: string; + revision: number; }; type DrawingMetaRow = Omit; +export type DrawingUpdateResult = + | { ok: true; drawing: DrawingRecord } + | { ok: false; reason: "not_found" } + | { ok: false; reason: "conflict"; drawing: DrawingRecord }; + export type DrawingStore = ReturnType; function createId(): string { @@ -44,6 +50,7 @@ function readMeta(row: DrawingMetaRow | null | undefined): DrawingMeta | null { title: row.title, createdAt: row.createdAt, updatedAt: row.updatedAt, + revision: row.revision, }; } @@ -70,19 +77,26 @@ export function createDrawingStore(databasePath = resolveDatabasePath()) { title TEXT NOT NULL, data TEXT NOT NULL, created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, - updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP + updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, + revision INTEGER NOT NULL DEFAULT 0 ); CREATE INDEX IF NOT EXISTS drawings_updated_at_idx ON drawings(updated_at DESC, created_at DESC); `); + const columns = db.query("PRAGMA table_info(drawings)").all() as Array<{ name: string }>; + if (!columns.some((column) => column.name === "revision")) { + db.exec("ALTER TABLE drawings ADD COLUMN revision INTEGER NOT NULL DEFAULT 0"); + } + const listQuery = db.query(` SELECT id, title, created_at AS createdAt, - updated_at AS updatedAt + updated_at AS updatedAt, + revision FROM drawings ORDER BY updated_at DESC, created_at DESC `); @@ -93,7 +107,8 @@ export function createDrawingStore(databasePath = resolveDatabasePath()) { title, data, created_at AS createdAt, - updated_at AS updatedAt + updated_at AS updatedAt, + revision FROM drawings WHERE id = ?1 `); @@ -105,22 +120,40 @@ export function createDrawingStore(databasePath = resolveDatabasePath()) { const updateSceneQuery = db.query(` UPDATE drawings - SET data = ?2, updated_at = CURRENT_TIMESTAMP + SET data = ?2, updated_at = CURRENT_TIMESTAMP, revision = revision + 1 WHERE id = ?1 `); const updateTitleQuery = db.query(` UPDATE drawings - SET title = ?2, updated_at = CURRENT_TIMESTAMP + SET title = ?2, updated_at = CURRENT_TIMESTAMP, revision = revision + 1 WHERE id = ?1 `); const updateBothQuery = db.query(` UPDATE drawings - SET title = ?2, data = ?3, updated_at = CURRENT_TIMESTAMP + SET title = ?2, data = ?3, updated_at = CURRENT_TIMESTAMP, revision = revision + 1 WHERE id = ?1 `); + const updateSceneExpectedQuery = db.query(` + UPDATE drawings + SET data = ?2, updated_at = CURRENT_TIMESTAMP, revision = revision + 1 + WHERE id = ?1 AND revision = ?3 + `); + + const updateTitleExpectedQuery = db.query(` + UPDATE drawings + SET title = ?2, updated_at = CURRENT_TIMESTAMP, revision = revision + 1 + WHERE id = ?1 AND revision = ?3 + `); + + const updateBothExpectedQuery = db.query(` + UPDATE drawings + SET title = ?2, data = ?3, updated_at = CURRENT_TIMESTAMP, revision = revision + 1 + WHERE id = ?1 AND revision = ?4 + `); + const deleteQuery = db.query(` DELETE FROM drawings WHERE id = ?1 @@ -148,28 +181,50 @@ export function createDrawingStore(databasePath = resolveDatabasePath()) { return getLatestDrawing() ?? createDrawing(); } - function updateDrawing(id: string, update: { scene?: ScenePayload; title?: string }): DrawingRecord | null { - const existing = getDrawing(id); - if (!existing) { - return null; - } - + function updateDrawing( + id: string, + update: { scene?: ScenePayload; title?: string; expectedRevision?: number }, + ): DrawingUpdateResult { const hasScene = update.scene !== undefined; const hasTitle = update.title !== undefined; if (!hasScene && !hasTitle) { - return existing; + const existing = getDrawing(id); + return existing ? { ok: true, drawing: existing } : { ok: false, reason: "not_found" }; } + let changes: number; if (hasScene && hasTitle) { - updateBothQuery.run(id, normalizeTitle(update.title), JSON.stringify(update.scene)); + const title = normalizeTitle(update.title); + const scene = JSON.stringify(update.scene); + changes = + update.expectedRevision === undefined + ? Number(updateBothQuery.run(id, title, scene).changes) + : Number(updateBothExpectedQuery.run(id, title, scene, update.expectedRevision).changes); } else if (hasScene) { - updateSceneQuery.run(id, JSON.stringify(update.scene)); + const scene = JSON.stringify(update.scene); + changes = + update.expectedRevision === undefined + ? Number(updateSceneQuery.run(id, scene).changes) + : Number(updateSceneExpectedQuery.run(id, scene, update.expectedRevision).changes); } else { - updateTitleQuery.run(id, normalizeTitle(update.title)); + const title = normalizeTitle(update.title); + changes = + update.expectedRevision === undefined + ? Number(updateTitleQuery.run(id, title).changes) + : Number(updateTitleExpectedQuery.run(id, title, update.expectedRevision).changes); } - return getDrawing(id); + const drawing = getDrawing(id); + if (changes > 0 && drawing) { + return { ok: true, drawing }; + } + + if (!drawing) { + return { ok: false, reason: "not_found" }; + } + + return { ok: false, reason: "conflict", drawing }; } function deleteDrawing(id: string): { deleted: boolean; next: DrawingMeta | null } { diff --git a/src/mcp-server.test.ts b/src/mcp-server.test.ts index 9d20e41..356fda9 100644 --- a/src/mcp-server.test.ts +++ b/src/mcp-server.test.ts @@ -54,8 +54,10 @@ describe("mcp tool handlers", () => { expect(result).toEqual({ id: result.id, title: "explicit", + revision: 1, url: `http://example.test/d/${result.id}`, }); + expect(drawing?.revision).toBe(1); expect(drawing?.scene).toEqual(scene); }); @@ -69,6 +71,7 @@ describe("mcp tool handlers", () => { expect(drawing).toMatchObject({ id: result.id, title: "read", + revision: 1, url: `http://example.test/d/${result.id}`, scene, }); @@ -83,10 +86,12 @@ describe("mcp tool handlers", () => { files: {}, }; - tools.replace_drawing({ id: result.id, title: "replaced", scene: nextScene }); + const replaced = tools.replace_drawing({ id: result.id, title: "replaced", scene: nextScene }); expect(store.listDrawings()).toHaveLength(1); + expect(replaced.revision).toBe(2); expect(store.getDrawing(result.id)?.title).toBe("replaced"); + expect(store.getDrawing(result.id)?.revision).toBe(2); expect(store.getDrawing(result.id)?.scene).toEqual(nextScene); }); @@ -94,7 +99,7 @@ describe("mcp tool handlers", () => { const { store, tools } = createTempTools(); const result = tools.create_drawing({ title: "patch", scene: testScene() }); - tools.patch_drawing({ + const patched = tools.patch_drawing({ id: result.id, patch: [ { op: "replace", path: "/elements/0/x", value: 42 }, @@ -102,6 +107,8 @@ describe("mcp tool handlers", () => { ], }); + expect(patched.revision).toBe(2); + expect(store.getDrawing(result.id)?.revision).toBe(2); expect(store.getDrawing(result.id)?.scene.elements).toEqual([{ id: "one", type: "rectangle", x: 42, y: 20 }]); }); diff --git a/src/mcp-server.ts b/src/mcp-server.ts index b8eb198..dc020f0 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -75,10 +75,11 @@ function notFound(id: string): never { throw new Error(`Drawing not found: ${id}`); } -function mutationResult(publicBaseUrl: string, drawing: { id: string; title: string }) { +function mutationResult(publicBaseUrl: string, drawing: { id: string; title: string; revision: number }) { return { id: drawing.id, title: drawing.title, + revision: drawing.revision, url: drawingUrl(publicBaseUrl, drawing.id), }; } @@ -88,8 +89,12 @@ function updateExistingDrawing( id: string, update: { scene?: ScenePayload; title?: string }, ) { - const updated = store.updateDrawing(id, update); - return updated ?? notFound(id); + const result = store.updateDrawing(id, update); + if (!result.ok) { + return notFound(id); + } + + return result.drawing; } function applyScenePatch(scene: ScenePayload, patch: Operation[]): ScenePayload { @@ -115,6 +120,7 @@ export function createMcpToolHandlers(store: DrawingStore, publicBaseUrl: string title: drawing.title, createdAt: drawing.createdAt, updatedAt: drawing.updatedAt, + revision: drawing.revision, url: drawingUrl(publicBaseUrl, drawing.id), scene: drawing.scene, }; diff --git a/src/shared.ts b/src/shared.ts index 5153b0d..e69c4fd 100644 --- a/src/shared.ts +++ b/src/shared.ts @@ -9,6 +9,7 @@ export type DrawingMeta = { title: string; createdAt: string; updatedAt: string; + revision: number; }; export type DrawingRecord = DrawingMeta & {