#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';
|
||||
|
||||
await this.pageRepo.updatePage(
|
||||
const updateResult = await this.pageRepo.updatePage(
|
||||
{
|
||||
position: dto.position,
|
||||
parentPageId: parentPageId,
|
||||
@@ -982,6 +1000,13 @@ export class PageService {
|
||||
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
|
||||
// used to drive the tree `moveTreeNode` broadcast: it also fires on rename /
|
||||
// content-save and carries neither oldParentId nor the new position. Emit a
|
||||
|
||||
Reference in New Issue
Block a user