From 3ebdbfec81cdd2b1f926e164fa757540ff0fd50f Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 02:19:42 +0300 Subject: [PATCH] fix(#348 review F1-F4): uncache the workspace-restriction gate + int-spec + docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- apps/server/src/common/helpers/cache-keys.ts | 4 - .../src/core/auth/strategies/jwt.strategy.ts | 10 +- .../20260705T120000-perf-indexes.ts | 11 ++ .../repos/page/page-permission.repo.ts | 57 ++++----- ...ge-permission-workspace-filter.int-spec.ts | 113 ++++++++++++++++++ 5 files changed, 153 insertions(+), 42 deletions(-) create mode 100644 apps/server/test/integration/page-permission-workspace-filter.int-spec.ts diff --git a/apps/server/src/common/helpers/cache-keys.ts b/apps/server/src/common/helpers/cache-keys.ts index 91276f04..128501ca 100644 --- a/apps/server/src/common/helpers/cache-keys.ts +++ b/apps/server/src/common/helpers/cache-keys.ts @@ -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 diff --git a/apps/server/src/core/auth/strategies/jwt.strategy.ts b/apps/server/src/core/auth/strategies/jwt.strategy.ts index b5a4df81..9e5604a9 100644 --- a/apps/server/src/core/auth/strategies/jwt.strategy.ts +++ b/apps/server/src/core/auth/strategies/jwt.strategy.ts @@ -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 diff --git a/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts b/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts index 4b457660..20429d2f 100644 --- a/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts +++ b/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts @@ -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): Promise { // Index-compatible, output-identical redefinition of f_unaccent (see header). 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 0fa00777..38e03b4e 100644 --- a/apps/server/src/database/repos/page/page-permission.repo.ts +++ b/apps/server/src/database/repos/page/page-permission.repo.ts @@ -52,22 +52,11 @@ export class PagePermissionRepo { trx?: KyselyTransaction, ): Promise { 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 { - 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); } /** diff --git a/apps/server/test/integration/page-permission-workspace-filter.int-spec.ts b/apps/server/test/integration/page-permission-workspace-filter.int-spec.ts new file mode 100644 index 00000000..5a5fdd32 --- /dev/null +++ b/apps/server/test/integration/page-permission-workspace-filter.int-spec.ts @@ -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; + 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, + ); + }); +});