fix: fail closed on cyclic permission ancestry

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<number>`0`.as('depth'),
+            sql<string[]>`ARRAY[pages.id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
           ])
           .where('pages.id', '=', pageId)
           .unionAll((eb) =>
@@ -350,19 +353,41 @@ export class PagePermissionRepo {
                 'pages.id as ancestorId',
                 'pages.parentPageId',
                 sql<number>`ancestors.depth + 1`.as('depth'),
-              ]),
+                sql<string[]>`ancestors.traversal_path || pages.id`.as(
+                  'traversalPath',
+                ),
+                sql<boolean>`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<number>`0`.as('depth'),
+            sql<string[]>`ARRAY[pages.id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
           ])
           .where('pages.id', '=', pageId)
           .unionAll((eb) =>
@@ -471,7 +518,14 @@ export class PagePermissionRepo {
                 'pages.id as ancestorId',
                 'pages.parentPageId',
                 sql<number>`ancestors.depth + 1`.as('depth'),
-              ]),
+                sql<string[]>`ancestors.traversal_path || pages.id`.as(
+                  'traversalPath',
+                ),
+                sql<boolean>`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),
     };
   }
 
@@ -865,21 +929,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<string[]>`ARRAY[pages.id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`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<string[]>`ancestors.traversal_path || pages.id`.as(
+                  'traversalPath',
+                ),
+                sql<boolean>`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<boolean>`bool_or(ancestors.is_cycle)`.as('hasHierarchyCycle'),
+        sql<boolean>`bool_or(page_access.id IS NOT NULL)`.as('hasPageAccess'),
+      ])
       .executeTakeFirst();
 
-    return !!result;
+    return Boolean(result?.hasHierarchyCycle || result?.hasPageAccess);
   }
 
   /**
@@ -1052,17 +1134,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

apps/server/test/page-hierarchy-cycle.integration-spec.ts

@@ -1,7 +1,9 @@
 import { randomUUID } from 'node:crypto';
+import { createCache } from 'cache-manager';
 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 { PagePermissionRepo } from '../src/database/repos/page/page-permission.repo';
 import { KyselyDB } from '../src/database/types/kysely.types';
 import { db, withStatementTimeout } from './support/database';
 import {
@@ -38,6 +40,72 @@ function createShareService(connection: KyselyDB): ShareService {
   );
 }
 
+function createPagePermissionRepo(connection: KyselyDB): PagePermissionRepo {
+  return new PagePermissionRepo(connection, undefined as never, createCache());
+}
+
+async function insertTestUser(pageId: string): Promise<string> {
+  const { workspaceId } = await db
+    .selectFrom('pages')
+    .select('workspaceId')
+    .where('id', '=', pageId)
+    .executeTakeFirstOrThrow();
+
+  const user = await db
+    .insertInto('users')
+    .values({
+      email: `cycle-test-${randomUUID()}@example.com`,
+      name: 'Page hierarchy test user',
+      workspaceId,
+    })
+    .returning('id')
+    .executeTakeFirstOrThrow();
+
+  return user.id;
+}
+
+async function restrictPage(
+  pageId: string,
+  permittedUserId?: string,
+): Promise<{
+  accessLevel: string;
+  pageAccessId: string;
+  pageId: string;
+}> {
+  const page = await db
+    .selectFrom('pages')
+    .select(['spaceId', 'workspaceId'])
+    .where('id', '=', pageId)
+    .executeTakeFirstOrThrow();
+  const pageAccess = await db
+    .insertInto('pageAccess')
+    .values({
+      accessLevel: 'restricted',
+      pageId,
+      spaceId: page.spaceId,
+      workspaceId: page.workspaceId,
+    })
+    .returning(['accessLevel', 'id', 'pageId'])
+    .executeTakeFirstOrThrow();
+
+  if (permittedUserId) {
+    await db
+      .insertInto('pagePermissions')
+      .values({
+        pageAccessId: pageAccess.id,
+        role: 'writer',
+        userId: permittedUserId,
+      })
+      .execute();
+  }
+
+  return {
+    accessLevel: pageAccess.accessLevel,
+    pageAccessId: pageAccess.id,
+    pageId: pageAccess.pageId,
+  };
+}
+
 async function insertShare(pageId: string, includeSubPages: boolean) {
   const page = await db
     .selectFrom('pages')
@@ -225,4 +293,185 @@ describe('cycle-safe page hierarchy reads', () => {
       });
     });
   });
+
+  describe('single-page permissions', () => {
+    it('preserves unrestricted acyclic access', async () => {
+      const { grandchild } = await seedAcyclicPageChain();
+      const userId = await insertTestUser(grandchild.id);
+      const repo = createPagePermissionRepo(db);
+
+      await expect(
+        repo.canUserEditPage(userId, grandchild.id),
+      ).resolves.toEqual({
+        hasAnyRestriction: false,
+        canAccess: true,
+        canEdit: true,
+      });
+      await expect(
+        repo.getUserPageAccessLevel(userId, grandchild.id),
+      ).resolves.toEqual({
+        hasDirectRestriction: false,
+        hasInheritedRestriction: false,
+        hasAnyRestriction: false,
+        canAccess: true,
+        canEdit: true,
+      });
+      await expect(repo.hasRestrictedAncestor(grandchild.id)).resolves.toBe(
+        false,
+      );
+      await expect(
+        repo.getUserIdsWithPageAccess(grandchild.id, [userId]),
+      ).resolves.toEqual([userId]);
+      await expect(
+        repo.findRestrictedAncestor(grandchild.id),
+      ).resolves.toBeUndefined();
+    });
+
+    it('preserves permitted acyclic access', async () => {
+      const { root, grandchild } = await seedAcyclicPageChain();
+      const userId = await insertTestUser(grandchild.id);
+      const restriction = await restrictPage(root.id, userId);
+      const repo = createPagePermissionRepo(db);
+
+      await expect(
+        repo.canUserEditPage(userId, grandchild.id),
+      ).resolves.toEqual({
+        hasAnyRestriction: true,
+        canAccess: true,
+        canEdit: true,
+      });
+      await expect(
+        repo.getUserPageAccessLevel(userId, grandchild.id),
+      ).resolves.toEqual({
+        hasDirectRestriction: false,
+        hasInheritedRestriction: true,
+        hasAnyRestriction: true,
+        canAccess: true,
+        canEdit: true,
+      });
+      await expect(repo.hasRestrictedAncestor(grandchild.id)).resolves.toBe(
+        true,
+      );
+      await expect(
+        repo.getUserIdsWithPageAccess(grandchild.id, [userId]),
+      ).resolves.toEqual([userId]);
+      await expect(repo.findRestrictedAncestor(grandchild.id)).resolves.toEqual(
+        { ...restriction, depth: 2 },
+      );
+    });
+
+    it('preserves denied acyclic access', async () => {
+      const { root, grandchild } = await seedAcyclicPageChain();
+      const userId = await insertTestUser(grandchild.id);
+      const restriction = await restrictPage(root.id);
+      const repo = createPagePermissionRepo(db);
+
+      await expect(
+        repo.canUserEditPage(userId, grandchild.id),
+      ).resolves.toEqual({
+        hasAnyRestriction: true,
+        canAccess: false,
+        canEdit: false,
+      });
+      await expect(
+        repo.getUserPageAccessLevel(userId, grandchild.id),
+      ).resolves.toEqual({
+        hasDirectRestriction: false,
+        hasInheritedRestriction: true,
+        hasAnyRestriction: true,
+        canAccess: false,
+        canEdit: false,
+      });
+      await expect(repo.hasRestrictedAncestor(grandchild.id)).resolves.toBe(
+        true,
+      );
+      await expect(
+        repo.getUserIdsWithPageAccess(grandchild.id, [userId]),
+      ).resolves.toEqual([]);
+      await expect(repo.findRestrictedAncestor(grandchild.id)).resolves.toEqual(
+        { ...restriction, depth: 2 },
+      );
+    });
+
+    describe.each([
+      ['a self-cycle', async () => (await seedSelfCycle()).self.id],
+      ['a two-page cycle', async () => (await seedTwoPageCycle()).a.id],
+    ])('%s', (_cycleName, seedCycle) => {
+      it('fails canUserEditPage closed', async () => {
+        const pageId = await seedCycle();
+        const userId = await insertTestUser(pageId);
+
+        await withStatementTimeout(async (connection) => {
+          const repo = createPagePermissionRepo(connection);
+
+          await expect(repo.canUserEditPage(userId, pageId)).resolves.toEqual({
+            hasAnyRestriction: true,
+            canAccess: false,
+            canEdit: false,
+          });
+        });
+      });
+
+      it('fails getUserPageAccessLevel closed', async () => {
+        const pageId = await seedCycle();
+        const userId = await insertTestUser(pageId);
+
+        await withStatementTimeout(async (connection) => {
+          const repo = createPagePermissionRepo(connection);
+
+          await expect(
+            repo.getUserPageAccessLevel(userId, pageId),
+          ).resolves.toEqual(
+            expect.objectContaining({
+              hasAnyRestriction: true,
+              canAccess: false,
+              canEdit: false,
+            }),
+          );
+        });
+      });
+
+      it('treats a cycle as a restricted ancestor', async () => {
+        const pageId = await seedCycle();
+
+        await withStatementTimeout(async (connection) => {
+          const repo = createPagePermissionRepo(connection);
+
+          await expect(repo.hasRestrictedAncestor(pageId)).resolves.toBe(true);
+        });
+      });
+
+      it('filters every candidate user from a cycle', async () => {
+        const pageId = await seedCycle();
+        const userId = await insertTestUser(pageId);
+
+        await withStatementTimeout(async (connection) => {
+          const repo = createPagePermissionRepo(connection);
+
+          await expect(
+            repo.getUserIdsWithPageAccess(pageId, [userId]),
+          ).resolves.toEqual([]);
+        });
+      });
+
+      it('raises PageHierarchyCycleError from findRestrictedAncestor', async () => {
+        const pageId = await seedCycle();
+
+        await withStatementTimeout(async (connection) => {
+          const repo = createPagePermissionRepo(connection);
+          const restrictedAncestor = repo.findRestrictedAncestor(pageId);
+
+          await expect(restrictedAncestor).rejects.toBeInstanceOf(
+            PageHierarchyCycleError,
+          );
+          await expect(restrictedAncestor).rejects.toEqual(
+            expect.objectContaining({
+              code: 'PAGE_HIERARCHY_CYCLE',
+              rootPageId: pageId,
+            }) satisfies Partial<PageHierarchyCycleError>,
+          );
+        });
+      });
+    });
+  });
 });