vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

refactor(share): single resolveReadableSharePage for the share access boundary (#92)

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-21 04:04:09 +03:00
parent 3953ecdb17
commit 3f46496192
7 changed files with 347 additions and 178 deletions
Download Patch File Download Diff File Expand all files Collapse all files

74
apps/server/src/core/ai-chat/public-share-chat.controller.spec.ts
View File

@@ -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 () => {

56
apps/server/src/core/ai-chat/public-share-chat.controller.ts
View File

@@ -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<AiSettingsService, 'isPublicShareAssistantEnabled'>;
// 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<PageRepo, 'findById'>;
pagePermissionRepo: Pick<PagePermissionRepo, 'hasRestrictedAncestor'>;
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<Awaited<ReturnType<ShareService['getShareForPage']>>>;
share: NonNullable<
Awaited<ReturnType<ShareService['resolveReadableSharePage']>>
>['share'];
model: Awaited<ReturnType<PublicShareChatService['getShareChatModel']>>;
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<ReturnType<ShareService['getShareForPage']>> | 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<ReturnType<ShareService['resolveReadableSharePage']>>
>['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,
}));
}
}

67
apps/server/src/core/ai-chat/public-share-chat.spec.ts
View File

@@ -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<unknown>;
};
// ...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', () => {

63
apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts
View File

@@ -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();
});
});

47
apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts
View File

@@ -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

136
apps/server/src/core/share/share-resolve-readable-page.spec.ts Normal file
View File

@@ -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,
});
});
});

82
apps/server/src/core/share/share.service.ts
View File

@@ -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<Awaited<ReturnType<ShareService['getShareForPage']>>>;
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);