fix(#348 review F1-F4): uncache the workspace-restriction gate + int-spec + docs

- F1 [medium — the substantive one]: hasRestrictedPagesInWorkspace is now UNCACHED (a plain EXISTS per call, like its sibling hasRestrictedPagesInSpace). Caching it (even 5s) reintroduced an access-control leak the space path never had: a concurrent whole-workspace read in the insert->commit window of the FIRST restricted page could re-populate `false` under withCache (read-then-set, no del-during-read guard) and override the insert-time bust, leaking that page to unauthorized users for up to the TTL. Uncaching removes both the DB/cache asymmetry and the TOCTOU race; the space path already accepts this per-call cost. Reverted the now-unnecessary insertPageAccess cache-bust and removed the dead HAS_RESTRICTED_PAGES_IN_WORKSPACE cache key. - F2 [test]: page-permission-workspace-filter.int-spec.ts (real PG) — the short-circuit returns the full input set with zero restrictions AND filters out the page the user can't reach when a restriction is present (proving the authz behavior is unchanged), the 0->1 transition flips immediately, and the flag is per-workspace scoped. - F3 [doc]: documented the deploy-time write-lock in the migration header — the non-CONCURRENT GIN trigram builds take a SHARE lock that blocks writes on pages/users/… for minutes on a large tenant; run in a maintenance window or build CONCURRENTLY out-of-band for big installs. - F4 [doc]: corrected the jwt.strategy comment — the reused req.raw.workspace is the middleware's selectAll superset (not "the exact row this query returns"), harmless because AuthWorkspace already preferred that object. Gate: server tsc 0; the new int-spec 3/3 on real Postgres. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-05 02:19:42 +03:00
parent 1e61042bf6
commit 3ebdbfec81
5 changed files with 153 additions and 42 deletions
@@ -4,10 +4,6 @@ export const CacheKey = {
    `perm:space-roles:${userId}:${spaceId}`,
  PAGE_CAN_EDIT: (userId: string, pageId: string) =>
    `perm:can-edit:${userId}:${pageId}`,
-  // #348 — "does this workspace have ANY restricted page?" Lets whole-workspace
-  // access filters skip the recursive-ancestor CTE when nothing is restricted.
-  HAS_RESTRICTED_PAGES_IN_WORKSPACE: (workspaceId: string) =>
-    `perm:ws-has-restricted:${workspaceId}`,
  // #348 — DomainMiddleware workspace resolution. Self-hosted resolves the single
  // workspace (constant key); cloud resolves by the request subdomain (lowercased
  // to match the case-insensitive `LOWER(hostname)` lookup). Every WorkspaceRepo
@@ -55,9 +55,13 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') {
    // instead of re-querying it. `validate()` above has confirmed
    // `req.raw.workspaceId === payload.workspaceId` (or that it is unset), and the
    // middleware sets `req.raw.workspace` alongside `req.raw.workspaceId` from the
-    // same row, so when the ids match the cached row is the exact one this query
-    // would return. Fall back to the query if the middleware did not populate it
-    // (e.g. a code path that bypasses DomainMiddleware).
+    // SAME workspace row, so when the ids match this is that row. NOTE it is the
+    // middleware's `selectAll` object (a superset of the fallback `findById` base
+    // fields — it also carries licenseKey/auditRetentionDays); that is harmless
+    // here because every consumer reads this workspace via the AuthWorkspace
+    // decorator, which already preferred `req.raw.workspace` (the selectAll object)
+    // over `req.user.workspace` before this change. Fall back to the query if the
+    // middleware did not populate it (a path that bypasses DomainMiddleware).
    const workspace =
      req.raw.workspace && req.raw.workspaceId === payload.workspaceId
        ? req.raw.workspace
@@ -32,6 +32,17 @@ import { type Kysely, sql } from 'kysely';
 *      DESC, but only `(page_id, created_at DESC)` exists → extra sort.
 *    - comments: `findPageComments` does WHERE page_id ORDER BY id ASC, but only
 *      `(page_id)` exists → extra sort.
+ *
+ * DEPLOY-TIME LOCK WARNING: these are plain (non-CONCURRENT) CREATE INDEX
+ * statements — CONCURRENTLY is impossible because Kysely runs each migration in a
+ * transaction. They take a SHARE lock that BLOCKS writes (INSERT/UPDATE/DELETE) on
+ * pages/users/groups/comments/page_history for the duration of the build. The two
+ * GIN trigram builds on pages.title / users.name are the slow ones and can take
+ * minutes on a large tenant → a write-outage window during the deploy migration.
+ * For large installations, run this migration in a maintenance window, or build
+ * the trigram indexes out-of-band with CREATE INDEX CONCURRENTLY before deploying
+ * (then this migration's `IF NOT EXISTS` is a no-op). Small/typical tenants are
+ * unaffected.
 */
 export async function up(db: Kysely<any>): Promise<void> {
  // Index-compatible, output-identical redefinition of f_unaccent (see header).
@@ -52,22 +52,11 @@ export class PagePermissionRepo {
    trx?: KyselyTransaction,
  ): Promise<PageAccess> {
    const db = dbOrTx(this.db, trx);
-    const row = await db
+    return db
      .insertInto('pageAccess')
      .values(data)
      .returningAll()
      .executeTakeFirst();
-    // Bust the workspace-level "has any restricted page" cache: a 0->1 transition
-    // (the FIRST restricted page in a workspace) must take effect immediately, or
-    // the filterAccessiblePageIds short-circuit would keep treating the workspace
-    // as unrestricted for up to PERMISSION_CACHE_TTL_MS and leak the just-
-    // restricted page into whole-workspace lists (search/favorites/recent/…).
-    if (data.workspaceId) {
-      await this.cacheManager.del(
-        CacheKey.HAS_RESTRICTED_PAGES_IN_WORKSPACE(data.workspaceId),
-      );
-    }
-    return row;
  }

  async deletePageAccess(
@@ -931,34 +920,32 @@ export class PagePermissionRepo {
   * whole workspace carry a restriction? Lets whole-workspace access filters
   * short-circuit the recursive-ancestor CTE when nothing is restricted at all.
   *
-   * Cached with the same short PERMISSION_CACHE_TTL_MS as PAGE_CAN_EDIT: this is
-   * the workspace-wide restriction flag, so it is read on nearly every list
-   * endpoint, and the 5s TTL bounds the window in which a just-added first
-   * restriction is not yet reflected — the identical staleness contract the other
-   * permission caches already accept. No explicit bust (mirrors PAGE_CAN_EDIT).
+   * UNCACHED (like the sibling hasRestrictedPagesInSpace) — a single cheap
+   * `EXISTS(pageAccess WHERE workspaceId=?)` per call. This is an ACCESS-CONTROL
+   * gate on whole-workspace list endpoints, so it must never go stale: caching it
+   * (even 5s) reintroduced a leak the space-path never had — a concurrent
+   * whole-workspace read in the insert->commit window of the FIRST restricted page
+   * could re-populate `false` under withCache (read-then-set, no del-during-read
+   * guard) and override the insert bust, leaking that page to unauthorized users
+   * for up to the TTL (#348 review F1). An uncached EXISTS removes both the
+   * cache/DB asymmetry with hasRestrictedPagesInSpace and that race; the space
+   * path already accepts this exact per-call cost.
   */
  async hasRestrictedPagesInWorkspace(workspaceId: string): Promise<boolean> {
-    return withCache(
-      this.cacheManager,
-      CacheKey.HAS_RESTRICTED_PAGES_IN_WORKSPACE(workspaceId),
-      PERMISSION_CACHE_TTL_MS,
-      async () => {
-        const result = await this.db
-          .selectNoFrom((eb) =>
+    const result = await this.db
+      .selectNoFrom((eb) =>
+        eb
+          .exists(
            eb
-              .exists(
-                eb
-                  .selectFrom('pageAccess')
-                  .select(sql`1`.as('one'))
-                  .where('pageAccess.workspaceId', '=', workspaceId),
-              )
-              .as('exists'),
+              .selectFrom('pageAccess')
+              .select(sql`1`.as('one'))
+              .where('pageAccess.workspaceId', '=', workspaceId),
          )
-          .executeTakeFirst();
+          .as('exists'),
+      )
+      .executeTakeFirst();

-        return Boolean(result?.exists);
-      },
-    );
+    return Boolean(result?.exists);
  }

  /**
@@ -0,0 +1,113 @@
+import { Kysely } from 'kysely';
+import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo';
+import { GroupRepo } from '@docmost/db/repos/group/group.repo';
+import {
+  getTestDb,
+  destroyTestDb,
+  createWorkspace,
+  createSpace,
+  createUser,
+  createPage,
+} from './db';
+
+/**
+ * #348 — the whole-workspace access-filter short-circuit is an ACCESS-CONTROL
+ * path, so it must produce the SAME result as the full recursive-ancestor CTE.
+ *
+ * filterAccessiblePageIds({ workspaceId }) (no spaceId — the favorites /
+ * notifications / recent / created-by / global-search callers) skips the CTE only
+ * when the workspace has ZERO restricted pages. A page is "restricted &
+ * inaccessible" when it (or an ancestor) has a `pageAccess` row and the user has
+ * no matching `pagePermissions`. Driven against real Postgres, asserts:
+ *   1. zero restrictions -> short-circuit returns the full input set;
+ *   2. a restriction present -> the CTE runs and drops the page the user can't
+ *      reach while keeping the reachable ones (behavior unchanged);
+ *   3. inserting the FIRST pageAccess flips hasRestrictedPagesInWorkspace
+ *      false -> true immediately (the 0->1 transition — now uncached, no stale
+ *      window, review F1); it is scoped per workspace.
+ */
+describe('#348 filterAccessiblePageIds workspace short-circuit (real PG)', () => {
+  let db: Kysely<any>;
+  let repo: PagePermissionRepo;
+  let workspaceId: string;
+  let otherWorkspaceId: string;
+  let userId: string;
+  let spaceId: string;
+
+  beforeAll(async () => {
+    db = getTestDb();
+    // hasRestrictedPagesInWorkspace is now uncached, and no other cached
+    // permission path is exercised here, so a no-op cache stub suffices.
+    const cacheStub = {
+      get: async () => undefined,
+      set: async () => undefined,
+      del: async () => undefined,
+    } as never;
+    repo = new PagePermissionRepo(db, new GroupRepo(db), cacheStub);
+
+    const ws = await createWorkspace(db);
+    workspaceId = ws.id;
+    const other = await createWorkspace(db);
+    otherWorkspaceId = other.id;
+    const user = await createUser(db, workspaceId);
+    userId = user.id;
+    const space = await createSpace(db, workspaceId);
+    spaceId = space.id;
+  });
+
+  afterAll(async () => {
+    await destroyTestDb();
+  });
+
+  it('zero restrictions: short-circuit returns the full input set', async () => {
+    const p1 = await createPage(db, { workspaceId, spaceId });
+    const p2 = await createPage(db, { workspaceId, spaceId });
+
+    expect(await repo.hasRestrictedPagesInWorkspace(workspaceId)).toBe(false);
+
+    const ids = [p1.id, p2.id];
+    const filtered = await repo.filterAccessiblePageIds({
+      pageIds: ids,
+      userId,
+      workspaceId,
+    });
+    expect(new Set(filtered)).toEqual(new Set(ids));
+  });
+
+  it('a restriction present: filters out the page the user cannot reach', async () => {
+    const openPage = await createPage(db, { workspaceId, spaceId });
+    const restrictedPage = await createPage(db, { workspaceId, spaceId });
+
+    // Add a pageAccess row on restrictedPage with NO matching pagePermissions for
+    // `userId` → the CTE anti-join marks it inaccessible for this user.
+    await db
+      .insertInto('pageAccess')
+      .values({
+        pageId: restrictedPage.id,
+        workspaceId,
+        spaceId,
+        accessLevel: 'read',
+        creatorId: userId,
+      })
+      .execute();
+
+    // 0->1 transition is reflected immediately (uncached).
+    expect(await repo.hasRestrictedPagesInWorkspace(workspaceId)).toBe(true);
+
+    const filtered = await repo.filterAccessiblePageIds({
+      pageIds: [openPage.id, restrictedPage.id],
+      userId,
+      workspaceId,
+    });
+    expect(filtered).toContain(openPage.id);
+    expect(filtered).not.toContain(restrictedPage.id);
+  });
+
+  it('hasRestrictedPagesInWorkspace is scoped per workspace', async () => {
+    // The other workspace has no pageAccess rows → still false, unaffected by the
+    // restriction added above in `workspaceId`.
+    expect(await repo.hasRestrictedPagesInWorkspace(otherWorkspaceId)).toBe(
+      false,
+    );
+  });
+});