diff --git a/apps/server/src/core/page/services/page.service.spec.ts b/apps/server/src/core/page/services/page.service.spec.ts index bb387b31..61cd74fe 100644 --- a/apps/server/src/core/page/services/page.service.spec.ts +++ b/apps/server/src/core/page/services/page.service.spec.ts @@ -57,11 +57,28 @@ describe('PageService', () => { const eventEmitter = { emit: jest.fn() }; + // movePage now runs the cycle-check + UPDATE inside executeTx(this.db), + // i.e. this.db.transaction().execute(fn => fn(trx)). A permissive chainable + // Proxy stands in for the Kysely trx so the per-space advisory-lock + // `sql``.execute(trx)` resolves; a thrown BadRequestException still + // propagates out of the transaction unchanged. + const trxStub: any = new Proxy(function () {}, { + get: (_t, p) => + p === 'then' + ? undefined + : p === 'execute' || p === 'executeTakeFirst' + ? () => Promise.resolve([]) + : () => trxStub, + }); + const db = { + transaction: () => ({ execute: (fn: any) => fn(trxStub) }), + }; + const svc = new PageService( pageRepo as any, // pageRepo {} as any, // pagePermissionRepo {} as any, // attachmentRepo - {} as any, // db + db as any, // db {} as any, // storageService {} as any, // attachmentQueue {} as any, // aiQueue @@ -268,9 +285,23 @@ describe('PageService', () => { }), updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }), }; + // movePage now runs the cycle-check + UPDATE inside executeTx(this.db), + // which calls this.db.transaction().execute(fn => fn(trx)). A permissive + // chainable Proxy stands in for the Kysely trx so the per-space + // advisory-lock `sql``.execute(trx)` resolves and updatePage receives it. + const trxStub: any = new Proxy(function () {}, { + get: (_t, p) => + p === 'then' + ? undefined + : p === 'execute' || p === 'executeTakeFirst' + ? () => Promise.resolve([]) + : () => trxStub, + }); const svc = makeSvc({ pageRepo, - db: {} as any, + db: { + transaction: () => ({ execute: (fn: any) => fn(trxStub) }), + } as any, }); // Legitimate move: destination ancestors do NOT include the moved page. jest diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index ff205350..fc6bfd56 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -15,13 +15,13 @@ import { executeWithCursorPagination, } from '@docmost/db/pagination/cursor-pagination'; import { InjectKysely } from 'nestjs-kysely'; -import { KyselyDB } from '@docmost/db/types/kysely.types'; +import { KyselyDB, KyselyTransaction } from '@docmost/db/types/kysely.types'; import { generateJitteredKeyBetween } from 'fractional-indexing-jittered'; import { MovePageDto } from '../dto/move-page.dto'; import { shapeSidebarPagesTree } from './sidebar-pages-tree.util'; import { generateSlugId } from '../../../common/helpers'; import { getPageTitle } from '../../../common/helpers'; -import { executeTx } from '@docmost/db/utils'; +import { dbOrTx, executeTx } from '@docmost/db/utils'; import { AttachmentRepo } from '@docmost/db/repos/attachment/attachment.repo'; import { v7 as uuid7 } from 'uuid'; import { @@ -62,6 +62,13 @@ import { agentSourceFields, } from '../../../common/decorators/auth-provenance.decorator'; +// Advisory-lock namespace (the first key of pg_advisory_xact_lock) used to +// serialize concurrent page moves within a single space so the cycle check and +// the move UPDATE stay atomic (see movePage, #207 #7). A dedicated namespace +// constant keeps these locks from colliding with any other advisory lock; the +// second key is hashtext(spaceId). Fits a signed int4 ('page' in ASCII). +const PAGE_MOVE_LOCK_NAMESPACE = 0x70616765; + @Injectable() export class PageService { private readonly logger = new Logger(PageService.name); @@ -915,34 +922,61 @@ 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'); - } - } + // Server-side cycle guard + the move UPDATE run in ONE transaction. 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. Crucially, doing the guard + // and the write as two separate, unlocked statements is a TOCTOU race: two + // concurrent moves ("A under B" and "B under A") can each read the same + // pre-write acyclic snapshot, both pass the guard, then persist + // A.parentPageId=B AND B.parentPageId=A — a parent/child cycle (#207 #7). A + // per-space advisory lock (held until COMMIT) serializes all moves within a + // space: the second mover blocks until the first commits and then sees the + // freshly written parent, so its guard rejects the cycle. + const updateResult = await executeTx(this.db, async (trx) => { + await sql`select pg_advisory_xact_lock(${sql.lit( + PAGE_MOVE_LOCK_NAMESPACE, + )}, hashtext(${movedPage.spaceId}))`.execute(trx); - const updateResult = await this.pageRepo.updatePage( - { - position: dto.position, - parentPageId: parentPageId, - // Agent-edit provenance: annotate the source on an agent move. A normal - // user request leaves the existing source value unchanged. - ...agentSourceFields(provenance, 'lastUpdatedSource', 'lastUpdatedAiChatId'), - }, - dto.pageId, - ); + // 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) inside the lock. 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, + trx, + ); + if (destAncestors.some((ancestor) => ancestor.id === dto.pageId)) { + throw new BadRequestException( + 'Cannot move a page into its own subtree', + ); + } + } + + return this.pageRepo.updatePage( + { + position: dto.position, + parentPageId: parentPageId, + // Agent-edit provenance: annotate the source on an agent move. A + // normal user request leaves the existing source value unchanged. + ...agentSourceFields( + provenance, + 'lastUpdatedSource', + 'lastUpdatedAiChatId', + ), + }, + dto.pageId, + trx, + ); + }); // 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 @@ -981,8 +1015,8 @@ export class PageService { }); } - async getPageBreadCrumbs(childPageId: string) { - const ancestors = await this.db + async getPageBreadCrumbs(childPageId: string, trx?: KyselyTransaction) { + const ancestors = await dbOrTx(this.db, trx) .withRecursive('page_ancestors', (db) => db .selectFrom('pages') diff --git a/apps/server/test/integration/page-move-cycle.int-spec.ts b/apps/server/test/integration/page-move-cycle.int-spec.ts new file mode 100644 index 00000000..be7dd5c6 --- /dev/null +++ b/apps/server/test/integration/page-move-cycle.int-spec.ts @@ -0,0 +1,133 @@ +import { Kysely } from 'kysely'; +import { generateJitteredKeyBetween } from 'fractional-indexing-jittered'; +import { PageRepo } from '@docmost/db/repos/page/page.repo'; +import { PageService } from 'src/core/page/services/page.service'; +import { Page } from '@docmost/db/types/entity.types'; +import { + getTestDb, + destroyTestDb, + createWorkspace, + createSpace, + createPage, +} from './db'; + +/** + * #207 #7 — TOCTOU in PageService.movePage: two concurrent moves + * ("A under B" + "B under A") must NOT be able to persist a parent/child cycle. + * + * Before the fix the cycle check (getPageBreadCrumbs) and the UPDATE were two + * separate, unlocked statements, so both movers could read the same pre-write + * acyclic snapshot, both pass the guard, and persist A.parentPageId=B AND + * B.parentPageId=A. The fix runs the guard + UPDATE in one transaction behind a + * per-space advisory lock, so the moves serialize: whichever commits second + * sees the first's write and its guard rejects the cycle. + * + * This test drives the real PageService.movePage against a real Postgres, + * firing the two opposing moves concurrently, and asserts that no cycle ever + * persists (walking parentPageId from both pages always reaches a root with no + * repeated id) and that exactly one of the two opposing moves is rejected. + */ +describe('PageService.movePage concurrent A<->B cycle guard [integration]', () => { + let db: Kysely; + let pageRepo: PageRepo; + let pageService: PageService; + let workspaceId: string; + let spaceId: string; + + // A valid fractional-index position key; movePage validates the position. + const position = generateJitteredKeyBetween(null, null); + + beforeAll(async () => { + db = getTestDb(); + // Event emission is a side effect movePage performs but the cycle behaviour + // does not depend on; a no-op emitter keeps the harness minimal. + const eventEmitter = { emit: () => true } as any; + pageRepo = new PageRepo(db as any, {} as any, eventEmitter); + // Only pageRepo (1), db (4) and eventEmitter (9) are touched by movePage; + // the remaining constructor deps are unused on this path. + pageService = new PageService( + pageRepo, + undefined as any, + undefined as any, + db as any, + undefined as any, + undefined as any, + undefined as any, + undefined as any, + eventEmitter, + undefined as any, + undefined as any, + undefined as any, + ); + + workspaceId = (await createWorkspace(db)).id; + spaceId = (await createSpace(db, workspaceId)).id; + }); + + afterAll(async () => { + await destroyTestDb(); + }); + + async function findPage(id: string): Promise { + const page = await pageRepo.findById(id); + if (!page) throw new Error(`page ${id} not found`); + return page; + } + + // Walk parentPageId upward from startId. Throws if a node repeats (cycle) or + // the walk fails to terminate; returns normally only when a root is reached. + async function assertReachesRoot(startId: string): Promise { + const seen = new Set(); + let cur: string | null = startId; + let steps = 0; + while (cur) { + if (seen.has(cur)) { + throw new Error(`cycle detected: revisited ${cur}`); + } + seen.add(cur); + const row: { parentPageId: string | null } | undefined = await db + .selectFrom('pages') + .select('parentPageId') + .where('id', '=', cur) + .executeTakeFirst(); + cur = row?.parentPageId ?? null; + if (++steps > 1000) { + throw new Error('parent walk did not terminate'); + } + } + } + + it('two opposing concurrent moves never persist a parent/child cycle', async () => { + // Repeat to exercise different scheduler interleavings of the two moves. + for (let i = 0; i < 8; i++) { + const a = await createPage(db, { workspaceId, spaceId, title: `A-${i}` }); + const b = await createPage(db, { workspaceId, spaceId, title: `B-${i}` }); + + const movedA = await findPage(a.id); + const movedB = await findPage(b.id); + + const results = await Promise.allSettled([ + pageService.movePage( + { pageId: a.id, parentPageId: b.id, position } as any, + movedA, + ), + pageService.movePage( + { pageId: b.id, parentPageId: a.id, position } as any, + movedB, + ), + ]); + + // No cycle may have been persisted by either ordering. + await assertReachesRoot(a.id); + await assertReachesRoot(b.id); + + // The serialization guarantees exactly one of the opposing moves wins; + // the other must be rejected as a subtree cycle. + const rejected = results.filter( + (r): r is PromiseRejectedResult => r.status === 'rejected', + ); + expect(rejected).toHaveLength(1); + expect(rejected[0].reason?.message).toMatch(/into its own subtree/); + } + }); +});