diff --git a/AGENTS.md b/AGENTS.md index ed200604..bf393fb6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -280,4 +280,4 @@ The git tag is the source of truth for the displayed version (UI reads `git desc ## Planning docs -`docs/*.md` hold design plans for in-progress / planned features (mobile app, offline sync, RAG improvements, voice dictation, arbitrary HTML embed). `docs/backlog/*.md` track known issues / follow-ups (e.g. AI-chat review follow-ups). Consult the relevant plan before working on one of those areas. +`docs/*.md` hold design plans for in-progress / planned features (mobile app, offline sync, RAG improvements, voice dictation). `docs/backlog/*.md` track known issues / follow-ups (e.g. AI-chat review follow-ups). Consult the relevant plan before working on one of those areas. diff --git a/CHANGELOG.md b/CHANGELOG.md index 29058510..80d6ec51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Admin-only "Analytics / tracker" workspace setting: a raw HTML/JS snippet + injected into the `` of public share pages only (for analytics such as + Google Analytics or Yandex.Metrika). + +### Changed + +- HTML embed blocks now render inside a sandboxed iframe (separate origin) and, + when the workspace HTML-embed toggle is on, can be inserted by any member + (previously admin-only). Turning the toggle off hides existing embeds and + stops serving them on public share pages. + ## [0.91.0] - 2026-06-18 Gitmost is a community-focused fork of Docmost. This release drops the diff --git a/apps/client/src/features/editor/components/html-embed/render-raw-html.test.ts b/apps/client/src/features/editor/components/html-embed/html-embed-sandbox.test.ts similarity index 85% rename from apps/client/src/features/editor/components/html-embed/render-raw-html.test.ts rename to apps/client/src/features/editor/components/html-embed/html-embed-sandbox.test.ts index 8129dd74..70534be3 100644 --- a/apps/client/src/features/editor/components/html-embed/render-raw-html.test.ts +++ b/apps/client/src/features/editor/components/html-embed/html-embed-sandbox.test.ts @@ -3,8 +3,8 @@ import { buildSandboxSrcdoc, canEdit, HTML_EMBED_HEIGHT_MESSAGE, - shouldExecute, -} from "./render-raw-html"; + shouldRender, +} from "./html-embed-sandbox"; describe("buildSandboxSrcdoc", () => { it("embeds the user source verbatim", () => { @@ -32,19 +32,19 @@ describe("buildSandboxSrcdoc", () => { }); }); -describe("shouldExecute (render policy)", () => { +describe("shouldRender (render policy)", () => { it("read-only renders regardless of the workspace toggle", () => { // isEditable=false → the server already gated the content. - expect(shouldExecute(false, false)).toBe(true); - expect(shouldExecute(false, true)).toBe(true); + expect(shouldRender(false, false)).toBe(true); + expect(shouldRender(false, true)).toBe(true); }); it("editable + toggle OFF does NOT render", () => { - expect(shouldExecute(true, false)).toBe(false); + expect(shouldRender(true, false)).toBe(false); }); it("editable + toggle ON renders", () => { - expect(shouldExecute(true, true)).toBe(true); + expect(shouldRender(true, true)).toBe(true); }); }); diff --git a/apps/client/src/features/editor/components/html-embed/render-raw-html.ts b/apps/client/src/features/editor/components/html-embed/html-embed-sandbox.ts similarity index 94% rename from apps/client/src/features/editor/components/html-embed/render-raw-html.ts rename to apps/client/src/features/editor/components/html-embed/html-embed-sandbox.ts index 50740f23..983269f9 100644 --- a/apps/client/src/features/editor/components/html-embed/render-raw-html.ts +++ b/apps/client/src/features/editor/components/html-embed/html-embed-sandbox.ts @@ -1,6 +1,6 @@ /** * Pure helpers for the HTML embed node view. Kept out of the React component so - * the sandbox srcdoc builder and the execution/edit policy can be unit-tested + * the sandbox srcdoc builder and the render/edit policy can be unit-tested * against a bare environment with no Tiptap/Mantine providers. */ @@ -51,7 +51,7 @@ export function buildSandboxSrcdoc(source: string): string { } /** - * Execution policy split by editor mode: + * Render policy split by editor mode: * - READ-ONLY / public-share view: the SERVER already decided whether to * include the embed (it strips htmlEmbed from shared content when the * workspace master toggle is OFF). An anonymous viewer has no workspace and @@ -60,7 +60,7 @@ export function buildSandboxSrcdoc(source: string): string { * - EDITABLE editor: gate on the per-workspace master toggle so an author sees * the inert placeholder when the feature is OFF. */ -export function shouldExecute( +export function shouldRender( isEditable: boolean, featureEnabled: boolean, ): boolean { diff --git a/apps/client/src/features/editor/components/html-embed/html-embed-view.tsx b/apps/client/src/features/editor/components/html-embed/html-embed-view.tsx index 0e6633b1..ae116a7f 100644 --- a/apps/client/src/features/editor/components/html-embed/html-embed-view.tsx +++ b/apps/client/src/features/editor/components/html-embed/html-embed-view.tsx @@ -25,8 +25,8 @@ import { buildSandboxSrcdoc, canEdit as computeCanEdit, HTML_EMBED_HEIGHT_MESSAGE, - shouldExecute as computeShouldExecute, -} from "./render-raw-html.ts"; + shouldRender as computeShouldRender, +} from "./html-embed-sandbox.ts"; // Sane bounds for the auto-resized iframe so a runaway embed cannot blow up the // page layout, and a sensible default before the first height message arrives. @@ -34,6 +34,10 @@ const MIN_IFRAME_HEIGHT = 40; const MAX_IFRAME_HEIGHT = 4000; const DEFAULT_IFRAME_HEIGHT = 150; +// Clamp a reported/configured height into the sane iframe bounds. +const clampHeight = (h: number) => + Math.min(MAX_IFRAME_HEIGHT, Math.max(MIN_IFRAME_HEIGHT, h)); + export default function HtmlEmbedView(props: NodeViewProps) { const { t } = useTranslation(); const { node, selected, updateAttributes, editor } = props; @@ -48,7 +52,7 @@ export default function HtmlEmbedView(props: NodeViewProps) { const workspace = useAtomValue(workspaceAtom); const htmlEmbedEnabled = workspace?.settings?.htmlEmbed === true; - const shouldExecute = computeShouldExecute( + const shouldRender = computeShouldRender( editor.isEditable, htmlEmbedEnabled, ); @@ -60,7 +64,9 @@ export default function HtmlEmbedView(props: NodeViewProps) { // Auto-resize height tracked in state (used only when no fixed height is set). const [autoHeight, setAutoHeight] = useState( - height ?? DEFAULT_IFRAME_HEIGHT, + typeof height === "number" && Number.isFinite(height) + ? height + : DEFAULT_IFRAME_HEIGHT, ); const srcdoc = useMemo(() => buildSandboxSrcdoc(source || ""), [source]); @@ -70,24 +76,22 @@ export default function HtmlEmbedView(props: NodeViewProps) { // match by event.origin — we match by event.source instead. No-op when a // fixed height is configured. useEffect(() => { - if (typeof height === "number") return; + if (typeof height === "number" && Number.isFinite(height)) return; function onMessage(event: MessageEvent) { if (event.source !== iframeRef.current?.contentWindow) return; const data = event.data as { type?: string; height?: number }; if (data?.type !== HTML_EMBED_HEIGHT_MESSAGE) return; const next = Number(data.height); if (!Number.isFinite(next)) return; - setAutoHeight( - Math.min(MAX_IFRAME_HEIGHT, Math.max(MIN_IFRAME_HEIGHT, next)), - ); + setAutoHeight(clampHeight(next)); } window.addEventListener("message", onMessage); return () => window.removeEventListener("message", onMessage); }, [height]); const effectiveHeight = - typeof height === "number" - ? Math.min(MAX_IFRAME_HEIGHT, Math.max(MIN_IFRAME_HEIGHT, height)) + typeof height === "number" && Number.isFinite(height) + ? clampHeight(height) : autoHeight; const openEditor = useCallback(() => { @@ -130,11 +134,11 @@ export default function HtmlEmbedView(props: NodeViewProps) { )} - {!shouldExecute ? ( + {!shouldRender ? ( // Feature disabled for this workspace AND we're in the editable editor: // render a neutral placeholder so an existing embed is visibly inert for // the author. Read-only / share viewers never hit this branch - // (`shouldExecute` is always true there) — they render exactly the + // (`shouldRender` is always true there) — they render exactly the // source the server chose to serve.
@@ -151,7 +155,7 @@ export default function HtmlEmbedView(props: NodeViewProps) { className={classes.htmlEmbedFrame} sandbox="allow-scripts allow-popups allow-forms" srcDoc={srcdoc} - title="HTML embed" + title={t("HTML embed")} referrerPolicy="no-referrer" style={{ width: "100%", border: "none", height: effectiveHeight }} /> diff --git a/apps/client/src/features/workspace/components/settings/components/tracker-settings.tsx b/apps/client/src/features/workspace/components/settings/components/tracker-settings.tsx index 8a0a6b39..4cff4eb7 100644 --- a/apps/client/src/features/workspace/components/settings/components/tracker-settings.tsx +++ b/apps/client/src/features/workspace/components/settings/components/tracker-settings.tsx @@ -46,9 +46,10 @@ export default function TrackerSettings() { }); notifications.show({ message: t("Updated successfully") }); } catch (err) { - console.log(err); + console.error("Failed to update tracker settings", err); notifications.show({ - message: t("Failed to update data"), + message: + (err as any)?.response?.data?.message ?? t("Failed to update data"), color: "red", }); } finally { 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 d4e3cf35..8b1054e8 100644 --- a/apps/server/src/common/helpers/prosemirror/html-embed.util.ts +++ b/apps/server/src/common/helpers/prosemirror/html-embed.util.ts @@ -42,7 +42,9 @@ export function stripHtmlEmbedNodes(pmJson: T): T { /** * Returns true if the document contains at least one `htmlEmbed` node anywhere * in its tree. Useful to decide whether a strip pass on the share read path - * actually changed anything. + * actually changed anything. After the write-path role gate removal this is no + * longer called by production code; it is retained as a test-only assertion + * helper (and a detection primitive should a future read path need it). */ export function hasHtmlEmbedNode(pmJson: unknown): boolean { if (!pmJson || typeof pmJson !== 'object') { diff --git a/apps/server/src/core/share/share-seo.controller.ts b/apps/server/src/core/share/share-seo.controller.ts index 1f159416..1c443dcc 100644 --- a/apps/server/src/core/share/share-seo.controller.ts +++ b/apps/server/src/core/share/share-seo.controller.ts @@ -1,4 +1,4 @@ -import { Controller, Get, Param, Req, Res } from '@nestjs/common'; +import { Controller, Get, Logger, Param, Req, Res } from '@nestjs/common'; import { ShareService } from './share.service'; import { FastifyReply, FastifyRequest } from 'fastify'; import { join } from 'path'; @@ -11,6 +11,8 @@ import { htmlEscape } from '../../common/helpers/html-escaper'; @Controller('share') export class ShareSeoController { + private readonly logger = new Logger(ShareSeoController.name); + constructor( private readonly shareService: ShareService, private workspaceRepo: WorkspaceRepo, @@ -96,10 +98,20 @@ export class ShareSeoController { // block itself is sandboxed and is the safe surface for everyone else. const trackerHead = (workspace?.settings as any)?.trackerHead; if (typeof trackerHead === 'string' && trackerHead.trim().length > 0) { - transformedHtml = transformedHtml.replace( - '', - `${trackerHead}\n`, - ); + if (transformedHtml.includes('')) { + // Function replacer: the snippet is admin-authored trusted content and + // must be injected verbatim. A string replacement would interpret `$&`, + // `$'`, `` $` `` and `$$` inside it as substitution patterns and mangle + // the tracker; a function return value is inserted literally. + transformedHtml = transformedHtml.replace( + '', + () => `${trackerHead}\n`, + ); + } else { + this.logger.warn( + 'trackerHead is configured but no marker was found in the share index HTML; tracker snippet was not injected.', + ); + } } res.type('text/html').send(transformedHtml); diff --git a/docs/test-strategy-report.md b/docs/test-strategy-report.md new file mode 100644 index 00000000..9aeb75f5 --- /dev/null +++ b/docs/test-strategy-report.md @@ -0,0 +1,208 @@ +# Отчёт по тест-стратегии — gitmost — 2026-06-21 + +> Область анализа сознательно **ограничена коммитом `81823fce`** +> «feat(html-embed): sandbox the embed block; split trusted trackers into an admin field». +> Это security-рефактор: html-embed переведён в песочницу (iframe `sandbox="allow-scripts +> allow-popups allow-forms"`, opaque-origin, `srcdoc`), вся прежняя ролевая обвязка стрипа +> удалена, добавлено admin-only поле `settings.trackerHead` (инъекция в `` только +> страниц публичного шаринга). Стратегия покрывает поведения, которые этот коммит ввёл/изменил. + +## 1. Исполнительное резюме + +- Проанализировано модулей: **8** (по одному `module-testability-analyst` на модуль). +- Предложено тестов в немедленный план (unit / integration / e2e / contract): **13 / 4 / 1 / 1** = 19. +- Отложено за рефакторингом или признано опциональными: **≈8** (см. §5–6). +- Отклонено как малоценные (wiring/тривиальное/уже покрыто): **≈12** (агрегировано в «НЕ тестировать»). +- Покрытие сейчас (проверено инструментом, см. §7): из ~10 security-значимых поведений коммита + **покрыто 4**, **не покрыто 6**. Прогноз после внедрения плана: **10 из 10**. + +Главный вывод: «чистое ядро» (prosemirror-утилиты стрипа) покрыто на **100 %**, но **все новые +security-поверхности коммита не покрыты вовсе**: инъекция `trackerHead` в `` (0 %), +валидация DTO (0 %), клиентские sandbox-атрибуты / валидация `postMessage` / клэмп высоты (0 %), +гейтинг slash-меню (0 %), атрибут `height` в схеме узла (0 %), CASL-гейт на запись `trackerHead` (0 %). + +## 2. Рекомендации по модулям + +### M1. client-html-embed-rendering (`apps/client/.../editor/components/html-embed/` + `slash-menu/`) +Самая важная клиентская логика — атрибуты sandbox, валидация источника `postMessage` и клэмп +высоты — живёт **внутри React-компонента** `html-embed-view.tsx` как JSX-литерал и inline-замыкания, +поэтому юнит-нетестируема. +- **Извлечь в чистые функции (R-c1/R-c2/R-c3):** + `html-embed-view.tsx:149-157` → `buildEmbedIframeProps()`; `:74-83` → `isTrustedHeightMessage(event, src)`; + `:80-82` и `:88-91` (дублируется) → `clampIframeHeight(n)`. +- **Unit-тесты добавить:** + - `isTrustedHeightMessage` — сообщение из чужого `window` (спуф) отвергается; верный source+неверный + `type` → null; `height` = NaN/Infinity → null; клэмп границ. *Ловит: инъекцию resize-сообщений.* **[HIGH]** + - `buildEmbedIframeProps` — sandbox ровно `allow-scripts allow-popups allow-forms`; **нет** `allow-same-origin`; + задан `srcDoc`, `src` отсутствует. *Ловит: ослабление песочницы → выход в origin/XSS.* **[HIGH]** + - `getSuggestionItems` (`menu-items.ts:815`) — пункт «HTML embed» виден при тоггле ON, скрыт при OFF/отсутствии + ключа (default OFF), битый JSON в localStorage → пункт скрыт и без исключения. *Ловит: показ пункта при + выключенной фиче.* **[HIGH]** (рефактор не нужен) + - `clampIframeHeight` — границы [40, 4000], отрицательные/0 → MIN. *Ловит: DoS-раздувание layout.* **[MED]** +- **НЕ тестировать:** модалка/`draft`-стейт/плейсхолдеры `HtmlEmbedView` (UI-обвязка, политика уже в чистых + хелперах); `slash-menu/types.ts` (только типы); `buildSandboxSrcdoc`-строка уже покрыта (`render-raw-html.test.ts`). + +### M2. editor-ext-html-embed-schema (`packages/editor-ext/src/lib/html-embed/`) +- **Извлечь (R-e1):** `html-embed.ts:96-104` → `parseHtmlEmbedHeight` / `renderHtmlEmbedHeight` (по образцу уже + вынесенного `encode/decodeHtmlEmbedSource`). +- **Unit-тесты добавить:** + - `parseHtmlEmbedHeight` — «120»→120, отсутствует→null, **«abc»→NaN (ДЕФЕКТ, см. §4)**, «120px»→120. **[MED]** + - `renderHtmlEmbedHeight` — 120→`{data-height:"120"}`, null→`{}`, 0→`{}`, NaN→`{}`. **[MED]** + - round-trip render→parse — 120↔120, 0→null (lossy), NaN→null — фиксирует асимметрию. **[MED]** +- **Contract-тест добавить:** markdown export/import узла с `data-height` — явно зафиксировать, что `height` + **теряется** (маркер несёт только base64-source). *Ловит: молчаливую потерю/непреднамеренное появление.* **[MED]** +- **НЕ тестировать:** `parseHTML`-селектор, `renderHTML`-`mergeAttributes`, `addCommands/addNodeView` (wiring); + base64-кодек source — уже полностью покрыт (`html-embed-codec.spec.ts`). + +### M3. client-workspace-settings-ui (`apps/client/.../workspace/.../settings/`) +Реальная граница безопасности — на сервере; здесь тесты ловят UX-регрессии видимости, а не сам бордер. +Конвенция проекта (см. `ai-provider-settings.spec.tsx`) — выносить логику в чистые хелперы и юнит-тестировать их. +- **Извлечь (R-cs1/R-cs2):** мерж-логику `applyHtmlEmbedToWorkspace` / `applyTrackerHeadToWorkspace` + (force-set значения даже если ответ его опускает; сохранение пустой строки; не затирать прочие `settings`-ключи). +- **Unit-тесты добавить:** мерж обоих хелперов (ключ, пустая строка, сохранность соседних ключей). **[MED]** +- **Component/Integration-тест добавить:** `TrackerSettings` для не-админа — textarea **и** Save кнопка `disabled` + (поле остаётся в DOM by design — проверять `disabled`, не отсутствие). *Ловит: редактируемость trackerHead не-админом + (UX-утечка привилегии).* **[MED]** +- **НЕ тестировать:** `WorkspaceSettings`-страница (рендерит карточки по порядку, ветвлений нет); типы + `IWorkspace*`; точные строки локализации (тавтологичный snapshot); виджеты Mantine (сторонние). + +### M4. server-prosemirror-html-embed-util (`apps/server/src/common/helpers/prosemirror/html-embed.util.ts`) +Чистые функции над PM-деревом — идеальный unit-уровень. **Покрытие 100 %** (проверено), но есть узкие +edge-пробелы в существующих спеках. +- **Unit-тесты добавить:** + - `stripHtmlEmbedNodes` — несколько embed-сиблингов на одном уровне и в разных ветвях; deep-clone vs shared + reference (вложенный сохранённый узел — новый объект); `content` не-массив / пустой `[]`. **[MED]** + - `hasHtmlEmbedNode` — embed только в глубоком потомке (3+ уровня); `content: []`. **[MED]** +- **НЕ тестировать:** `HTML_EMBED_NODE_NAME` (константа); привязка тоггл→стрип на share-пути (уже покрыто + на уровне реального консьюмера в `share-html-embed.spec.ts`); base64-кодек (это `@docmost/editor-ext`). +- **Открытый риск:** `prepareContentForShare` вызывает `getProsemirrorContent()` перед стрипом; если оттуда + приходит **инстанс PM-`Node` (класс)**, а не POJO, то `{ ...node }` (line 39) теряет прототип/методы. Спеки + гоняют только POJO — контракт на границе не проверен. + +### M5. server-collab-import-write-paths (collaboration + import) +Коммит удалил ролевой стрип на путях записи (collab REST/MCP+socket, импорт). Новое поведение — +**сквозной pass-through**: html-embed сохраняется. Это уже покрыто **ниже по пирамиде** +(`html-embed-import-detect.spec.ts` гоняет реальный `markdownToHtml`→`htmlToJson`). +- **Новых html-embed-тестов не требуется** — поведение «стрип отсутствует» = pass-through, уже зафиксировано. +- **Опционально (вне scope коммита):** `extractTitleAndRemoveHeading` (`import.service.ts:161`) — единственная + ветвящаяся чистая логика модуля, не покрыта; требует R-i1 (вынос в leaf-модуль, т.к. сервис не грузится в jest — + см. §5). **[LOW]** +- **НЕ тестировать:** конструкторы/DI, `onLoadDocument/onStoreDocument` (hocuspocus/yjs/BullMQ/Kysely-обвязка), + `updatePageContent`/`withYdocConnection` (pass-through), очередь импорта (FS/DB-wiring). +- **Грепы dangling-ссылок:** `htmlEmbedAllowed`, `canAuthorHtmlEmbed`, `stripDisallowedHtmlEmbedNodes`, + `collectHtmlEmbedSources` → **0 ссылок** в `apps/`/`packages/`. DI согласован, битых инъекций нет. + +### M6. server-page-transclusion (`page.service.ts`, `page.controller.ts`, `transclusion.service.ts`) +Удалён параметр `callerRole` у `PageService.create` и стрип у create/duplicate/unsync. +- **Integration-тесты добавить (мок-репозитории):** + - `unsyncReference` сохраняет htmlEmbed (замена удалённого `transclusion-unsync-html-embed.spec.ts`, + инвертированная на «сохраняет»). *Ловит: повторное появление стрипа.* **[MED]** (рефактор не нужен) +- **Отложены за рефакторингом (НЕ обещать без него):** `create`/`duplicatePage` сохраняют htmlEmbed (нужен R-p1 — + сервис не грузится в jest); контракт `page.controller`→`create` ровно с 4 аргументами (нужен R-p2 — снять + спек из exclude-листа). Контрактный тест критичен: оба удалённый `callerRole` и `provenance` были трейлинг- + опциональными → устаревший вызов с `user.role` сдвинул бы аргумент в слот `provenance` (тихий баг). +- **НЕ тестировать:** DB-оркестрацию create/duplicate (insertPage/insertMany/позиции/вотчеры), удаление + `WorkspaceRepo` из конструктора (деталь реализации), `parseProsemirrorContent` (не менялся). +- **Греп `callerRole`:** в активном дереве **0 ссылок** (совпадения только в отдельном git-worktree). + +### M7. server-share-trackerhead (`share-seo.controller.ts`, `share.service.ts`) — **SECURITY-CRITICAL** +- **Извлечь (R-s1):** `share-seo.controller.ts:97-103` → `injectTrackerHead(html, trackerHead): string` + (verbatim-конкатенация + guard на пустое/whitespace/не-строку + позиция перед ``). +- **Unit-тесты добавить:** `injectTrackerHead` — непустой сниппет вставлен **дословно** перед ``; + спецсимволы `< > & "` не экранируются (фиксирует намеренный no-escape); пустая/whitespace/не-строка → + html без изменений; позиция — только перед **первым** ``, не в ``. *Ловит: потерю инъекции, + вставку не туда (XSS-позиционирование), случайное экранирование, `[object Object]`.* **[HIGH]** +- **Integration-тесты добавить (контроллер с моками; застабить `fs.existsSync`→true, `fs.readFileSync`→фикстура):** + - сниппет присутствует → есть в ``; пустой → разметка не ломается; + - **нет share → инъекции НЕ происходит** (даже если у workspace настроен trackerHead) — ключевой негатив; + - **нет workspace → инъекции НЕ происходит**; wrong-workspace → `getShareForPage` отбрасывает чужой share. **[HIGH]** +- **Уже покрыто:** стрип на share-read-пути (`share-html-embed.spec.ts:77-262`: ON/OFF/absent/fail-closed, + `updatePublicAttachments`, transclusion-for-share). `trackerHead`-инъекция — **0 % (проверено)**. +- **НЕ тестировать:** `sendIndex`/резолв workspace по host (wiring), сборку meta-тегов (не менялась), + CASL-гейт записи (другой модуль — M8). + +### M8. server-workspace-settings (`workspace.service.ts`, `dto/update-workspace.dto.ts`) +Единственный admin-гейт `trackerHead` — на контроллере (`workspace.controller.ts:90-95`, +`ability.cannot(Manage, Settings)`); ни DTO, ни сервис не ролевые. +- **Unit-тесты добавить (class-validator):** + - `trackerHead` — валидная строка ок; ровно 20000 символов ок; 20001 → ошибка `maxLength`; не-строка → `isString`. **[HIGH]** + - `htmlEmbed` — true/false ок; не-boolean (`"true"`, `1`) → `isBoolean` (важно: глобальный pipe `transform:true` + **не** коэрсит строку — проверить, что `"true"` отвергается). **[HIGH]** +- **Integration-тест добавить:** `WorkspaceController.updateWorkspace` с ability роли MEMBER → `ForbiddenException`, + `workspaceService.update` **не вызывается**; OWNER/ADMIN → вызывается. *Ловит: запись `trackerHead` не-админом.* **[HIGH]** +- **Уже покрыто:** call-shape персиста настроек (`workspace-html-embed.spec.ts`: htmlEmbed/trackerHead через + `updateSetting`, audit-diff, «отсутствует → не вызывается»). **Но валидация там обойдена `as any`** — это не покрытие валидации. +- **НЕ тестировать:** соседние поля DTO (не менялись), SQL-тело `updateSetting` (kysely/PG — нужен реальный Postgres, + отложено), `delete dto.trackerHead`-чистку (деталь), декларации CASL-фабрики (wiring). + +## 3. Сквозные аспекты +- **Contract-тесты:** один — markdown round-trip узла html-embed теряет `height` (M2). Кросс-сервисных контрактов нет. +- **Property-based:** кандидат — `stripHtmlEmbedNodes` (инвариант: в результате нет узлов `htmlEmbed` при любой + форме дерева; вход не мутируется). Реализуемо после того, как edge-кейсы из M4 пройдут. +- **Дымовой/нагрузочный:** не применимо к scope коммита. +- **Test-data factories:** фабрика PM-дока с произвольно вложенными `htmlEmbed` (для M4/M6); фикстура `index.html` + с маркерами ``/`` (для M7); билдер `UpdateWorkspaceDto`-payload (для M8). + +## 4. Обнаруженные дефекты и антипаттерны +- **[ДЕФЕКТ, low-med] `html-embed.ts:98-100`** парсит `data-height` голым `parseInt` → на мусоре возвращает **NaN** + (соседний `drawio.ts:105-109` защищён `isNaN(...)?null`). NaN-высота утекает в PM-JSON и ломает resize в NodeView. + Рекомендация: добавить тот же guard `isNaN`; тест M2 фиксирует исправление. +- **[Антипаттерн] Тесты-заглушки, исключённые из CI.** `page.service.spec.ts`, `page.controller.spec.ts`, + `workspace.service.spec.ts` — в `jest.testPathIgnorePatterns` (не запускаются) **и** содержат лишь + `expect(...).toBeDefined()`. Создают ложное ощущение покрытия create/duplicate/controller — фактически **0 %**. +- **[Антипаттерн] Обход валидации.** `workspace-html-embed.spec.ts` зовёт `service.update('w1', {...} as any)`, + минуя DTO/`ValidationPipe` — «безопасность trackerHead» там не проверяется. +- **[Блокер тестируемости] Сервисы не грузятся в jest.** `PageService`/`ImportService` тянут ESM-зависимость + `@sindresorhus/slugify`, не входящую в `transformIgnorePatterns` (`apps/server/package.json:208`) — поэтому + удалённые спеки использовали source-pin. Блокирует integration-тесты путей записи (см. рефакторинги). +- **[Риск] Param-shift** после удаления `callerRole` (M6) — не покрыт. +- **[Риск] PM-`Node` vs POJO** на входе `stripHtmlEmbedNodes` (M4) — не покрыт. +- Order-dependent / flaky тестов в scope **не обнаружено**; существующие спеки детерминированы. + +## 5. Необходимые рефакторинги перед написанием тестов +- **R-s1** — вынести `injectTrackerHead` из `share-seo.controller.ts:97-103` → блокирует unit-тесты M7 (HIGH). +- **R-c1/R-c2/R-c3** — вынести `buildEmbedIframeProps` / `isTrustedHeightMessage` / `clampIframeHeight` из + `html-embed-view.tsx` → блокирует unit-тесты M1 (sandbox, source-validation, clamp; HIGH). +- **R-e1** — вынести `parse/renderHtmlEmbedHeight` из `html-embed.ts:96-104` → блокирует unit-тесты M2. +- **R-cs1/R-cs2** — вынести мерж-хелперы настроек (M3) → блокирует unit-тесты мержа. +- **R-p1** — сделать `PageService` загружаемым в jest (добавить `@sindresorhus/slugify` в `transformIgnorePatterns` + **или** вынести вывод контента в чистый модуль) → блокирует `create`/`duplicatePage`-тесты (M6). +- **R-p2** — снять `page.controller.spec.ts`/`page.service.spec.ts` из exclude-листа и переписать → блокирует + контракт-тест 4 аргументов (M6). +- **R-i1** (опц.) — вынести `extractTitleAndRemoveHeading` в leaf-модуль (M5). + +## 6. План внедрения (по фазам) +- **Фаза 1 — security-граница, без рефакторинга (макс. ROI).** Integration `ShareSeoController.getShare` + (M7, incl. негативы no-share/no-workspace); integration CASL-гейт MEMBER→Forbidden (M8); unit-валидация DTO + `trackerHead`/`htmlEmbed` (M8); unit `getSuggestionItems`-гейтинг (M1); unit edge-кейсы + `stripHtmlEmbedNodes`/`hasHtmlEmbedNode` (M4); integration `unsyncReference` сохраняет embed (M6). +- **Фаза 2 — извлечения + клиентское ядро.** R-s1/R-c1..3/R-e1/R-cs1..2 и их unit-тесты: `injectTrackerHead`, + `isTrustedHeightMessage`, `buildEmbedIframeProps`, `clampIframeHeight`, `parse/renderHtmlEmbedHeight` + round-trip, + contract-тест потери `height`, мерж-хелперы и component-тест `TrackerSettings` (не-админ disabled). Здесь же — + исправить NaN-парсер высоты (§4). +- **Фаза 3 — инфраструктура тестов.** R-p1/R-p2: разблокировать загрузку `PageService` в jest и un-exclude + спеков; добавить `create`/`duplicatePage`-preservation и контракт 4 аргументов; завести `@vitest/coverage-v8` + для измеримого порога покрытия. Опционально — E2E (Playwright). +- **E2E (≤1, user journey «анонимный читатель открывает публичную страницу с вредоносным html-embed»):** + проверить, что встроенный iframe в реальном браузере не достаёт cookies/сессию/origin читателя — единственная + гарантия, которую jsdom проверить не может (атрибут sandbox в jsdom не enforce-ится). + +## 7. Источники и трассировка фильтров +- Отчёты **8** аналитиков `module-testability-analyst` (по модулю M1–M8). +- **Независимая проверка покрытия** (не доверяя заявлениям аналитиков): + - server jest, провайдер V8 (дефолтный babel-провайдер падает на SWC-трансформе): + `html-embed.util.ts` **100/100/100/100**; `share-seo.controller.ts` **0 %**; `update-workspace.dto.ts` **0 %**; + `share.service.ts` — путь стрипа покрыт; `workspace.service.ts` — ветка персиста настроек покрыта. + Прогон спеков html-embed/share/workspace — **зелёные** (4 suite / 35 тестов; полный прогон 42 suite / 567 тестов). + - client/editor-ext vitest — **зелёные** (client 22, editor-ext 25); `@vitest/coverage-v8` не установлен, + поэтому пробелы подтверждены grep'ом по тестам: sandbox/postMessage/clamp, `html-embed-view`, гейтинг slash-меню, + `trackerHead`-UI, `data-height` — **нулевые ссылки в тестах**. +- **Фильтрация (по шагам Phase 3):** + - Шаг 1 (кросс-модульный дедуп): импорт-preservation (M5) и привязка тоггл→стрип сведены к нижнему слою — снято ~2. + - Шаг 2 (skip-list): wiring/DI/тривиальное/сторонние агрегированы в «НЕ тестировать» — отклонено ~12. + - Шаг 4 (refactor-blocking): `create`/`duplicate`/контракт-4-арг (M6) переведены в отложенные за R-p1/R-p2. + - Шаг 6 (adversarial): отброшены `buildSandboxSrcdoc`-hardening (дублирует существующий тест) и + `extractTitleAndRemoveHeading` (вне scope коммита). +- **Бюджет пирамиды (немедленный план, 19 тестов):** unit+contract 14 (**74 %**), integration 4 (21 %), + e2e 1 (5 %; абсолют ≤10). Лёгкое смещение к integration оправдано: ценность security-рефактора — на границе + (CASL-гейт, инъекция в ``, сохранность на путях записи), что по природе integration-уровень. diff --git a/packages/editor-ext/src/lib/html-embed/html-embed.ts b/packages/editor-ext/src/lib/html-embed/html-embed.ts index 93428d2f..02f84f9c 100644 --- a/packages/editor-ext/src/lib/html-embed/html-embed.ts +++ b/packages/editor-ext/src/lib/html-embed/html-embed.ts @@ -97,7 +97,12 @@ export const HtmlEmbed = Node.create({ default: null, parseHTML: (el) => { const v = el.getAttribute("data-height"); - return v ? parseInt(v, 10) : null; + if (!v) return null; + const n = parseInt(v, 10); + // A non-numeric data-height (e.g. crafted/corrupted import) must not + // become NaN: NaN is typeof "number" and would disable auto-resize and + // yield an unclamped iframe height downstream. Treat it as auto (null). + return Number.isFinite(n) ? n : null; }, renderHTML: (attrs: HtmlEmbedAttributes) => attrs.height ? { "data-height": String(attrs.height) } : {}, diff --git a/packages/mcp/src/lib/docmost-schema.ts b/packages/mcp/src/lib/docmost-schema.ts index 3d8d25d7..63bef5c2 100644 --- a/packages/mcp/src/lib/docmost-schema.ts +++ b/packages/mcp/src/lib/docmost-schema.ts @@ -797,6 +797,60 @@ const Embed = Node.create({ }, }); +/** + * Docmost raw HTML embed. Block atom; the client renders `source` inside a + * sandboxed iframe. The MCP server never renders it — it only needs the + * schema to accept and carry the node so a fromYdoc -> transform -> toYdoc + * round-trip does not throw "Unknown node type: htmlEmbed". Mirrors the + * @docmost/editor-ext node name, attribute keys and flags; keep in sync when + * the editor-ext htmlEmbed schema changes. + * + * NOTE: unlike the canonical editor-ext node, `data-source` here is mapped as + * plain text rather than base64-encoded. That is intentional: the MCP write + * path carries the node through Yjs (fromYdoc -> toYdoc) on its JSON `source` + * attribute and never invokes parseHTML/renderHTML, and htmlEmbed is not + * produced from the markdown/HTML (generateJSON) path. If a future HTML path + * for htmlEmbed is added here, this mapping must adopt editor-ext's base64 + * encode/decode to avoid double-encoding `source`. + */ +const HtmlEmbed = Node.create({ + name: "htmlEmbed", + group: "block", + inline: false, + isolating: true, + atom: true, + defining: true, + draggable: true, + addAttributes() { + return { + source: { + default: "", + parseHTML: (el: HTMLElement) => el.getAttribute("data-source") ?? "", + renderHTML: (attrs: Record) => ({ + "data-source": attrs.source ?? "", + }), + }, + height: { + default: null, + parseHTML: (el: HTMLElement) => { + const v = el.getAttribute("data-height"); + if (!v) return null; + const n = parseInt(v, 10); + return Number.isFinite(n) ? n : null; + }, + renderHTML: (attrs: Record) => + attrs.height != null ? { "data-height": String(attrs.height) } : {}, + }, + }; + }, + parseHTML() { + return [{ tag: 'div[data-type="htmlEmbed"]' }]; + }, + renderHTML({ HTMLAttributes }) { + return ["div", { "data-type": "htmlEmbed", ...HTMLAttributes }, 0]; + }, +}); + /** Shared attribute set for drawio/excalidraw diagram nodes. */ const diagramAttributes = () => ({ src: { @@ -1158,6 +1212,7 @@ export const docmostExtensions = [ Video, Youtube, Embed, + HtmlEmbed, Drawio, Excalidraw, Columns,