diff --git a/apps/client/src/features/page/components/breadcrumbs/breadcrumb.tsx b/apps/client/src/features/page/components/breadcrumbs/breadcrumb.tsx index 03ce127d..c2eeba16 100644 --- a/apps/client/src/features/page/components/breadcrumbs/breadcrumb.tsx +++ b/apps/client/src/features/page/components/breadcrumbs/breadcrumb.tsx @@ -1,7 +1,7 @@ import { useAtomValue } from "jotai"; import { treeDataAtom } from "@/features/page/tree/atoms/tree-data-atom.ts"; import React, { useCallback, useEffect, useState } from "react"; -import { findBreadcrumbPath } from "@/features/page/tree/utils"; +import { resolveBreadcrumbNodes } from "./breadcrumb.utils"; import { Button, Anchor, @@ -15,6 +15,7 @@ import { IconCornerDownRightDouble, IconDots } from "@tabler/icons-react"; import { Link, useParams } from "react-router-dom"; import classes from "./breadcrumb.module.css"; import { SpaceTreeNode } from "@/features/page/tree/types.ts"; +import { IPage } from "@/features/page/types/page.types.ts"; import { buildPageUrl } from "@/features/page/page.utils.ts"; import { usePageQuery, @@ -50,32 +51,16 @@ export default function Breadcrumb() { useEffect(() => { if (!currentPage) return; - // Prefer the sidebar tree once it actually contains this page's ancestor - // chain — it stays live with renames/moves happening in the sidebar. - if (treeData?.length > 0) { - const breadcrumb = findBreadcrumbPath(treeData, currentPage.id); - if (breadcrumb) { - setBreadcrumbNodes(breadcrumb); - return; - } - } - - // Otherwise fall back to the page's own ancestor data so the breadcrumb - // resolves immediately instead of staying blank. - if (ancestors?.length) { - setBreadcrumbNodes( - (ancestors as any[]).map((node) => ({ - id: node.id, - slugId: node.slugId, - name: node.title, - icon: node.icon, - position: node.position, - spaceId: node.spaceId, - parentPageId: node.parentPageId, - hasChildren: node.hasChildren ?? false, - children: [], - })) as SpaceTreeNode[], - ); + // Selection/mapping lives in a pure, unit-tested helper (#218). Only update + // when it resolves nodes so a transient miss keeps the prior breadcrumb + // rather than blanking it. + const nodes = resolveBreadcrumbNodes( + treeData, + ancestors as IPage[] | undefined, + currentPage.id, + ); + if (nodes) { + setBreadcrumbNodes(nodes); } }, [currentPage?.id, treeData, ancestors]); diff --git a/apps/client/src/features/page/components/breadcrumbs/breadcrumb.utils.test.ts b/apps/client/src/features/page/components/breadcrumbs/breadcrumb.utils.test.ts new file mode 100644 index 00000000..a8dd9a2c --- /dev/null +++ b/apps/client/src/features/page/components/breadcrumbs/breadcrumb.utils.test.ts @@ -0,0 +1,81 @@ +import { describe, it, expect } from "vitest"; +import { resolveBreadcrumbNodes } from "./breadcrumb.utils"; +import { SpaceTreeNode } from "@/features/page/tree/types.ts"; +import { IPage } from "@/features/page/types/page.types.ts"; + +// Pure selection/mapping behind the breadcrumb (#218): tree-hit prefers the live +// sidebar tree, tree-miss maps the page's own ancestors, and "no data" returns +// null so the component keeps its prior state. + +function treeNode(id: string, over?: Partial): SpaceTreeNode { + return { + id, + slugId: `slug-${id}`, + name: `node-${id}`, + icon: null, + position: "a", + hasChildren: false, + spaceId: "space-1", + parentPageId: null, + children: [], + ...over, + } as SpaceTreeNode; +} + +function ancestorPage(id: string, over?: Partial): IPage { + return { + id, + slugId: `slug-${id}`, + title: `title-${id}`, + icon: "📄", + position: "m", + spaceId: "space-1", + parentPageId: null, + hasChildren: true, + ...over, + } as IPage; +} + +describe("resolveBreadcrumbNodes", () => { + it("tree-hit: returns the path found in the live sidebar tree", () => { + const child = treeNode("child"); + const root = treeNode("root", { hasChildren: true, children: [child] }); + // findBreadcrumbPath walks the tree; the chain ends at the target page. + const result = resolveBreadcrumbNodes([root], [ancestorPage("child")], "child"); + + expect(result).not.toBeNull(); + expect(result!.map((n) => n.id)).toEqual(["root", "child"]); + // Came from the tree, NOT the ancestor mapping (icon stays the tree's null). + expect(result![result!.length - 1].icon).toBeNull(); + }); + + it("tree-miss: maps the page's own ancestors (title->name, hasChildren default)", () => { + // Tree has no node for the target page -> findBreadcrumbPath misses. + const unrelated = treeNode("unrelated"); + const ancestors = [ + ancestorPage("a", { hasChildren: true }), + ancestorPage("b", { hasChildren: undefined as any }), + ]; + + const result = resolveBreadcrumbNodes([unrelated], ancestors, "missing-page"); + + expect(result).not.toBeNull(); + expect(result!.map((n) => n.id)).toEqual(["a", "b"]); + // Non-trivial field transform: title -> name. + expect(result![0].name).toBe("title-a"); + // hasChildren defaults to false when the ancestor row omits it. + expect(result![1].hasChildren).toBe(false); + expect(result![0].hasChildren).toBe(true); + }); + + it("falls back to ancestors when the tree is empty", () => { + const result = resolveBreadcrumbNodes([], [ancestorPage("a")], "a"); + expect(result!.map((n) => n.id)).toEqual(["a"]); + }); + + it("returns null when there is no tree hit and no ancestor data", () => { + expect(resolveBreadcrumbNodes([], [], "x")).toBeNull(); + expect(resolveBreadcrumbNodes(undefined, undefined, "x")).toBeNull(); + expect(resolveBreadcrumbNodes(null, null, "x")).toBeNull(); + }); +}); diff --git a/apps/client/src/features/page/components/breadcrumbs/breadcrumb.utils.ts b/apps/client/src/features/page/components/breadcrumbs/breadcrumb.utils.ts new file mode 100644 index 00000000..0190cb37 --- /dev/null +++ b/apps/client/src/features/page/components/breadcrumbs/breadcrumb.utils.ts @@ -0,0 +1,34 @@ +import { IPage } from "@/features/page/types/page.types.ts"; +import { SpaceTreeNode } from "@/features/page/tree/types.ts"; +import { findBreadcrumbPath, pageToTreeNode } from "@/features/page/tree/utils"; + +/** + * Pure selection/mapping for the breadcrumb nodes (#218). Three branches: + * 1. tree-hit — the lazily-built sidebar tree already contains this page's + * ancestor chain, so prefer it (stays live with sidebar renames/moves). + * 2. tree-miss — fall back to the page's own ancestor data so a deep page + * resolves immediately instead of rendering a blank breadcrumb for seconds + * while the tree backfills. Mapped through the canonical `pageToTreeNode` + * (title -> name, hasChildren defaulted to false). + * 3. neither — no data yet, return null so the caller keeps its prior state. + */ +export function resolveBreadcrumbNodes( + treeData: SpaceTreeNode[] | null | undefined, + ancestors: IPage[] | null | undefined, + pageId: string, +): SpaceTreeNode[] | null { + if (treeData && treeData.length > 0) { + const breadcrumb = findBreadcrumbPath(treeData, pageId); + if (breadcrumb) { + return breadcrumb; + } + } + + if (ancestors && ancestors.length > 0) { + return ancestors.map((page) => + pageToTreeNode(page, { hasChildren: page.hasChildren ?? false }), + ); + } + + return null; +} diff --git a/apps/client/src/features/share/components/share-modal.test.tsx b/apps/client/src/features/share/components/share-modal.test.tsx new file mode 100644 index 00000000..c3d96afd --- /dev/null +++ b/apps/client/src/features/share/components/share-modal.test.tsx @@ -0,0 +1,74 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, fireEvent, waitFor } from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; +import { MemoryRouter } from "react-router-dom"; + +// matchMedia / storage are stubbed globally in vitest.setup.ts. + +// Enabling a public share must NOT silently expose the whole sub-tree (#216): +// the create call defaults includeSubPages to false. This was a one-literal, +// security-relevant default with no test — lock it. + +const createMutateAsync = vi.fn(async () => ({})); +const deleteMutateAsync = vi.fn(async () => ({})); + +// No existing share for this page (toggle starts OFF). +let shareData: any = undefined; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock("@/features/share/queries/share-query.ts", () => ({ + useCreateShareMutation: () => ({ mutateAsync: createMutateAsync }), + useDeleteShareMutation: () => ({ mutateAsync: deleteMutateAsync }), + useUpdateShareMutation: () => ({ mutateAsync: vi.fn() }), + useShareForPageQuery: () => ({ data: shareData }), +})); + +vi.mock("@/features/page/queries/page-query.ts", () => ({ + usePageQuery: () => ({ data: { id: "page-1", title: "Doc" } }), +})); + +vi.mock("@/features/space/queries/space-query.ts", () => ({ + useSpaceQuery: () => ({ data: { settings: {} } }), +})); + +import ShareModal from "./share-modal"; + +function renderModal() { + return render( + + + + + , + ); +} + +describe("ShareModal — enabling a share defaults includeSubPages to false (#216)", () => { + beforeEach(() => { + createMutateAsync.mockClear(); + deleteMutateAsync.mockClear(); + shareData = undefined; + }); + + it("creates the share with includeSubPages: false when the user turns it on", async () => { + renderModal(); + + // Open the share popover. + fireEvent.click(screen.getByRole("button", { name: "Share" })); + + // The "Share to web" toggle is the only switch in the not-yet-shared state. + const toggle = await screen.findByRole("switch"); + fireEvent.click(toggle); + + await waitFor(() => expect(createMutateAsync).toHaveBeenCalledTimes(1)); + expect(createMutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ + pageId: "page-1", + includeSubPages: false, + }), + ); + }); +}); diff --git a/apps/server/src/core/share/share-public-payload.ts b/apps/server/src/core/share/share-public-payload.ts new file mode 100644 index 00000000..e26749bf --- /dev/null +++ b/apps/server/src/core/share/share-public-payload.ts @@ -0,0 +1,69 @@ +import { Page } from '@docmost/db/types/entity.types'; + +/** + * The EXACT shape returned to anonymous public-share viewers by the + * `/shares/page-info` route — the only unauthenticated path that serializes the + * full {page, share} records. This is a security boundary (#218): the raw rows + * carry internal metadata — creatorId/lastUpdatedById/contributorIds, + * spaceId/workspaceId, AI/source bookkeeping, lock/template flags, + * parent/position and raw timestamps — none of which may leak to an + * unauthenticated viewer. Keeping the allowlist as an explicit TYPE plus a + * single mapper means a new leaking field cannot be returned without also + * widening this contract (and tripping its key-test in share.controller.spec.ts). + */ +export interface PublicSharePayload { + page: { + id: string; + slugId: string; + title: string | null; + icon: string | null; + content: unknown; + }; + share: { + id: string; + key: string; + includeSubPages: boolean | null; + searchIndexing: boolean | null; + level: number; + sharedPage: unknown; + }; +} + +/** + * The subset of the resolved share read by the public payload. Declared + * structurally so the richer getShareForPage result (which adds `level` and + * `sharedPage` on top of the base Shares row) passes without a cast. + */ +interface PublicShareSource { + id: string; + key: string; + includeSubPages: boolean | null; + searchIndexing: boolean | null; + // `level` is derived via a SQL literal in getShareForPage, so it surfaces as + // `unknown` in the resolved share; it is a number at runtime. + level: unknown; + sharedPage: unknown; +} + +export function toPublicSharePayload( + page: Page, + share: PublicShareSource, +): PublicSharePayload { + return { + page: { + id: page.id, + slugId: page.slugId, + title: page.title, + icon: page.icon, + content: page.content, + }, + share: { + id: share.id, + key: share.key, + includeSubPages: share.includeSubPages, + searchIndexing: share.searchIndexing, + level: share.level as number, + sharedPage: share.sharedPage, + }, + }; +} diff --git a/apps/server/src/core/share/share.controller.spec.ts b/apps/server/src/core/share/share.controller.spec.ts new file mode 100644 index 00000000..afb0ca37 --- /dev/null +++ b/apps/server/src/core/share/share.controller.spec.ts @@ -0,0 +1,190 @@ +import { ShareController } from './share.controller'; +import { + PublicSharePayload, + toPublicSharePayload, +} from './share-public-payload'; + +// The `/shares/page-info` route is the ONLY anonymous path that serializes the +// full {page, share} records. Trimming the response to an explicit allowlist is +// a security control (#218): a regression that returns `...shareData` (or adds a +// new field to the allowlist) must fail loudly. These tests lock the exact key +// set returned to anonymous viewers so internal metadata can never silently leak. + +const PAGE_KEYS = ['id', 'slugId', 'title', 'icon', 'content'].sort(); +const SHARE_KEYS = [ + 'id', + 'key', + 'includeSubPages', + 'searchIndexing', + 'level', + 'sharedPage', +].sort(); + +// A page row carrying internal metadata that MUST NOT reach anonymous viewers. +function internalPage() { + return { + id: 'page-1', + slugId: 'slug-1', + title: 'Public Title', + icon: '📄', + content: { type: 'doc', content: [] }, + // --- leaky internals --- + creatorId: 'user-1', + lastUpdatedById: 'user-2', + contributorIds: ['user-1', 'user-2'], + spaceId: 'space-1', + workspaceId: 'ws-1', + parentPageId: 'parent-1', + position: 'aa', + isLocked: true, + isTemplate: false, + textContent: 'secret text content', + ydoc: Buffer.from('binary'), + createdAt: new Date('2020-01-01'), + updatedAt: new Date('2020-01-02'), + deletedAt: null, + } as any; +} + +// A resolved share carrying internal metadata. +function internalShare() { + return { + id: 'share-1', + key: 'share-key', + includeSubPages: false, + searchIndexing: true, + level: 0, + sharedPage: { id: 'page-1', slugId: 'slug-1', title: 'Public Title' }, + // --- leaky internals --- + creatorId: 'user-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + pageId: 'page-1', + createdAt: new Date('2020-01-01'), + updatedAt: new Date('2020-01-02'), + deletedAt: null, + } as any; +} + +function buildController(over?: { aiAssistant?: boolean }) { + const shareService = { + // Deliberately returns the FULL internal records (as the real service does). + getSharedPage: jest.fn(async () => ({ + page: internalPage(), + share: internalShare(), + })), + isSharingAllowed: jest.fn(async () => true), + }; + const aiSettings = { + isPublicShareAssistantEnabled: jest.fn( + async () => over?.aiAssistant ?? false, + ), + resolvePublicShareAssistantName: jest.fn(async () => 'Assistant'), + }; + const licenseCheckService = { + resolveFeatures: jest.fn(() => ({ tier: 'free' })), + }; + + const controller = new ShareController( + shareService as any, + {} as any, // shareRepo + {} as any, // pageRepo + {} as any, // pagePermissionRepo + {} as any, // pageAccessService + licenseCheckService as any, + aiSettings as any, + {} as any, // auditService + ); + + return { controller, shareService, aiSettings, licenseCheckService }; +} + +const workspace = { + id: 'ws-1', + licenseKey: null, + plan: 'free', +} as any; + +describe('ShareController.getSharedPageInfo — public payload whitelist (#218)', () => { + it('returns EXACTLY the page allowlist keys (no leaked internals)', async () => { + const { controller } = buildController(); + + const res = await controller.getSharedPageInfo( + { pageId: 'page-1' } as any, + workspace, + ); + + expect(Object.keys(res.page).sort()).toEqual(PAGE_KEYS); + for (const leaked of [ + 'creatorId', + 'lastUpdatedById', + 'contributorIds', + 'spaceId', + 'workspaceId', + 'parentPageId', + 'position', + 'textContent', + 'ydoc', + 'createdAt', + 'updatedAt', + 'deletedAt', + ]) { + expect((res.page as any)[leaked]).toBeUndefined(); + } + // The serialized payload must not carry the secret text content either. + expect(JSON.stringify(res.page)).not.toContain('secret text content'); + }); + + it('returns EXACTLY the share allowlist keys (no leaked internals)', async () => { + const { controller } = buildController(); + + const res = await controller.getSharedPageInfo( + { pageId: 'page-1' } as any, + workspace, + ); + + expect(Object.keys(res.share).sort()).toEqual(SHARE_KEYS); + for (const leaked of [ + 'creatorId', + 'spaceId', + 'workspaceId', + 'pageId', + 'createdAt', + 'updatedAt', + 'deletedAt', + ]) { + expect((res.share as any)[leaked]).toBeUndefined(); + } + }); + + it('surfaces the public AI-assistant flags and license features alongside the trimmed payload', async () => { + const { controller } = buildController({ aiAssistant: true }); + + const res = await controller.getSharedPageInfo( + { pageId: 'page-1' } as any, + workspace, + ); + + expect(res.aiAssistant).toBe(true); + expect(res.aiAssistantName).toBe('Assistant'); + expect(res.features).toEqual({ tier: 'free' }); + // Top-level keys are limited to the trimmed payload + the public extras. + expect(Object.keys(res).sort()).toEqual( + ['page', 'share', 'aiAssistant', 'aiAssistantName', 'features'].sort(), + ); + }); +}); + +describe('toPublicSharePayload — key set is the contract', () => { + it('copies only the allowlisted page/share keys', () => { + const payload: PublicSharePayload = toPublicSharePayload( + internalPage(), + internalShare(), + ); + + expect(Object.keys(payload.page).sort()).toEqual(PAGE_KEYS); + expect(Object.keys(payload.share).sort()).toEqual(SHARE_KEYS); + expect(payload.page.id).toBe('page-1'); + expect(payload.share.key).toBe('share-key'); + }); +}); diff --git a/apps/server/src/core/share/share.controller.ts b/apps/server/src/core/share/share.controller.ts index cbf6d256..34fc800c 100644 --- a/apps/server/src/core/share/share.controller.ts +++ b/apps/server/src/core/share/share.controller.ts @@ -36,6 +36,7 @@ import { IAuditService, } from '../../integrations/audit/audit.service'; import { AiSettingsService } from '../../integrations/ai/ai-settings.service'; +import { toPublicSharePayload } from './share-public-payload'; @UseGuards(JwtAuthGuard) @Controller('shares') @@ -93,30 +94,13 @@ export class ShareController { ? await this.aiSettings.resolvePublicShareAssistantName(workspace.id) : null; - // Trim the public payload to what the anonymous renderer actually needs - // (#218). Internal metadata — creatorId/lastUpdatedById/contributorIds, - // spaceId/workspaceId, AI/source bookkeeping, lock/template flags, - // parent/position, raw timestamps — must not leak to anonymous viewers. + // Trim the public payload to the explicit allowlist the anonymous renderer + // needs (#218); the PublicSharePayload type + mapper guarantee internal + // metadata can never leak to anonymous viewers (see share-public-payload.ts). const { page, share } = shareData; - const publicPage = { - id: page.id, - slugId: page.slugId, - title: page.title, - icon: page.icon, - content: page.content, - }; - const publicShare = { - id: share.id, - key: share.key, - includeSubPages: share.includeSubPages, - searchIndexing: share.searchIndexing, - level: share.level, - sharedPage: share.sharedPage, - }; return { - page: publicPage, - share: publicShare, + ...toPublicSharePayload(page, share), aiAssistant, aiAssistantName, features: this.licenseCheckService.resolveFeatures( diff --git a/apps/server/src/core/share/share.service.ts b/apps/server/src/core/share/share.service.ts index a2d8d2ac..e5452820 100644 --- a/apps/server/src/core/share/share.service.ts +++ b/apps/server/src/core/share/share.service.ts @@ -213,6 +213,11 @@ export class ShareService { // request with no shareId keeps the legacy slug-capability behavior (the // `/share/p/:slug` route + internal title look-ups); the slug nanoid stays // the access secret there — an inherited Docmost design we don't widen. + // FUTURE: this ancestor-aware match could fold INTO resolveReadableSharePage + // (so the boundary's narrow `share.id === shareId` gate isn't effectively + // dead). Deferred — it widens the contract for the 4 other callers that pass + // no shareId, so kept here as a local post-check until that's worth the blast + // radius. if (dto.shareId) { const reachable = await this.isPageReachableThroughShare( dto.shareId, diff --git a/apps/server/src/integrations/export/utils.ts b/apps/server/src/integrations/export/utils.ts index 05ae9af4..6fe370a0 100644 --- a/apps/server/src/integrations/export/utils.ts +++ b/apps/server/src/integrations/export/utils.ts @@ -109,8 +109,10 @@ export function getInternalLinkPageName(path: string, currentFilePath?: string): // Strip a trailing file extension from the basename, but only when there IS // one: an extensionless link target (e.g. "My Page") has no extension to drop, // so `split('.').slice(0,-1)` would otherwise collapse it to an empty string, - // producing an internal link with no visible text (#204 export bug). Dotted - // page names without an extension (e.g. "v1.2") keep their dots. + // producing an internal link with no visible text (#204 export bug). The last + // dot-segment is always treated as an extension and dropped whenever there is + // more than one segment, so dots are preserved only in multi-segment names + // like `v1.2.md` -> `v1.2`; a bare `v1.2` becomes `v1`. const base = path?.split('/').pop(); const parts = base?.split('.'); const name = parts && parts.length > 1 ? parts.slice(0, -1).join('.') : base;