From 3f464961929ffd3055b83ca9294c9249df464e20 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sun, 21 Jun 2026 04:04:09 +0300 Subject: [PATCH] refactor(share): single resolveReadableSharePage for the share access boundary (#92) The '(shareId,pageId) -> usable non-restricted page in THIS share' boundary was written as 3 must-be-identical async sequences. They weren't: the chat funnel omitted an explicit page.deletedAt check (latently safe via getShareForPage's CTE) and layered isSharingAllowed separately. Add ShareService.resolveReadable- SharePage(shareId,pageId,workspaceId) running the single canonical sequence (getShareForPage -> id match (skipped when null) -> findById -> !deletedAt -> !hasRestrictedAncestor) returning {share,page}|null; getSharedPage, the funnel, and the getSharePage tool all use it. hasRestrictedAncestor now lives in the one method no caller can skip; the funnel still returns uniform 404s and keeps isSharingAllowed. Adds a direct security-invariant test. Co-Authored-By: Claude Opus 4.8 --- .../public-share-chat.controller.spec.ts | 74 +++++----- .../ai-chat/public-share-chat.controller.ts | 56 ++++---- .../core/ai-chat/public-share-chat.spec.ts | 67 ++++----- .../public-share-chat-tools.service.spec.ts | 63 ++++---- .../tools/public-share-chat-tools.service.ts | 47 +++--- .../share/share-resolve-readable-page.spec.ts | 136 ++++++++++++++++++ apps/server/src/core/share/share.service.ts | 82 +++++++++-- 7 files changed, 347 insertions(+), 178 deletions(-) create mode 100644 apps/server/src/core/share/share-resolve-readable-page.spec.ts diff --git a/apps/server/src/core/ai-chat/public-share-chat.controller.spec.ts b/apps/server/src/core/ai-chat/public-share-chat.controller.spec.ts index 83f6252e..13c19f8b 100644 --- a/apps/server/src/core/ai-chat/public-share-chat.controller.spec.ts +++ b/apps/server/src/core/ai-chat/public-share-chat.controller.spec.ts @@ -21,12 +21,16 @@ import type { UIMessage } from 'ai'; */ describe('resolveShareAssistantRequest (extracted controller funnel)', () => { /** A fully-passing dep set; individual tests override single collaborators. */ + /** + * Default share + page resolve: the canonical boundary returns a usable share + * (matching SHARE-A) with a live, unrestricted page. The default share id is + * SHARE-A so the share-id match passes; tests override `resolveReadableSharePage` + * to simulate a cross-share swap / restricted / out-of-tree (all => null). + */ function makeDeps(over: { assistantEnabled?: boolean; - getShareForPage?: jest.Mock; + resolveReadableSharePage?: jest.Mock; isSharingAllowed?: jest.Mock; - findById?: jest.Mock; - hasRestrictedAncestor?: jest.Mock; resolveShareRole?: jest.Mock; getShareChatModel?: jest.Mock; tryConsumeWorkspaceQuota?: jest.Mock; @@ -37,25 +41,23 @@ describe('resolveShareAssistantRequest (extracted controller funnel)', () => { .mockResolvedValue(over.assistantEnabled ?? true), }; const shareService = { - getShareForPage: - over.getShareForPage ?? + // The SINGLE canonical (shareId, pageId) -> readable page boundary. + // Returns { share, page } on success, null on ANY access failure + // (out-of-tree / cross-share id swap / deleted / restricted descendant). + resolveReadableSharePage: + over.resolveReadableSharePage ?? jest.fn().mockResolvedValue({ - id: 'SHARE-A', - pageId: 'root-page', - spaceId: 'space-1', - sharedPage: { id: 'root-page', title: 'Root' }, + share: { + id: 'SHARE-A', + pageId: 'root-page', + spaceId: 'space-1', + sharedPage: { id: 'root-page', title: 'Root' }, + }, + page: { id: 'opened-uuid' }, }), isSharingAllowed: over.isSharingAllowed ?? jest.fn().mockResolvedValue(true), }; - const pageRepo = { - findById: - over.findById ?? jest.fn().mockResolvedValue({ id: 'opened-uuid' }), - }; - const pagePermissionRepo = { - hasRestrictedAncestor: - over.hasRestrictedAncestor ?? jest.fn().mockResolvedValue(false), - }; const publicShareChat = { resolveShareRole: over.resolveShareRole ?? jest.fn().mockResolvedValue(null), @@ -67,16 +69,12 @@ describe('resolveShareAssistantRequest (extracted controller funnel)', () => { const deps: ShareAssistantDeps = { aiSettings: aiSettings as never, shareService: shareService as never, - pageRepo: pageRepo as never, - pagePermissionRepo: pagePermissionRepo as never, publicShareChat: publicShareChat as never, }; return { deps, aiSettings, shareService, - pageRepo, - pagePermissionRepo, publicShareChat, }; } @@ -119,42 +117,46 @@ describe('resolveShareAssistantRequest (extracted controller funnel)', () => { }); it('assistant disabled => 404 and NO share/page/model lookups', async () => { - const { deps, shareService, pageRepo, publicShareChat } = makeDeps({ + const { deps, shareService, publicShareChat } = makeDeps({ assistantEnabled: false, }); expect(await statusOf(deps, body())).toBe(404); - expect(shareService.getShareForPage).not.toHaveBeenCalled(); - expect(pageRepo.findById).not.toHaveBeenCalled(); + // The whole share/page resolve is skipped when the feature is off. + expect(shareService.resolveReadableSharePage).not.toHaveBeenCalled(); expect(publicShareChat.getShareChatModel).not.toHaveBeenCalled(); }); it('share.id !== body.shareId => 404 (cross-share id swap rejected)', async () => { - const { deps, publicShareChat } = makeDeps({ - getShareForPage: jest.fn().mockResolvedValue({ - id: 'OTHER-SHARE', - pageId: 'root', - spaceId: 'space-1', - sharedPage: null, - }), + // A cross-share id swap makes the canonical boundary return null (it checks + // share.id === requested shareId internally). + const { deps, shareService, publicShareChat } = makeDeps({ + resolveReadableSharePage: jest.fn().mockResolvedValue(null), }); expect(await statusOf(deps, body({ shareId: 'SHARE-A' }))).toBe(404); + expect(shareService.resolveReadableSharePage).toHaveBeenCalledWith( + 'SHARE-A', + 'opened-page', + 'ws-1', + ); // Never reached the model resolution for an unusable share. expect(publicShareChat.getShareChatModel).not.toHaveBeenCalled(); }); - it('opened page unresolvable (pageRepo.findById -> null) => fail-closed 404', async () => { + it('opened page unresolvable / deleted (resolve -> null) => fail-closed 404', async () => { const { deps } = makeDeps({ - findById: jest.fn().mockResolvedValue(null), + resolveReadableSharePage: jest.fn().mockResolvedValue(null), }); expect(await statusOf(deps, body())).toBe(404); }); it('restricted descendant => 404 (same as out-of-tree, no existence leak)', async () => { - const { deps, pagePermissionRepo } = makeDeps({ - hasRestrictedAncestor: jest.fn().mockResolvedValue(true), + // The canonical boundary folds the restricted-ancestor gate in: a restricted + // descendant resolves to null, indistinguishable from an out-of-tree page. + const { deps, shareService } = makeDeps({ + resolveReadableSharePage: jest.fn().mockResolvedValue(null), }); expect(await statusOf(deps, body())).toBe(404); - expect(pagePermissionRepo.hasRestrictedAncestor).toHaveBeenCalled(); + expect(shareService.resolveReadableSharePage).toHaveBeenCalled(); }); it('getShareChatModel throws AiNotConfiguredException => 503', async () => { diff --git a/apps/server/src/core/ai-chat/public-share-chat.controller.ts b/apps/server/src/core/ai-chat/public-share-chat.controller.ts index 726b6487..74f8b538 100644 --- a/apps/server/src/core/ai-chat/public-share-chat.controller.ts +++ b/apps/server/src/core/ai-chat/public-share-chat.controller.ts @@ -19,8 +19,6 @@ import { AuthWorkspace } from '../../common/decorators/auth-workspace.decorator' import { SkipTransform } from '../../common/decorators/skip-transform.decorator'; import { PUBLIC_SHARE_AI_THROTTLER } from '../../integrations/throttle/throttler-names'; import { ShareService } from '../share/share.service'; -import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'; -import { PageRepo } from '@docmost/db/repos/page/page.repo'; import { AiSettingsService } from '../../integrations/ai/ai-settings.service'; import { AiNotConfiguredException } from '../../integrations/ai/ai-not-configured.exception'; import { @@ -53,8 +51,6 @@ export class PublicShareChatController { constructor( private readonly shareService: ShareService, - private readonly pagePermissionRepo: PagePermissionRepo, - private readonly pageRepo: PageRepo, private readonly aiSettings: AiSettingsService, private readonly publicShareChat: PublicShareChatService, ) {} @@ -89,8 +85,6 @@ export class PublicShareChatController { { aiSettings: this.aiSettings, shareService: this.shareService, - pageRepo: this.pageRepo, - pagePermissionRepo: this.pagePermissionRepo, publicShareChat: this.publicShareChat, }, { workspaceId: workspace.id, body }, @@ -145,12 +139,13 @@ export class PublicShareChatController { */ export interface ShareAssistantDeps { aiSettings: Pick; + // The (shareId, pageId) -> readable page resolve is the SINGLE canonical + // share-access boundary (resolveReadableSharePage); isSharingAllowed remains a + // separate workspace/space toggle this funnel layers on top of it. shareService: Pick< ShareService, - 'getShareForPage' | 'isSharingAllowed' + 'resolveReadableSharePage' | 'isSharingAllowed' >; - pageRepo: Pick; - pagePermissionRepo: Pick; publicShareChat: Pick< PublicShareChatService, | 'resolveShareRole' @@ -162,7 +157,9 @@ export interface ShareAssistantDeps { /** The resolved, validated request ready to stream (everything is non-null). */ export interface ResolvedShareAssistantRequest { shareId: string; - share: NonNullable>>; + share: NonNullable< + Awaited> + >['share']; model: Awaited>; role: AiAgentRole | null; messages: UIMessage[]; @@ -196,33 +193,40 @@ export async function resolveShareAssistantRequest( const assistantEnabled = await deps.aiSettings.isPublicShareAssistantEnabled(workspaceId); - // 2/3. Share usable? Page in share? Resolved via the page's share membership, - // since getShareForPage ALSO yields the share + workspace. The opened - // page is then gated by an explicit restricted-ancestor check (which - // getShareForPage does NOT do) so a restricted page hidden from the - // public view is graded not-in-share. - let share: Awaited> | undefined; + // 2/3. Share usable? Page in share? The (shareId, pageId) -> readable page + // resolve is delegated WHOLE to the single canonical share-access + // boundary: resolveReadableSharePage returns non-null ONLY when the page + // resolves to THIS share, matches the requested shareId, is live, and has + // NO restricted ancestor (the gate getShareForPage does NOT itself do). + // So `pageInShare` is exactly "resolve succeeded". `isSharingAllowed` + // stays a SEPARATE workspace/space toggle layered on top (it is NOT part + // of the resolve), feeding `shareUsable` via deriveShareAccess. + let share: + | NonNullable< + Awaited> + >['share'] + | undefined; let shareUsable = false; let pageInShare = false; if (assistantEnabled && shareId && pageId) { - share = await deps.shareService.getShareForPage(pageId, workspaceId); - if (share && share.id === shareId) { + const resolved = await deps.shareService.resolveReadableSharePage( + shareId, + pageId, + workspaceId, + ); + if (resolved) { + share = resolved.share; const sharingAllowed = await deps.shareService.isSharingAllowed( workspaceId, share.spaceId, ); - // hasRestrictedAncestor matches on the page UUID only, while the opened - // pageId may be a slugId, so resolve to the UUID first (cheap base-fields - // lookup). An unresolvable opened page fails closed (not-in-share). - const openedPageRow = await deps.pageRepo.findById(pageId); - const restricted = openedPageRow - ? await deps.pagePermissionRepo.hasRestrictedAncestor(openedPageRow.id) - : true; + // The resolve already guarantees the page is in THIS share AND not + // restricted; deriveShareAccess folds in the orthogonal sharing toggle. ({ shareUsable, pageInShare } = deriveShareAccess({ resolvedShareId: share.id, requestedShareId: shareId, sharingAllowed, - restricted, + restricted: false, })); } } diff --git a/apps/server/src/core/ai-chat/public-share-chat.spec.ts b/apps/server/src/core/ai-chat/public-share-chat.spec.ts index 55e5571b..a1ef621c 100644 --- a/apps/server/src/core/ai-chat/public-share-chat.spec.ts +++ b/apps/server/src/core/ai-chat/public-share-chat.spec.ts @@ -525,17 +525,15 @@ describe('PublicShareChatService.tryConsumeWorkspaceQuota', () => { describe('PublicShareChatToolsService share scoping', () => { it('getSharePage rejects a page that does not resolve to THIS share (no existence leak)', async () => { const shareService = { - // The page resolves to a DIFFERENT share id. - getShareForPage: jest.fn().mockResolvedValue({ id: 'OTHER-SHARE' }), + // An out-of-share / cross-share page => the canonical boundary returns null. + resolveReadableSharePage: jest.fn().mockResolvedValue(null), updatePublicAttachments: jest.fn(), }; - const pageRepo = { findById: jest.fn() }; - const pagePermissionRepo = { hasRestrictedAncestor: jest.fn() }; const svc = new PublicShareChatToolsService( shareService as never, {} as never, - pageRepo as never, - pagePermissionRepo as never, + {} as never, + {} as never, ); const tools = svc.forShare('THIS-SHARE', 'ws-1'); @@ -546,37 +544,29 @@ describe('PublicShareChatToolsService share scoping', () => { await expect(getSharePage.execute({ pageId: 'p-outside' })).rejects.toThrow( /not part of this published share/i, ); - // It must NOT have fetched/returned any content for an out-of-share page. - expect(pageRepo.findById).not.toHaveBeenCalled(); + // The tool delegated the resolve to the canonical boundary with the + // forShare-scoped shareId, and returned NO content for a non-resolving page. + expect(shareService.resolveReadableSharePage).toHaveBeenCalledWith( + 'THIS-SHARE', + 'p-outside', + 'ws-1', + ); expect(shareService.updatePublicAttachments).not.toHaveBeenCalled(); - // The restricted check is never even reached for an out-of-share page. - expect(pagePermissionRepo.hasRestrictedAncestor).not.toHaveBeenCalled(); }); it('getSharePage BLOCKS a restricted descendant inside THIS share with the SAME generic error (content leak fix)', async () => { + // A restricted descendant resolves to this share but is hidden from the + // public view; the canonical boundary folds that gate in and returns null, + // so the tool 404s it with the same generic message as out-of-share. const shareService = { - // The restricted page DOES resolve to this share (includeSubPages tree)... - getShareForPage: jest.fn().mockResolvedValue({ id: 'THIS-SHARE' }), + resolveReadableSharePage: jest.fn().mockResolvedValue(null), updatePublicAttachments: jest.fn(), }; - // ...and the page itself exists and is not deleted. - const pageRepo = { - findById: jest - .fn() - .mockResolvedValue({ id: 'p-restricted', title: 'Secret', content: {} }), - }; - // ...but it has a restricted ancestor (its own page_permissions row), so the - // public view 404s it — the tool must NOT return its content. - const pagePermissionRepo = { - hasRestrictedAncestor: jest - .fn() - .mockImplementation(async (id: string) => id === 'p-restricted'), - }; const svc = new PublicShareChatToolsService( shareService as never, {} as never, - pageRepo as never, - pagePermissionRepo as never, + {} as never, + {} as never, ); const tools = svc.forShare('THIS-SHARE', 'ws-1'); @@ -587,11 +577,7 @@ describe('PublicShareChatToolsService share scoping', () => { await expect( getSharePage.execute({ pageId: 'p-restricted' }), ).rejects.toThrow(/not part of this published share/i); - // The restricted check ran on the resolved page id... - expect(pagePermissionRepo.hasRestrictedAncestor).toHaveBeenCalledWith( - 'p-restricted', - ); - // ...and no content was ever sanitized/returned. + // No content was ever sanitized/returned for the blocked page. expect(shareService.updatePublicAttachments).not.toHaveBeenCalled(); }); @@ -735,25 +721,32 @@ describe('public-share assistant boundary locks (red-team regression guards)', ( // Even if a caller forged body.shareId, getSharePage re-derives the share for // the requested pageId and rejects anything not resolving to THIS share — // exactly the boundary that held under red-team. + // forShare is scoped to the FORGED share id the attacker passed; the page + // resolves to a DIFFERENT (REAL) share, so the canonical boundary — which + // matches share.id === requested shareId internally — returns null. const shareService = { - getShareForPage: jest.fn().mockResolvedValue({ id: 'REAL-SHARE' }), + resolveReadableSharePage: jest.fn().mockResolvedValue(null), updatePublicAttachments: jest.fn(), }; const svc = new PublicShareChatToolsService( shareService as never, {} as never, - { findById: jest.fn() } as never, - { hasRestrictedAncestor: jest.fn() } as never, + {} as never, + {} as never, ); - // forShare is scoped to the FORGED share id the attacker passed... const tools = svc.forShare('FORGED-SHARE', 'ws-1'); const getSharePage = tools.getSharePage as { execute: (args: { pageId: string }) => Promise; }; - // ...but the page resolves to REAL-SHARE, so the re-derivation rejects it. await expect( getSharePage.execute({ pageId: 'p-elsewhere' }), ).rejects.toThrow(/not part of this published share/i); + // The forged share id is the scope the boundary re-derivation rejects against. + expect(shareService.resolveReadableSharePage).toHaveBeenCalledWith( + 'FORGED-SHARE', + 'p-elsewhere', + 'ws-1', + ); }); it('transcript injection is filtered: only user|assistant survive; forged tool/system roles are dropped', () => { diff --git a/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts b/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts index 1c3f6f8d..9069dfda 100644 --- a/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts +++ b/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts @@ -13,11 +13,13 @@ describe('PublicShareChatToolsService.forShare', () => { getShareTree?: jest.Mock; findById?: jest.Mock; searchPage?: jest.Mock; - getShareForPage?: jest.Mock; + resolveReadableSharePage?: jest.Mock; } = {}) { const shareService = { getShareTree: over.getShareTree ?? jest.fn(), - getShareForPage: over.getShareForPage ?? jest.fn(), + // The single canonical (shareId, pageId) -> readable page boundary. + resolveReadableSharePage: + over.resolveReadableSharePage ?? jest.fn(), updatePublicAttachments: jest.fn(), }; const searchService = { searchPage: over.searchPage ?? jest.fn() }; @@ -120,13 +122,15 @@ describe('PublicShareChatToolsService.forShare', () => { }); describe('getSharePage blank guard', () => { - it('blank pageId => throws "A pageId is required." WITHOUT calling getShareForPage', async () => { - const { svc, shareService } = makeService({ getShareForPage: jest.fn() }); + it('blank pageId => throws "A pageId is required." WITHOUT resolving the share', async () => { + const { svc, shareService } = makeService({ + resolveReadableSharePage: jest.fn(), + }); const tools = svc.forShare('SHARE-A', 'ws-1'); await expect( (tools.getSharePage as unknown as ToolExec).execute({ pageId: ' ' }), ).rejects.toThrow('A pageId is required.'); - expect(shareService.getShareForPage).not.toHaveBeenCalled(); + expect(shareService.resolveReadableSharePage).not.toHaveBeenCalled(); }); }); @@ -168,13 +172,15 @@ describe('PublicShareChatToolsService.forShare', () => { content: rawContent, }; - const { svc, shareService, pageRepo, pagePermissionRepo } = makeService({ - // getShareForPage resolves to THIS share (id matches the forShare scope). - getShareForPage: jest.fn().mockResolvedValue({ id: 'SHARE-A' }), - findById: jest.fn().mockResolvedValue(page), + const { svc, shareService } = makeService({ + // The canonical boundary resolves the page to THIS share, live and + // unrestricted, returning { share, page }. (Membership + liveness + + // restriction are now asserted directly in the resolveReadableSharePage + // unit test in share.service.spec.ts.) + resolveReadableSharePage: jest + .fn() + .mockResolvedValue({ share: { id: 'SHARE-A' }, page }), }); - // Page has no restricted ancestor => passes the restriction gate. - pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(false); // The sanitizer returns the SANITIZED content (raw secrets removed). shareService.updatePublicAttachments.mockResolvedValue(sanitizedContent); @@ -183,17 +189,13 @@ describe('PublicShareChatToolsService.forShare', () => { pageId: ' page-1 ', })) as { title: string; markdown: string }; - // Membership + liveness + restriction checks were all consulted. - expect(shareService.getShareForPage).toHaveBeenCalledWith( + // The tool delegates the whole access resolve to the canonical boundary, + // passing the forShare-scoped shareId + the (trimmed) requested pageId. + expect(shareService.resolveReadableSharePage).toHaveBeenCalledWith( + 'SHARE-A', 'page-1', 'ws-1', ); - expect(pageRepo.findById).toHaveBeenCalledWith('page-1', { - includeContent: true, - }); - expect(pagePermissionRepo.hasRestrictedAncestor).toHaveBeenCalledWith( - 'page-1', - ); // CRITICAL: the sanitizer MUST be called with the page before any content // is converted. If a future change drops/reorders this, raw comment marks @@ -210,30 +212,23 @@ describe('PublicShareChatToolsService.forShare', () => { }); }); - describe('getSharePage soft-deleted page', () => { - it('findById returns a soft-deleted page (deletedAt set) => generic error, NO content fetch (updatePublicAttachments not called, nothing leaked)', async () => { - const deletedPage = { - id: 'page-1', - title: 'Deleted Page', - deletedAt: new Date(), - content: { type: 'doc', content: [] }, - }; - const { svc, shareService, pagePermissionRepo } = makeService({ - getShareForPage: jest.fn().mockResolvedValue({ id: 'SHARE-A' }), - findById: jest.fn().mockResolvedValue(deletedPage), + describe('getSharePage non-resolving page (deleted / restricted / out-of-share)', () => { + it('resolveReadableSharePage returns null (e.g. soft-deleted page) => generic error, NO content sanitized/returned', async () => { + // The canonical boundary 404s a soft-deleted / restricted / out-of-tree + // page uniformly by returning null; the tool must surface the SAME generic + // message and never sanitize/return any content. + const { svc, shareService } = makeService({ + resolveReadableSharePage: jest.fn().mockResolvedValue(null), }); const tools = svc.forShare('SHARE-A', 'ws-1'); - // Same generic message as an out-of-share page (no info leak). await expect( (tools.getSharePage as unknown as ToolExec).execute({ pageId: 'page-1', }), ).rejects.toThrow('That page is not part of this published share.'); - // Short-circuits before the restriction gate AND before the sanitizer: - // no content is ever fetched/returned for a soft-deleted page. - expect(pagePermissionRepo.hasRestrictedAncestor).not.toHaveBeenCalled(); + // No content is ever fetched/returned for a non-resolving page. expect(shareService.updatePublicAttachments).not.toHaveBeenCalled(); }); }); diff --git a/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts b/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts index b15a6f69..8f0998f5 100644 --- a/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts +++ b/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts @@ -22,10 +22,10 @@ import { jsonToMarkdown } from '../../../collaboration/collaboration.util'; * - search uses the existing share-scoped FTS branch * (`shareId && !spaceId && !userId`), which itself restricts results to the * share's pages and excludes restricted descendants; - * - reading a page first confirms, via `getShareForPage`, that the page - * resolves to THIS share AND (because getShareForPage does NOT itself - * exclude restricted descendants) that the page has no restricted ancestor, - * before returning any content. + * - reading a page first confirms, via the single canonical + * `ShareService.resolveReadableSharePage` boundary, that the page resolves + * to THIS share, is live, and has no restricted ancestor (which + * getShareForPage does NOT itself check), before returning any content. */ @Injectable() export class PublicShareChatToolsService { @@ -99,38 +99,23 @@ export class PublicShareChatToolsService { if (!id) { throw new Error('A pageId is required.'); } - // Confirm the page resolves to THIS share (recursive CTE up the tree, - // honouring includeSubPages + workspace check). NOTE: getShareForPage - // joins only the `shares` table — it does NOT exclude restricted - // descendants — so membership alone is not sufficient (see the - // explicit restricted check below, which the public view also does). - // Not in this share => tool error WITHOUT leaking whether the page - // exists at all. - const share = await this.shareService.getShareForPage( + // Resolve via the SINGLE canonical share-access boundary: confirms the + // page resolves to THIS share (recursive CTE up the tree, honouring + // includeSubPages + workspace), the share id matches, the page is live + // (not soft-deleted), and it has NO restricted ancestor (a restricted + // descendant is hidden from the public view even inside an + // includeSubPages share). Any failure => null. Use the SAME generic + // message for every failure so the model cannot distinguish + // "restricted" / "deleted" / "not in share" / "doesn't exist". + const resolved = await this.shareService.resolveReadableSharePage( + shareId, id, workspaceId, ); - if (!share || share.id !== shareId) { - throw new Error('That page is not part of this published share.'); - } - - const page = await this.pageRepo.findById(id, { - includeContent: true, - }); - if (!page || page.deletedAt) { - throw new Error('That page is not part of this published share.'); - } - - // A restricted descendant (a page with its own page_permissions / - // pageAccess row) is hidden from the public share view even when it - // sits inside an includeSubPages share. getShareForPage does NOT - // exclude it, so we must replicate the public view's restricted- - // ancestor gate here (ShareService.getSharedPage). Use the SAME - // generic message as an out-of-share page so the model cannot - // distinguish "restricted" from "not in share" (no info leak). - if (await this.pagePermissionRepo.hasRestrictedAncestor(page.id)) { + if (!resolved) { throw new Error('That page is not part of this published share.'); } + const { page } = resolved; // Reuse the public share-content sanitizer: strips comment marks and // tokenizes attachments for public delivery, exactly as the public diff --git a/apps/server/src/core/share/share-resolve-readable-page.spec.ts b/apps/server/src/core/share/share-resolve-readable-page.spec.ts new file mode 100644 index 00000000..237e483e --- /dev/null +++ b/apps/server/src/core/share/share-resolve-readable-page.spec.ts @@ -0,0 +1,136 @@ +import { ShareService } from './share.service'; + +/** + * Focused unit test for ShareService.resolveReadableSharePage — THE single + * share-access boundary that every public-share read path funnels through. + * + * The security invariant, in one place: a (shareId, pageId) pair resolves to a + * usable page ONLY when it is reachable in this workspace's share graph, is the + * SAME share the caller asked for, is a live (non-deleted) page, and has NO + * restricted ancestor. ANY failure must return null (no exception, no leak of + * which check failed). These cases pin the boundary directly so it cannot drift + * even if a downstream call-site is refactored. + * + * getShareForPage itself is a raw recursive-CTE db query, so it is spied; every + * other collaborator is a plain mock. The restricted-ancestor gate is exercised + * for real (it is the gate getShareForPage does NOT itself perform). + */ +const WS = 'ws-1'; +const SHARE = 'SHARE-A'; +const PAGE = 'page-1'; + +function buildService(over: { + resolvedShare?: unknown; + page?: unknown; + restricted?: boolean; +} = {}) { + const pageRepo = { + findById: jest.fn(async () => + 'page' in over + ? over.page + : { id: PAGE, deletedAt: null, content: {} }, + ), + }; + const pagePermissionRepo = { + hasRestrictedAncestor: jest.fn(async () => over.restricted ?? false), + }; + + const service = new ShareService( + {} as any, // shareRepo (unused on this path) + pageRepo as any, + pagePermissionRepo as any, + {} as any, // db (getShareForPage is spied) + {} as any, // tokenService (unused) + {} as any, // transclusionService (unused) + {} as any, // workspaceRepo (unused) + ); + + jest + .spyOn(service, 'getShareForPage') + .mockResolvedValue( + ('resolvedShare' in over + ? over.resolvedShare + : { id: SHARE, pageId: PAGE, spaceId: 'space-1' }) as any, + ); + + return { service, pageRepo, pagePermissionRepo }; +} + +describe('ShareService.resolveReadableSharePage (the share-access boundary)', () => { + it('resolves { share, page } for a readable, in-share, live, unrestricted page', async () => { + const page = { id: PAGE, deletedAt: null, content: { type: 'doc' } }; + const { service, pageRepo, pagePermissionRepo } = buildService({ page }); + + const out = await service.resolveReadableSharePage(SHARE, PAGE, WS); + + expect(out).not.toBeNull(); + expect(out!.share.id).toBe(SHARE); + expect(out!.page).toBe(page); + // The restricted-ancestor gate ran on the resolved page id. + expect(pagePermissionRepo.hasRestrictedAncestor).toHaveBeenCalledWith(PAGE); + // Content is fetched (callers sanitize it); creator off by default. + expect(pageRepo.findById).toHaveBeenCalledWith(PAGE, { + includeContent: true, + includeCreator: false, + }); + }); + + it('null when the page is not reachable in the share graph (getShareForPage => undefined)', async () => { + const { service, pageRepo } = buildService({ resolvedShare: undefined }); + expect(await service.resolveReadableSharePage(SHARE, PAGE, WS)).toBeNull(); + // Short-circuits before fetching the page. + expect(pageRepo.findById).not.toHaveBeenCalled(); + }); + + it('null on a cross-share id swap: page resolves to a DIFFERENT share than requested', async () => { + const { service, pageRepo } = buildService({ + resolvedShare: { id: 'OTHER-SHARE', pageId: PAGE, spaceId: 'space-1' }, + }); + expect(await service.resolveReadableSharePage(SHARE, PAGE, WS)).toBeNull(); + expect(pageRepo.findById).not.toHaveBeenCalled(); + }); + + it('null for a soft-deleted page (deletedAt set), without consulting the restricted gate', async () => { + const { service, pagePermissionRepo } = buildService({ + page: { id: PAGE, deletedAt: new Date(), content: {} }, + }); + expect(await service.resolveReadableSharePage(SHARE, PAGE, WS)).toBeNull(); + expect(pagePermissionRepo.hasRestrictedAncestor).not.toHaveBeenCalled(); + }); + + it('null when the page row is missing (findById => null)', async () => { + const { service } = buildService({ page: null }); + expect(await service.resolveReadableSharePage(SHARE, PAGE, WS)).toBeNull(); + }); + + it('null for a restricted descendant (hidden from the public view)', async () => { + const { service } = buildService({ + page: { id: PAGE, deletedAt: null, content: {} }, + restricted: true, + }); + expect(await service.resolveReadableSharePage(SHARE, PAGE, WS)).toBeNull(); + }); + + it('skips the share-id match when shareId is null (getSharedPage path: share resolved FROM the page)', async () => { + const { service } = buildService({ + // The page resolves to whatever share owns it; there is no independent + // requested shareId to cross-check. + resolvedShare: { id: 'ANY-SHARE', pageId: PAGE, spaceId: 'space-1' }, + page: { id: PAGE, deletedAt: null, content: {} }, + }); + const out = await service.resolveReadableSharePage(null, PAGE, WS); + expect(out).not.toBeNull(); + expect(out!.share.id).toBe('ANY-SHARE'); + }); + + it('passes includeCreator through to the page fetch when requested', async () => { + const { service, pageRepo } = buildService(); + await service.resolveReadableSharePage(SHARE, PAGE, WS, { + includeCreator: true, + }); + expect(pageRepo.findById).toHaveBeenCalledWith(PAGE, { + includeContent: true, + includeCreator: true, + }); + }); +}); diff --git a/apps/server/src/core/share/share.service.ts b/apps/server/src/core/share/share.service.ts index 846cc96a..9de364ef 100644 --- a/apps/server/src/core/share/share.service.ts +++ b/apps/server/src/core/share/share.service.ts @@ -128,28 +128,82 @@ export class ShareService { } } - async getSharedPage(dto: ShareInfoDto, workspaceId: string) { - const share = await this.getShareForPage(dto.pageId, workspaceId); + /** + * THE share access boundary in ONE place. + * + * Answers exactly: "does this (shareId, pageId) pair resolve to a usable, + * non-restricted, live page WITHIN this share?" Returns the resolved + * `{ share, page }` on success, or `null` on ANY failure (share not found / + * wrong workspace / out-of-tree page / share-id mismatch / missing / + * soft-deleted / restricted ancestor). + * + * This is the single canonical sequence that every public-share read path + * must funnel through, so no path can skip a check (most importantly the + * restricted-ancestor gate, which `getShareForPage` does NOT perform on its + * own). The checks run in this fixed order: + * 1. getShareForPage(pageId, workspaceId) — page reachable in this ws? + * 2. share.id === shareId — and it is THIS share? + * (pass `null`/`undefined` shareId to skip the match when the caller has + * no independent requested shareId — getSharedPage resolves the share + * FROM the page, so there is nothing to cross-check.) + * 3. pageRepo.findById(pageId, ...) — page row (+ content/creator) + * 4. !page.deletedAt — live (defense in depth: + * getShareForPage already excludes deleted anchors) + * 5. !hasRestrictedAncestor(page.id) — not a restricted descendant + * + * `isSharingAllowed` is intentionally NOT part of this boundary: it is an + * orthogonal workspace/space toggle that each call-site layers separately + * (share.controller after getSharedPage; the assistant funnel as its own + * gate). Folding it in here would silently change those call-sites' grading. + */ + async resolveReadableSharePage( + shareId: string | null | undefined, + pageId: string, + workspaceId: string, + opts?: { includeCreator?: boolean }, + ): Promise<{ + share: NonNullable>>; + page: Page; + } | null> { + const share = await this.getShareForPage(pageId, workspaceId); + if (!share) return null; - if (!share) { - throw new NotFoundException('Shared page not found'); - } + // Only ever an equality check against the server-resolved share id; an + // attacker-supplied shareId can never widen access. Skipped when the caller + // passes no shareId (it resolved the share from the page itself). + if (shareId != null && share.id !== shareId) return null; - const page = await this.pageRepo.findById(dto.pageId, { + const page = await this.pageRepo.findById(pageId, { includeContent: true, - includeCreator: true, + includeCreator: opts?.includeCreator ?? false, }); + if (!page || page.deletedAt) return null; - if (!page || page.deletedAt) { + // Restricted descendants are hidden from the public view even inside an + // includeSubPages share; getShareForPage does NOT exclude them. + if (await this.pagePermissionRepo.hasRestrictedAncestor(page.id)) { + return null; + } + + return { share, page }; + } + + async getSharedPage(dto: ShareInfoDto, workspaceId: string) { + // Resolve via the single canonical boundary. There is no independent + // requested shareId here (the share is resolved FROM the page), so no + // share-id match is performed. + const resolved = await this.resolveReadableSharePage( + null, + dto.pageId, + workspaceId, + { includeCreator: true }, + ); + + if (!resolved) { throw new NotFoundException('Shared page not found'); } - // Block access to restricted pages - const isRestricted = - await this.pagePermissionRepo.hasRestrictedAncestor(page.id); - if (isRestricted) { - throw new NotFoundException('Shared page not found'); - } + const { share, page } = resolved; page.content = await this.updatePublicAttachments(page);