From 932a4080f713f452cc196d8caa8c33f580e21100 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 13:16:32 +0300 Subject: [PATCH] fix(public-share): block restricted descendants in the anonymous assistant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Release-cycle red-team found getShareForPage joins only the shares table, so it does not exclude restricted descendants. The public share VIEW (getSharedPage) compensates with hasRestrictedAncestor, but the assistant's getSharePage tool and the controller funnel did not — so an anonymous caller could read a restricted descendant's content (tool) or surface its title into the system prompt (funnel) within an includeSubPages share. - getSharePage: after the share-membership check and before returning content, reject with the generic 'not part of this published share' message when hasRestrictedAncestor(page.id) is true (page.id is the resolved UUID, so slugId inputs work). Inject PagePermissionRepo. - funnel: resolve the OPENED page to its UUID and treat a restricted opened page as not-in-share (same uniform 404, fail closed if unresolvable) so its title never reaches buildShareSystemPrompt. search/list already exclude restricted subtrees (getPageAndDescendantsExcludingRestricted), so these were the only two bypasses. Generic messages keep restricted indistinguishable from not-in-share. Co-Authored-By: Claude Opus 4.8 --- .../ai-chat/public-share-chat.controller.ts | 37 +++++- .../core/ai-chat/public-share-chat.spec.ts | 106 ++++++++++++++++++ .../tools/public-share-chat-tools.service.ts | 22 +++- 3 files changed, 157 insertions(+), 8 deletions(-) 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 d142215c..6ed6691b 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,6 +19,8 @@ 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 { @@ -50,6 +52,8 @@ export class PublicShareChatController { constructor( private readonly shareService: ShareService, + private readonly pagePermissionRepo: PagePermissionRepo, + private readonly pageRepo: PageRepo, private readonly aiSettings: AiSettingsService, private readonly publicShareChat: PublicShareChatService, ) {} @@ -78,18 +82,26 @@ export class PublicShareChatController { ); // 2. Share usable? Resolved via the page's share membership, since the page - // resolution (getShareForPage) ALSO yields the share + workspace + - // restricted checks. We still need basic input to attempt it. + // resolution (getShareForPage) ALSO yields the share + workspace. We + // still need basic input to attempt it. // 3. Page in share? The same getShareForPage lookup confirms the opened page - // actually resolves to THIS share tree (shareUsable + pageInShare are set - // together below; the funnel grades them as distinct ordered steps). + // resolves to THIS share tree, PLUS an explicit restricted-ancestor gate + // (getShareForPage itself does NOT exclude restricted descendants) so a + // restricted page hidden from the public view is graded not-in-share. + // (shareUsable + pageInShare are set together below; the funnel grades + // them as distinct ordered steps.) let share: Awaited> | undefined; let shareUsable = false; let pageInShare = false; if (assistantEnabled && shareId && pageId) { // getShareForPage walks up the tree to the nearest ancestor share, // enforces share.workspaceId === workspaceId and includeSubPages, and - // returns undefined when the page is not publicly reachable. + // returns undefined when the page is not publicly reachable. NOTE: it + // joins only the `shares` table — it does NOT exclude restricted + // descendants — so a restricted page inside an includeSubPages share + // still resolves here. We add an explicit restricted-ancestor gate below + // (same as the public view) so the opened page's title never leaks into + // the system prompt for a page the public view 404s. share = await this.shareService.getShareForPage(pageId, workspace.id); if (share && share.id === shareId) { // Confirm sharing is still allowed for the share's space (and not @@ -99,7 +111,20 @@ export class PublicShareChatController { share.spaceId, ); shareUsable = sharingAllowed; - pageInShare = sharingAllowed; + // A restricted descendant is hidden from the public share view; treat + // the opened page as not-in-share so the funnel returns the SAME 404 it + // returns for an out-of-tree page (uniform, no existence leak). + // 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, mirroring how getSharedPage resolves the page + // before its restricted check). + const openedPageRow = await this.pageRepo.findById(pageId); + const restricted = openedPageRow + ? await this.pagePermissionRepo.hasRestrictedAncestor( + openedPageRow.id, + ) + : true; // unresolvable opened page => fail closed (treat as not-in-share) + pageInShare = sharingAllowed && !restricted; } } 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 44cf9d17..c6699591 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 @@ -67,6 +67,65 @@ describe('evaluateShareAssistantFunnel ordering', () => { }); }); +describe('controller funnel: restricted opened page is graded not-in-share', () => { + /** + * Mirrors the controller's pageInShare decision for the opened page: + * pageInShare = sharingAllowed && !hasRestrictedAncestor(resolvedPageId) + * A restricted descendant inside an includeSubPages share resolves via + * getShareForPage but must be graded not-in-share so the funnel returns the + * SAME 404 it returns for an out-of-tree page (uniform, no existence leak). + */ + function decidePageInShare( + sharingAllowed: boolean, + restricted: boolean, + ): boolean { + return sharingAllowed && !restricted; + } + + it('a restricted descendant funnels to the SAME 404 as an out-of-tree page', () => { + // Out-of-tree page: getShareForPage returns a different/no share => the + // controller never sets pageInShare (stays false). + const outOfTree = evaluateShareAssistantFunnel({ + assistantEnabled: true, + shareUsable: true, + pageInShare: false, + providerConfigured: true, + }); + + // Restricted descendant: share resolves, sharing allowed, but the explicit + // restricted-ancestor gate flips pageInShare to false. + const restrictedPageInShare = decidePageInShare(true, /* restricted */ true); + const restricted = evaluateShareAssistantFunnel({ + assistantEnabled: true, + shareUsable: true, + pageInShare: restrictedPageInShare, + providerConfigured: true, + }); + + expect(restrictedPageInShare).toBe(false); + // Same outcome, same reason, same status: indistinguishable. + expect(restricted).toEqual(outOfTree); + expect(restricted).toEqual({ + ok: false, + status: 404, + reason: 'page-not-in-share', + }); + }); + + it('an unrestricted page inside the share is allowed through the funnel', () => { + const pageInShare = decidePageInShare(true, /* restricted */ false); + expect(pageInShare).toBe(true); + expect( + evaluateShareAssistantFunnel({ + assistantEnabled: true, + shareUsable: true, + pageInShare, + providerConfigured: true, + }), + ).toEqual({ ok: true }); + }); +}); + describe('buildShareSystemPrompt locking', () => { it('always includes the immutable read-only / share-scope safety rules', () => { const prompt = buildShareSystemPrompt({ share: null, openedPage: null }); @@ -173,10 +232,12 @@ describe('PublicShareChatToolsService share scoping', () => { 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, ); const tools = svc.forShare('THIS-SHARE', 'ws-1'); @@ -190,6 +251,50 @@ describe('PublicShareChatToolsService share scoping', () => { // It must NOT have fetched/returned any content for an out-of-share page. expect(pageRepo.findById).not.toHaveBeenCalled(); 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 () => { + const shareService = { + // The restricted page DOES resolve to this share (includeSubPages tree)... + getShareForPage: jest.fn().mockResolvedValue({ id: 'THIS-SHARE' }), + 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, + ); + + const tools = svc.forShare('THIS-SHARE', 'ws-1'); + const getSharePage = tools.getSharePage as { + execute: (args: { pageId: string }) => Promise; + }; + + 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. + expect(shareService.updatePublicAttachments).not.toHaveBeenCalled(); }); it('searchSharePages forwards the share scope (shareId, no spaceId/userId) to the FTS branch', async () => { @@ -202,6 +307,7 @@ describe('PublicShareChatToolsService share scoping', () => { {} as never, searchService as never, {} as never, + {} as never, ); const tools = svc.forShare('THIS-SHARE', 'ws-1'); const searchSharePages = tools.searchSharePages as { 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 3d2284d8..b15a6f69 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 @@ -4,6 +4,7 @@ import { z } from 'zod'; import { ShareService } from '../../share/share.service'; import { SearchService } from '../../search/search.service'; import { PageRepo } from '@docmost/db/repos/page/page.repo'; +import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'; import { jsonToMarkdown } from '../../../collaboration/collaboration.util'; /** @@ -22,7 +23,9 @@ import { jsonToMarkdown } from '../../../collaboration/collaboration.util'; * (`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 before returning any content. + * resolves to THIS share AND (because getShareForPage does NOT itself + * exclude restricted descendants) that the page has no restricted ancestor, + * before returning any content. */ @Injectable() export class PublicShareChatToolsService { @@ -32,6 +35,7 @@ export class PublicShareChatToolsService { private readonly shareService: ShareService, private readonly searchService: SearchService, private readonly pageRepo: PageRepo, + private readonly pagePermissionRepo: PagePermissionRepo, ) {} /** @@ -96,7 +100,10 @@ export class PublicShareChatToolsService { throw new Error('A pageId is required.'); } // Confirm the page resolves to THIS share (recursive CTE up the tree, - // honouring includeSubPages + restricted exclusion + workspace check). + // 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( @@ -114,6 +121,17 @@ export class PublicShareChatToolsService { 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)) { + throw new Error('That page is not part of this published share.'); + } + // Reuse the public share-content sanitizer: strips comment marks and // tokenizes attachments for public delivery, exactly as the public // shared-page view does.