From 791bff419a934cc7dc30534b4b4a523e7b98d5ac Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:33:57 +0300 Subject: [PATCH] fix(mcp): refuse ambiguous patch_node/delete_node on duplicated ids (#159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Docmost duplicates block ids on copy/paste, and copyPageContent writes the source document verbatim with the same ids. `patchNode`/`deleteNode` address a block by `attrs.id` via replaceNodeById/deleteNodeById, which act on EVERY node sharing the id — so a single patch_node/delete_node could silently replace/remove multiple unrelated blocks with no signal to the model (red-team finding #6). Guard both write paths: when more than one node matches the id, skip the write entirely (the transform returns null -> no mutation) and throw a clear "ambiguous id — N nodes share it" error so the model re-targets with a more specific anchor. Only an unambiguous single match is written; the 0-match and 1-match behavior is unchanged. The duplicate-count basis is covered by node-ops.test.mjs (replaceNodeById / deleteNodeById report count===2 for a 2-duplicate doc). The end-to-end guard is not unit-tested because patchNode/deleteNode require a live collab provider and the test suite has no provider mock. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/build/client.js | 25 +++++++++++++++++++++---- packages/mcp/src/client.ts | 25 +++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/mcp/build/client.js b/packages/mcp/build/client.js index 46380a0c..e1d2d82e 100644 --- a/packages/mcp/build/client.js +++ b/packages/mcp/build/client.js @@ -1292,13 +1292,22 @@ export class DocmostClient { replaced = 0; const { doc: nd, replaced: r } = replaceNodeById(liveDoc, nodeId, target); replaced = r; - if (replaced === 0) - return null; // no match -> skip the write entirely + // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS: Docmost + // duplicates block ids on copy/paste (and copyPageContent writes them + // verbatim), so replacing "the node with id X" would silently clobber + // EVERY duplicate (#159). Refuse: skip the write and throw below so the + // model re-targets with a more specific anchor instead of corrupting the + // page. Only an unambiguous single match is written. + if (replaced !== 1) + return null; return nd; }); if (replaced === 0) { throw new Error(`patch_node: no node with id "${nodeId}" found on page ${pageId}`); } + if (replaced > 1) { + throw new Error(`patch_node: id "${nodeId}" is ambiguous — ${replaced} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to replace all of them; nothing was changed. Re-target with a more specific anchor.`); + } return { success: true, replaced, nodeId, verify: mutation.verify }; } /** @@ -1381,13 +1390,21 @@ export class DocmostClient { deleted = 0; const { doc: nd, deleted: d } = deleteNodeById(liveDoc, nodeId); deleted = d; - if (deleted === 0) - return null; // no match -> skip the write entirely + // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS (block + // ids are duplicated on copy/paste, #159): deleting "the node with id X" + // would silently remove EVERY duplicate. Refuse: skip the write and throw + // below so the model re-targets. Only an unambiguous single match is + // deleted. + if (deleted !== 1) + return null; return nd; }); if (deleted === 0) { throw new Error(`delete_node: no node with id "${nodeId}" found on page ${pageId}`); } + if (deleted > 1) { + throw new Error(`delete_node: id "${nodeId}" is ambiguous — ${deleted} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to delete all of them; nothing was changed. Re-target with a more specific anchor.`); + } return { success: true, deleted, nodeId, verify: mutation.verify }; } /** Build the public share URL for a page. */ diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 6293d5ee..c6419563 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -1625,7 +1625,13 @@ export class DocmostClient { replaced = 0; const { doc: nd, replaced: r } = replaceNodeById(liveDoc, nodeId, target); replaced = r; - if (replaced === 0) return null; // no match -> skip the write entirely + // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS: Docmost + // duplicates block ids on copy/paste (and copyPageContent writes them + // verbatim), so replacing "the node with id X" would silently clobber + // EVERY duplicate (#159). Refuse: skip the write and throw below so the + // model re-targets with a more specific anchor instead of corrupting the + // page. Only an unambiguous single match is written. + if (replaced !== 1) return null; return nd; }, ); @@ -1635,6 +1641,11 @@ export class DocmostClient { `patch_node: no node with id "${nodeId}" found on page ${pageId}`, ); } + if (replaced > 1) { + throw new Error( + `patch_node: id "${nodeId}" is ambiguous — ${replaced} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to replace all of them; nothing was changed. Re-target with a more specific anchor.`, + ); + } return { success: true, replaced, nodeId, verify: mutation.verify }; } @@ -1755,7 +1766,12 @@ export class DocmostClient { deleted = 0; const { doc: nd, deleted: d } = deleteNodeById(liveDoc, nodeId); deleted = d; - if (deleted === 0) return null; // no match -> skip the write entirely + // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS (block + // ids are duplicated on copy/paste, #159): deleting "the node with id X" + // would silently remove EVERY duplicate. Refuse: skip the write and throw + // below so the model re-targets. Only an unambiguous single match is + // deleted. + if (deleted !== 1) return null; return nd; }, ); @@ -1765,6 +1781,11 @@ export class DocmostClient { `delete_node: no node with id "${nodeId}" found on page ${pageId}`, ); } + if (deleted > 1) { + throw new Error( + `delete_node: id "${nodeId}" is ambiguous — ${deleted} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to delete all of them; nothing was changed. Re-target with a more specific anchor.`, + ); + } return { success: true, deleted, nodeId, verify: mutation.verify }; }