refactor(html-embed): extract the admin-gate strip into one tested helper (#90)

The 4-step html-embed gate (feature-enabled AND role-allowed -> stripHtmlEmbedNodes)
was replicated across call-sites, pinned only by brittle source-regex tests. Add
stripHtmlEmbedIfNotAllowed(json, {featureEnabled, role, onStrip}) and migrate the
5 plain strip-all sites (collab handler, page create+duplicate, both import paths,
transclusion) to it, each keeping its own feature/role resolve + log via onStrip.
Left the 2 sites with different semantics: persistence.extension (#29 preserve-
admin) and share.service (feature-only kill-switch, no role gate). Real unit tests
replace the regex pins; behavior identical.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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,

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: '<script>x()</script>' } },
+    ],
+  });
+  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

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<T>(
+  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

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;

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 };
   }

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<string, ImportPageNode>();
 
@@ -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,

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;