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

[test-coverage] Тесты для security-рефактора html-embed (commit 81823fce): sandbox + trackerHead #98

New Issue
Closed
opened 2026-06-21 03:10:49 +03:00 by Ghost · 0 comments
Ghost commented 2026-06-21 03:10:49 +03:00
Copy Link

Контекст

Область — изменения коммита 81823fce («feat(html-embed): sandbox the embed block; split trusted trackers into an admin field»): html-embed переведён в песочницу-iframe (sandbox="allow-scripts allow-popups allow-forms", opaque-origin, srcdoc), удалена вся ролевая обвязка стрипа, добавлено admin-only поле settings.trackerHead (инъекция в <head> только страниц публичного шаринга).

Полный анализ (8 модулей, по одному read-only субагенту на модуль, покрытие проверено инструментом) — в docs/test-strategy-report.md. Этот issue агрегирует все предложенные тесты в один список.

Проверено coverage-инструментом: «чистое ядро» html-embed.util.ts покрыто на 100 %, но все новые security-поверхности коммита не покрыты вовсе — инъекция trackerHead (0 %), валидация DTO (0 %), клиентские sandbox/postMessage/clamp (0 %), гейтинг slash-меню (0 %), атрибут height (0 %), CASL-гейт записи trackerHead (0 %).

