From 8e35bd0a62632e0f59931327fed00679592ec54d Mon Sep 17 00:00:00 2001 From: Philipinho <16838612+Philipinho@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:42:16 +0100 Subject: [PATCH] fix: stop cyclic breadcrumb and share traversal --- .../src/core/page/services/page.service.ts | 28 ++- apps/server/src/core/share/share.service.ts | 87 ++++++--- .../page-hierarchy-cycle.integration-spec.ts | 183 ++++++++++++++++-- 3 files changed, 249 insertions(+), 49 deletions(-) diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index cabfb0c74..6e8db0fc0 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -55,6 +55,10 @@ import { markdownToHtml } from '@docmost/editor-ext'; import { WatcherService } from '../../watcher/watcher.service'; import { sql } from 'kysely'; import { TransclusionService } from '../transclusion/transclusion.service'; +import { + assertAcyclicPageTraversal, + stripPageTraversalMetadata, +} from '../../../database/helpers/page-hierarchy-cycle'; @Injectable() export class PageService { @@ -867,6 +871,8 @@ export class PageService { 'parentPageId', 'spaceId', 'deletedAt', + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where('id', '=', childPageId) .where('deletedAt', 'is', null) @@ -883,13 +889,27 @@ export class PageService { 'p.parentPageId', 'p.spaceId', 'p.deletedAt', + sql`pa.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(pa.traversal_path)`.as('isCycle'), ]) .innerJoin('page_ancestors as pa', 'pa.parentPageId', 'p.id') - .where('p.deletedAt', 'is', null), + .where('p.deletedAt', 'is', null) + .where('pa.isCycle', '=', false), ), ) .selectFrom('page_ancestors') - .selectAll('page_ancestors') + .select([ + 'id', + 'slugId', + 'title', + 'icon', + 'isBase', + 'position', + 'parentPageId', + 'spaceId', + 'deletedAt', + 'isCycle', + ]) .select((eb) => eb .exists( @@ -903,7 +923,9 @@ export class PageService { ) .execute(); - return ancestors.reverse(); + assertAcyclicPageTraversal(ancestors, childPageId); + + return ancestors.reverse().map(stripPageTraversalMetadata); } async getRecentSpacePages( diff --git a/apps/server/src/core/share/share.service.ts b/apps/server/src/core/share/share.service.ts index e567e6caa..6051ba4cd 100644 --- a/apps/server/src/core/share/share.service.ts +++ b/apps/server/src/core/share/share.service.ts @@ -26,6 +26,7 @@ import { validate as isValidUUID } from 'uuid'; import { sql } from 'kysely'; import { TransclusionService } from '../page/transclusion/transclusion.service'; import { TransclusionLookup } from '../page/transclusion/transclusion.types'; +import { stripPageTraversalMetadata } from '../../database/helpers/page-hierarchy-cycle'; @Injectable() export class ShareService { @@ -144,7 +145,7 @@ export class ShareService { async getShareForPage(pageId: string, workspaceId: string) { // here we try to check if a page was shared directly or if it inherits the share from its closest shared ancestor - const share = await this.db + const traversal = await this.db .withRecursive('page_hierarchy', (cte) => cte .selectFrom('pages') @@ -164,41 +165,67 @@ export class ShareService { 'shares.spaceId', 'shares.workspaceId', 'shares.createdAt', + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where(isValidUUID(pageId) ? 'pages.id' : 'pages.slugId', '=', pageId) .where('pages.deletedAt', 'is', null) - .unionAll( - (union) => - union - .selectFrom('pages as p') - .innerJoin('page_hierarchy as ph', 'ph.parentPageId', 'p.id') - .leftJoin('shares as s', 's.pageId', 'p.id') - .select([ - 'p.id', - 'p.slugId', - 'p.title', - 'p.icon', - 'p.parentPageId', - sql`ph.level + 1`.as('level'), - 's.id as shareId', - 's.key as shareKey', - 's.includeSubPages', - 's.searchIndexing', - 's.creatorId', - 's.spaceId', - 's.workspaceId', - 's.createdAt', - ]) - .where('p.deletedAt', 'is', null) - .where(sql`ph.share_id`, 'is', null) // stop if share found - .where(sql`ph.level`, '<', sql`25`), // prevent loop + .unionAll((union) => + union + .selectFrom('pages as p') + .innerJoin('page_hierarchy as ph', 'ph.parentPageId', 'p.id') + .leftJoin('shares as s', 's.pageId', 'p.id') + .select([ + 'p.id', + 'p.slugId', + 'p.title', + 'p.icon', + 'p.parentPageId', + sql`ph.level + 1`.as('level'), + 's.id as shareId', + 's.key as shareKey', + 's.includeSubPages', + 's.searchIndexing', + 's.creatorId', + 's.spaceId', + 's.workspaceId', + 's.createdAt', + sql`ph.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(ph.traversal_path)`.as('isCycle'), + ]) + .where('p.deletedAt', 'is', null) + .where(sql`ph.share_id`, 'is', null) // stop if share found + .where('ph.isCycle', '=', false), ), ) .selectFrom('page_hierarchy') - .selectAll() - .where('shareId', 'is not', null) - .limit(1) - .executeTakeFirst(); + .select([ + 'id', + 'slugId', + 'title', + 'icon', + 'parentPageId', + 'level', + 'shareId', + 'shareKey', + 'includeSubPages', + 'searchIndexing', + 'creatorId', + 'spaceId', + 'workspaceId', + 'createdAt', + 'isCycle', + ]) + .execute(); + + if (traversal.some((row) => row.isCycle)) { + return undefined; + } + + const matchedShare = traversal.find((row) => row.shareId !== null); + const share = matchedShare + ? stripPageTraversalMetadata(matchedShare) + : undefined; if (!share || share.workspaceId !== workspaceId) { return undefined; diff --git a/apps/server/test/page-hierarchy-cycle.integration-spec.ts b/apps/server/test/page-hierarchy-cycle.integration-spec.ts index 5ecdb76de..5111a4983 100644 --- a/apps/server/test/page-hierarchy-cycle.integration-spec.ts +++ b/apps/server/test/page-hierarchy-cycle.integration-spec.ts @@ -1,21 +1,172 @@ -import { db } from './support/database'; -import { seedAcyclicPageChain } from './support/page-hierarchy-fixtures'; +import { randomUUID } from 'node:crypto'; +import { PageService } from '../src/core/page/services/page.service'; +import { ShareService } from '../src/core/share/share.service'; +import { PageHierarchyCycleError } from '../src/database/helpers/page-hierarchy-cycle'; +import { KyselyDB } from '../src/database/types/kysely.types'; +import { db, withStatementTimeout } from './support/database'; +import { + seedAcyclicPageChain, + seedSelfCycle, + seedTwoPageCycle, +} from './support/page-hierarchy-fixtures'; -describe('page hierarchy cycle integration harness', () => { - it('inserts and reads an acyclic page chain through the test Kysely instance', async () => { - const { root, child, grandchild } = await seedAcyclicPageChain(); +function createPageService(connection: KyselyDB): PageService { + return new PageService( + undefined as never, + undefined as never, + undefined as never, + connection, + undefined as never, + undefined as never, + undefined as never, + undefined as never, + undefined as never, + undefined as never, + undefined as never, + undefined as never, + ); +} - const pages = await db - .selectFrom('pages') - .select(['id', 'parentPageId', 'title']) - .where('id', 'in', [root.id, child.id, grandchild.id]) - .orderBy('title') - .execute(); +function createShareService(connection: KyselyDB): ShareService { + return new ShareService( + undefined as never, + undefined as never, + undefined as never, + connection, + undefined as never, + undefined as never, + ); +} - expect(pages).toEqual([ - { id: child.id, parentPageId: root.id, title: 'Child' }, - { id: grandchild.id, parentPageId: child.id, title: 'Grandchild' }, - { id: root.id, parentPageId: null, title: 'Root' }, - ]); +async function insertShare(pageId: string, includeSubPages: boolean) { + const page = await db + .selectFrom('pages') + .select(['spaceId', 'workspaceId']) + .where('id', '=', pageId) + .executeTakeFirstOrThrow(); + + return db + .insertInto('shares') + .values({ + includeSubPages, + key: `cycle-test-${randomUUID()}`, + pageId, + searchIndexing: false, + spaceId: page.spaceId, + workspaceId: page.workspaceId, + }) + .returning(['id', 'workspaceId']) + .executeTakeFirstOrThrow(); +} + +describe('cycle-safe page hierarchy reads', () => { + describe('PageService.getPageBreadCrumbs', () => { + it('returns every acyclic breadcrumb exactly once in root-to-child order', async () => { + const { root, child, grandchild } = await seedAcyclicPageChain(); + const pageService = createPageService(db); + + const breadcrumbs = await pageService.getPageBreadCrumbs(grandchild.id); + + expect(breadcrumbs.map(({ id, title }) => ({ id, title }))).toEqual([ + { id: root.id, title: 'Root' }, + { id: child.id, title: 'Child' }, + { id: grandchild.id, title: 'Grandchild' }, + ]); + expect(breadcrumbs).toHaveLength(3); + for (const breadcrumb of breadcrumbs) { + expect(breadcrumb).not.toHaveProperty('isCycle'); + expect(breadcrumb).not.toHaveProperty('traversalPath'); + } + }); + + it('raises PageHierarchyCycleError for a self-cycle', async () => { + const { self } = await seedSelfCycle(); + + await withStatementTimeout(async (connection) => { + const pageService = createPageService(connection); + + await expect(pageService.getPageBreadCrumbs(self.id)).rejects.toEqual( + expect.objectContaining({ + code: 'PAGE_HIERARCHY_CYCLE', + rootPageId: self.id, + }) satisfies Partial, + ); + }); + }); + + it('raises PageHierarchyCycleError for a two-page cycle', async () => { + const { a } = await seedTwoPageCycle(); + + await withStatementTimeout(async (connection) => { + const pageService = createPageService(connection); + + await expect(pageService.getPageBreadCrumbs(a.id)).rejects.toEqual( + expect.objectContaining({ + code: 'PAGE_HIERARCHY_CYCLE', + rootPageId: a.id, + }) satisfies Partial, + ); + }); + }); + }); + + describe('ShareService.getShareForPage', () => { + it('returns the nearest valid inherited share without a depth limit', async () => { + const { root, grandchild } = await seedAcyclicPageChain(); + const context = await db + .selectFrom('pages') + .select(['spaceId', 'workspaceId']) + .where('id', '=', root.id) + .executeTakeFirstOrThrow(); + let descendantId = grandchild.id; + + for (let depth = 3; depth <= 26; depth += 1) { + const descendant = await db + .insertInto('pages') + .values({ + parentPageId: descendantId, + slugId: randomUUID(), + spaceId: context.spaceId, + title: `Descendant ${depth}`, + workspaceId: context.workspaceId, + }) + .returning('id') + .executeTakeFirstOrThrow(); + descendantId = descendant.id; + } + + const storedShare = await insertShare(root.id, true); + const shareService = createShareService(db); + + const share = await shareService.getShareForPage( + descendantId, + context.workspaceId, + ); + + expect(share).toEqual( + expect.objectContaining({ + id: storedShare.id, + pageId: root.id, + level: 26, + }), + ); + }); + + it('returns undefined for a cyclic chain with no reachable share', async () => { + const { a } = await seedTwoPageCycle(); + const { workspaceId } = await db + .selectFrom('pages') + .select('workspaceId') + .where('id', '=', a.id) + .executeTakeFirstOrThrow(); + + await withStatementTimeout(async (connection) => { + const shareService = createShareService(connection); + + await expect( + shareService.getShareForPage(a.id, workspaceId), + ).resolves.toBeUndefined(); + }); + }); }); });