fix(html-embed): complete kill-switch on read paths (#28) + total strip helper (#30) #46

Ghost · 2026-06-20T21:53:28+03:00

Ghost commented

2026-06-20 21:53:28 +03:00

Усиления безопасности фичи html-embed, найденные в security-review PR #16 (смержен в develop). Ветка от develop, по коммиту на issue. Closes #28, #30.

Что сделано

#28 — завершение kill-switch: strip эмбедов на серверe на всех authenticated read-путях (`6a052b88`)

Тоггл htmlEmbed должен мгновенно нейтрализовать все эмбеды при выключении. Путь публичной шары уже вырезал на serve-time, но аутентифицированные read-пути (/info и /history/info) отдавали content с эмбедами как есть → после выключения фичи страница с уже сохранённым (админским) эмбедом продолжала исполнять JS у view-only пользователей до следующего сохранения. Теперь оба пути резолвят workspace-тоггл и прогоняют stripHtmlEmbedNodes, когда он OFF (fail-closed при отсутствии workspace), ДО конвертации форматов. Только админский контент — это полнота kill-switch, не privilege escalation. В PageController инъектится WorkspaceRepo.

(/history/info добавлен после замечания ревью — снапшоты истории это тоже read-путь.)

#30 — `stripHtmlEmbedNodes` тотальна (проверка корневого узла) (`8ee4279d`)

Функция фильтровала только детей, поэтому (никогда не встречающийся на практике) голый htmlEmbed-корень вернулся бы как есть. Добавлена защитная проверка корня → возвращает embed-free doc; функция больше не может вернуть узел, для которого hasHtmlEmbedNode === true. Добавлен юнит-тест на корневой кейс (html-embed.spec.ts — 17/17 зелёные).

Как рассуждал

#28: вырезку поставил ДО конвертации формата (markdown/html), чтобы покрыть все форматы выдачи; мутирую page.content/history.content на месте — оба return-пути берут очищенный контент. Переиспользовал готовые хелперы (isHtmlEmbedFeatureEnabled, stripHtmlEmbedNodes) и WorkspaceRepo, как в share.service.
Подтвердил, что PageRepo.findById всегда селектит workspaceId (иначе fail-closed резал бы и при ON) и что WorkspaceRepo экспортируется из @Global DatabaseModule (нет циклов).

Косяки, найденные на ревью

Ревью (APPROVE WITH SUGGESTIONS) указал, что /history/info тоже отдаёт сырой контент → расширил #28 и на него.
Подтверждено: write-пути (create/update) уже режут на write-time; нормальный ON-кейс не вырезается; структура doc без эмбедов не портится.

Проверка

tsc --noEmit — без новых ошибок (есть известные pre-existing TS2554 в transclusion-спеках, не связаны). jest html-embed.spec.ts — 17/17.

НЕ вошло (осознанно)

#26 (transient-эмбед в окне collab-броадкаста до persist-strip) и #29 (консервативный strip убирает админский эмбед при store не-админа) — в issue помечены как accepted residual risk / intentional, documented in-code. Фикс #26 (inbound strip до броадкаста) рискован для коллаба, #29 — намеренное fail-closed поведение. Не трогал без явного решения — оставил как задокументированный принятый риск.
#27 (переписать gate-тесты с regex на реальное исполнение) — test-quality, отдельно.

🤖 Generated with Claude Code

Усиления безопасности фичи **html-embed**, найденные в security-review PR #16 (смержен в develop). Ветка от `develop`, по коммиту на issue. Closes #28, #30. ## Что сделано ### #28 — завершение kill-switch: strip эмбедов на серверe на всех authenticated read-путях (`6a052b88`) Тоггл `htmlEmbed` должен мгновенно нейтрализовать все эмбеды при выключении. Путь публичной шары уже вырезал на serve-time, но аутентифицированные read-пути (`/info` и `/history/info`) отдавали `content` с эмбедами как есть → после выключения фичи страница с уже сохранённым (админским) эмбедом продолжала исполнять JS у view-only пользователей до следующего сохранения. Теперь оба пути резолвят workspace-тоггл и прогоняют `stripHtmlEmbedNodes`, когда он OFF (fail-closed при отсутствии workspace), ДО конвертации форматов. Только админский контент — это полнота kill-switch, не privilege escalation. В `PageController` инъектится `WorkspaceRepo`. (`/history/info` добавлен после замечания ревью — снапшоты истории это тоже read-путь.) ### #30 — `stripHtmlEmbedNodes` тотальна (проверка корневого узла) (`8ee4279d`) Функция фильтровала только детей, поэтому (никогда не встречающийся на практике) голый `htmlEmbed`-корень вернулся бы как есть. Добавлена защитная проверка корня → возвращает embed-free doc; функция больше не может вернуть узел, для которого `hasHtmlEmbedNode === true`. Добавлен юнит-тест на корневой кейс (html-embed.spec.ts — 17/17 зелёные). ## Как рассуждал - #28: вырезку поставил ДО конвертации формата (markdown/html), чтобы покрыть все форматы выдачи; мутирую `page.content`/`history.content` на месте — оба return-пути берут очищенный контент. Переиспользовал готовые хелперы (`isHtmlEmbedFeatureEnabled`, `stripHtmlEmbedNodes`) и `WorkspaceRepo`, как в share.service. - Подтвердил, что `PageRepo.findById` всегда селектит `workspaceId` (иначе fail-closed резал бы и при ON) и что `WorkspaceRepo` экспортируется из @Global DatabaseModule (нет циклов). ## Косяки, найденные на ревью - Ревью (APPROVE WITH SUGGESTIONS) указал, что `/history/info` тоже отдаёт сырой контент → расширил #28 и на него. - Подтверждено: write-пути (create/update) уже режут на write-time; нормальный ON-кейс не вырезается; структура doc без эмбедов не портится. ## Проверка `tsc --noEmit` — без новых ошибок (есть известные pre-existing TS2554 в transclusion-спеках, не связаны). `jest html-embed.spec.ts` — 17/17. ## НЕ вошло (осознанно) - **#26** (transient-эмбед в окне collab-броадкаста до persist-strip) и **#29** (консервативный strip убирает админский эмбед при store не-админа) — в issue помечены как **accepted residual risk / intentional, documented in-code**. Фикс #26 (inbound strip до броадкаста) рискован для коллаба, #29 — намеренное fail-closed поведение. Не трогал без явного решения — оставил как задокументированный принятый риск. - **#27** (переписать gate-тесты с regex на реальное исполнение) — test-quality, отдельно. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Ghost added 2 commits 2026-06-20 21:53:29 +03:00

fix(html-embed): strip embeds at serve time on authenticated read paths (#28 ) 6a052b88b4

Completes the workspace htmlEmbed kill-switch. The public-share path already
strips at serve time when the toggle is OFF, but the authenticated read paths
(/info and /history/info) returned page/history content with embeds intact, so
a disabled feature kept executing for in-workspace view-only viewers until the
page was next saved. Now both paths resolve the workspace toggle and run
stripHtmlEmbedNodes when it's OFF (fail-closed on a missing workspace), before
any markdown/html format conversion. Admin-authored content only — completeness,
not privilege escalation. Injects WorkspaceRepo into PageController.

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

harden(html-embed): make stripHtmlEmbedNodes total with a root-type check (#30 ) 8ee4279d30

stripHtmlEmbedNodes only filtered children, so a (never-in-practice) bare
htmlEmbed root node would be returned as-is. Add a defensive root check that
returns an embed-free doc, making the helper total — it can never return a node
for which hasHtmlEmbedNode is true. Adds a unit test for the root case.

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

Ghost added 1 commit 2026-06-20 22:49:30 +03:00

test(html-embed): real-execution gate tests for create/duplicate/import (#27 ) 8191c37daa

The create/duplicate/import gate tests asserted gate presence via brittle
expect(SRC).toMatch(/regex/) over the source text plus a reimplemented
applyGate() stand-in, so a refactor could break the real gate while they still
passed. Rewrite both specs to execute the REAL methods (PageService.create /
duplicatePage; ImportService.importPage; FileImportTaskService.processGenericImport)
with each caller role and assert on the PERSISTED content via hasHtmlEmbedNode:
member -> stripped, admin/owner+toggle ON -> preserved, toggle OFF -> stripped
for everyone, unknown/missing role -> fail-closed. No source-regex assertions
remain.

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

Ghost added 2 commits 2026-06-20 23:20:14 +03:00

fix(html-embed): preserve admin's existing embed on a non-admin co-editor's store (#29 ) b7ea8c850e

The collab persist strip keyed to the storing connection's user, so when a
non-admin co-editor stored, it removed an admin's legitimately-authored embed
too (data loss). Now: toggle OFF still strips all (feature disabled); toggle ON
+ non-admin storer strips only NEWLY-introduced embeds and preserves those
already present in the persisted content (admin-vetted), via new helpers
collectHtmlEmbedSources + stripDisallowedHtmlEmbedNodes (identity = attrs.source,
already-vetted HTML). The ydoc reflect is now guarded by a deep-equal check so
an unrelated non-admin edit that touches no new embed doesn't churn the doc.
A non-admin still cannot add a new embed. Documents the allow-list TOCTOU
(best-effort snapshot read outside the lock; converges on next store).

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

fix(html-embed): shrink the collab broadcast window with an early onChange guard (#26 ) 424761753e

A non-admin's transient htmlEmbed could execute in other open editors until the
debounced (10s) onStoreDocument strip. Add a ~300ms onChange-debounced early
strip (guardHtmlEmbed) that converges the shared ydoc for everyone far sooner.

Safety-critical details:
- Scheduled from onChange ONLY for non-admins AND only when the workspace toggle
  is ON (cached per-document in onLoadDocument), so the common toggle-OFF case
  does zero extra work.
- guardHtmlEmbed does ALL async work (toggle + persisted allow-list read) FIRST,
  then performs fromYdoc -> strip -> fragment.delete -> applyUpdate in a single
  SYNCHRONOUS, await-free block, so no inbound Yjs update can interleave and a
  concurrent edit can never be clobbered. Bails if document.isDestroyed.
- Reuses the #29 preserve logic (admin-vetted embeds survive; only the non-admin's
  new ones are stripped). Loop-safe (corrective update has null origin -> no
  reschedule; post-strip no embed -> cheap no-op). Per-document timer cleared on
  unload. onStoreDocument stays the authoritative backstop.

The irreducible residual is only the very first inbound broadcast before the
debounce fires — Hocuspocus exposes no synchronous beforeBroadcast filter.

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

vvzvlad added 1 commit 2026-06-21 01:59:35 +03:00

Merge develop into fix/html-embed-hardening (#46 )

Test / test (pull_request) Has been cancelled

4bf6d9f36b

Resolve the html-embed.spec.ts conflict as a union: both #46 and #49 (already in
develop) added different test cases to the same file. Keep all of them —
stripHtmlEmbedNodes gets #46's root-node case plus develop's deeply-nested,
non-object and empty-content cases; #46's collectHtmlEmbedSources and
stripDisallowedHtmlEmbedNodes suites and develop's hasHtmlEmbedNode suite all
kept; imports unioned. No production code conflicted.

Full suite green: server 651, client (16 files), editor-ext 56, mcp 247.

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

vvzvlad referenced this issue from a commit

2026-06-21 01:59:35 +03:00

Merge develop into fix/html-embed-hardening (#46)

Ghost merged commit 881610f5df into develop

2026-06-21 01:59:41 +03:00

Ghost referenced this issue from a commit

2026-06-21 01:59:42 +03:00

Merge pull request 'fix(html-embed): complete kill-switch on read paths (#28) + total strip helper (#30)' (#46) from fix/html-embed-hardening into develop

~~Ghost referenced this pull request 2026-06-21 02:05:02 +03:00~~

html-embed: complete the kill-switch — strip embeds at serve time on all read paths (not just shares) #28

~~Ghost referenced this pull request 2026-06-21 02:05:22 +03:00~~

html-embed: minor hardening (strip root-node type-check; document client read-only relies solely on server strip) #30