vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

fix(mcp): refuse ambiguous patch_node/delete_node on duplicated ids (#159)

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-25 04:33:57 +03:00
parent 3ebe24bee2
commit 0441a2ee75
2 changed files with 44 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
packages/mcp/build/client.js
View File

@@ -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. */

25
packages/mcp/src/client.ts
View File

@@ -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 };
}