From 7a9bba744029c929fb8e5678de1294dcb530ca22 Mon Sep 17 00:00:00 2001 From: Philipinho <16838612+Philipinho@users.noreply.github.com> Date: Tue, 8 Sep 2026 01:53:20 +0100 Subject: [PATCH] poc via visited ids --- apps/server/package.json | 1 + .../src/core/page/services/page.service.ts | 88 ++++-- .../page/services/trash-cleanup.service.ts | 57 ++-- apps/server/src/core/share/share.service.ts | 148 ++++------ .../helpers/page-hierarchy-cycle.spec.ts | 53 ++++ .../database/helpers/page-hierarchy-cycle.ts | 28 ++ .../repos/page/page-permission.repo.ts | 252 ++++++++++++------ .../src/database/repos/page/page.repo.ts | 211 ++++++++++----- apps/server/src/ee | 2 +- 9 files changed, 553 insertions(+), 287 deletions(-) create mode 100644 apps/server/src/database/helpers/page-hierarchy-cycle.spec.ts create mode 100644 apps/server/src/database/helpers/page-hierarchy-cycle.ts diff --git a/apps/server/package.json b/apps/server/package.json index c0a9ec88d..f47fa7f5a 100644 --- a/apps/server/package.json +++ b/apps/server/package.json @@ -24,6 +24,7 @@ "migration:codegen": "kysely-codegen --dialect=postgres --camel-case --env-file=../../.env --out-file=./src/database/types/db.d.ts", "lint": "eslint \"{src,apps,libs,test}/**/*.ts\" --fix", "test": "jest", + "test:integration": "jest --config test/jest-integration.json --runInBand", "test:watch": "jest --watch", "test:cov": "jest --coverage", "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand", diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index cabfb0c74..38d7c1b7f 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 { @@ -129,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 @@ -867,6 +874,9 @@ export class PageService { 'parentPageId', 'spaceId', 'deletedAt', + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + sql`0`.as('traversalDepth'), ]) .where('id', '=', childPageId) .where('deletedAt', 'is', null) @@ -883,13 +893,28 @@ 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'), + sql`pa.traversal_depth + 1`.as('traversalDepth'), ]) .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( @@ -901,9 +926,12 @@ export class PageService { ) .as('hasChildren'), ) + .orderBy('traversalDepth', 'desc') .execute(); - return ancestors.reverse(); + assertAcyclicPageTraversal(ancestors, childPageId); + + return ancestors.map(stripPageTraversalMetadata); } async getRecentSpacePages( @@ -1009,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..1f8263d43 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; @@ -46,13 +48,9 @@ export class TrashCleanupService { for (const page of oldDeletedPages) { try { - await this.cleanupPage(page.id); - totalCleaned++; + totalCleaned += await this.cleanupPage(page.id); } catch (error) { - this.logger.error( - `Failed to cleanup page ${page.id}: ${error instanceof Error ? error.message : 'Unknown error'}`, - error instanceof Error ? error.stack : undefined, - ); + this.logCleanupError(page.id, error); } } } @@ -70,26 +68,43 @@ export class TrashCleanupService { } } - private async cleanupPage(pageId: string) { + private async getPageDescendants(pageId: string) { // Get all descendants using recursive CTE (including the page itself) - const descendants = await this.db + return this.db .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(); + } - const pageIds = descendants.map((d) => d.id); + private async cleanupPage(pageId: string) { + const descendants = await this.getPageDescendants(pageId); + assertAcyclicPageTraversal(descendants, pageId); + + const pageIds = descendants.map((descendant) => descendant.id); + if (pageIds.length === 0) { + return 0; + } this.logger.debug( `Cleaning up page ${pageId} with ${pageIds.length - 1} descendants`, @@ -114,14 +129,24 @@ export class TrashCleanupService { } try { - if (pageIds.length > 0) { - await this.db.deleteFrom('pages').where('id', 'in', pageIds).execute(); - } + const result = await this.db + .deleteFrom('pages') + .where('id', 'in', pageIds) + .executeTakeFirst(); + return Number(result.numDeletedRows); } catch (error) { // Log but don't throw - pages might have been deleted by another node this.logger.warn( `Error deleting pages, they may have been already deleted: ${error instanceof Error ? error.message : 'Unknown error'}`, ); + return 0; } } + + private logCleanupError(pageId: string, error: unknown) { + this.logger.error( + `Failed to cleanup page ${pageId}: ${error instanceof Error ? error.message : 'Unknown error'}`, + error instanceof Error ? error.stack : undefined, + ); + } } diff --git a/apps/server/src/core/share/share.service.ts b/apps/server/src/core/share/share.service.ts index e567e6caa..1abdfc95b 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; @@ -228,67 +255,6 @@ export class ShareService { }; } - async getShareAncestorPage( - ancestorPageId: string, - childPageId: string, - ): Promise { - let ancestor = null; - try { - ancestor = await this.db - .withRecursive('page_ancestors', (db) => - db - .selectFrom('pages') - .select([ - 'id', - 'slugId', - 'title', - 'parentPageId', - 'spaceId', - (eb) => - eb - .case() - .when(eb.ref('id'), '=', ancestorPageId) - .then(true) - .else(false) - .end() - .as('found'), - ]) - .where(isValidUUID(childPageId) ? 'id' : 'slugId', '=', childPageId) - .unionAll((exp) => - exp - .selectFrom('pages as p') - .select([ - 'p.id', - 'p.slugId', - 'p.title', - 'p.parentPageId', - 'p.spaceId', - (eb) => - eb - .case() - .when(eb.ref('p.id'), '=', ancestorPageId) - .then(true) - .else(false) - .end() - .as('found'), - ]) - .innerJoin('page_ancestors as pa', 'pa.parentPageId', 'p.id') - // Continue recursing only when the target ancestor hasn't been found on that branch. - .where('pa.found', '=', false), - ), - ) - .selectFrom('page_ancestors') - .selectAll() - .where('found', '=', true) - .limit(1) - .executeTakeFirst(); - } catch (err) { - // empty - } - - return ancestor; - } - /** * Resolve transclusion content for a public share viewer. Each requested * source page must itself be reachable via the share graph (its own share diff --git a/apps/server/src/database/helpers/page-hierarchy-cycle.spec.ts b/apps/server/src/database/helpers/page-hierarchy-cycle.spec.ts new file mode 100644 index 000000000..a4a1cfdff --- /dev/null +++ b/apps/server/src/database/helpers/page-hierarchy-cycle.spec.ts @@ -0,0 +1,53 @@ +import { + assertAcyclicPageTraversal, + PageHierarchyCycleError, + stripPageTraversalMetadata, +} from './page-hierarchy-cycle'; + +describe('page hierarchy cycle contract', () => { + it('does nothing when no row is marked as a cycle', () => { + expect(() => + assertAcyclicPageTraversal( + [ + { id: 'page-1', title: 'Root', isCycle: false }, + { id: 'page-2', title: 'Child', isCycle: false }, + ], + 'page-1', + ), + ).not.toThrow(); + }); + + it('throws PageHierarchyCycleError when a row is marked as a cycle', () => { + expect(() => + assertAcyclicPageTraversal( + [{ id: 'page-1', title: 'Root', isCycle: true }], + 'page-1', + ), + ).toThrow(PageHierarchyCycleError); + }); + + it('keeps the root page id on the error for safe logging', () => { + try { + assertAcyclicPageTraversal( + [{ id: 'page-1', title: 'Root', isCycle: true }], + 'root-page', + ); + throw new Error('expected a cycle error'); + } catch (error) { + expect(error).toBeInstanceOf(PageHierarchyCycleError); + expect((error as PageHierarchyCycleError).rootPageId).toBe('root-page'); + expect((error as PageHierarchyCycleError).code).toBe( + 'PAGE_HIERARCHY_CYCLE', + ); + } + }); + + it('removes traversal metadata without changing the public row fields', () => { + const row = { id: 'page-1', title: 'Root', isCycle: false }; + + expect(stripPageTraversalMetadata(row)).toEqual({ + id: 'page-1', + title: 'Root', + }); + }); +}); diff --git a/apps/server/src/database/helpers/page-hierarchy-cycle.ts b/apps/server/src/database/helpers/page-hierarchy-cycle.ts new file mode 100644 index 000000000..847c9470f --- /dev/null +++ b/apps/server/src/database/helpers/page-hierarchy-cycle.ts @@ -0,0 +1,28 @@ +export type CycleTrackedRow = { + isCycle: boolean; +}; + +export class PageHierarchyCycleError extends Error { + readonly code = 'PAGE_HIERARCHY_CYCLE'; + + constructor(readonly rootPageId: string) { + super('Cyclic page hierarchy detected'); + this.name = 'PageHierarchyCycleError'; + } +} + +export function assertAcyclicPageTraversal( + rows: readonly T[], + rootPageId: string, +): void { + if (rows.some((row) => row.isCycle)) { + throw new PageHierarchyCycleError(rootPageId); + } +} + +export function stripPageTraversalMetadata( + row: T, +): Omit { + const { isCycle: _isCycle, ...publicRow } = row; + return publicRow; +} diff --git a/apps/server/src/database/repos/page/page-permission.repo.ts b/apps/server/src/database/repos/page/page-permission.repo.ts index f753526cd..a046a7a9a 100644 --- a/apps/server/src/database/repos/page/page-permission.repo.ts +++ b/apps/server/src/database/repos/page/page-permission.repo.ts @@ -24,6 +24,7 @@ import { CacheKey, PERMISSION_CACHE_TTL_MS, } from '../../../common/helpers/cache-keys'; +import { assertAcyclicPageTraversal } from '../../helpers/page-hierarchy-cycle'; export { PagePermissionMember } from './types/page-permission.types'; @@ -332,7 +333,7 @@ export class PagePermissionRepo { } | undefined > { - return this.db + const ancestors = await this.db .withRecursive('ancestors', (qb) => qb .selectFrom('pages') @@ -340,6 +341,8 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`0`.as('depth'), + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where('pages.id', '=', pageId) .unionAll((eb) => @@ -350,19 +353,41 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`ancestors.depth + 1`.as('depth'), - ]), + sql`ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('ancestors.isCycle', '=', false), ), ) .selectFrom('ancestors') - .innerJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') + .leftJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') .select([ 'pageAccess.id as pageAccessId', 'pageAccess.pageId', 'pageAccess.accessLevel', 'ancestors.depth', + 'ancestors.isCycle', ]) .orderBy('ancestors.depth', 'asc') - .executeTakeFirst(); + .execute(); + + assertAcyclicPageTraversal(ancestors, pageId); + + const restrictedAncestor = ancestors.find( + (ancestor) => ancestor.pageAccessId !== null, + ); + if (!restrictedAncestor) return undefined; + + return { + pageAccessId: restrictedAncestor.pageAccessId, + pageId: restrictedAncestor.pageId, + accessLevel: restrictedAncestor.accessLevel, + depth: restrictedAncestor.depth, + }; } /** @@ -396,17 +421,30 @@ export class PagePermissionRepo { const result = await sql<{ canAccess: boolean | null; canEdit: boolean | null; + hasHierarchyCycle: boolean | null; }>` WITH RECURSIVE ancestors AS ( - SELECT id AS ancestor_id, parent_page_id, 0 AS depth + SELECT + id AS ancestor_id, + parent_page_id, + 0 AS depth, + ARRAY[id]::uuid[] AS traversal_path, + false AS is_cycle FROM pages WHERE id = ${pageId}::uuid UNION ALL - SELECT p.id, p.parent_page_id, a.depth + 1 + SELECT + p.id, + p.parent_page_id, + a.depth + 1, + a.traversal_path || p.id, + p.id = ANY(a.traversal_path) AS is_cycle FROM pages p JOIN ancestors a ON a.parent_page_id = p.id + WHERE NOT a.is_cycle ) SELECT + (SELECT bool_or(is_cycle) FROM ancestors) AS "hasHierarchyCycle", bool_and(pp.id IS NOT NULL) AS "canAccess", -- nearest restricted ancestor's highest role wins (DESC: 'writer' > 'reader', NULLS LAST: no-permission after real roles) (array_agg(pp.role ORDER BY a.depth ASC, pp.role DESC NULLS LAST))[1] = 'writer' AS "canEdit" @@ -422,6 +460,13 @@ export class PagePermissionRepo { `.execute(this.db); const row = result.rows[0]; + if (row?.hasHierarchyCycle) { + return { + hasAnyRestriction: true, + canAccess: false, + canEdit: false, + }; + } if (!row || row.canAccess === null) { return { hasAnyRestriction: false, canAccess: true, canEdit: true }; } @@ -461,6 +506,8 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`0`.as('depth'), + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where('pages.id', '=', pageId) .unionAll((eb) => @@ -471,7 +518,14 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`ancestors.depth + 1`.as('depth'), - ]), + sql`ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('ancestors.isCycle', '=', false), ), ) .selectFrom('pages') @@ -511,6 +565,14 @@ export class PagePermissionRepo { .else(false) .end() .as('hasInheritedRestriction'), + eb + .exists( + eb + .selectFrom('ancestors') + .select('ancestors.ancestorId') + .where('ancestors.isCycle', '=', true), + ) + .as('hasHierarchyCycle'), // canAccess: no restricted ancestor without ANY permission eb .case() @@ -638,13 +700,15 @@ export class PagePermissionRepo { const hasDirectRestriction = Boolean(result?.hasDirectRestriction); const hasInheritedRestriction = Boolean(result?.hasInheritedRestriction); + const hasHierarchyCycle = Boolean(result?.hasHierarchyCycle); return { hasDirectRestriction, hasInheritedRestriction, - hasAnyRestriction: hasDirectRestriction || hasInheritedRestriction, - canAccess: Boolean(result?.canAccess), - canEdit: Boolean(result?.canEdit), + hasAnyRestriction: + hasDirectRestriction || hasInheritedRestriction || hasHierarchyCycle, + canAccess: !hasHierarchyCycle && Boolean(result?.canAccess), + canEdit: !hasHierarchyCycle && Boolean(result?.canEdit), }; } @@ -676,6 +740,8 @@ export class PagePermissionRepo { 'pages.id as pageId', 'pages.id as ancestorId', 'pages.parentPageId', + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where(sql`pages.id = ANY(${pageIds}::uuid[])`) .unionAll((eb) => @@ -690,12 +756,29 @@ export class PagePermissionRepo { 'allAncestors.pageId', 'pages.id as ancestorId', 'pages.parentPageId', - ]), + sql`all_ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(all_ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('allAncestors.isCycle', '=', false), ), ) .selectFrom('pages') .select('pages.id') .where(sql`pages.id = ANY(${pageIds}::uuid[])`) + .where(({ not, exists, selectFrom }) => + not( + exists( + selectFrom('allAncestors') + .select('allAncestors.ancestorId') + .whereRef('allAncestors.pageId', '=', 'pages.id') + .where('allAncestors.isCycle', '=', true), + ), + ), + ) .where(({ not, exists, selectFrom }) => not( exists( @@ -745,6 +828,8 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`0`.as('depth'), + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where(sql`pages.id = ANY(${pageIds}::uuid[])`) .unionAll((eb) => @@ -760,7 +845,14 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`all_ancestors.depth + 1`.as('depth'), - ]), + sql`all_ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(all_ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('allAncestors.isCycle', '=', false), ), ) .selectFrom('pages') @@ -821,6 +913,16 @@ export class PagePermissionRepo { .as('canEdit'), ) .where(sql`pages.id = ANY(${pageIds}::uuid[])`) + .where(({ not, exists, selectFrom }) => + not( + exists( + selectFrom('allAncestors') + .select('allAncestors.ancestorId') + .whereRef('allAncestors.pageId', '=', 'pages.id') + .where('allAncestors.isCycle', '=', true), + ), + ), + ) // view filter: no restricted ancestor without any permission .where(({ not, exists, selectFrom }) => not( @@ -865,21 +967,39 @@ export class PagePermissionRepo { .withRecursive('ancestors', (qb) => qb .selectFrom('pages') - .select(['pages.id as ancestorId', 'pages.parentPageId']) + .select([ + 'pages.id as ancestorId', + 'pages.parentPageId', + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + ]) .where('pages.id', '=', pageId) .unionAll((eb) => eb .selectFrom('pages') .innerJoin('ancestors', 'ancestors.parentPageId', 'pages.id') - .select(['pages.id as ancestorId', 'pages.parentPageId']), + .select([ + 'pages.id as ancestorId', + 'pages.parentPageId', + sql`ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('ancestors.isCycle', '=', false), ), ) .selectFrom('ancestors') - .innerJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') - .select('pageAccess.id') + .leftJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') + .select([ + sql`bool_or(ancestors.is_cycle)`.as('hasHierarchyCycle'), + sql`bool_or(page_access.id IS NOT NULL)`.as('hasPageAccess'), + ]) .executeTakeFirst(); - return !!result; + return Boolean(result?.hasHierarchyCycle || result?.hasPageAccess); } /** @@ -921,6 +1041,8 @@ export class PagePermissionRepo { 'child.id as childId', 'child.id as ancestorId', 'child.parentPageId as ancestorParentId', + sql`ARRAY[child.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where('child.parentPageId', 'in', parentIds) .where('child.deletedAt', 'is', null) @@ -936,7 +1058,14 @@ export class PagePermissionRepo { 'childAncestors.childId', 'pages.id as ancestorId', 'pages.parentPageId as ancestorParentId', - ]), + sql`child_ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(child_ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('childAncestors.isCycle', '=', false), ), ) .selectFrom('pages as child') @@ -944,6 +1073,16 @@ export class PagePermissionRepo { .distinct() .where('child.parentPageId', 'in', parentIds) .where('child.deletedAt', 'is', null) + .where(({ not, exists, selectFrom }) => + not( + exists( + selectFrom('childAncestors') + .select('childAncestors.ancestorId') + .whereRef('childAncestors.childId', '=', 'child.id') + .where('childAncestors.isCycle', '=', true), + ), + ), + ) .where(({ not, exists, selectFrom }) => not( exists( @@ -978,67 +1117,6 @@ export class PagePermissionRepo { return results.map((r) => r.parentPageId); } - /** - * Get all page IDs within a subtree that are restricted OR are descendants of restricted pages. - * Used to filter pages from public shares - if a page is restricted, it and all its - * children should be hidden. - */ - async getRestrictedSubtreeIds(rootPageId: string): Promise { - const results = await this.db - .withRecursive('descendants', (qb) => - qb - .selectFrom('pages') - .select(['pages.id as descendantId', 'pages.parentPageId']) - .where('pages.id', '=', rootPageId) - .unionAll((eb) => - eb - .selectFrom('pages') - .innerJoin( - 'descendants', - 'descendants.descendantId', - 'pages.parentPageId', - ) - .select(['pages.id as descendantId', 'pages.parentPageId']) - .where('pages.deletedAt', 'is', null), - ), - ) - .withRecursive('descendantAncestors', (qb) => - qb - .selectFrom('descendants') - .innerJoin('pages', 'pages.id', 'descendants.descendantId') - .select([ - 'descendants.descendantId', - 'pages.id as ancestorId', - 'pages.parentPageId as ancestorParentId', - ]) - .unionAll((eb) => - eb - .selectFrom('pages') - .innerJoin( - 'descendantAncestors', - 'descendantAncestors.ancestorParentId', - 'pages.id', - ) - .select([ - 'descendantAncestors.descendantId', - 'pages.id as ancestorId', - 'pages.parentPageId as ancestorParentId', - ]), - ), - ) - .selectFrom('descendantAncestors') - .innerJoin( - 'pageAccess', - 'pageAccess.pageId', - 'descendantAncestors.ancestorId', - ) - .select('descendantAncestors.descendantId') - .distinct() - .execute(); - - return results.map((r) => r.descendantId); - } - /** * Given a pageId and a set of candidate userIds, return the subset who can * access the page (have permission on ALL restricted ancestors). @@ -1052,17 +1130,27 @@ export class PagePermissionRepo { const results = await sql<{ userId: string }>` WITH RECURSIVE ancestors AS ( - SELECT id AS ancestor_id, parent_page_id + SELECT + id AS ancestor_id, + parent_page_id, + ARRAY[id]::uuid[] AS traversal_path, + false AS is_cycle FROM pages WHERE id = ${pageId}::uuid UNION ALL - SELECT p.id, p.parent_page_id + SELECT + p.id, + p.parent_page_id, + a.traversal_path || p.id, + p.id = ANY(a.traversal_path) AS is_cycle FROM pages p JOIN ancestors a ON a.parent_page_id = p.id + WHERE NOT a.is_cycle ) SELECT cu.user_id AS "userId" FROM unnest(${userIds}::uuid[]) AS cu(user_id) - WHERE NOT EXISTS ( + WHERE NOT EXISTS (SELECT 1 FROM ancestors WHERE is_cycle) + AND NOT EXISTS ( SELECT 1 FROM ancestors a JOIN page_access pa ON pa.page_id = a.ancestor_id 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/src/ee b/apps/server/src/ee index 844f2003c..5e7120dcc 160000 --- a/apps/server/src/ee +++ b/apps/server/src/ee @@ -1 +1 @@ -Subproject commit 844f2003cdc36268478fdfb5ad64b656df6b633b +Subproject commit 5e7120dcc86a344f5af09ccda0a29d5784659938