Review #6 (approve-with-comments) follow-ups:
1. canonicalize step 7 now strips bare footnoteDefinitions at ANY depth
(stripFootnoteDefinitionsDeep), not just footnotesList, in BOTH copies. A
definition hand-authored outside a list (e.g. nested in a callout via a
raw-JSON write path) was left in place while a copy was also added to the
rebuilt list -> duplicate, idempotent, self-perpetuating. Runs only in the
rebuild path (after the lists are stripped); the fast-path / placement-keep
branch is untouched. Added a shared-corpus case (bare def nested in a callout)
to pin it in both mirrors.
2. markdown-clipboard: removed the dead top-level footnoteReference check in
canonicalizePastedFootnotes (an inline atom is never a top-level slice child;
only the descendants scan can find it).
Test coverage:
4. New MCP binding tests (full-doc-write-canonicalize.test.mjs): update_page_json
and copy_page_content canonicalize the persisted full doc, asserted via a new
`replacePage` seam (symmetric to the existing `mutatePage` seam) so no live
collab socket is needed. Routed both writers through the seam.
5. New server spec (file-import-task.service.footnote-canonicalize.spec.ts): the
zip-import path (processGenericImport) canonicalizes footnotes — real
markdown->HTML->JSON via a real ImportService over a temp-dir .md file, DB trx
stubbed to capture the persisted page content. FileImportTaskService had no
spec before.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Must-fix:
- Move canonicalizeFootnotes OUT of parseProsemirrorContent. It now runs only
on FULL writes (createPage, updatePageContent operation==='replace'), never on
an append/prepend fragment (a fragment would lose definition-only footnotes or
synthesize a bogus empty list). Add a server binding spec.
- Match the live plugin's list PLACEMENT: a single already-canonical
footnotesList is left exactly where it sits (the plugin never repositions a
sole correct list), so the first write no longer reorders content that follows
the list. Applied to BOTH the editor-ext copy and the MCP mirror; pinned by a
shared golden corpus case with content after the list.
- Fix MCP tool count 38 -> 39 (README x3, AGENTS.md) and the transformJs param
help (add canonicalizeFootnotes/insertInlineFootnote).
Simplifications:
- Remove the dead duplicate re-id mechanism (deriveFootnoteId/suffix/occurrence)
from the PURE canonicalizer in both copies — references are never renamed, so
the derived ids were never requested; first-wins-drop is the real behaviour.
This also makes the editor-ext footnote-util note about "no cross-package copy"
true again.
- Remove the sentinel round-trip in insertInlineFootnote: a generalized
insertNodesAfterAnchor core inserts the footnoteReference node directly.
- Drop the redundant per-definition deep clone in step 4 (shallow id-normalizing
copy; out is already deep-cloned).
Docs / architecture:
- Correct the editor-ext copy's "It exists because…" header to its real
consumers (server import, page.service create/update, client paste).
- Note markdownToProseMirror reuse for create/update comment in collaboration.ts.
- A: shared golden JSON corpus exercised by BOTH the editor-ext copy and the MCP
mirror (footnote-corpus.ts / .mjs) so "the two copies behave identically" is
checkable.
- C: split the MCP canonicalizer into a pure mirror + footnote-authoring.ts.
- B: import services persist via a different path, so left one-line consolidation
comments at the call sites rather than folding (does not fall out cleanly).
Tests: insertFootnote wrapper guards + docmost_transform dryRun auto-canonicalize
(MCP mock), page.service create/update + append/prepend binding (server jest),
shared corpus incl. nested-container reference.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The footnote canonicalizer was wired into the MCP and editor-ext write paths
but NOT into the server's user-facing markdown/HTML import paths, so importing
or pasting markdown with out-of-order, reused, or orphan footnotes did not
canonicalize -- the exact trigger bug #228 fixes was still reproduced on
import. markdownToHtml -> htmlToJson builds ProseMirror JSON directly and never
runs the editor's footnoteSyncPlugin, and that plugin does not reorder an
existing list, so the stored footnotes kept the source's physical definition
order, retained orphans, and did not collapse reused references.
Wire canonicalizeFootnotes (already exported from @docmost/editor-ext) into
every server markdown/HTML -> page-JSON seam, before persisting:
- ImportService.importPage (REST single-file .md/.html import)
- FileImportTaskService (zip import worker)
- PageService.parseProsemirrorContent (API createPage / updatePageContent)
Also hook the client markdown paste: handlePaste applies a manual transaction
(returns true), bypassing transformPasted/footnoteSyncPlugin, so a pasted
out-of-order markdown footnote block would persist out of order.
canonicalizePastedFootnotes reorders a self-contained pasted block (one that
carries its own footnotesList) to reference order, deduped and orphan-free; it
is deliberately scoped to whole-block pastes so a reference-only paste that
reuses a footnote already defined in the target doc is left untouched.
canonicalizeFootnotes is pure, idempotent and shape-safe (a doc with no
footnotes is unchanged), so it is safe on every write path.
Residual: when a pasted block merges into a doc that already has footnotes,
ordering relative to the pre-existing footnotes is still governed by the live
sync plugin (which does not reorder across the boundary).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Convert the htmlEmbed node from same-origin raw-HTML execution to a sandboxed
iframe (sandbox="allow-scripts allow-popups allow-forms", no allow-same-origin,
srcdoc) with postMessage auto-resize (validated by event.source) and an optional
manual height attr. The block now runs in an opaque origin and cannot reach the
viewer's cookies/session/API, so it is safe for any member.
Because the block is now harmless, remove the entire admin/role gating apparatus:
drop htmlEmbedAllowed/canAuthorHtmlEmbed/stripDisallowedHtmlEmbedNodes/
collectHtmlEmbedSources and every role-based strip on the write paths (collab
REST/MCP + socket, page create/duplicate, import x2, transclusion unsync), along
with the now-unused WorkspaceRepo/UserRepo injections and the PageService.create
callerRole param. Keep one strip: prepareContentForShare still removes htmlEmbed
on the anonymous public-share read path when the workspace master toggle is OFF.
The workspace settings.htmlEmbed toggle is now a plain feature switch (gates the
slash-menu and share rendering); when ON the block is available to all members.
Add settings.trackerHead: an admin-only raw HTML/JS analytics snippet injected
verbatim into the <head> of public share pages only (ShareSeoController), for
trackers that genuinely need same-origin. Admin-gated via the existing CASL
Manage/Settings ability; never injected into the authenticated app shell.
Closes security-review findings #1, #2, #4, #5, #10 (and #3 as a security issue).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- make addTreeNode receivers idempotent (invalidateOnCreatePage guard +
buildTree dedup) so the author's self-echo no longer duplicates the node
- broadcast realtime tree updates for bulk copy/duplicate and import via a
root refetch: PAGE_CREATED now carries spaceId and the WS listener falls
back to refetchRootTreeNodeEvent when no per-node snapshot is present
- remove the now-dead client-relay inbound path (isTreeEvent/handleTreeEvent)
that remained a stale-restriction-cache attack surface
- honest string|null cast for a root move's parent id
- add tests: buildTree dedup; onPageCreated per-node vs refetch branching
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The admin-only raw HTML/JS embed is a deliberate stored-XSS surface, so gate the
whole feature behind a workspace toggle that is OFF by default; it only works
when a workspace admin explicitly enables it.
- settings.htmlEmbed (boolean, default false) + workspace-update field htmlEmbed,
persisted via WorkspaceRepo.updateSetting with an audit diff. Flipping it is
admin-only (same Manage Settings CASL as other workspace toggles).
- New gate htmlEmbedAllowed(featureEnabled, role) = featureEnabled && admin/owner.
All 7 server write paths (create, duplicate, collab onStoreDocument, REST/MCP/AI
updatePageContent, single + zip import, transclusion unsync) now read the
workspace's settings.htmlEmbed and strip unless (toggle ON AND admin). OFF
(default, or a failed/empty workspace lookup) strips htmlEmbed for EVERYONE
including admins -> existing embeds are cleaned up on next save, none persist.
- Client (defense-in-depth): the /html slash item is hidden unless toggle ON +
admin; the NodeView executes nothing and shows a 'disabled in this workspace'
placeholder when OFF; an admin Switch in Workspace Settings -> General with a
description of the behavior.
- docs/html-embed-admin.md documents the toggle + admin-only + fail-closed
coedit (a non-admin save strips an admin's embed) + execution semantics.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Release-cycle test audit: the strip boundary was tested only via a stand-in
helper re-implemented in the spec, so a deleted/misplaced guard kept CI green
(the missing create() guard was proof). Replace it with tests against real code:
- persistence.extension.onStoreDocument: real ydoc from a rich doc (columns/
table/mention/htmlEmbed) -> non-admin strip removes only htmlEmbed, every other
node preserved (data-loss guard); admin keeps; empty fragment no-throw.
- collaboration.handler.updatePageContent: real path, user?.role gate, decoded
ydoc embed-free for non-admin, kept for admin.
- transclusion unsync: member stripped, admin preserved.
- editor-ext gains a vitest setup (was zero tests) + a markdown round-trip:
the <!--html-embed:BASE64--> marker -> htmlEmbed node with decoded source, and
hasHtmlEmbedNode matches it — pinning the marked/turndown shape the import
strip relies on. tsconfig now excludes specs from the shipped dist.
- Fail-closed identity: source-pinned contracts that the gate keys on
fileTask.creatorId (zip) / request userId (single) / callerRole (create) /
authUser.role (duplicate), and missing-user -> strip (services can't load under
jest's ESM graph; helpers replay the exact predicate).
Adds the verified-safe ^src/ jest moduleNameMapper (identical fail set).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds an htmlEmbed block node that renders and executes raw HTML/CSS/JS in the
wiki origin (e.g. an analytics tracker) — the owner-chosen variant C. Because
this is stored-XSS by design, only workspace admins/owners may get such a node
persisted; everyone executes it when reading.
- Node (editor-ext): htmlEmbed atom/isolating block; source stored base64 in
data-source for lossless HTML<->JSON round-trip. renderHTML emits only the
encoded marker (never inlines raw markup), so generateHTML/export/search are
not themselves injection vectors. Registered in BOTH client extensions and
server tiptapExtensions. Markdown round-trip via an <!--html-embed:b64-->
comment (turndown) + a marked rule.
- Client NodeView: injects source and re-creates <script> elements so they
actually run; edit modal; renders in read-only/share too. Slash item is
admin-gated (adminOnly filtered by the user's workspace role).
- SERVER ENFORCEMENT (the real control — UI gating alone is insufficient):
stripHtmlEmbedNodes() removes htmlEmbed from any document persisted by a
non-admin, applied at every write path that introduces content from an
untrusted author: collab onStoreDocument, REST/MCP/AI updatePageContent,
single-file import, zip/multi-file import, page duplication, and transclusion
unsync. Page restore introduces no new content. Public share/readonly viewers
render fetched (already-stripped) content and do NOT open a collab socket, so
the only residual is a transient broadcast window to concurrent authenticated
editors (documented).
Implements docs/arbitrary-html-embed-plan.md (variant C).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
These import paths relied on the private EE module that was deleted from
the repo. In the community build they either threw 'enterprise license'
(DOCX/PDF) or silently no-op'd (Confluence). The frontend buttons were
already removed in 38064064; this cleans up the dead backend stubs.
- import.service.ts: drop processDocx/processPdf methods, their dispatcher
branches, the pageId computation + insertPage spread, and the now-unused
moduleRef param/ModuleRef import
- file-import-task.service.ts: drop the Confluence branch and the now-unused
moduleRef param/ModuleRef import
- import.controller.ts: restrict file extensions to .md/.html and zip
sources to generic/notion; update the error message accordingly
- file.utils.ts: remove Confluence from the FileImportSource enum
- features.ts: remove the unused CONFLUENCE_IMPORT/DOCX_IMPORT/PDF_IMPORT
feature keys
The isConfluenceImport logic in import-attachment.service.ts is intentionally
left in place (real shared attachment-parsing code, not a stub); its removal
is a separate, riskier refactor.
The two catch blocks in importPage() threw an opaque "Error processing file
content" / "Failed to create imported page" BadRequest, hiding the real cause
from the HTTP response. This made a production 400 regression impossible to
diagnose without server log access, and violated the project convention that
errors must never be swallowed.
Extract `${err.name}: ${err.message}` into both the log (full err object kept
for the stack) and the thrown BadRequestException. Inner processMarkdown/
processHTML rethrowing catches and the EE processDocx/processPdf license
catches are left unchanged.
Local reproduction of the happy-dom 14->20 theory failed (full import chain
+ 22 edge cases pass on happy-dom@20.8.9), so the root cause is still pending
the now-visible reason from a recurring 400. Diagnostic script test-import.tsx
added; backlog doc updated with findings.
* add cursor pagination function
* support custom order modifier
* refactor returned object
* feat(db): migrate paginated endpoints to cursor-based pagination
* sync
* support hasPrevPage boolean
* feat(client): migrate pagination from offset to cursor-based
* support beforeCursor/prevCursor
* wrap search results in items array for API consistency
* feat(export): add metadata file to preserve page icons and ordering on import
- Export includes `docmost-metadata.json`
- Import reads metadata to restore icons and sort siblings by original position
* cleanup
* bonus fixes
* handle unknown prosemirror nodes
* add docmost app version
- Add explicit C collation to position ordering queries to fix incorrect page placement in PostgreSQL 17+
- Ensures consistent ASCII-based ordering regardless of database locale settings
- Fixes issue where new pages were incorrectly placed at random positions instead of bottom
