#67: movePage didn't check the destination wasn't the page itself or inside its own subtree, so MCP/REST/agent/fast-drag could persist+broadcast a cycle. Reject before the update (self-parent, or moved page among the destination parent's ancestors via getPageBreadCrumbs). #64: movePage emitted PAGE_MOVED from a stale pre-read even when the row didn't change / was concurrently deleted (phantom move). Gate the emit on updateResult.numUpdatedRows !== 0n. Both are movePage hardening in one method. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -964,9 +964,27 @@ export class PageService {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Server-side cycle guard: a page may not be moved into itself or into any
|
||||||
|
// page within its own subtree. Without this, an MCP/REST/agent caller (or a
|
||||||
|
// fast drag racing the client check) could persist a cycle and broadcast it.
|
||||||
|
// Only relevant when re-parenting under a concrete parent; moving to root
|
||||||
|
// (parentPageId null/undefined) can never create a cycle.
|
||||||
|
if (dto.parentPageId) {
|
||||||
|
if (dto.parentPageId === dto.pageId) {
|
||||||
|
throw new BadRequestException('Cannot move a page into its own subtree');
|
||||||
|
}
|
||||||
|
// Walk the destination parent's ancestor chain (reusing the breadcrumb
|
||||||
|
// ancestor CTE). If the page being moved appears among those ancestors,
|
||||||
|
// the destination lives inside the moved page's subtree -> cycle.
|
||||||
|
const destAncestors = await this.getPageBreadCrumbs(dto.parentPageId);
|
||||||
|
if (destAncestors.some((ancestor) => ancestor.id === dto.pageId)) {
|
||||||
|
throw new BadRequestException('Cannot move a page into its own subtree');
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
const isAgent = provenance?.actor === 'agent';
|
const isAgent = provenance?.actor === 'agent';
|
||||||
|
|
||||||
await this.pageRepo.updatePage(
|
const updateResult = await this.pageRepo.updatePage(
|
||||||
{
|
{
|
||||||
position: dto.position,
|
position: dto.position,
|
||||||
parentPageId: parentPageId,
|
parentPageId: parentPageId,
|
||||||
@@ -982,6 +1000,13 @@ export class PageService {
|
|||||||
dto.pageId,
|
dto.pageId,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Guard against a phantom broadcast: if the row was concurrently deleted or
|
||||||
|
// otherwise not updated, skip the PAGE_MOVED event so we don't replay a move
|
||||||
|
// built from the stale pre-read snapshot to every connected client.
|
||||||
|
if (!updateResult || updateResult.numUpdatedRows === 0n) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
// The generic PAGE_UPDATED emitted by updatePage above is intentionally NOT
|
// The generic PAGE_UPDATED emitted by updatePage above is intentionally NOT
|
||||||
// used to drive the tree `moveTreeNode` broadcast: it also fires on rename /
|
// used to drive the tree `moveTreeNode` broadcast: it also fires on rename /
|
||||||
// content-save and carries neither oldParentId nor the new position. Emit a
|
// content-save and carries neither oldParentId nor the new position. Emit a
|
||||||
|
|||||||
Reference in New Issue
Block a user