diff --git a/apps/server/src/collaboration/collaboration.handler.ts b/apps/server/src/collaboration/collaboration.handler.ts index debfde60..7513ca35 100644 --- a/apps/server/src/collaboration/collaboration.handler.ts +++ b/apps/server/src/collaboration/collaboration.handler.ts @@ -9,10 +9,8 @@ import { setYjsMark, updateYjsMarkAttribute, YjsSelection } from './yjs.util'; import * as Y from 'yjs'; import { User } from '@docmost/db/types/entity.types'; import { - hasHtmlEmbedNode, - htmlEmbedAllowed, isHtmlEmbedFeatureEnabled, - stripHtmlEmbedNodes, + stripHtmlEmbedIfNotAllowed, } from '../common/helpers/prosemirror/html-embed.util'; import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo'; @@ -106,14 +104,14 @@ export class CollaborationHandler { const htmlEmbedEnabled = isHtmlEmbedFeatureEnabled( (await this.workspaceRepo.findById(user?.workspaceId))?.settings, ); - if (!htmlEmbedAllowed(htmlEmbedEnabled, user?.role)) { - if (hasHtmlEmbedNode(prosemirrorJson)) { + prosemirrorJson = stripHtmlEmbedIfNotAllowed(prosemirrorJson, { + featureEnabled: htmlEmbedEnabled, + role: user?.role, + onStrip: () => this.logger.warn( `Stripping htmlEmbed node(s) from update by user ${user?.id} on ${documentName}`, - ); - prosemirrorJson = stripHtmlEmbedNodes(prosemirrorJson); - } - } + ), + }); await this.withYdocConnection( hocuspocus, diff --git a/apps/server/src/common/helpers/prosemirror/html-embed.spec.ts b/apps/server/src/common/helpers/prosemirror/html-embed.spec.ts index 55cbe982..f54850d3 100644 --- a/apps/server/src/common/helpers/prosemirror/html-embed.spec.ts +++ b/apps/server/src/common/helpers/prosemirror/html-embed.spec.ts @@ -5,6 +5,7 @@ import { htmlEmbedAllowed, isHtmlEmbedFeatureEnabled, stripDisallowedHtmlEmbedNodes, + stripHtmlEmbedIfNotAllowed, stripHtmlEmbedNodes, } from './html-embed.util'; import { htmlToJson, jsonToHtml } from '../../../collaboration/collaboration.util'; @@ -413,6 +414,119 @@ describe('htmlEmbedAllowed (toggle AND admin)', () => { }); }); +// The shared write-path strip ritual extracted from the 5 plain call-sites +// (collab handler, page create/duplicate, import, file-import-task, +// transclusion-unsync). Tested here once instead of being re-verified in each +// call-site's spec. +describe('stripHtmlEmbedIfNotAllowed (shared write-path gate)', () => { + const docWithEmbed = () => ({ + type: 'doc', + content: [ + { type: 'paragraph', content: [{ type: 'text', text: 'keep' }] }, + { type: 'htmlEmbed', attrs: { source: '' } }, + ], + }); + const docWithoutEmbed = () => ({ + type: 'doc', + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'keep' }] }], + }); + + it('keeps the doc unchanged when feature is ON and role is admin (allowed)', () => { + const json = docWithEmbed(); + const onStrip = jest.fn(); + const result = stripHtmlEmbedIfNotAllowed(json, { + featureEnabled: true, + role: 'admin', + onStrip, + }); + // Allowed => same reference returned, embed preserved, no side-effect. + expect(result).toBe(json); + expect(hasHtmlEmbedNode(result)).toBe(true); + expect(onStrip).not.toHaveBeenCalled(); + }); + + it('keeps the doc unchanged for an owner when feature is ON (allowed)', () => { + const json = docWithEmbed(); + const onStrip = jest.fn(); + const result = stripHtmlEmbedIfNotAllowed(json, { + featureEnabled: true, + role: 'owner', + onStrip, + }); + expect(result).toBe(json); + expect(hasHtmlEmbedNode(result)).toBe(true); + expect(onStrip).not.toHaveBeenCalled(); + }); + + it('strips the embed when the feature is OFF (even for an admin)', () => { + const json = docWithEmbed(); + const onStrip = jest.fn(); + const result = stripHtmlEmbedIfNotAllowed(json, { + featureEnabled: false, + role: 'admin', + onStrip, + }); + expect(hasHtmlEmbedNode(result)).toBe(false); + expect(onStrip).toHaveBeenCalledTimes(1); + }); + + it('strips the embed for a non-admin when the feature is ON', () => { + const json = docWithEmbed(); + const onStrip = jest.fn(); + const result = stripHtmlEmbedIfNotAllowed(json, { + featureEnabled: true, + role: 'member', + onStrip, + }); + expect(hasHtmlEmbedNode(result)).toBe(false); + expect(onStrip).toHaveBeenCalledTimes(1); + }); + + it('strips the embed for a null/undefined role when the feature is ON', () => { + for (const role of [null, undefined]) { + const onStrip = jest.fn(); + const result = stripHtmlEmbedIfNotAllowed(docWithEmbed(), { + featureEnabled: true, + role, + onStrip, + }); + expect(hasHtmlEmbedNode(result)).toBe(false); + expect(onStrip).toHaveBeenCalledTimes(1); + } + }); + + it('returns input unchanged and does NOT call onStrip when no embed is present', () => { + const json = docWithoutEmbed(); + const onStrip = jest.fn(); + // Not allowed (feature OFF), but there is nothing to strip. + const result = stripHtmlEmbedIfNotAllowed(json, { + featureEnabled: false, + role: 'member', + onStrip, + }); + expect(result).toBe(json); + expect(onStrip).not.toHaveBeenCalled(); + }); + + it('calls onStrip exactly once per strip', () => { + const onStrip = jest.fn(); + stripHtmlEmbedIfNotAllowed(docWithEmbed(), { + featureEnabled: false, + role: 'member', + onStrip, + }); + expect(onStrip).toHaveBeenCalledTimes(1); + }); + + it('works without an onStrip callback (optional)', () => { + const result = stripHtmlEmbedIfNotAllowed(docWithEmbed(), { + featureEnabled: false, + role: 'member', + }); + expect(hasHtmlEmbedNode(result)).toBe(false); + }); +}); + // NOTE: a previous revision of this file re-implemented the write-path admin // gate as a local `applyAdminGate` stand-in and asserted against THAT. A // deleted/misplaced real guard would have kept those green. The stand-in is diff --git a/apps/server/src/common/helpers/prosemirror/html-embed.util.ts b/apps/server/src/common/helpers/prosemirror/html-embed.util.ts index 146ea9d3..e25d4139 100644 --- a/apps/server/src/common/helpers/prosemirror/html-embed.util.ts +++ b/apps/server/src/common/helpers/prosemirror/html-embed.util.ts @@ -197,6 +197,30 @@ export function htmlEmbedAllowed( return featureEnabled === true && canAuthorHtmlEmbed(role); } +/** + * Strip htmlEmbed nodes unless the (feature-enabled AND role-allowed) gate + * passes. Returns the possibly-stripped doc. The caller resolves featureEnabled + * (from workspace settings) and role (actor) itself — those legitimately differ + * per call-site (e.g. share path uses role=null) — this helper owns only the + * has-check + AND + strip + optional onStrip callback. + * + * Centralizes the 4-step write-path ritual (resolve role -> resolve + * featureEnabled -> htmlEmbedAllowed AND -> stripHtmlEmbedNodes) so the plain + * strip-all call-sites share one tested decision. Sites with CUSTOM strip logic + * (e.g. the collab persist path's preserve-admin variant) keep their own code. + */ +export function stripHtmlEmbedIfNotAllowed( + json: T, + opts: { featureEnabled: boolean; role: string | null | undefined; onStrip?: () => void }, +): T { + if (htmlEmbedAllowed(opts.featureEnabled, opts.role)) return json; + if (hasHtmlEmbedNode(json)) { + opts.onStrip?.(); + return stripHtmlEmbedNodes(json); + } + return json; +} + /** * Read the workspace-level htmlEmbed feature toggle from a workspace's settings * jsonb. ABSENT/non-true => OFF (the default). Kept here so every server write diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 93a13bfe..3dff5a8a 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -32,10 +32,8 @@ import { removeMarkTypeFromDoc, } from '../../../common/helpers/prosemirror/utils'; import { - hasHtmlEmbedNode, - htmlEmbedAllowed, isHtmlEmbedFeatureEnabled, - stripHtmlEmbedNodes, + stripHtmlEmbedIfNotAllowed, } from '../../../common/helpers/prosemirror/html-embed.util'; import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo'; import { @@ -157,15 +155,14 @@ export class PageService { const htmlEmbedEnabled = isHtmlEmbedFeatureEnabled( (await this.workspaceRepo.findById(workspaceId))?.settings, ); - if ( - !htmlEmbedAllowed(htmlEmbedEnabled, callerRole) && - hasHtmlEmbedNode(prosemirrorJson) - ) { - this.logger.warn( - `Stripping htmlEmbed node(s) from page creation by user ${userId} (space ${createPageDto.spaceId})`, - ); - prosemirrorJson = stripHtmlEmbedNodes(prosemirrorJson); - } + prosemirrorJson = stripHtmlEmbedIfNotAllowed(prosemirrorJson, { + featureEnabled: htmlEmbedEnabled, + role: callerRole, + onStrip: () => + this.logger.warn( + `Stripping htmlEmbed node(s) from page creation by user ${userId} (space ${createPageDto.spaceId})`, + ), + }); content = prosemirrorJson; textContent = jsonToText(prosemirrorJson); @@ -782,15 +779,14 @@ export class PageService { // that contains an embed into a new page authored by them. Strip every // htmlEmbed node from each duplicated page when the duplicating user is // not an admin, BEFORE computing textContent/ydoc/insert. - if ( - !htmlEmbedAllowed(htmlEmbedEnabled, authUser.role) && - hasHtmlEmbedNode(prosemirrorJson) - ) { - this.logger.warn( - `Stripping htmlEmbed node(s) from page duplication by user ${authUser.id} (source page ${page.id})`, - ); - prosemirrorJson = stripHtmlEmbedNodes(prosemirrorJson); - } + prosemirrorJson = stripHtmlEmbedIfNotAllowed(prosemirrorJson, { + featureEnabled: htmlEmbedEnabled, + role: authUser.role, + onStrip: () => + this.logger.warn( + `Stripping htmlEmbed node(s) from page duplication by user ${authUser.id} (source page ${page.id})`, + ), + }); // Add "Copy of " prefix to the root page title only for duplicates in same space let title = page.title; diff --git a/apps/server/src/core/page/transclusion/transclusion.service.ts b/apps/server/src/core/page/transclusion/transclusion.service.ts index 4dc0de9c..c3397031 100644 --- a/apps/server/src/core/page/transclusion/transclusion.service.ts +++ b/apps/server/src/core/page/transclusion/transclusion.service.ts @@ -34,10 +34,8 @@ import { jsonToNode } from '../../../collaboration/collaboration.util'; import { Page, User } from '@docmost/db/types/entity.types'; import { PageAccessService } from '../page-access/page-access.service'; import { - hasHtmlEmbedNode, - htmlEmbedAllowed, isHtmlEmbedFeatureEnabled, - stripHtmlEmbedNodes, + stripHtmlEmbedIfNotAllowed, } from '../../../common/helpers/prosemirror/html-embed.util'; import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo'; @@ -774,12 +772,14 @@ export class TransclusionService { const htmlEmbedEnabled = isHtmlEmbedFeatureEnabled( (await this.workspaceRepo.findById(user.workspaceId))?.settings, ); - if (!htmlEmbedAllowed(htmlEmbedEnabled, user.role) && hasHtmlEmbedNode(content)) { - this.logger.warn( - `Stripping htmlEmbed node(s) from transclusion unsync by user ${user.id} (reference page ${referencePageId}, source page ${sourcePageId})`, - ); - content = stripHtmlEmbedNodes(content); - } + content = stripHtmlEmbedIfNotAllowed(content, { + featureEnabled: htmlEmbedEnabled, + role: user.role, + onStrip: () => + this.logger.warn( + `Stripping htmlEmbed node(s) from transclusion unsync by user ${user.id} (reference page ${referencePageId}, source page ${sourcePageId})`, + ), + }); return { content }; } diff --git a/apps/server/src/integrations/import/services/file-import-task.service.ts b/apps/server/src/integrations/import/services/file-import-task.service.ts index c87d4d8f..aa39a085 100644 --- a/apps/server/src/integrations/import/services/file-import-task.service.ts +++ b/apps/server/src/integrations/import/services/file-import-task.service.ts @@ -21,10 +21,8 @@ import { FileTask, InsertablePage } from '@docmost/db/types/entity.types'; import { markdownToHtml } from '@docmost/editor-ext'; import { getProsemirrorContent } from '../../../common/helpers/prosemirror/utils'; import { - hasHtmlEmbedNode, - htmlEmbedAllowed, isHtmlEmbedFeatureEnabled, - stripHtmlEmbedNodes, + stripHtmlEmbedIfNotAllowed, } from '../../../common/helpers/prosemirror/html-embed.util'; import { UserRepo } from '@docmost/db/repos/user/user.repo'; import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo'; @@ -177,10 +175,6 @@ export class FileImportTaskService { const htmlEmbedEnabled = isHtmlEmbedFeatureEnabled( (await this.workspaceRepo.findById(fileTask.workspaceId))?.settings, ); - const importerCanAuthorHtmlEmbed = htmlEmbedAllowed( - htmlEmbedEnabled, - importingUser?.role, - ); const pagesMap = new Map(); @@ -534,15 +528,16 @@ export class FileImportTaskService { // SECURITY (Variant C admin gate): strip htmlEmbed nodes from pages // imported by a non-admin BEFORE computing textContent/ydoc/insert. - if ( - !importerCanAuthorHtmlEmbed && - hasHtmlEmbedNode(prosemirrorJson) - ) { - this.logger.warn( - `Stripping htmlEmbed node(s) from non-admin import by user ${fileTask.creatorId} (page ${page.id}, file ${filePath})`, - ); - prosemirrorJson = stripHtmlEmbedNodes(prosemirrorJson); - } + // Gate (featureEnabled AND admin) is resolved once above and recomputed + // by the helper from the same htmlEmbedEnabled + importer role. + prosemirrorJson = stripHtmlEmbedIfNotAllowed(prosemirrorJson, { + featureEnabled: htmlEmbedEnabled, + role: importingUser?.role, + onStrip: () => + this.logger.warn( + `Stripping htmlEmbed node(s) from non-admin import by user ${fileTask.creatorId} (page ${page.id}, file ${filePath})`, + ), + }); const insertablePage: InsertablePage = { id: page.id, diff --git a/apps/server/src/integrations/import/services/import.service.ts b/apps/server/src/integrations/import/services/import.service.ts index 574a13ab..cf602b77 100644 --- a/apps/server/src/integrations/import/services/import.service.ts +++ b/apps/server/src/integrations/import/services/import.service.ts @@ -3,9 +3,8 @@ import { PageRepo } from '@docmost/db/repos/page/page.repo'; import { UserRepo } from '@docmost/db/repos/user/user.repo'; import { hasHtmlEmbedNode, - htmlEmbedAllowed, isHtmlEmbedFeatureEnabled, - stripHtmlEmbedNodes, + stripHtmlEmbedIfNotAllowed, } from '../../../common/helpers/prosemirror/html-embed.util'; import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo'; import { MultipartFile } from '@fastify/multipart'; @@ -102,6 +101,8 @@ export class ImportService { // serialized form), which would execute raw JS in readers' browsers. Only // workspace admins/owners may author it, so strip htmlEmbed nodes from // imports performed by a non-admin user. + // Outer has-check first so the user/workspace lookups below run only when an + // embed is actually present (the common case carries none). if (prosemirrorJson && hasHtmlEmbedNode(prosemirrorJson)) { const importingUser = await this.userRepo.findById(userId, workspaceId); // Toggle-AND-admin gate: htmlEmbed survives only when the workspace @@ -110,12 +111,14 @@ export class ImportService { const htmlEmbedEnabled = isHtmlEmbedFeatureEnabled( (await this.workspaceRepo.findById(workspaceId))?.settings, ); - if (!htmlEmbedAllowed(htmlEmbedEnabled, importingUser?.role)) { - this.logger.warn( - `Stripping htmlEmbed node(s) from import by user ${userId}`, - ); - prosemirrorJson = stripHtmlEmbedNodes(prosemirrorJson); - } + prosemirrorJson = stripHtmlEmbedIfNotAllowed(prosemirrorJson, { + featureEnabled: htmlEmbedEnabled, + role: importingUser?.role, + onStrip: () => + this.logger.warn( + `Stripping htmlEmbed node(s) from import by user ${userId}`, + ), + }); } const pageTitle = title || fileName;