diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 1c3a3e256..38d7c1b7f 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -133,24 +133,27 @@ export class PageService { ydoc = createYdocFromJson(prosemirrorJson); } - const page = await this.pageRepo.insertPage({ - slugId: generateSlugId(), - title: createPageDto.title, - position: await this.nextPagePosition( - createPageDto.spaceId, - parentPageId, - ), - icon: createPageDto.icon, - parentPageId: parentPageId, - spaceId: createPageDto.spaceId, - creatorId: userId, - workspaceId: workspaceId, - lastUpdatedById: userId, - isBase, - content, - textContent, - ydoc, - }, trx); + const page = await this.pageRepo.insertPage( + { + slugId: generateSlugId(), + title: createPageDto.title, + position: await this.nextPagePosition( + createPageDto.spaceId, + parentPageId, + ), + icon: createPageDto.icon, + parentPageId: parentPageId, + spaceId: createPageDto.spaceId, + creatorId: userId, + workspaceId: workspaceId, + lastUpdatedById: userId, + isBase, + content, + textContent, + ydoc, + }, + trx, + ); if (trx) { // Add the watcher inside the caller's transaction so the async worker @@ -1034,19 +1037,29 @@ export class PageService { .withRecursive('page_descendants', (db) => db .selectFrom('pages') - .select(['id']) + .select([ + 'id', + sql`ARRAY[id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + ]) .where('id', '=', pageId) .unionAll((exp) => exp .selectFrom('pages as p') - .select(['p.id']) - .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId'), + .select([ + 'p.id', + sql`pd.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(pd.traversal_path)`.as('isCycle'), + ]) + .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId') + .where('pd.isCycle', '=', false), ), ) .selectFrom('page_descendants') - .selectAll() + .select(['id', 'isCycle']) .execute(); + assertAcyclicPageTraversal(descendants, pageId); const pageIds = descendants.map((d) => d.id); // Queue attachment deletion for all pages with unique job IDs to prevent duplicates diff --git a/apps/server/src/core/page/services/trash-cleanup.service.ts b/apps/server/src/core/page/services/trash-cleanup.service.ts index 42a4a11a3..792dba026 100644 --- a/apps/server/src/core/page/services/trash-cleanup.service.ts +++ b/apps/server/src/core/page/services/trash-cleanup.service.ts @@ -5,6 +5,8 @@ import { KyselyDB } from '@docmost/db/types/kysely.types'; import { InjectQueue } from '@nestjs/bullmq'; import { Queue } from 'bullmq'; import { QueueJob, QueueName } from '../../../integrations/queue/constants'; +import { assertAcyclicPageTraversal } from '../../../database/helpers/page-hierarchy-cycle'; +import { sql } from 'kysely'; const DEFAULT_RETENTION_DAYS = 30; @@ -76,19 +78,29 @@ export class TrashCleanupService { .withRecursive('page_descendants', (db) => db .selectFrom('pages') - .select(['id']) + .select([ + 'id', + sql`ARRAY[id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + ]) .where('id', '=', pageId) .unionAll((exp) => exp .selectFrom('pages as p') - .select(['p.id']) - .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId'), + .select([ + 'p.id', + sql`pd.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(pd.traversal_path)`.as('isCycle'), + ]) + .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId') + .where('pd.isCycle', '=', false), ), ) .selectFrom('page_descendants') - .selectAll() + .select(['id', 'isCycle']) .execute(); + assertAcyclicPageTraversal(descendants, pageId); const pageIds = descendants.map((d) => d.id); this.logger.debug( diff --git a/apps/server/src/database/repos/page/page.repo.ts b/apps/server/src/database/repos/page/page.repo.ts index 9eb9f3a50..a2906c100 100644 --- a/apps/server/src/database/repos/page/page.repo.ts +++ b/apps/server/src/database/repos/page/page.repo.ts @@ -16,6 +16,10 @@ import { jsonArrayFrom, jsonObjectFrom } from 'kysely/helpers/postgres'; import { SpaceMemberRepo } from '@docmost/db/repos/space/space-member.repo'; import { EventEmitter2 } from '@nestjs/event-emitter'; import { EventName } from '../../../common/events/event.contants'; +import { + assertAcyclicPageTraversal, + stripPageTraversalMetadata, +} from '../../helpers/page-hierarchy-cycle'; @Injectable() export class PageRepo { @@ -203,21 +207,31 @@ export class PageRepo { .withRecursive('page_descendants', (db) => db .selectFrom('pages') - .select(['id']) + .select([ + 'id', + sql`ARRAY[id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + ]) .where('id', '=', pageId) .where('deletedAt', 'is', null) .unionAll((exp) => exp .selectFrom('pages as p') - .select(['p.id']) + .select([ + 'p.id', + sql`pd.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(pd.traversal_path)`.as('isCycle'), + ]) .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId') - .where('p.deletedAt', 'is', null), + .where('p.deletedAt', 'is', null) + .where('pd.isCycle', '=', false), ), ) .selectFrom('page_descendants') - .selectAll() + .select(['id', 'isCycle']) .execute(); + assertAcyclicPageTraversal(descendants, pageId); const pageIds = descendants.map((d) => d.id); if (pageIds.length > 0) { @@ -272,19 +286,29 @@ export class PageRepo { .withRecursive('page_descendants', (db) => db .selectFrom('pages') - .select(['id']) + .select([ + 'id', + sql`ARRAY[id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + ]) .where('id', '=', pageId) .unionAll((exp) => exp .selectFrom('pages as p') - .select(['p.id']) - .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId'), + .select([ + 'p.id', + sql`pd.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(pd.traversal_path)`.as('isCycle'), + ]) + .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId') + .where('pd.isCycle', '=', false), ), ) .selectFrom('page_descendants') - .selectAll() + .select(['id', 'isCycle']) .execute(); + assertAcyclicPageTraversal(pages, pageId); const pageIds = pages.map((p) => p.id); // Restore all pages, but only detach the root page if its parent is deleted @@ -354,7 +378,12 @@ export class PageRepo { }); } - async getCreatedByPages(creatorId: string, requestingUserId: string, pagination: PaginationOptions, spaceId?: string) { + async getCreatedByPages( + creatorId: string, + requestingUserId: string, + pagination: PaginationOptions, + spaceId?: string, + ) { let query = this.db .selectFrom('pages') .select(this.baseFields) @@ -365,7 +394,11 @@ export class PageRepo { if (spaceId) { query = query.where('spaceId', '=', spaceId); } else { - query = query.where('spaceId', 'in', this.spaceMemberRepo.getUserSpaceIdsQuery(requestingUserId)); + query = query.where( + 'spaceId', + 'in', + this.spaceMemberRepo.getUserSpaceIdsQuery(requestingUserId), + ); } return executeWithCursorPagination(query, { @@ -491,7 +524,7 @@ export class PageRepo { parentPageId: string, opts: { includeContent: boolean }, ) { - return this.db + const pages = await this.db .withRecursive('page_hierarchy', (db) => db .selectFrom('pages') @@ -506,6 +539,8 @@ export class PageRepo { 'workspaceId', 'createdAt', 'updatedAt', + sql`ARRAY[id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .$if(opts?.includeContent, (qb) => qb.select('content')) .where('id', '=', parentPageId) @@ -524,15 +559,34 @@ export class PageRepo { 'p.workspaceId', 'p.createdAt', 'p.updatedAt', + sql`ph.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(ph.traversal_path)`.as('isCycle'), ]) .$if(opts?.includeContent, (qb) => qb.select('p.content')) .innerJoin('page_hierarchy as ph', 'p.parentPageId', 'ph.id') - .where('p.deletedAt', 'is', null), + .where('p.deletedAt', 'is', null) + .where('ph.isCycle', '=', false), ), ) .selectFrom('page_hierarchy') - .selectAll() + .select([ + 'id', + 'slugId', + 'title', + 'icon', + 'position', + 'parentPageId', + 'spaceId', + 'workspaceId', + 'createdAt', + 'updatedAt', + 'isCycle', + ]) + .$if(opts?.includeContent, (qb) => qb.select('content')) .execute(); + + assertAcyclicPageTraversal(pages, parentPageId); + return pages.map((page) => stripPageTraversalMetadata(page)); } /** @@ -540,69 +594,82 @@ export class PageRepo { * More efficient than getPageAndDescendants + filtering because: * 1. Single DB query (no separate restricted IDs query) * 2. Stops traversing at restricted pages (doesn't fetch data to discard) - * 3. No in-memory filtering needed + * 3. Filters the bounded traversal only after hierarchy validation */ async getPageAndDescendantsExcludingRestricted( parentPageId: string, opts: { includeContent: boolean }, ) { - return ( - this.db - .withRecursive('page_hierarchy', (db) => - db - .selectFrom('pages') - .leftJoin('pageAccess', 'pageAccess.pageId', 'pages.id') - .select([ - 'pages.id', - 'pages.slugId', - 'pages.title', - 'pages.icon', - 'pages.position', - 'pages.parentPageId', - 'pages.spaceId', - 'pages.workspaceId', - sql`page_access.id IS NOT NULL`.as('isRestricted'), - ]) - .$if(opts?.includeContent, (qb) => qb.select('pages.content')) - .where('pages.id', '=', parentPageId) - .where('pages.deletedAt', 'is', null) - .unionAll((exp) => - exp - .selectFrom('pages as p') - .innerJoin('page_hierarchy as ph', 'p.parentPageId', 'ph.id') - .leftJoin('pageAccess', 'pageAccess.pageId', 'p.id') - .select([ - 'p.id', - 'p.slugId', - 'p.title', - 'p.icon', - 'p.position', - 'p.parentPageId', - 'p.spaceId', - 'p.workspaceId', - sql`page_access.id IS NOT NULL`.as('isRestricted'), - ]) - .$if(opts?.includeContent, (qb) => qb.select('p.content')) - .where('p.deletedAt', 'is', null) - // Only recurse into children of non-restricted pages - .where('ph.isRestricted', '=', false), - ), - ) - .selectFrom('page_hierarchy') - .select([ - 'id', - 'slugId', - 'title', - 'icon', - 'position', - 'parentPageId', - 'spaceId', - 'workspaceId', - ]) - .$if(opts?.includeContent, (qb) => qb.select('content')) - // Filter out restricted pages from the result - .where('isRestricted', '=', false) - .execute() - ); + const pages = await this.db + .withRecursive('page_hierarchy', (db) => + db + .selectFrom('pages') + .leftJoin('pageAccess', 'pageAccess.pageId', 'pages.id') + .select([ + 'pages.id', + 'pages.slugId', + 'pages.title', + 'pages.icon', + 'pages.position', + 'pages.parentPageId', + 'pages.spaceId', + 'pages.workspaceId', + sql`page_access.id IS NOT NULL`.as('isRestricted'), + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + ]) + .$if(opts?.includeContent, (qb) => qb.select('pages.content')) + .where('pages.id', '=', parentPageId) + .where('pages.deletedAt', 'is', null) + .unionAll((exp) => + exp + .selectFrom('pages as p') + .innerJoin('page_hierarchy as ph', 'p.parentPageId', 'ph.id') + .leftJoin('pageAccess', 'pageAccess.pageId', 'p.id') + .select([ + 'p.id', + 'p.slugId', + 'p.title', + 'p.icon', + 'p.position', + 'p.parentPageId', + 'p.spaceId', + 'p.workspaceId', + sql`page_access.id IS NOT NULL`.as('isRestricted'), + sql`ph.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(ph.traversal_path)`.as('isCycle'), + ]) + .$if(opts?.includeContent, (qb) => qb.select('p.content')) + .where('p.deletedAt', 'is', null) + // Only recurse into children of non-restricted pages + .where('ph.isRestricted', '=', false) + .where('ph.isCycle', '=', false), + ), + ) + .selectFrom('page_hierarchy') + .select([ + 'id', + 'slugId', + 'title', + 'icon', + 'position', + 'parentPageId', + 'spaceId', + 'workspaceId', + 'isRestricted', + 'isCycle', + ]) + .$if(opts?.includeContent, (qb) => qb.select('content')) + .execute(); + + assertAcyclicPageTraversal(pages, parentPageId); + return pages + .filter((page) => !page.isRestricted) + .map((page) => { + const withoutCycleMetadata = stripPageTraversalMetadata(page); + const { isRestricted: _isRestricted, ...publicPage } = + withoutCycleMetadata; + return publicPage; + }); } } diff --git a/apps/server/test/page-hierarchy-cycle.integration-spec.ts b/apps/server/test/page-hierarchy-cycle.integration-spec.ts index 95d9ad632..8235c0ca7 100644 --- a/apps/server/test/page-hierarchy-cycle.integration-spec.ts +++ b/apps/server/test/page-hierarchy-cycle.integration-spec.ts @@ -1,9 +1,12 @@ import { randomUUID } from 'node:crypto'; +import { Logger } from '@nestjs/common'; import { createCache } from 'cache-manager'; import { PageService } from '../src/core/page/services/page.service'; +import { TrashCleanupService } from '../src/core/page/services/trash-cleanup.service'; import { ShareService } from '../src/core/share/share.service'; import { PageHierarchyCycleError } from '../src/database/helpers/page-hierarchy-cycle'; import { PagePermissionRepo } from '../src/database/repos/page/page-permission.repo'; +import { PageRepo } from '../src/database/repos/page/page.repo'; import { KyselyDB } from '../src/database/types/kysely.types'; import { db, withStatementTimeout } from './support/database'; import { @@ -13,23 +16,36 @@ import { seedTwoPageCycle, } from './support/page-hierarchy-fixtures'; -function createPageService(connection: KyselyDB): PageService { +function createPageService( + connection: KyselyDB, + dependencies: { + attachmentQueue?: { add: jest.Mock }; + eventEmitter?: { emit: jest.Mock }; + } = {}, +): PageService { return new PageService( undefined as never, undefined as never, undefined as never, connection, undefined as never, + dependencies.attachmentQueue as never, undefined as never, undefined as never, - undefined as never, - undefined as never, + dependencies.eventEmitter as never, undefined as never, undefined as never, undefined as never, ); } +function createPageRepo( + connection: KyselyDB, + eventEmitter: { emit: jest.Mock } = { emit: jest.fn() }, +): PageRepo { + return new PageRepo(connection, undefined as never, eventEmitter as never); +} + function createShareService(connection: KyselyDB): ShareService { return new ShareService( undefined as never, @@ -167,7 +183,251 @@ async function insertShare(pageId: string, includeSubPages: boolean) { .executeTakeFirstOrThrow(); } +async function expectPageHierarchyCycle( + operation: Promise, + rootPageId: string, +): Promise { + const error = await operation.then( + () => undefined, + (reason: unknown) => reason, + ); + + expect(error).toBeInstanceOf(PageHierarchyCycleError); + expect(error).toEqual( + expect.objectContaining({ + code: 'PAGE_HIERARCHY_CYCLE', + rootPageId, + }) satisfies Partial, + ); +} + +afterEach(() => { + jest.restoreAllMocks(); +}); + describe('cycle-safe page hierarchy reads', () => { + describe('descendant traversal', () => { + it('returns every page in an acyclic branching tree exactly once without internal metadata', async () => { + const { root, firstChild, secondChild, grandchild } = + await seedBranchingDescendantTree(); + const repo = createPageRepo(db); + + const pages = await repo.getPageAndDescendants(root.id, { + includeContent: false, + }); + + expect(pages.map((page) => page.id).sort()).toEqual( + [root.id, firstChild.id, secondChild.id, grandchild.id].sort(), + ); + expect(pages).toHaveLength(4); + for (const page of pages) { + expect(page).not.toHaveProperty('isCycle'); + expect(page).not.toHaveProperty('traversalPath'); + } + }); + + describe.each([ + ['a self-cycle', async () => (await seedSelfCycle()).self.id], + ['a two-page cycle', async () => (await seedTwoPageCycle()).a.id], + ])('%s', (_cycleName, seedCycle) => { + it('raises PageHierarchyCycleError from the structural descendant read', async () => { + const pageId = await seedCycle(); + + await withStatementTimeout(async (connection) => { + const repo = createPageRepo(connection); + + await expectPageHierarchyCycle( + repo.getPageAndDescendants(pageId, { includeContent: false }), + pageId, + ); + }); + }); + + it('raises PageHierarchyCycleError from the restricted descendant read', async () => { + const pageId = await seedCycle(); + + await withStatementTimeout(async (connection) => { + const repo = createPageRepo(connection); + + await expectPageHierarchyCycle( + repo.getPageAndDescendantsExcludingRestricted(pageId, { + includeContent: false, + }), + pageId, + ); + }); + }); + }); + + it('preserves restricted-subtree exclusion for an acyclic tree', async () => { + const { root, firstChild, secondChild, grandchild } = + await seedBranchingDescendantTree(); + await restrictPage(firstChild.id); + const repo = createPageRepo(db); + + const pages = await repo.getPageAndDescendantsExcludingRestricted( + root.id, + { includeContent: false }, + ); + + expect(pages.map((page) => page.id).sort()).toEqual( + [root.id, secondChild.id].sort(), + ); + expect(pages.map((page) => page.id)).not.toContain(firstChild.id); + expect(pages.map((page) => page.id)).not.toContain(grandchild.id); + for (const page of pages) { + expect(page).not.toHaveProperty('isCycle'); + expect(page).not.toHaveProperty('isRestricted'); + expect(page).not.toHaveProperty('traversalPath'); + } + }); + + it('removePage leaves pages and shares unchanged and emits no event on a cycle', async () => { + const { a, b } = await seedTwoPageCycle(); + const deletedById = await insertTestUser(a.id); + const share = await insertShare(b.id, true); + const eventEmitter = { emit: jest.fn() }; + + await withStatementTimeout(async (connection) => { + const repo = createPageRepo(connection, eventEmitter); + + await expectPageHierarchyCycle( + repo.removePage(a.id, deletedById, share.workspaceId), + a.id, + ); + }); + + const storedPages = await db + .selectFrom('pages') + .select(['id', 'deletedAt', 'deletedById']) + .where('id', 'in', [a.id, b.id]) + .orderBy('id') + .execute(); + const storedShare = await db + .selectFrom('shares') + .select('id') + .where('id', '=', share.id) + .executeTakeFirst(); + + expect(storedPages).toEqual( + [a.id, b.id] + .sort() + .map((id) => ({ id, deletedAt: null, deletedById: null })), + ); + expect(storedShare).toEqual({ id: share.id }); + expect(eventEmitter.emit).not.toHaveBeenCalled(); + }); + + it('restorePage leaves deleted pages unchanged and emits no event on a cycle', async () => { + const { a, b } = await seedTwoPageCycle(); + const deletedAt = new Date('2024-01-02T03:04:05.000Z'); + await db + .updateTable('pages') + .set({ deletedAt }) + .where('id', 'in', [a.id, b.id]) + .execute(); + const eventEmitter = { emit: jest.fn() }; + + await withStatementTimeout(async (connection) => { + const repo = createPageRepo(connection, eventEmitter); + + await expectPageHierarchyCycle( + repo.restorePage(a.id, randomUUID()), + a.id, + ); + }); + + const storedPages = await db + .selectFrom('pages') + .select(['id', 'parentPageId', 'deletedAt']) + .where('id', 'in', [a.id, b.id]) + .orderBy('id') + .execute(); + + expect(storedPages).toEqual( + [ + { id: a.id, parentPageId: b.id, deletedAt }, + { id: b.id, parentPageId: a.id, deletedAt }, + ].sort((left, right) => left.id.localeCompare(right.id)), + ); + expect(eventEmitter.emit).not.toHaveBeenCalled(); + }); + + it('forceDelete leaves pages intact and enqueues and emits nothing on a cycle', async () => { + const { a, b } = await seedTwoPageCycle(); + const attachmentQueue = { add: jest.fn() }; + const eventEmitter = { emit: jest.fn() }; + + await withStatementTimeout(async (connection) => { + const pageService = createPageService(connection, { + attachmentQueue, + eventEmitter, + }); + + await expectPageHierarchyCycle( + pageService.forceDelete(a.id, randomUUID()), + a.id, + ); + }); + + const storedPageIds = await db + .selectFrom('pages') + .select('id') + .where('id', 'in', [a.id, b.id]) + .orderBy('id') + .execute(); + + expect(storedPageIds).toEqual([a.id, b.id].sort().map((id) => ({ id }))); + expect(attachmentQueue.add).not.toHaveBeenCalled(); + expect(eventEmitter.emit).not.toHaveBeenCalled(); + }); + + it('trash cleanup logs and skips a corrupt root before continuing with acyclic trash', async () => { + const { self } = await seedSelfCycle(); + const { root, child, grandchild } = await seedAcyclicPageChain(); + const expiredAt = new Date('2024-01-02T03:04:05.000Z'); + await db + .updateTable('pages') + .set({ deletedAt: expiredAt }) + .where('id', 'in', [self.id, root.id]) + .execute(); + const attachmentQueue = { add: jest.fn() }; + const loggerError = jest + .spyOn(Logger.prototype, 'error') + .mockImplementation(); + + await withStatementTimeout(async (connection) => { + const timeoutCleanupService = new TrashCleanupService( + connection, + attachmentQueue as never, + ); + await timeoutCleanupService.cleanupOldTrash(); + }); + + const corruptPage = await db + .selectFrom('pages') + .select(['id', 'deletedAt']) + .where('id', '=', self.id) + .executeTakeFirst(); + const cleanedPageIds = await db + .selectFrom('pages') + .select('id') + .where('id', 'in', [root.id, child.id, grandchild.id]) + .execute(); + + expect(corruptPage).toEqual({ id: self.id, deletedAt: expiredAt }); + expect(cleanedPageIds).toEqual([]); + expect(attachmentQueue.add).toHaveBeenCalledTimes(3); + expect( + attachmentQueue.add.mock.calls.map(([, payload]) => payload.pageId), + ).toEqual(expect.arrayContaining([root.id, child.id, grandchild.id])); + expect(loggerError).toHaveBeenCalledWith( + expect.stringContaining(`Failed to cleanup page ${self.id}`), + expect.any(String), + ); + }); + }); + describe('PageService.getPageBreadCrumbs', () => { it('returns every acyclic breadcrumb exactly once in root-to-child order', async () => { const { root, child, grandchild } = await seedAcyclicPageChain();