Дефекты, которые должны быть закрыты тестами

  • BUG-1 — trackerHead вставляется НЕ дословно ($&-hazard). apps/server/src/core/share/share-seo.controller.ts:99-102: transformedHtml.replace('</head>', \{trackerHead}\n</head>\`)`. `String.prototype.replace` со строковым заменителем трактует спецшаблоны `, &, `` $ , `$'`, `$n`. Так как `trackerHead` подставлен В строку-заменитель, сниппет с `$&` молча заменяется на `</head>`, а ``/'вставят весь окружающий документ — комментарий «injected verbatim» неверен. Вход admin-only, но это реальная тихая порча. **Fix:** заменитель-функция() => `${trackerHead}\n</head>`(функция не интерпретирует-шаблоны) либо экранирование →$$`.
  • BUG-2 — парсер height возвращает NaN на мусоре. packages/editor-ext/src/lib/html-embed/html-embed.ts:98-100: голый parseInt(data-height) без guard'а isNaN (соседний drawio.ts:105-109 защищён). NaN-высота утекает в PM-JSON и ломает resize в NodeView. Fix: добавить тот же isNaN(...)?null.

Тесты — Фаза 1 (security-граница, рефакторинг НЕ нужен; максимальный ROI)

  • Инъекция trackerHead в ShareSeoController — спека share-seo.controller.spec.ts отсутствует. Не покрыты ветки нового кода: guard trim().length > 0 (пропуск пустого/пробельного), сам факт вставки до </head>, и буквальность вставки. Контроллер строить с моками, застабить fs.existsSync→true и fs.readFileSync→фикст��рный index.html. Сценарии: сниппет вставлен перед </head>; пустой/undefined/whitespace → html без изменений; буквальный round-trip сниппета с $& (вскрывает BUG-1); нет share → инъекции НЕ происходит (даже если trackerHead настроен); нет workspace → инъекции нет; wrong-workspace отбрасывается getShareForPage. (layer: unit для injectTrackerHead после R-s1 + integration для контроллера; ловит: BUG-1, утечку трекера на не-share-ответ, кросс-workspace утечку.) [HIGH]
  • CASL-гейт записи trackerHead — WorkspaceController.updateWorkspace с ability роли MEMBER → ForbiddenException, workspaceService.update НЕ вызывается; OWNER/ADMIN → вызывается. (integration, моки; ловит: запись trackerHead не-админом — единственный гейт на контроллере workspace.controller.ts:90-95.) [HIGH]
  • Валидация DTO UpdateWorkspaceDto (class-validator, через plainToInstance+validate): trackerHead — валидная строка ок, ровно 20000 ок, 20001 → maxLength, не-строка → isString; htmlEmbed — true/false ок, не-boolean ("true", 1) → isBoolean (с учётом transform:true глобального pipe — "true" НЕ коэрсится, проверить отказ). (unit; ловит: проход oversized/неверного типа до verbatim-инъекции. Существующий workspace-html-embed.spec.ts обходит валидацию через as any.) [HIGH]
  • Гейтинг slash-меню — getSuggestionItems (menu-items.ts:815): пункт «HTML embed» виден при тоггле ON, скрыт при OFF/отсутствии ключа (default OFF), битый JSON в localStorage → скрыт без исключения. (unit, рефактор не нужен; ловит: показ пункта при выключенной фиче.) [HIGH]
  • Edge-кейсы stripHtmlEmbedNodes / hasHtmlEmbedNode (html-embed.util.ts): несколько embed-сиблингов и в разных ветвях; deep-clone vs shared reference; content не-массив / пустой []; embed только в глубоком потомке (3+ уровня). (unit; ловит: фильтр, останавливающийся после первого совпадения, и aliasing входа.) [MED]
  • unsyncReference сохраняет html-embed — transclusion.service.ts с мок-репозиториями (замена удалённого transclusion-unsync-html-embed.spec.ts, инвертированная на «сохраняет»). (integration, рефактор не нужен; ловит: повторное появление стрипа на unsync.) [MED]

Тесты — Фаза 2 (требуется вынос чистых функций)

  • Логика авто-ресайза iframe (clamp + обработчик message) — html-embed-view.tsx:72-91 (guard event.source !== iframeRef.current?.contentWindow L75, guard типа сообщения, отказ при !Number.isFinite L79, clamp по обеим границам L81 и L90, short-circuit при фиксированной высоте). Проект тестирует компоненты через @testing-library/react, но дешевле вынести чистый clampHeight + предикат приёма сообщения в render-raw-html.ts (рядом с уже юнит-тестируемыми shouldExecute/canEdit). Тестировать границы клэмпа и guard по event.source. (unit после R-c2/R-c3; ловит: приём resize-сообщений ��з чужих фреймов, NaN в layout, DoS-раздувание.) [HIGH]
  • Атрибуты sandbox iframe — вынести buildEmbedIframeProps из html-embed-view.tsx:152: sandbox ровно allow-scripts allow-popups allow-forms, нет allow-same-origin, задан srcDoc, src отсутствует. (unit после R-c1; ловит: ослабление песочницы → выход в origin/XSS.) [HIGH]
  • Round-trip атрибута height в editor-ext — html-embed-codec.spec.ts проверяет только base64-source. Вынести parse/renderHtmlEmbedHeight (html-embed.ts:96-104) и протестировать: data-height="300"→300; отсутствует→null; число→data-height; falsy-skip (null/0) против truthy-emit в renderHTML; round-trip render→parse (0→null lossy, NaN→null — вскрывает BUG-2). Editor-ext импортируется и клиентом, и сервером — расхождение parse/serialize молча уронит/исказит высоту. (unit + 1 contract на потерю height в markdown, после R-e1.) [MED]
  • Мерж-хелперы настроек (UI) — вынести applyHtmlEmbedToWorkspace/applyTrackerHeadToWorkspace (html-embed-settings.tsx, tracker-settings.tsx): force-set значения даже если ответ его опускает, сохранение пустой строки, неперезатирание соседних settings-ключей. (unit после R-cs1/R-cs2.) [MED]
  • TrackerSettings для не-админа — textarea И Save кнопка disabled (поле остаётся в DOM by design — проверять disabled, не отсутствие). (component/integration, рефактор не нужен; ловит: UX-утечку привилегии. Реальная граница — на сервере.) [MED]

Тесты — Фаза 3 (требуется инфраструктура тестов)

  • PageService.create / duplicatePage сохраняют html-embed — нужен R-p1: PageService не грузится в jest из-за ESM-зависимости @sindresorhus/slugify вне transformIgnorePatterns (apps/server/package.json:208). (integration после R-p1.) [MED]
  • Контракт page.controller→PageService.create ровно с 4 аргументами — нужен R-p2: page.controller.spec.ts/page.service.spec.ts исключены из CI и являются заглушками toBeDefined(). Удалённый callerRole и provenance были трейлинг-опциональными → устаревший вызов с user.role сдвинул бы аргумент в слот provenance (тихий баг). (integration после un-exclude.) [MED]
  • Завести @vitest/coverage-v8 для измеримого порога покрытия клиента/editor-ext (сейчас не установлен).
  • (опц., ≤1 E2E) Playwright, user journey «анонимный читатель открывает публичную страницу с вредоносным html-embed»: встроенный iframe в реальном браузере не достаёт cookies/сессию/origin читателя — единственная гарантия, которую jsdom проверить не может (sandbox в jsdom не enforce-ится).

Необходимые рефакторинги (prerequisites)

  • R-s1 — вынести injectTrackerHead(html, trackerHead) из share-seo.controller.ts:97-103 (заодно закрыть BUG-1).
  • R-c1/R-c2/R-c3 — вынести buildEmbedIframeProps / isTrustedHeightMessage / clampIframeHeight из html-embed-view.tsx.
  • R-e1 — вынести parse/renderHtmlEmbedHeight из html-embed.ts:96-104 (заодно BUG-2).
  • R-cs1/R-cs2 — вынести мерж-хелперы настроек (UI).
  • R-p1 — сделать PageService загружаемым в jest (@sindresorhus/slugify в transformIgnorePatterns либо вынос вывода контента в чистый модуль).
  • R-p2 — снять page.controller.spec.ts/page.service.spec.ts из exclude-листа и переписать вместо заглушек.

Антипаттерны (выявлено)

  • Тесты-заглушки, исключённые из CI: page.service.spec.ts, page.controller.spec.ts, workspace.service.spec.ts — в jest.testPathIgnorePatterns И лишь expect(...).toBeDefined() → ложное ощущение покрытия.
  • workspace-html-embed.spec.ts зовёт service.update('w1', {...} as any), минуя DTO/ValidationPipe → «безопасность trackerHead» там не проверяется.

НЕ тестировать (skip-list)

DI/конструкторы, hocuspocus/yjs/BullMQ/Kysely-обвязка, pass-through пути записи (уже покрыты ниже по пирамиде через html-embed-import-detect.spec.ts), sendIndex/резолв workspace по host, виджеты Mantine, типы, строки локализации, SQL-тело updateSetting (нужен реальный Postgres — отложено).

## Контекст Область — изменения коммита `81823fce` («feat(html-embed): sandbox the embed block; split trusted trackers into an admin field»): html-embed переведён в песочницу-iframe (`sandbox="allow-scripts allow-popups allow-forms"`, opaque-origin, `srcdoc`), удалена вся ролевая обвязка стрипа, добавлено admin-only поле `settings.trackerHead` (инъекция в `<head>` только страниц публичного шаринга). Полный анализ (8 модулей, по одному read-only субагенту на модуль, покрытие проверено инструментом) — в `docs/test-strategy-report.md`. Этот issue агрегирует **все предложенные тесты в один список**. Проверено coverage-инструментом: «чистое ядро» `html-embed.util.ts` покрыто на **100 %**, но **все новые security-поверхности коммита не покрыты вовсе** — инъекция `trackerHead` (0 %), валидация DTO (0 %), клиентские sandbox/`postMessage`/clamp (0 %), гейтинг slash-меню (0 %), атрибут `height` (0 %), CASL-гейт записи `trackerHead` (0 %). --- ## Дефекты, которые должны быть закрыты тестами - [ ] **BUG-1 — `trackerHead` вставляется НЕ дословно (`$&`-hazard).** `apps/server/src/core/share/share-seo.controller.ts:99-102`: `transformedHtml.replace('</head>', \`${trackerHead}\n</head>\`)`. `String.prototype.replace` со строковым заменителем трактует спецшаблоны `$$`, `$&`, `` $` ``, `$'`, `$n`. Так как `trackerHead` подставлен В строку-заменитель, сниппет с `$&` молча заменяется на `</head>`, а `` $` ``/`$'` вставят весь окружающий документ — комментарий «injected verbatim» неверен. Вход admin-only, но это реальная тихая порча. **Fix:** заменитель-функция `() => \`${trackerHead}\n</head>\`` (функция не интерпретирует `$`-шаблоны) либо экранирование `$`→`$$`. - [ ] **BUG-2 — парсер `height` возвращает `NaN` на мусоре.** `packages/editor-ext/src/lib/html-embed/html-embed.ts:98-100`: голый `parseInt(data-height)` без guard'а `isNaN` (соседний `drawio.ts:105-109` защищён). `NaN`-высота утекает в PM-JSON и ломает resize в NodeView. **Fix:** добавить тот же `isNaN(...)?null`. --- ## Тесты — Фаза 1 (security-граница, рефакторинг НЕ нужен; максимальный ROI) - [ ] **Инъекция `trackerHead` в ShareSeoController** — спека `share-seo.controller.spec.ts` отсутствует. Не покрыты ветки нового кода: guard `trim().length > 0` (пропуск пустого/пробельного), сам факт вставки до `</head>`, и буквальность вставки. Контроллер строить с моками, застабить `fs.existsSync`→true и `fs.readFileSync`→фикст��рный `index.html`. Сценарии: сниппет вставлен перед `</head>`; пустой/`undefined`/whitespace → html без изменений; **буквальный round-trip сниппета с `$&`** (вскрывает BUG-1); **нет share → инъекции НЕ происходит** (даже если `trackerHead` настроен); нет workspace → инъекции нет; wrong-workspace отбрасывается `getShareForPage`. *(layer: unit для `injectTrackerHead` после R-s1 + integration для контроллера; ловит: BUG-1, утечку трекера на не-share-ответ, кросс-workspace утечку.)* **[HIGH]** - [ ] **CASL-гейт записи `trackerHead`** — `WorkspaceController.updateWorkspace` с ability роли MEMBER → `ForbiddenException`, `workspaceService.update` НЕ вызывается; OWNER/ADMIN → вызывается. *(integration, моки; ловит: запись trackerHead не-админом — единственный гейт на контроллере `workspace.controller.ts:90-95`.)* **[HIGH]** - [ ] **Валидация DTO `UpdateWorkspaceDto`** (class-validator, через `plainToInstance`+`validate`): `trackerHead` — валидная строка ок, ровно 20000 ок, 20001 → `maxLength`, не-строка → `isString`; `htmlEmbed` — true/false ок, не-boolean (`"true"`, `1`) → `isBoolean` (с учётом `transform:true` глобального pipe — `"true"` НЕ коэрсится, проверить отказ). *(unit; ловит: проход oversized/неверного типа до verbatim-инъекции. Существующий `workspace-html-embed.spec.ts` обходит валидацию через `as any`.)* **[HIGH]** - [ ] **Гейтинг slash-меню** — `getSuggestionItems` (`menu-items.ts:815`): пункт «HTML embed» виден при тоггле ON, скрыт при OFF/отсутствии ключа (default OFF), битый JSON в localStorage → скрыт без исключения. *(unit, рефактор не нужен; ловит: показ пункта при выключенной фиче.)* **[HIGH]** - [ ] **Edge-кейсы `stripHtmlEmbedNodes` / `hasHtmlEmbedNode`** (`html-embed.util.ts`): несколько embed-сиблингов и в разных ветвях; deep-clone vs shared reference; `content` не-массив / пустой `[]`; embed только в глубоком потомке (3+ уровня). *(unit; ловит: фильтр, останавливающийся после первого совпадения, и aliasing входа.)* **[MED]** - [ ] **`unsyncReference` сохраняет html-embed** — `transclusion.service.ts` с мок-репозиториями (замена удалённого `transclusion-unsync-html-embed.spec.ts`, инвертированная на «сохраняет»). *(integration, рефактор не нужен; ловит: повторное появление стрипа на unsync.)* **[MED]** ## Тесты — Фаза 2 (требуется вынос чистых функций) - [ ] **Логика авто-ресайза iframe (clamp + обработчик `message`)** — `html-embed-view.tsx:72-91` (guard `event.source !== iframeRef.current?.contentWindow` L75, guard типа сообщения, отказ при `!Number.isFinite` L79, clamp по обеим границам L81 и L90, short-circuit при фиксированной высоте). Проект тестирует компоненты через `@testing-library/react`, но дешевле вынести чистый `clampHeight` + предикат приёма сообщения в `render-raw-html.ts` (рядом с уже юнит-тестируемыми `shouldExecute`/`canEdit`). Тестировать границы клэмпа и guard по `event.source`. *(unit после R-c2/R-c3; ловит: приём resize-сообщений ��з чужих фреймов, NaN в layout, DoS-раздувание.)* **[HIGH]** - [ ] **Атрибуты sandbox iframe** — вынести `buildEmbedIframeProps` из `html-embed-view.tsx:152`: sandbox ровно `allow-scripts allow-popups allow-forms`, **нет** `allow-same-origin`, задан `srcDoc`, `src` отсутствует. *(unit после R-c1; ловит: ослабление песочницы → выход в origin/XSS.)* **[HIGH]** - [ ] **Round-trip атрибута `height` в editor-ext** — `html-embed-codec.spec.ts` проверяет только base64-source. Вынести `parse/renderHtmlEmbedHeight` (`html-embed.ts:96-104`) и протестировать: `data-height="300"`→300; отсутствует→null; число→`data-height`; falsy-skip (null/0) против truthy-emit в renderHTML; round-trip render→parse (0→null lossy, NaN→null — вскрывает BUG-2). Editor-ext импортируется и клиентом, и сервером — расхождение parse/serialize молча уронит/исказит высоту. *(unit + 1 contract на потерю height в markdown, после R-e1.)* **[MED]** - [ ] **Мерж-хелперы настроек (UI)** — вынести `applyHtmlEmbedToWorkspace`/`applyTrackerHeadToWorkspace` (`html-embed-settings.tsx`, `tracker-settings.tsx`): force-set значения даже если ответ его опускает, сохранение пустой строки, неперезатирание соседних `settings`-ключей. *(unit после R-cs1/R-cs2.)* **[MED]** - [ ] **`TrackerSettings` для не-админа** — textarea И Save кнопка `disabled` (поле остаётся в DOM by design — проверять `disabled`, не отсутствие). *(component/integration, рефактор не нужен; ловит: UX-утечку привилегии. Реальная граница — на сервере.)* **[MED]** ## Тесты — Фаза 3 (требуется инфраструктура тестов) - [ ] **`PageService.create` / `duplicatePage` сохраняют html-embed** — нужен **R-p1**: `PageService` не грузится в jest из-за ESM-зависимости `@sindresorhus/slugify` вне `transformIgnorePatterns` (`apps/server/package.json:208`). *(integration после R-p1.)* **[MED]** - [ ] **Контракт `page.controller`→`PageService.create` ровно с 4 аргументами** — нужен **R-p2**: `page.controller.spec.ts`/`page.service.spec.ts` исключены из CI и являются заглушками `toBeDefined()`. Удалённый `callerRole` и `provenance` были трейлинг-опциональными → устаревший вызов с `user.role` сдвинул бы аргумент в слот `provenance` (тихий баг). *(integration после un-exclude.)* **[MED]** - [ ] Завести `@vitest/coverage-v8` для измеримого порога покрытия клиента/editor-ext (сейчас не установлен). - [ ] **(опц., ≤1 E2E)** Playwright, user journey «анонимный читатель открывает публичную страницу с вредоносным html-embed»: встроенный iframe в реальном браузере не достаёт cookies/сессию/origin читателя — единственная гарантия, которую jsdom проверить не может (sandbox в jsdom не enforce-ится). --- ## Необходимые рефакторинги (prerequisites) - **R-s1** — вынести `injectTrackerHead(html, trackerHead)` из `share-seo.controller.ts:97-103` (заодно закрыть BUG-1). - **R-c1/R-c2/R-c3** — вынести `buildEmbedIframeProps` / `isTrustedHeightMessage` / `clampIframeHeight` из `html-embed-view.tsx`. - **R-e1** — вынести `parse/renderHtmlEmbedHeight` из `html-embed.ts:96-104` (заодно BUG-2). - **R-cs1/R-cs2** — вынести мерж-хелперы настроек (UI). - **R-p1** — сделать `PageService` загружаемым в jest (`@sindresorhus/slugify` в `transformIgnorePatterns` либо вынос вывода контента в чистый модуль). - **R-p2** — снять `page.controller.spec.ts`/`page.service.spec.ts` из exclude-листа и переписать вместо заглушек. ## Антипаттерны (выявлено) - Тесты-заглушки, исключённые из CI: `page.service.spec.ts`, `page.controller.spec.ts`, `workspace.service.spec.ts` — в `jest.testPathIgnorePatterns` И лишь `expect(...).toBeDefined()` → ложное ощущение покрытия. - `workspace-html-embed.spec.ts` зовёт `service.update('w1', {...} as any)`, минуя DTO/`ValidationPipe` → «безопасность trackerHead» там не проверяется. ## НЕ тестировать (skip-list) DI/конструкторы, hocuspocus/yjs/BullMQ/Kysely-обвязка, pass-through пути записи (уже покрыты ниже по пирамиде через `html-embed-import-detect.spec.ts`), `sendIndex`/резолв workspace по host, виджеты Mantine, типы, строки локализации, SQL-тело `updateSetting` (нужен реальный Postgres — отложено).
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 epic
Large multi-phase effort spanning many changes
 feature
New functionality request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 idea
Idea / proposal for discussion
 invalid
This doesn't seem right
 needs-human
эскалация: нужно решение человека
 question
Further information is requested
 refactor
Code cleanup / refactoring
 review/approved
в последнем ревью нет открытых blocking-находок
 review/changes-requested
последнее ревью оставило открытые blocking-находки
 review/needs
head не ревьюился (head != reviewed_head)
 security
Security / hardening issue
 status/blocked
ждёт зависимость blocked_by
 status/done
закрыто и проверено
 status/in-progress
в активной работе (мягкая заявка)
 status/ready
специфицировано, не заблокировано, ждёт исполнителя
 test
Test coverage / test infrastructure
 wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent_coder agent_reviewer vvzvlad
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#98
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?