From 71fc58dbedc27e4b61ad84e5172464269e4405d2 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 15:16:15 +0300 Subject: [PATCH] harden(page-templates): throttle lookup/toggle; workspace-scope ref writes Release-cycle review: POST /pages/template/lookup had only JwtAuthGuard and the embed depth cap was client-only, so a scripted client could drive heavy full-content fan-out (access control holds per-id, but a cost/DoS gap). And page_template_references rows were written for any sourcePageId with no workspace check at sync time (no leak today since lookup re-checks access, but the graph could accumulate cross-space rows). - Apply the standard per-user throttler (PAGE_TEMPLATE_THROTTLER, 30/min) to /pages/template/lookup and /pages/toggle-template (mirrors ai-chat); auth + the toggle's validateCanEdit CASL are unchanged. - syncPageTemplateReferences / insertTemplateReferencesForPages now restrict inserts to in-workspace source ids (filterInWorkspaceSourceIds, workspace + not-deleted scoped, trx-aware) and still delete stale out-of-workspace rows (self-heal). SECURITY comment: the ref table is NOT access-filtered; every consumer must permission-filter at read time (as lookupTemplate does). - Tests: lookup access exercises the REAL filterViewerAccessiblePageIds (no_access / cross-workspace excluded / accessible+comment-stripped / <=50); toggle controller CASL (cannot-edit -> Forbidden, flag not flipped); ref-sync excludes cross-workspace and keeps in-workspace. Co-Authored-By: Claude Opus 4.8 --- .../transclusion/page-template.controller.ts | 12 + .../spec/page-template-access.spec.ts | 267 ++++++++++++++++++ .../spec/page-template.controller.spec.ts | 93 ++++++ .../page/transclusion/transclusion.service.ts | 90 +++++- .../integrations/throttle/throttle.module.ts | 11 +- .../integrations/throttle/throttler-names.ts | 1 + 6 files changed, 464 insertions(+), 10 deletions(-) create mode 100644 apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts create mode 100644 apps/server/src/core/page/transclusion/spec/page-template.controller.spec.ts diff --git a/apps/server/src/core/page/transclusion/page-template.controller.ts b/apps/server/src/core/page/transclusion/page-template.controller.ts index a86bfcbc..555a487f 100644 --- a/apps/server/src/core/page/transclusion/page-template.controller.ts +++ b/apps/server/src/core/page/transclusion/page-template.controller.ts @@ -7,6 +7,7 @@ import { Post, UseGuards, } from '@nestjs/common'; +import { Throttle } from '@nestjs/throttler'; import { JwtAuthGuard } from '../../../common/guards/jwt-auth.guard'; import { AuthUser } from '../../../common/decorators/auth-user.decorator'; import { User } from '@docmost/db/types/entity.types'; @@ -15,6 +16,8 @@ import { TemplateLookupDto } from './dto/template-lookup.dto'; import { PageRepo } from '@docmost/db/repos/page/page.repo'; import { PageAccessService } from '../page-access/page-access.service'; import { ToggleTemplateDto } from './dto/toggle-template.dto'; +import { UserThrottlerGuard } from '../../../integrations/throttle/user-throttler.guard'; +import { PAGE_TEMPLATE_THROTTLER } from '../../../integrations/throttle/throttler-names'; @UseGuards(JwtAuthGuard) @Controller('pages') @@ -28,7 +31,14 @@ export class PageTemplateController { /** * Whole-page live embed lookup for authenticated viewers. Returns current * content (comment marks stripped) for accessible source pages. + * + * DoS note: the embed cycle/depth cap (PAGE_EMBED_MAX_DEPTH=5) is enforced + * CLIENT-side only — a scripted client could otherwise drive heavy full-doc + * fan-out. The server bounds the cost with this per-user throttle plus the + * DTO's ArrayMaxSize(50) cap; server-side recursive expansion is out of scope. */ + @UseGuards(JwtAuthGuard, UserThrottlerGuard) + @Throttle({ [PAGE_TEMPLATE_THROTTLER]: { limit: 30, ttl: 60000 } }) @HttpCode(HttpStatus.OK) @Post('template/lookup') async lookup(@Body() dto: TemplateLookupDto, @AuthUser() user: User) { @@ -44,6 +54,8 @@ export class PageTemplateController { * inside `validateCanEdit`). The flag only affects template picker discovery; * it does not restrict editing or embedding. */ + @UseGuards(JwtAuthGuard, UserThrottlerGuard) + @Throttle({ [PAGE_TEMPLATE_THROTTLER]: { limit: 30, ttl: 60000 } }) @HttpCode(HttpStatus.OK) @Post('toggle-template') async toggleTemplate( diff --git a/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts b/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts new file mode 100644 index 00000000..3c497d80 --- /dev/null +++ b/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts @@ -0,0 +1,267 @@ +import { TransclusionService } from '../transclusion.service'; + +/** + * Exercises the REAL security core of the whole-page template feature rather + * than mocking it away: + * - `filterViewerAccessiblePageIds` runs for real (space-visibility query + + * page-permission filter are stubbed, but the branching/AND-ing is real), so + * `lookupTemplate` actually maps no_access vs content based on it. + * - the workspace scoping of `page_template_references` writes is verified to + * drop cross-workspace source ids before they are persisted. + */ +describe('TransclusionService — template access core (real filter)', () => { + /** + * Build a chainable kysely `db` stub. `selectFrom(...).select(...).where(...)` + * all return the same builder; `.execute()` resolves the supplied rows. The + * `where('spaceId','in', getUserSpaceIdsQuery(...))` sub-query argument is + * ignored — space visibility is decided by what `execute()` returns. + */ + function makeDb(executeRows: Array<{ id: string }>) { + const builder: any = {}; + builder.selectFrom = jest.fn(() => builder); + builder.select = jest.fn(() => builder); + builder.where = jest.fn(() => builder); + builder.execute = jest.fn(async () => executeRows); + return builder; + } + + function makeService(opts: { + /** rows returned by the space-visibility query (workspace + space scoped) */ + spaceVisibleRows: Array<{ id: string }>; + /** ids that survive page-level permission filtering */ + permissionAccessibleIds: string[]; + pages?: Array<{ + id: string; + slugId?: string; + title: string | null; + icon: string | null; + content: unknown; + updatedAt: Date; + }>; + }) { + const db = makeDb(opts.spaceVisibleRows); + + const spaceMemberRepo = { + // The real code only passes this query object into `.where(...)`; our db + // stub ignores it, so a sentinel is fine. + getUserSpaceIdsQuery: jest.fn(() => ({ __subquery: true })), + }; + + const pagePermissionRepo = { + filterAccessiblePageIds: jest + .fn() + .mockResolvedValue(opts.permissionAccessibleIds), + }; + + const pageRepo = { + findManyByIds: jest.fn().mockResolvedValue(opts.pages ?? []), + }; + + const service = new TransclusionService( + db as any, + {} as any, // pageTransclusionsRepo + {} as any, // pageTransclusionReferencesRepo + {} as any, // pageTemplateReferencesRepo + pageRepo as any, + pagePermissionRepo as any, + spaceMemberRepo as any, + {} as any, // attachmentRepo + {} as any, // storageService + {} as any, // pageAccessService + ); + + return { service, db, pageRepo, spaceMemberRepo, pagePermissionRepo }; + } + + const now = new Date('2026-06-20T00:00:00.000Z'); + + it('returns no_access when the viewer fails the page-permission filter (real filter runs)', async () => { + // Space-visible, but page-permission filter rejects it. + const { service, pagePermissionRepo } = makeService({ + spaceVisibleRows: [{ id: 'p1' }], + permissionAccessibleIds: [], + }); + + const { items } = await service.lookupTemplate(['p1'], 'u1', 'w1'); + expect(items).toEqual([{ sourcePageId: 'p1', status: 'no_access' }]); + // proves the real filter executed and consulted page permissions + expect(pagePermissionRepo.filterAccessiblePageIds).toHaveBeenCalledWith({ + pageIds: ['p1'], + userId: 'u1', + }); + }); + + it('returns no_access for a cross-workspace id (space-visibility query excludes it)', async () => { + // The workspace/space-scoped query returns nothing → permission filter is + // never reached and the id is not returned as accessible. + const { service, pagePermissionRepo } = makeService({ + spaceVisibleRows: [], + permissionAccessibleIds: ['cross-ws'], + }); + + const { items } = await service.lookupTemplate(['cross-ws'], 'u1', 'w1'); + expect(items).toEqual([{ sourcePageId: 'cross-ws', status: 'no_access' }]); + // short-circuited before page-permission filtering + expect(pagePermissionRepo.filterAccessiblePageIds).not.toHaveBeenCalled(); + }); + + it('returns content with comment marks stripped for an accessible page', async () => { + const content = { + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { + type: 'text', + text: 'hello', + marks: [{ type: 'comment', attrs: { commentId: 'c1' } }], + }, + ], + }, + ], + }; + + const { service } = makeService({ + spaceVisibleRows: [{ id: 'p1' }], + permissionAccessibleIds: ['p1'], + pages: [ + { + id: 'p1', + slugId: 's1', + title: 'Tmpl', + icon: '📄', + content, + updatedAt: now, + }, + ], + }); + + const { items } = await service.lookupTemplate(['p1'], 'u1', 'w1'); + const item = items[0] as any; + expect(item.status).toBeUndefined(); + expect(item.title).toBe('Tmpl'); + const json = JSON.stringify(item.content); + expect(json).not.toContain('comment'); + expect(json).toContain('hello'); + }); + + it('mixes accessible and inaccessible ids in one batch positionally', async () => { + const { service } = makeService({ + spaceVisibleRows: [{ id: 'ok' }, { id: 'denied' }], + permissionAccessibleIds: ['ok'], + pages: [ + { + id: 'ok', + slugId: 's', + title: 'A', + icon: null, + content: { type: 'doc', content: [] }, + updatedAt: now, + }, + ], + }); + + const { items } = await service.lookupTemplate( + ['denied', 'ok', 'cross'], + 'u1', + 'w1', + ); + expect((items[0] as any).status).toBe('no_access'); // space-visible but no perm + expect((items[1] as any).status).toBeUndefined(); // accessible + expect((items[2] as any).status).toBe('no_access'); // not space-visible + }); + + it('honours the DTO-level ≤50 cap by deduping ids passed to the filter', async () => { + // The DTO enforces ArrayMaxSize(50); the service dedupes before filtering. + const ids = ['a', 'a', 'b']; + const { service, db } = makeService({ + spaceVisibleRows: [], + permissionAccessibleIds: [], + }); + + await service.lookupTemplate(ids, 'u1', 'w1'); + // db.where('id','in', ) — verify the in-clause got deduped ids + const inCall = db.where.mock.calls.find((c: any[]) => c[0] === 'id'); + expect(inCall?.[2]).toEqual(['a', 'b']); + }); +}); + +describe('TransclusionService.syncPageTemplateReferences — workspace scoping', () => { + function makeService(opts: { inWorkspaceIds: string[] }) { + // db stub: the in-workspace existence query returns only allowed ids. + const builder: any = {}; + builder.selectFrom = jest.fn(() => builder); + builder.select = jest.fn(() => builder); + builder.where = jest.fn(() => builder); + builder.execute = jest.fn(async () => + opts.inWorkspaceIds.map((id) => ({ id })), + ); + + const insertMany = jest.fn().mockResolvedValue(undefined); + const deleteByReferenceAndSources = jest.fn().mockResolvedValue(undefined); + const pageTemplateReferencesRepo = { + findByReferencePageId: jest.fn().mockResolvedValue([]), + insertMany, + deleteByReferenceAndSources, + }; + + const service = new TransclusionService( + builder as any, + {} as any, + {} as any, + pageTemplateReferencesRepo as any, + {} as any, + {} as any, + {} as any, + {} as any, + {} as any, + {} as any, + ); + + return { service, insertMany, pageTemplateReferencesRepo }; + } + + function docWithEmbeds(sourceIds: string[]) { + return { + type: 'doc', + content: sourceIds.map((id) => ({ + type: 'pageEmbed', + attrs: { sourcePageId: id }, + })), + }; + } + + it('does NOT write a row for a cross-workspace sourcePageId, but writes the in-workspace one', async () => { + const { service, insertMany } = makeService({ + // only the in-workspace id survives the existence query + inWorkspaceIds: ['in-ws'], + }); + + const result = await service.syncPageTemplateReferences( + 'host', + 'w1', + docWithEmbeds(['in-ws', 'cross-ws']), + ); + + expect(result.inserted).toBe(1); + expect(insertMany).toHaveBeenCalledTimes(1); + const rows = insertMany.mock.calls[0][0]; + expect(rows).toEqual([ + { workspaceId: 'w1', referencePageId: 'host', sourcePageId: 'in-ws' }, + ]); + }); + + it('inserts nothing when every embed points at a cross-workspace source', async () => { + const { service, insertMany } = makeService({ inWorkspaceIds: [] }); + + const result = await service.syncPageTemplateReferences( + 'host', + 'w1', + docWithEmbeds(['cross-a', 'cross-b']), + ); + + expect(result.inserted).toBe(0); + expect(insertMany).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/core/page/transclusion/spec/page-template.controller.spec.ts b/apps/server/src/core/page/transclusion/spec/page-template.controller.spec.ts new file mode 100644 index 00000000..2de644e0 --- /dev/null +++ b/apps/server/src/core/page/transclusion/spec/page-template.controller.spec.ts @@ -0,0 +1,93 @@ +import { Test } from '@nestjs/testing'; +import { ForbiddenException, NotFoundException } from '@nestjs/common'; +import { PageTemplateController } from '../page-template.controller'; +import { TransclusionService } from '../transclusion.service'; +import { PageRepo } from '@docmost/db/repos/page/page.repo'; +import { PageAccessService } from '../../page-access/page-access.service'; +import { JwtAuthGuard } from '../../../../common/guards/jwt-auth.guard'; +import { UserThrottlerGuard } from '../../../../integrations/throttle/user-throttler.guard'; + +describe('PageTemplateController.toggleTemplate', () => { + let controller: PageTemplateController; + let pageRepo: { findById: jest.Mock; updatePage: jest.Mock }; + let pageAccessService: { validateCanEdit: jest.Mock }; + let transclusionService: Partial>; + + const user = { id: 'u1', workspaceId: 'w1' } as any; + const page = { + id: 'p1', + workspaceId: 'w1', + deletedAt: null, + isTemplate: false, + } as any; + + beforeEach(async () => { + pageRepo = { + findById: jest.fn().mockResolvedValue(page), + updatePage: jest.fn().mockResolvedValue(undefined), + }; + pageAccessService = { + validateCanEdit: jest.fn().mockResolvedValue(undefined), + }; + transclusionService = { lookupTemplate: jest.fn() }; + + const module = await Test.createTestingModule({ + controllers: [PageTemplateController], + providers: [ + { provide: TransclusionService, useValue: transclusionService }, + { provide: PageRepo, useValue: pageRepo }, + { provide: PageAccessService, useValue: pageAccessService }, + ], + }) + .overrideGuard(JwtAuthGuard) + .useValue({ canActivate: () => true }) + .overrideGuard(UserThrottlerGuard) + .useValue({ canActivate: () => true }) + .compile(); + + controller = module.get(PageTemplateController); + }); + + it('throws NotFound and does not touch the page when the page is missing', async () => { + pageRepo.findById.mockResolvedValue(null); + await expect( + controller.toggleTemplate({ pageId: 'p1' } as any, user), + ).rejects.toBeInstanceOf(NotFoundException); + expect(pageAccessService.validateCanEdit).not.toHaveBeenCalled(); + expect(pageRepo.updatePage).not.toHaveBeenCalled(); + }); + + it('enforces CASL edit: when validateCanEdit throws, the flag is NOT flipped', async () => { + pageAccessService.validateCanEdit.mockRejectedValue( + new ForbiddenException(), + ); + await expect( + controller.toggleTemplate({ pageId: 'p1' } as any, user), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(pageAccessService.validateCanEdit).toHaveBeenCalledWith(page, user); + expect(pageRepo.updatePage).not.toHaveBeenCalled(); + }); + + it('flips is_template (toggle) when the user can edit', async () => { + const out = await controller.toggleTemplate( + { pageId: 'p1' } as any, + user, + ); + expect(pageAccessService.validateCanEdit).toHaveBeenCalledWith(page, user); + // page.isTemplate was false → toggled to true + expect(pageRepo.updatePage).toHaveBeenCalledWith({ isTemplate: true }, 'p1'); + expect(out).toEqual({ pageId: 'p1', isTemplate: true }); + }); + + it('respects an explicit isTemplate flag instead of toggling', async () => { + const out = await controller.toggleTemplate( + { pageId: 'p1', isTemplate: false } as any, + user, + ); + expect(pageRepo.updatePage).toHaveBeenCalledWith( + { isTemplate: false }, + 'p1', + ); + expect(out).toEqual({ pageId: 'p1', isTemplate: false }); + }); +}); diff --git a/apps/server/src/core/page/transclusion/transclusion.service.ts b/apps/server/src/core/page/transclusion/transclusion.service.ts index 419158e2..afe08e44 100644 --- a/apps/server/src/core/page/transclusion/transclusion.service.ts +++ b/apps/server/src/core/page/transclusion/transclusion.service.ts @@ -232,10 +232,41 @@ export class TransclusionService { // Whole-page live embeds (pageEmbed node) // --------------------------------------------------------------------------- + /** + * Restrict a set of candidate `pageEmbed` source ids to the pages that + * actually live in `workspaceId` (and are not soft-deleted). Defense in depth: + * `page_template_references` is NOT access-filtered, so we must never persist a + * reference to a cross-workspace source page. This is a single workspace-scoped + * existence query; it does NOT do per-viewer permission filtering (that stays + * the job of `lookupTemplate` at read time — see the warning below). + */ + private async filterInWorkspaceSourceIds( + sourceIds: string[], + workspaceId: string, + trx?: KyselyTransaction, + ): Promise> { + if (sourceIds.length === 0) return new Set(); + const db = trx ?? this.db; + const rows = await db + .selectFrom('pages') + .select('id') + .where('id', 'in', sourceIds) + .where('workspaceId', '=', workspaceId) + .where('deletedAt', 'is', null) + .execute(); + return new Set(rows.map((r) => r.id)); + } + /** * Diff `page_template_references` for a host page against the `pageEmbed` * nodes currently in its content. Mirror of `syncPageReferences` but keyed by * `sourcePageId` only (whole-page, no transclusionId). Idempotent. + * + * SECURITY: `page_template_references` rows are NOT access-filtered. Inserts + * are restricted here to in-workspace source pages so the graph can never + * accumulate cross-workspace edges, but rows are still NOT per-viewer + * permission-filtered. EVERY consumer of these rows MUST permission-filter at + * read time (as `lookupTemplate` does via `filterViewerAccessiblePageIds`). */ async syncPageTemplateReferences( referencePageId: string, @@ -244,7 +275,14 @@ export class TransclusionService { trx?: KyselyTransaction, ): Promise<{ inserted: number; deleted: number }> { const desired = collectPageEmbedsFromPmJson(pmJson); - const desiredIds = new Set(desired.map((d) => d.sourcePageId)); + const inWorkspace = await this.filterInWorkspaceSourceIds( + desired.map((d) => d.sourcePageId), + workspaceId, + trx, + ); + const desiredIds = new Set( + desired.map((d) => d.sourcePageId).filter((id) => inWorkspace.has(id)), + ); const existing = await this.pageTemplateReferencesRepo.findByReferencePageId( @@ -253,12 +291,12 @@ export class TransclusionService { ); const existingIds = new Set(existing.map((e) => e.sourcePageId)); - const toInsert = desired - .filter((d) => !existingIds.has(d.sourcePageId)) - .map((d) => ({ + const toInsert = Array.from(desiredIds) + .filter((id) => !existingIds.has(id)) + .map((sourcePageId) => ({ workspaceId, referencePageId, - sourcePageId: d.sourcePageId, + sourcePageId, })); const toDelete = existing @@ -282,23 +320,57 @@ export class TransclusionService { /** * Bulk-insert `page_template_references` for brand-new pages (duplication, * import) where there is nothing to diff against. + * + * SECURITY: like `syncPageTemplateReferences`, inserts are restricted to + * in-workspace source pages so the (non-access-filtered) reference graph never + * gains a cross-workspace edge. Read-time per-viewer permission filtering is + * still required by every consumer. */ async insertTemplateReferencesForPages( pages: Array<{ id: string; workspaceId: string; content: unknown }>, trx?: KyselyTransaction, ): Promise<{ inserted: number }> { + // Collect candidate source ids per workspace, then validate each workspace's + // set in a single existence query before building insert rows. + const candidatesByWorkspace = new Map>(); + const pageEmbeds = pages.map((page) => { + const sourceIds = collectPageEmbedsFromPmJson(page.content).map( + (e) => e.sourcePageId, + ); + let set = candidatesByWorkspace.get(page.workspaceId); + if (!set) { + set = new Set(); + candidatesByWorkspace.set(page.workspaceId, set); + } + for (const id of sourceIds) set.add(id); + return { page, sourceIds }; + }); + + const inWorkspaceByWorkspace = new Map>(); + for (const [workspaceId, candidates] of candidatesByWorkspace) { + inWorkspaceByWorkspace.set( + workspaceId, + await this.filterInWorkspaceSourceIds( + Array.from(candidates), + workspaceId, + trx, + ), + ); + } + const rows: Array<{ workspaceId: string; referencePageId: string; sourcePageId: string; }> = []; - for (const page of pages) { - const embeds = collectPageEmbedsFromPmJson(page.content); - for (const e of embeds) { + for (const { page, sourceIds } of pageEmbeds) { + const inWorkspace = inWorkspaceByWorkspace.get(page.workspaceId); + for (const sourcePageId of sourceIds) { + if (!inWorkspace?.has(sourcePageId)) continue; rows.push({ workspaceId: page.workspaceId, referencePageId: page.id, - sourcePageId: e.sourcePageId, + sourcePageId, }); } } diff --git a/apps/server/src/integrations/throttle/throttle.module.ts b/apps/server/src/integrations/throttle/throttle.module.ts index 42dd0ec4..1db3c5e6 100644 --- a/apps/server/src/integrations/throttle/throttle.module.ts +++ b/apps/server/src/integrations/throttle/throttle.module.ts @@ -4,7 +4,11 @@ import { ThrottlerStorageRedisService } from '@nest-lab/throttler-storage-redis' import { EnvironmentService } from '../environment/environment.service'; import { EnvironmentModule } from '../environment/environment.module'; import { parseRedisUrl } from '../../common/helpers'; -import { AUTH_THROTTLER, AI_CHAT_THROTTLER } from './throttler-names'; +import { + AUTH_THROTTLER, + AI_CHAT_THROTTLER, + PAGE_TEMPLATE_THROTTLER, +} from './throttler-names'; import Redis from 'ioredis'; @Module({ @@ -18,6 +22,11 @@ import Redis from 'ioredis'; throttlers: [ { name: AUTH_THROTTLER, ttl: 60_000, limit: 10 }, { name: AI_CHAT_THROTTLER, ttl: 60_000, limit: 25 }, + // Whole-page template lookup returns full ProseMirror docs for up + // to 50 ids per call and the embed depth cap is client-side only, so + // a scripted client could drive heavy content fan-out. 30 req/min + // per user is plenty for legitimate render-time batched lookups. + { name: PAGE_TEMPLATE_THROTTLER, ttl: 60_000, limit: 30 }, ], errorMessage: 'Too many requests', storage: new ThrottlerStorageRedisService( diff --git a/apps/server/src/integrations/throttle/throttler-names.ts b/apps/server/src/integrations/throttle/throttler-names.ts index 388ba29d..2877ff51 100644 --- a/apps/server/src/integrations/throttle/throttler-names.ts +++ b/apps/server/src/integrations/throttle/throttler-names.ts @@ -1,2 +1,3 @@ export const AUTH_THROTTLER = 'auth'; export const AI_CHAT_THROTTLER = 'ai-chat'; +export const PAGE_TEMPLATE_THROTTLER = 'page-template';