diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index fc6bfd56..5f16aa88 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -62,6 +62,16 @@ import { agentSourceFields, } from '../../../common/decorators/auth-provenance.decorator'; +// Hard upper bound on how deep the recursive page-tree CTEs (ancestor / +// descendant traversals) may walk. Real page trees are only a handful of levels +// deep, so this cap never truncates a legitimate result; it purely defends the +// recursive CTEs against runaway iteration if a parent/child cycle ever exists +// in the data (e.g. one slipped in before the move guard, #207 #8). Without it a +// cycle makes `withRecursive` loop forever (hang / statement timeout), and the +// move guard itself calls one of these CTEs — so a cycle would disable the very +// guard meant to prevent it. Each CTE carries a depth counter and stops here. +const MAX_PAGE_TREE_DEPTH = 10_000; + // 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 @@ -1030,6 +1040,9 @@ export class PageService { 'spaceId', 'deletedAt', ]) + // Depth counter: bounds the walk so a parent/child cycle in the data + // can't make this recursive CTE loop forever (#207 #8). + .select(sql`0`.as('depth')) .where('id', '=', childPageId) .where('deletedAt', 'is', null) .unionAll((exp) => @@ -1045,12 +1058,25 @@ export class PageService { 'p.spaceId', 'p.deletedAt', ]) + .select(sql`pa.depth + 1`.as('depth')) .innerJoin('page_ancestors as pa', 'pa.parentPageId', 'p.id') - .where('p.deletedAt', 'is', null), + .where('p.deletedAt', 'is', null) + .where(sql`pa.depth`, '<', MAX_PAGE_TREE_DEPTH), ), ) .selectFrom('page_ancestors') - .selectAll('page_ancestors') + // Explicit column list (not selectAll) so the internal `depth` counter + // never leaks into the breadcrumb result shape. + .select([ + 'id', + 'slugId', + 'title', + 'icon', + 'position', + 'parentPageId', + 'spaceId', + 'deletedAt', + ]) .select((eb) => eb .exists( @@ -1171,16 +1197,21 @@ export class PageService { db .selectFrom('pages') .select(['id']) + // Depth counter: bounds the walk so a parent/child cycle in the data + // can't make this recursive CTE loop forever (#207 #8). + .select(sql`0`.as('depth')) .where('id', '=', pageId) .unionAll((exp) => exp .selectFrom('pages as p') .select(['p.id']) - .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId'), + .select(sql`pd.depth + 1`.as('depth')) + .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId') + .where(sql`pd.depth`, '<', MAX_PAGE_TREE_DEPTH), ), ) .selectFrom('page_descendants') - .selectAll() + .select(['id']) .execute(); const pageIds = descendants.map((d) => d.id); diff --git a/apps/server/test/integration/page-recursive-cte-cycle-guard.int-spec.ts b/apps/server/test/integration/page-recursive-cte-cycle-guard.int-spec.ts new file mode 100644 index 00000000..a415edb7 --- /dev/null +++ b/apps/server/test/integration/page-recursive-cte-cycle-guard.int-spec.ts @@ -0,0 +1,134 @@ +import { CamelCasePlugin, Kysely } from 'kysely'; +import { PostgresJSDialect } from 'kysely-postgres-js'; +import * as postgres from 'postgres'; +import { PageService } from 'src/core/page/services/page.service'; +import { + getTestDb, + destroyTestDb, + createWorkspace, + createSpace, + createPage, + TEST_DATABASE_URL, +} from './db'; + +/** + * #207 #8 — recursive page-tree CTEs (ancestors in getPageBreadCrumbs, + * descendants in forceDelete) must not hang when a parent/child cycle already + * exists in the data. Before the fix neither CTE had a CYCLE clause or a depth + * cap, so a cycle (e.g. one persisted by the #7 TOCTOU race) made withRecursive + * loop forever — and since the move guard itself runs the ancestor CTE, a cycle + * would disable the very guard meant to prevent it. + * + * The fix adds a depth counter bounded by MAX_PAGE_TREE_DEPTH to both CTEs. + * These tests seed an A<->B cycle directly (bypassing the guard), then run the + * real CTE paths against Postgres with a short connection-level statement_timeout + * so a regression (an unbounded CTE) fails fast as a query timeout instead of a + * bounded result. + */ +describe('recursive page-tree CTEs cycle/depth guard [integration]', () => { + // Upper bound on rows the depth-capped CTEs can emit for a 2-node cycle: one + // row per depth level 0..MAX. Kept loose so the assertion does not couple to + // the exact constant, only to "bounded". + const BOUNDED_MAX_ROWS = 20_000; + + let db: Kysely; + // Dedicated Kysely whose connections carry a short statement_timeout, so an + // unbounded recursive CTE aborts quickly instead of hanging the suite. + let timeoutDb: Kysely; + let workspaceId: string; + let spaceId: string; + + beforeAll(async () => { + db = getTestDb(); + timeoutDb = new Kysely({ + dialect: new PostgresJSDialect({ + postgres: postgres(TEST_DATABASE_URL, { + max: 2, + onnotice: () => {}, + // Applied to every connection on connect: cap any single statement. + connection: { statement_timeout: 4000 }, + types: { + bigint: { + to: 20, + from: [20, 1700], + serialize: (value: number) => value.toString(), + parse: (value: string) => Number.parseInt(value), + }, + }, + }), + }), + plugins: [new CamelCasePlugin()], + }); + workspaceId = (await createWorkspace(db)).id; + spaceId = (await createSpace(db, workspaceId)).id; + }); + + afterAll(async () => { + await timeoutDb.destroy(); + await destroyTestDb(); + }); + + // Seed two fresh pages and wire them into a direct parent/child cycle, + // bypassing PageService.movePage's guard the way the #7 race would. + async function seedCycle(): Promise<{ aId: string; bId: string }> { + const a = await createPage(db, { workspaceId, spaceId, title: 'cycle-A' }); + const b = await createPage(db, { workspaceId, spaceId, title: 'cycle-B' }); + await db + .updateTable('pages') + .set({ parentPageId: b.id }) + .where('id', '=', a.id) + .execute(); + await db + .updateTable('pages') + .set({ parentPageId: a.id }) + .where('id', '=', b.id) + .execute(); + return { aId: a.id, bId: b.id }; + } + + function makeService(database: Kysely): PageService { + const eventEmitter = { emit: () => true } as any; + const attachmentQueue = { add: async () => undefined } as any; + return new PageService( + undefined as any, // pageRepo (unused by these paths) + undefined as any, // pagePermissionRepo + undefined as any, // attachmentRepo + database as any, // db + undefined as any, // storageService + attachmentQueue, // attachmentQueue + undefined as any, // aiQueue + undefined as any, // generalQueue + eventEmitter, // eventEmitter + undefined as any, // collaborationGateway + undefined as any, // watcherService + undefined as any, // transclusionService + ); + } + + it('getPageBreadCrumbs returns a bounded result (no hang) when a cycle exists', async () => { + const { aId } = await seedCycle(); + const service = makeService(timeoutDb); + + // Must resolve (the depth cap stops the walk) rather than time out. + const crumbs = await service.getPageBreadCrumbs(aId); + + expect(Array.isArray(crumbs)).toBe(true); + expect(crumbs.length).toBeGreaterThan(1); + expect(crumbs.length).toBeLessThanOrEqual(BOUNDED_MAX_ROWS); + }); + + it('forceDelete descendant CTE is bounded (no hang) and removes the cyclic pages', async () => { + const { aId, bId } = await seedCycle(); + const service = makeService(timeoutDb); + + // Must complete instead of looping on the descendant CTE. + await service.forceDelete(aId, workspaceId); + + const survivors = await db + .selectFrom('pages') + .select('id') + .where('id', 'in', [aId, bId]) + .execute(); + expect(survivors).toHaveLength(0); + }); +});