From d6d1195abdfc83d7f527b9590bd2b1da664cece3 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 03:05:24 +0300 Subject: [PATCH 1/7] refactor(#345 step 1): server markdown EXPORT via canonical converter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move every SERVER ProseMirror->Markdown path off the editor-ext markdown layer (`htmlToMarkdown`, a second turndown-based converter) onto the canonical `@docmost/prosemirror-markdown` package. - `ExportService.exportPage` (page/space markdown export) and `collaboration.util.jsonToMarkdown` (used by page.controller's markdown responses and the AI public-share chat tool) now serialize DIRECTLY from ProseMirror JSON via `convertProseMirrorToMarkdown` — no HTML intermediate, no `` scrub (the converter emits GFM tables directly). This is the SAME serializer the git-sync vault writer feeds, so an exported page BODY is byte-identical to its vault representation: no more export-md vs vault-md drift. The HTML export path is unchanged (still `jsonToHtml`). Emitted markdown moves to the canonical forms: callouts `> [!type]` (not `:::type`), inline footnotes `^[…]` (not `[^id]`), lossless images `![alt](src) ` (editor-ext dropped width/height/align). Fixtures-first: export-markdown.spec asserts those canonical forms and the export==vault-by-construction equality (both call the package converter). The one deliberate export/vault delta — export prepends the page title as an H1 while the vault carries it in frontmatter — is pinned by a test. Test infra: declare the `@docmost/prosemirror-markdown` workspace dep; teach jest to load its ESM build (babel-jest) and stub `@tiptap/react` (server code imports editor-ext, whose node views reference React renderers only used in a live browser editor — never on the server). Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/server/package.json | 8 +- .../src/collaboration/collaboration.util.ts | 10 +- .../export/export-markdown.spec.ts | 145 ++++++++++++++++++ .../src/integrations/export/export.service.ts | 18 ++- apps/server/test/stubs/tiptap-react.js | 13 ++ 5 files changed, 180 insertions(+), 14 deletions(-) create mode 100644 apps/server/src/integrations/export/export-markdown.spec.ts create mode 100644 apps/server/test/stubs/tiptap-react.js diff --git a/apps/server/package.json b/apps/server/package.json index 517f1b3d..86745b57 100644 --- a/apps/server/package.json +++ b/apps/server/package.json @@ -43,6 +43,7 @@ "@clickhouse/client": "^1.18.2", "@docmost/mcp": "workspace:*", "@docmost/pdf-inspector": "1.9.6", + "@docmost/prosemirror-markdown": "workspace:*", "@fastify/cookie": "^11.0.2", "@fastify/multipart": "^10.0.0", "@fastify/static": "^9.1.3", @@ -175,7 +176,7 @@ "/node_modules/" ], "transform": { - "happy-dom.+\\.js$": [ + "(happy-dom.+|prosemirror-markdown/build/.+)\\.js$": [ "babel-jest", { "presets": [ @@ -193,7 +194,7 @@ "^.+\\.(t|j)sx?$": "ts-jest" }, "transformIgnorePatterns": [ - "/node_modules/(?!(\\.pnpm/)?(nanoid|uuid|image-dimensions|marked|happy-dom|lib0)(@|/))" + "/node_modules/(?!(\\.pnpm/)?(nanoid|uuid|image-dimensions|marked|happy-dom|lib0|@docmost/prosemirror-markdown)(@|/))" ], "collectCoverageFrom": [ "**/*.(t|j)s" @@ -204,7 +205,8 @@ "^@docmost/db/(.*)$": "/database/$1", "^@docmost/transactional/(.*)$": "/integrations/transactional/$1", "^@docmost/ee/(.*)$": "/ee/$1", - "^src/(.*)$": "/$1" + "^src/(.*)$": "/$1", + "^@tiptap/react$": "/../test/stubs/tiptap-react.js" } } } diff --git a/apps/server/src/collaboration/collaboration.util.ts b/apps/server/src/collaboration/collaboration.util.ts index 7970051b..50e961ce 100644 --- a/apps/server/src/collaboration/collaboration.util.ts +++ b/apps/server/src/collaboration/collaboration.util.ts @@ -43,7 +43,6 @@ import { Column, Status, addUniqueIdsToDoc, - htmlToMarkdown, TransclusionSource, TransclusionReference, FootnoteReference, @@ -51,6 +50,7 @@ import { FootnoteDefinition, PageEmbed, } from '@docmost/editor-ext'; +import { convertProseMirrorToMarkdown } from '@docmost/prosemirror-markdown'; import { generateText, getSchema, JSONContent } from '@tiptap/core'; import { generateHTML, generateJSON } from '../common/helpers/prosemirror/html'; // @tiptap/html library works best for generating prosemirror json state but not HTML @@ -239,6 +239,10 @@ export function prosemirrorNodeToYElement(node: any): Y.XmlElement | Y.XmlText { } export function jsonToMarkdown(tiptapJson: any): string { - const html = jsonToHtml(tiptapJson); - return htmlToMarkdown(html); + // Direct ProseMirror JSON -> Markdown via the canonical converter + // (`@docmost/prosemirror-markdown`) — no HTML intermediate, no second + // editor-ext markdown layer. Same serializer as the page/space export and the + // git-sync vault writer, so every server PM->MD path emits identical canonical + // markdown (issue #345). + return convertProseMirrorToMarkdown(tiptapJson); } diff --git a/apps/server/src/integrations/export/export-markdown.spec.ts b/apps/server/src/integrations/export/export-markdown.spec.ts new file mode 100644 index 00000000..a242e6cb --- /dev/null +++ b/apps/server/src/integrations/export/export-markdown.spec.ts @@ -0,0 +1,145 @@ +// export.service.ts imports the ESM-only @sindresorhus/slugify (not in jest's +// transform allowlist). It is irrelevant to the markdown-serialization path under +// test (only used for page-mention link slugs on the DB path), so it is mocked +// out to keep the module graph loadable under ts-jest (mirrors the import specs). +jest.mock('@sindresorhus/slugify', () => ({ + __esModule: true, + default: (input: string) => String(input), +})); + +import { convertProseMirrorToMarkdown } from '@docmost/prosemirror-markdown'; +import { ExportService } from './export.service'; +import { ExportFormat } from './dto/export-dto'; + +/** + * STEP 1 golden test for issue #345: server MARKDOWN export runs DIRECTLY through + * the canonical converter (`convertProseMirrorToMarkdown`) — no HTML intermediate + * and no `@docmost/editor-ext` markdown layer — so the emitted markdown is in the + * canonical package forms and is byte-identical to the git-sync vault body. + * + * These are the goldens the swap has to satisfy: they assert the CANONICAL + * surface (callout `> [!type]`, inline footnote `^[…]`, lossless image + * ``) rather than the old editor-ext forms (`:::type`, `[^id]`, + * lossy `![alt](src)`). + * + * `exportPage(..., singlePage=false)` takes no DB path (no mention rewriting), so + * the service is constructed with null collaborators and only the pure + * PM -> Markdown path is exercised. + */ + +function makeService(): ExportService { + return new ExportService( + null as any, // pageRepo + null as any, // pagePermissionRepo + null as any, // db + null as any, // storageService + null as any, // environmentService + null as any, // domainService + ); +} + +// A representative page exercising the node types whose canonical markdown form +// changed with the move off the editor-ext layer: callout, inline footnote, and a +// lossless image carrying width/align attrs that the old layer dropped. +const REPRESENTATIVE_DOC = { + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { type: 'text', text: 'Body ' }, + { type: 'footnoteReference', attrs: { id: 'fn-1' } }, + { type: 'text', text: ' end.' }, + ], + }, + { + type: 'callout', + attrs: { type: 'info', icon: null }, + content: [ + { + type: 'paragraph', + content: [{ type: 'text', text: 'Heads up' }], + }, + ], + }, + { + type: 'image', + attrs: { + src: '/files/pic.png', + alt: 'Pic', + width: 320, + align: 'left', + }, + }, + { + type: 'footnotesList', + content: [ + { + type: 'footnoteDefinition', + attrs: { id: 'fn-1' }, + content: [ + { + type: 'paragraph', + content: [{ type: 'text', text: 'the note' }], + }, + ], + }, + ], + }, + ], +}; + +describe('ExportService — markdown export via the canonical converter (#345)', () => { + it('emits canonical callout, inline footnote and lossless image forms', async () => { + const service = makeService(); + const md = (await service.exportPage(ExportFormat.Markdown, { + title: '', + content: REPRESENTATIVE_DOC, + } as any)) as string; + + // Callout: Obsidian `> [!type]`, NOT the legacy `:::type`. + expect(md).toContain('> [!info]'); + expect(md).not.toContain(':::'); + + // Inline footnote: `^[…]`, NOT the reference `[^id]` form. + expect(md).toContain('^[the note]'); + expect(md).not.toMatch(/\[\^/); + + // Lossless image: trailing `` carrying the dropped attrs. + expect(md).toContain('![Pic](/files/pic.png)'); + expect(md).toContain('` images, …). + * Import, however, ingests FOREIGN files (GitHub/GFM, Notion, old Docmost + * exports). Those use surfaces the canonical parser does not accept, most notably + * GitHub-flavoured *reference* footnotes: + * + * Text with a note[^1] and another[^long]. + * + * [^1]: The first definition. + * [^long]: A second one. + * + * Left untouched, the parser does NOT recognise `[^id]` (it only parses `^[body]`), + * so the reference leaks as literal text — and worse, the trailing `[^id]: def` + * line is a valid CommonMark *link-reference definition*, so `[^id]` is silently + * rendered as a bogus link. This normalizer rewrites reference footnotes into the + * canonical inline form so the parser materialises real footnote nodes. + * + * This is a TEXT pre-pass, NOT a second parser fork: it does not re-implement any + * converter logic. Callout surfaces (`:::type` and `> [!type]`) are intentionally + * NOT touched here — the canonical parser already accepts BOTH natively (its + * `preprocessCallouts` pass), so normalizing them would be redundant and would + * only risk degrading the parser's nesting/code-fence-aware handling. + */ + +/** Matches a fenced code block delimiter (``` or ~~~), capturing the marker run. */ +const CODE_FENCE_RE = /^(\s*)(`{3,}|~{3,})/; + +/** + * Matches a GFM footnote DEFINITION line: `[^id]: body`. The id is any run of + * non-`]` characters; the body is the remainder of the line (possibly empty). + */ +const FOOTNOTE_DEF_RE = /^\[\^([^\]]+)\]:[ \t]?(.*)$/; + +/** True when a line is a code-fence delimiter that toggles fenced-code state. */ +function fenceMarker(line: string): string | null { + const m = line.match(CODE_FENCE_RE); + return m ? m[2] : null; +} + +/** True when a line is indented (leading space/tab) and not blank — a continuation. */ +function isIndentedContinuation(line: string): boolean { + return /^[ \t]+\S/.test(line); +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +/** + * Convert GFM reference footnotes (`[^id]` + `[^id]: def`) into canonical inline + * footnotes (`^[def]`). + * + * - Definitions are collected first (a leading `[^id]: text` line plus any + * immediately-following indented continuation lines, joined with a space) and + * removed from the output. + * - Each in-text reference `[^id]` for which a definition was found is replaced by + * `^[def]`. References with no matching definition are left literal (there is no + * body to inline; the parser fails them open the same way). + * - Code fences are respected on both passes: `[^id]` inside a ``` / ~~~ block is + * never rewritten, and a `[^id]:` line inside a fence is never treated as a + * definition. + * + * Deduplication / reference-ordering / orphan-dropping of the resulting footnotes + * is handled downstream by the canonical parser (`assembleFootnotes`); this pass + * only changes the surface syntax. + */ +function convertReferenceFootnotes(markdown: string): string { + const lines = markdown.split('\n'); + + // Pass 1: collect definitions and mark their lines for removal. + const defs = new Map(); + const dropped = new Array(lines.length).fill(false); + let inFence = false; + let fence = ''; + + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + const marker = fenceMarker(line); + if (inFence) { + if (marker && marker[0] === fence[0] && marker.length >= fence.length) { + inFence = false; + fence = ''; + } + continue; + } + if (marker) { + inFence = true; + fence = marker; + continue; + } + + const def = line.match(FOOTNOTE_DEF_RE); + if (!def) continue; + + const id = def[1]; + const body: string[] = [def[2].trim()]; + dropped[i] = true; + + // Consume immediately-following indented continuation lines (GFM lazy + // continuation is not supported by design — keep it simple and predictable). + let j = i + 1; + while (j < lines.length && isIndentedContinuation(lines[j])) { + body.push(lines[j].trim()); + dropped[j] = true; + j++; + } + i = j - 1; + + // Last definition wins for a duplicated id (matches CommonMark link-ref + // semantics closely enough for a foreign-input adapter). + defs.set(id, body.filter((s) => s.length > 0).join(' ')); + } + + if (defs.size === 0) { + return markdown; + } + + // Pass 2: rewrite in-text references, skipping fenced code and dropped lines. + const out: string[] = []; + inFence = false; + fence = ''; + for (let i = 0; i < lines.length; i++) { + if (dropped[i]) continue; + let line = lines[i]; + + const marker = fenceMarker(line); + if (inFence) { + out.push(line); + if (marker && marker[0] === fence[0] && marker.length >= fence.length) { + inFence = false; + fence = ''; + } + continue; + } + if (marker) { + inFence = true; + fence = marker; + out.push(line); + continue; + } + + for (const [id, body] of defs) { + const ref = new RegExp('\\[\\^' + escapeRegExp(id) + '\\]', 'g'); + line = line.replace(ref, `^[${body}]`); + } + out.push(line); + } + + return out.join('\n'); +} + +/** + * Normalize a foreign markdown string into Docmost's canonical markdown surface + * so the strict canonical parser accepts it losslessly. Currently this rewrites + * GFM reference footnotes into inline footnotes; add further fixture-driven + * foreign-surface cases here as they are found. + */ +export function normalizeForeignMarkdown(markdown: string): string { + if (!markdown) return markdown; + return convertReferenceFootnotes(markdown); +} diff --git a/apps/server/test/stubs/tiptap-react.js b/apps/server/test/stubs/tiptap-react.js index e11ef884..edd98738 100644 --- a/apps/server/test/stubs/tiptap-react.js +++ b/apps/server/test/stubs/tiptap-react.js @@ -1,11 +1,21 @@ -// Jest stub for @tiptap/react. The server export/import code paths transitively -// import editor-ext, whose node extensions reference `ReactNodeViewRenderer` -// inside `addNodeView()` — code that only runs inside a live browser editor and -// is NEVER invoked on the server. The real module eagerly pulls react-dom, which -// throws `navigator is not defined` under jest's node environment. This stub -// supplies the named exports the extensions bind at import time; if any were -// actually called on the server that would (correctly) surface as a test error. +// Jest stub for @tiptap/react. +// +// The server export/import code paths transitively import editor-ext, whose node +// extensions import from `@tiptap/react`. The real module re-exports all of +// `@tiptap/core` (headless, safe under node) AND adds React view helpers +// (`ReactNodeViewRenderer`, …) that eagerly pull in react-dom — which throws +// `navigator is not defined` under jest's node environment. +// +// So this stub DELEGATES to the real `@tiptap/core` (keeping `mergeAttributes`, +// `Node`, `Mark`, `nodeInputRule`, … working — they are used by +// `jsonToHtml`/`htmlToJson` on the server) and overrides ONLY the React view +// helpers with no-ops. Those helpers are referenced solely inside `addNodeView()` +// — code that runs only in a live browser editor, never on the server; if any +// were actually invoked here it would (correctly) surface as a test failure. +const core = require('@tiptap/core'); + module.exports = { + ...core, ReactNodeViewRenderer: () => () => ({}), NodeViewWrapper: () => null, NodeViewContent: () => null, -- 2.52.0 From 14172099155997c384ea54434becb6855c6132a6 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 03:27:01 +0300 Subject: [PATCH 3/7] fix(#345): drop stray prom-client dep + add prosemirror-markdown to the lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The step-1 package.json declared the new @docmost/prosemirror-markdown workspace dep but the lock was not regenerated (CI frozen install would fail), and it also added a stray prom-client dep (a coder env-workaround for a pre-existing hoisted import, unrelated to #345 — removed). Regenerated the lock with only the prosemirror-markdown dep; faithful frozen install now passes. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/server/package.json | 1 - pnpm-lock.yaml | 27 +++------------------------ 2 files changed, 3 insertions(+), 25 deletions(-) diff --git a/apps/server/package.json b/apps/server/package.json index 86745b57..dd9b7c03 100644 --- a/apps/server/package.json +++ b/apps/server/package.json @@ -112,7 +112,6 @@ "pino-pretty": "^13.1.3", "postgres": "^3.4.8", "postmark": "^4.0.7", - "prom-client": "^15.1.3", "react": "^18.3.1", "react-email": "6.0.8", "reflect-metadata": "^0.2.2", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0d99a612..9bedf2e5 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -543,6 +543,9 @@ importers: '@docmost/pdf-inspector': specifier: 1.9.6 version: 1.9.6 + '@docmost/prosemirror-markdown': + specifier: workspace:* + version: link:../../packages/prosemirror-markdown '@fastify/cookie': specifier: ^11.0.2 version: 11.0.2 @@ -747,9 +750,6 @@ importers: postmark: specifier: ^4.0.7 version: 4.0.7 - prom-client: - specifier: ^15.1.3 - version: 15.1.3 react: specifier: ^18.3.1 version: 18.3.1 @@ -5994,9 +5994,6 @@ packages: bind-event-listener@3.0.0: resolution: {integrity: sha512-PJvH288AWQhKs2v9zyfYdPzlPqf5bXbGMmhmUIY9x4dAUGIWgomO771oBQNwJnMQSnUIXhKu6sgzpBRXTlvb8Q==} - bintrees@1.0.2: - resolution: {integrity: sha512-VOMgTMwjAaUG580SXn3LacVgjurrbMme7ZZNYGSSV7mmtY6QQRh0Eg3pwIcntQ77DErK1L0NxkbetjcoXzVwKw==} - bl@4.1.0: resolution: {integrity: sha512-1W07cM9gS6DcLperZfFSj+bWLtaPGSOHWhPiGzXmvVJbRLdG82sH/Kn8EtW1VqWVA54AKf2h5k5BbnIbwF3h6w==} @@ -9327,10 +9324,6 @@ packages: process-warning@5.0.0: resolution: {integrity: sha512-a39t9ApHNx2L4+HBnQKqxxHNs1r7KF+Intd8Q/g1bUh6q0WIp9voPXJ/x0j+ZL45KF1pJd9+q2jLIRMfvEshkA==} - prom-client@15.1.3: - resolution: {integrity: sha512-6ZiOBfCywsD4k1BN9IX0uZhF+tJkV8q8llP64G5Hajs4JOeVLPCwpPVcpXy3BwYiUGgyJzsJJQeOIv7+hDSq8g==} - engines: {node: ^16 || ^18 || >=20} - prompts@2.4.2: resolution: {integrity: sha512-NxNv/kLguCA7p3jE8oL2aEBsrJWgAakBpgmgK6lpPWV+WuOmY6r2/zbAVnP+T8bQlA0nzHXSJSJW0Hq7ylaD2Q==} engines: {node: '>= 6'} @@ -10158,9 +10151,6 @@ packages: resolution: {integrity: sha512-4LeEWl96twnS2Q7Bz4MGqgazLqO+hJN63GZxXoIqh1T3VweYD997gbU1ItNsQafqqXTXd5WFyFdReLtwvRBNiw==} engines: {node: '>=18'} - tdigest@0.1.2: - resolution: {integrity: sha512-+G0LLgjjo9BZX2MfdvPfH+MKLCrxlXSYec5DaPYP1fe6Iyhf0/fSmJ0bFiZ1F8BT6cGXl2LpltQptzjXKWEkKA==} - terser-webpack-plugin@5.4.0: resolution: {integrity: sha512-Bn5vxm48flOIfkdl5CaD2+1CiUVbonWQ3KQPyP7/EuIl9Gbzq/gQFOzaMFUEgVjB1396tcK0SG8XcNJ/2kDH8g==} engines: {node: '>= 10.13.0'} @@ -16681,8 +16671,6 @@ snapshots: bind-event-listener@3.0.0: {} - bintrees@1.0.2: {} - bl@4.1.0: dependencies: buffer: 5.7.1 @@ -20494,11 +20482,6 @@ snapshots: process-warning@5.0.0: {} - prom-client@15.1.3: - dependencies: - '@opentelemetry/api': 1.9.0 - tdigest: 0.1.2 - prompts@2.4.2: dependencies: kleur: 3.0.3 @@ -21544,10 +21527,6 @@ snapshots: minizlib: 3.1.0 yallist: 5.0.0 - tdigest@0.1.2: - dependencies: - bintrees: 1.0.2 - terser-webpack-plugin@5.4.0(@swc/core@1.5.25(@swc/helpers@0.5.5))(webpack@5.106.0(@swc/core@1.5.25(@swc/helpers@0.5.5))): dependencies: '@jridgewell/trace-mapping': 0.3.31 -- 2.52.0 From 2c2d60a5dcc0fc308ffd0ee2bb664757c7c2199a Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 03:39:01 +0300 Subject: [PATCH 4/7] fix(#345): protect inline-code refs and escape footnote-body brackets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The foreign-markdown import normalizer rewrote GFM reference footnotes (`[^id]` + `[^id]: def`) into canonical inline `^[def]` footnotes, but two edge cases corrupted content: 1. A `[^id]` inside an inline-code span (backticks) was rewritten like prose text — only fenced code blocks were protected. Now the rewrite pass splits each line on inline-code spans and only touches the text outside them. 2. An unbalanced `]` in a definition body truncated the resulting `^[...]` footnote at the canonical tokenizer, leaking the tail as literal text. The body's square brackets are now backslash-escaped before wrapping. Adds golden cases for both. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../import/utils/foreign-markdown.spec.ts | 21 +++++++ .../import/utils/foreign-markdown.ts | 58 ++++++++++++++++--- 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts b/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts index fb22f1dd..94318075 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts +++ b/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts @@ -48,6 +48,27 @@ describe('normalizeForeignMarkdown — GFM reference footnotes', () => { expect(out).not.toContain('[^1]: def.'); }); + it('never rewrites a reference inside an INLINE-code span (backticks)', () => { + // The `[^1]` inside backticks is literal code and must survive verbatim; + // the one outside is rewritten. (Bug #1: only fenced blocks were protected.) + const out = normalizeForeignMarkdown( + 'Use `arr[^1]` in code but note[^1] in prose.\n\n[^1]: def.', + ); + expect(out).toBe('Use `arr[^1]` in code but note^[def.] in prose.\n'); + }); + + it('escapes brackets in a body so an unbalanced ] cannot truncate the footnote', () => { + // A foreign definition body with a stray `]` would, unescaped, close the + // canonical `^[...]` early and leak the tail as text (bug #2). The body's + // brackets are backslash-escaped so the footnote stays whole. + const out = normalizeForeignMarkdown( + 'Ref[^1] here.\n\n[^1]: see item ] and [more] later', + ); + expect(out).toBe('Ref^[see item \\] and \\[more\\] later] here.\n'); + // The tokenizer must see exactly one unescaped closing bracket (our own). + expect(out.match(/(? { const out = normalizeForeignMarkdown('Dangling[^x] ref.'); expect(out).toBe('Dangling[^x] ref.'); diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.ts b/apps/server/src/integrations/import/utils/foreign-markdown.ts index 6c8e99f6..531171fe 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.ts +++ b/apps/server/src/integrations/import/utils/foreign-markdown.ts @@ -52,6 +52,43 @@ function escapeRegExp(value: string): string { return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); } +/** + * Backslash-escape any square bracket in a footnote body before it is wrapped in + * `^[...]`. The canonical inline-footnote tokenizer scans the body with bracket + * balancing and closes on the first UNMATCHED `]`, so an unbalanced bracket in a + * foreign definition (e.g. `[^1]: see item ] later`) would otherwise truncate the + * footnote and leak the tail as literal text. Escaping every `[`/`]` makes the + * body an inert run of characters — the tokenizer then closes only on our own + * closing `]`. (A balanced `[link](url)` inside a body still round-trips because + * the escaped form renders the literal brackets, which is the safe reading for a + * footnote body; the alternative — brittle balance tracking — risks worse.) + */ +function escapeFootnoteBody(body: string): string { + return body.replace(/[[\]]/g, '\\$&'); +} + +/** + * Rewrite every `[^id]` reference on a line to its `^[body]` form, but ONLY in the + * text OUTSIDE inline-code spans. A `[^id]` inside backticks is literal code + * content and must be preserved verbatim (a footnote ref never lives inside code). + * We split the line on inline-code spans (paired backtick runs) and rewrite only + * the non-code segments. + */ +function rewriteRefsOutsideInlineCode( + line: string, + replace: (text: string) => string, +): string { + // Alternation: an inline-code span (one or more backticks, then anything up to + // the SAME run of backticks) OR a run of non-backtick text. Unterminated + // backticks fall through as ordinary text (matched by the second branch on the + // leftover), so a stray backtick never swallows the rest of the line. + const parts = line.match(/(`+)(?:(?!\1)[\s\S])*\1|[^`]+|`+/g); + if (!parts) return line; + return parts + .map((seg) => (seg.startsWith('`') ? seg : replace(seg))) + .join(''); +} + /** * Convert GFM reference footnotes (`[^id]` + `[^id]: def`) into canonical inline * footnotes (`^[def]`). @@ -62,9 +99,12 @@ function escapeRegExp(value: string): string { * - Each in-text reference `[^id]` for which a definition was found is replaced by * `^[def]`. References with no matching definition are left literal (there is no * body to inline; the parser fails them open the same way). - * - Code fences are respected on both passes: `[^id]` inside a ``` / ~~~ block is - * never rewritten, and a `[^id]:` line inside a fence is never treated as a - * definition. + * - Code is respected on both passes: `[^id]` inside a fenced ``` / ~~~ block is + * never rewritten and a `[^id]:` line inside a fence is never a definition; and + * on the rewrite pass a `[^id]` inside an INLINE-code span (backticks) is left + * literal too. + * - The inlined body is bracket-escaped so an unbalanced `[`/`]` in a foreign + * definition cannot truncate the resulting `^[...]` footnote. * * Deduplication / reference-ordering / orphan-dropping of the resulting footnotes * is handled downstream by the canonical parser (`assembleFootnotes`); this pass @@ -145,10 +185,14 @@ function convertReferenceFootnotes(markdown: string): string { continue; } - for (const [id, body] of defs) { - const ref = new RegExp('\\[\\^' + escapeRegExp(id) + '\\]', 'g'); - line = line.replace(ref, `^[${body}]`); - } + line = rewriteRefsOutsideInlineCode(line, (segment) => { + let s = segment; + for (const [id, body] of defs) { + const ref = new RegExp('\\[\\^' + escapeRegExp(id) + '\\]', 'g'); + s = s.replace(ref, `^[${escapeFootnoteBody(body)}]`); + } + return s; + }); out.push(line); } -- 2.52.0 From e17d5bc060f3ad9fa2994e5d50ad4d3c2cfe84cf Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 04:54:07 +0300 Subject: [PATCH 5/7] fix(#345): restore prom-client, harden normalizer against ReDoS, strip frontmatter (review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the round-1 review of #369: F1 [CRITICAL] Restore prom-client. The prior commit removed it as a 'stray dep', but metrics.registry.ts imports it unconditionally at startup (main.ts boot), so a clean frozen install had no prom-client -> server tsc TS2307 + boot crash. It was surviving only via hoisting from a warm store. Restored to apps/server dependencies + regenerated the lock (prom-client/tdigest/bintrees return), keeping the @docmost/prosemirror-markdown dep. Verified: clean frozen install -> require.resolve('prom-client') ok, server tsc EXIT 0. F2 [HIGH] Two quadratic ReDoS vectors in foreign-markdown.ts on untrusted import (runs synchronously on the request thread, 30MB cap): (a) pass-2 was O(lines x defs) — a per-def RegExp rebuilt and run over every line. Replaced with ONE precompiled alternation regex over all def ids, built once per document, with an id->body lookup in the replacer: O(text). (b) the inline-code split alternation backtracks quadratically on a long UNCLOSED backtick run. Lines over 8KB now skip the split (left untouched) — a real footnote line is never that long. F3 [WARNING] Restore the leading YAML front-matter strip that the retired markdownToHtml layer did. Without it, Obsidian/Hugo/Jekyll/git-sync files leak their front-matter into the body (and 'title:' renders as a setext heading that title extraction can hijack). F4 [WARNING] Extend the zip-import spec with an image (width+align) + callout fidelity assertion through the PM->HTML->PM hop (the one hop the package suite does not cover). F5/F6 Update AGENTS.md (apps/server is now a prosemirror-markdown consumer) and make the server pretest build prosemirror-markdown too. Co-Authored-By: Claude Opus 4.8 (1M context) --- AGENTS.md | 4 +- apps/server/package.json | 3 +- ...task.service.footnote-canonicalize.spec.ts | 191 +++++++++++------- .../import/utils/foreign-markdown.spec.ts | 43 ++++ .../import/utils/foreign-markdown.ts | 66 ++++-- pnpm-lock.yaml | 24 +++ 6 files changed, 240 insertions(+), 91 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a68d5d7a..4093d15c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -201,7 +201,7 @@ pnpm workspace (`pnpm@10.4.0`) orchestrated by **Nx**. Four workspace packages: | `apps/client` | `client` | React 18 + Vite + Mantine 8 + TanStack Query + Jotai | SPA frontend | | `packages/editor-ext` | `@docmost/editor-ext` | Tiptap/ProseMirror | Shared Tiptap node/mark extensions, imported by both the client and the server | | `packages/mcp` | `@docmost/mcp` | MCP SDK, Tiptap, Yjs | Standalone MCP server, also bundled into the server at `/mcp`. Consumes the shared converter/schema from `@docmost/prosemirror-markdown` (#293) — it no longer carries its own vendored converter/schema copy | -| `packages/prosemirror-markdown` | `@docmost/prosemirror-markdown` | Tiptap, marked, jsdom | The single, canonical ProseMirror↔Markdown converter + Docmost schema mirror (#293). Consumed by `mcp` and `git-sync`; there is exactly ONE copy of the converter now | +| `packages/prosemirror-markdown` | `@docmost/prosemirror-markdown` | Tiptap, marked, jsdom | The single, canonical ProseMirror↔Markdown converter + Docmost schema mirror (#293). Consumed by `mcp`, `git-sync`, AND `apps/server` (server-side markdown import/export, #345); there is exactly ONE copy of the converter now | `build` targets are Nx-cached and dependency-ordered (`dependsOn: ["^build"]`), so `editor-ext` builds before the apps. `nx.json` sets `affected.defaultBase: main`. @@ -284,7 +284,7 @@ The API server is a Fastify app with a global `/api` prefix (`main.ts` excludes ### Client structure Vite SPA. Code is organized by feature under `apps/client/src/features/*` (mirrors the server domains: `page`, `space`, `comment`, `ai-chat`, `editor`, …). Conventions: - **TanStack Query** for server state (one `queries/` file per feature), **Jotai** atoms for local/shared UI state, **Mantine 8** + CSS modules (`*.module.css`) + `postcss-preset-mantine` for UI. -- The editor is Tiptap; shared node/mark extensions live in `packages/editor-ext` and are imported by **both the client and the server** (collaboration, import/export) — editor schema changes often need to be made in `editor-ext`, not just the client. The ProseMirror↔Markdown converter and its Docmost schema mirror now live in a SINGLE package, `@docmost/prosemirror-markdown` (#293), consumed by both `mcp` and `git-sync` — do NOT reintroduce a per-package copy. `editor-ext` is the upstream source of the Tiptap schema; the package's `docmost-schema.ts` mirrors it and a serializer-contract test (`packages/prosemirror-markdown/test/serializer-contract.test.ts`) guards the boundary (every schema node must have a converter case), so a drift surfaces as a failing test rather than silent divergence. +- The editor is Tiptap; shared node/mark extensions live in `packages/editor-ext` and are imported by **both the client and the server** (collaboration, schema, `canonicalizeFootnotes`) — editor schema changes often need to be made in `editor-ext`, not just the client. Server-side markdown import/export no longer lives in `editor-ext`: it goes through the canonical converter (#345, see below). The ProseMirror↔Markdown converter and its Docmost schema mirror now live in a SINGLE package, `@docmost/prosemirror-markdown` (#293), consumed by `mcp`, `git-sync`, and `apps/server` (#345) — do NOT reintroduce a per-package copy. `editor-ext` is the upstream source of the Tiptap schema; the package's `docmost-schema.ts` mirrors it and a serializer-contract test (`packages/prosemirror-markdown/test/serializer-contract.test.ts`) guards the boundary (every schema node must have a converter case), so a drift surfaces as a failing test rather than silent divergence. - API access goes through `apps/client/src/lib/api-client.ts` (axios). The `@` alias maps to `apps/client/src`. - Runtime config is injected at build time by `vite.config.ts` via `define` (`APP_URL`, `COLLAB_URL`, `APP_VERSION`, …) — these come from the root `.env`, not from `import.meta.env`. diff --git a/apps/server/package.json b/apps/server/package.json index dd9b7c03..f12c1b2d 100644 --- a/apps/server/package.json +++ b/apps/server/package.json @@ -23,7 +23,7 @@ "migration:reset": "tsx src/database/migrate.ts down-to NO_MIGRATIONS", "migration:codegen": "kysely-codegen --dialect=postgres --camel-case --env-file=../../.env --out-file=./src/database/types/db.d.ts", "lint": "eslint \"{src,apps,libs,test}/**/*.ts\" --fix", - "pretest": "pnpm --filter @docmost/editor-ext build", + "pretest": "pnpm --filter @docmost/editor-ext build && pnpm --filter @docmost/prosemirror-markdown build", "test": "jest", "test:int": "jest --config test/jest-integration.json", "test:watch": "jest --watch", @@ -112,6 +112,7 @@ "pino-pretty": "^13.1.3", "postgres": "^3.4.8", "postmark": "^4.0.7", + "prom-client": "^15.1.3", "react": "^18.3.1", "react-email": "6.0.8", "reflect-metadata": "^0.2.2", diff --git a/apps/server/src/integrations/import/services/file-import-task.service.footnote-canonicalize.spec.ts b/apps/server/src/integrations/import/services/file-import-task.service.footnote-canonicalize.spec.ts index 10d36902..114999e9 100644 --- a/apps/server/src/integrations/import/services/file-import-task.service.footnote-canonicalize.spec.ts +++ b/apps/server/src/integrations/import/services/file-import-task.service.footnote-canonicalize.spec.ts @@ -91,88 +91,127 @@ function chainable(result: any): any { return proxy; } +/** + * Run one markdown file through the REAL zip-import pipeline + * (`processGenericImport` -> `markdownToProseMirror` -> `jsonToHtml` -> + * `processHTML`/`htmlToJson`) and return the persisted page `content`. This is + * the server-specific PM->HTML->PM hop that the package's own PM<->MD tests do + * NOT cover. + */ +async function runZipImport(markdown: string): Promise { + const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'fit-canon-')); + await fs.writeFile(path.join(extractDir, 'note.md'), markdown, 'utf-8'); + + const importService = new ImportService( + {} as any, + {} as any, + {} as any, + {} as any, + ); + jest + .spyOn(importService as any, 'createYdoc') + .mockResolvedValue(Buffer.from([]) as any); + + let captured: any = null; + const trx = { + insertInto: (table: string) => ({ + values: (v: any) => { + if (table === 'pages') captured = v; + return { execute: async () => {} }; + }, + }), + }; + const db: any = { + selectFrom: () => chainable({ slug: 'space-slug' }), + transaction: () => ({ execute: (fn: any) => fn(trx) }), + }; + + const importAttachmentService = { + processAttachments: async ({ html }: any) => html, + }; + const service = new FileImportTaskService( + {} as any, // storageService + importService as any, + { nextPagePosition: async () => 'a0' } as any, + { insertBacklink: jest.fn() } as any, + db, + importAttachmentService as any, + { emit: jest.fn() } as any, + { logBatchWithContext: jest.fn() } as any, + ); + + const fileTask: any = { + id: 'task-1', + source: 'generic', + spaceId: 'space-1', + workspaceId: 'ws-1', + creatorId: 'user-1', + }; + + try { + await service.processGenericImport({ extractDir, fileTask }); + expect(captured).toBeTruthy(); + return captured.content; + } finally { + await fs.rm(extractDir, { recursive: true, force: true }); + } +} + +/** Find the first node of a given type anywhere in a PM content tree. */ +function findFirst(node: any, type: string): any { + if (!node || typeof node !== 'object') return null; + if (node.type === type) return node; + for (const child of node.content ?? []) { + const hit = findFirst(child, type); + if (hit) return hit; + } + return null; +} + describe('FileImportTaskService.processGenericImport — footnote canonicalization (#228)', () => { it('orders footnotes by first reference, dedupes reuse, and drops orphans on zip import', async () => { - const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'fit-canon-')); - await fs.writeFile(path.join(extractDir, 'note.md'), MARKDOWN, 'utf-8'); - - // Real ImportService for the html -> JSON conversion; stub the yjs encode. - const importService = new ImportService( - {} as any, - {} as any, - {} as any, - {} as any, + const content = await runZipImport(MARKDOWN); + // Definitions ordered by FIRST REFERENCE (C, A, B), NOT the markdown + // definition order (A, B, C). Ids are the parser's fresh `fn-*`, so pin + // the BODIES. + expect(footnoteListBodies(content)).toEqual(['note C', 'note A', 'note B']); + // Orphan [^z] dropped; reused [^a] collapses to one definition; one list. + expect(footnoteListBodies(content)).not.toContain('orphan note'); + const lists = (content.content ?? []).filter( + (n: any) => n.type === 'footnotesList', ); - jest - .spyOn(importService as any, 'createYdoc') - .mockResolvedValue(Buffer.from([]) as any); + expect(lists).toHaveLength(1); + expect( + footnoteListBodies(content).filter((b) => b === 'note A'), + ).toHaveLength(1); + }); - let captured: any = null; - const trx = { - insertInto: (table: string) => ({ - values: (v: any) => { - if (table === 'pages') captured = v; - return { execute: async () => {} }; - }, - }), - }; - const db: any = { - selectFrom: () => chainable({ slug: 'space-slug' }), - transaction: () => ({ execute: (fn: any) => fn(trx) }), - }; + // #345 F4: the zip path routes markdown through jsonToHtml -> processHTML -> + // htmlToJson (the shared HTML attachment pipeline). #345's headline is LOSSLESS + // image width/align via the `` comment; a callout carries its + // `type`. This asserts those survive the PM->HTML->PM hop — the one hop the + // package's PM<->MD suite does not exercise. + it('preserves image width/align and callout type through the PM->HTML->PM hop', async () => { + const md = [ + '# Doc', + '', + '![a picture](https://example.com/i.png) ', + '', + ':::warning', + 'Careful now.', + ':::', + ].join('\n'); - const importAttachmentService = { - processAttachments: async ({ html }: any) => html, - }; - const backlinkRepo = { insertBacklink: jest.fn() }; - const eventEmitter = { emit: jest.fn() }; - const auditService = { logBatchWithContext: jest.fn() }; + const content = await runZipImport(md); - const pageService = { nextPagePosition: async () => 'a0' }; + const image = findFirst(content, 'image'); + expect(image).toBeTruthy(); + // The lossless sizing/alignment must survive the HTML hop. + expect(String(image.attrs?.width)).toBe('320'); + expect(image.attrs?.align).toBe('left'); - const service = new FileImportTaskService( - {} as any, // storageService - importService as any, - pageService as any, - backlinkRepo as any, - db, - importAttachmentService as any, - eventEmitter as any, - auditService as any, - ); - - const fileTask: any = { - id: 'task-1', - source: 'generic', - spaceId: 'space-1', - workspaceId: 'ws-1', - creatorId: 'user-1', - }; - - try { - await service.processGenericImport({ extractDir, fileTask }); - - expect(captured).toBeTruthy(); - const content = captured.content; - // Definitions ordered by FIRST REFERENCE (C, A, B), NOT the markdown - // definition order (A, B, C). Ids are the parser's fresh `fn-*`, so pin - // the BODIES. - expect(footnoteListBodies(content)).toEqual([ - 'note C', - 'note A', - 'note B', - ]); - // Orphan [^z] dropped; reused [^a] collapses to one definition; one list. - expect(footnoteListBodies(content)).not.toContain('orphan note'); - const lists = (content.content ?? []).filter( - (n: any) => n.type === 'footnotesList', - ); - expect(lists).toHaveLength(1); - expect( - footnoteListBodies(content).filter((b) => b === 'note A'), - ).toHaveLength(1); - } finally { - await fs.rm(extractDir, { recursive: true, force: true }); - } + const callout = findFirst(content, 'callout'); + expect(callout).toBeTruthy(); + expect(callout.attrs?.type).toBe('warning'); }); }); diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts b/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts index 94318075..0e50dd2c 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts +++ b/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts @@ -83,6 +83,49 @@ describe('normalizeForeignMarkdown — GFM reference footnotes', () => { const callouts = ':::info\nHi\n:::\n\n> [!warning]\n> Careful'; expect(normalizeForeignMarkdown(callouts)).toBe(callouts); }); + + it('strips a leading YAML front-matter block (Obsidian/Hugo/git-sync files)', () => { + const out = normalizeForeignMarkdown( + '---\ntitle: My Page\ntags: [a, b]\n---\n\n# Heading\n\nBody.', + ); + expect(out).toBe('# Heading\n\nBody.'); + // The front-matter must not leak into the body as a setext heading. + expect(out).not.toContain('title: My Page'); + expect(out).not.toContain('---'); + }); + + it('does not strip a horizontal rule that is not leading front-matter', () => { + const md = 'Intro paragraph.\n\n---\n\nAfter the rule.'; + expect(normalizeForeignMarkdown(md)).toBe(md); + }); + + it('is linear on a document with thousands of definitions (no quadratic blowup)', () => { + // F2(a): the pass-2 rewrite must be O(text), not O(text × defs). Build a + // pathological doc (many defs + many plain text lines) and assert it + // completes well under a second — a quadratic implementation took ~14s. + const N = 4000; + const refs = Array.from({ length: N }, (_, i) => `line ${i} plain text`).join('\n'); + const defs = Array.from({ length: N }, (_, i) => `[^n${i}]: def ${i}`).join('\n'); + const doc = `start[^n0] and[^n${N - 1}] end\n\n${refs}\n\n${defs}`; + const t0 = Date.now(); + const out = normalizeForeignMarkdown(doc); + const elapsed = Date.now() - t0; + expect(elapsed).toBeLessThan(2000); + // Sanity: the two real references were still inlined. + expect(out).toContain('^[def 0]'); + expect(out).toContain(`^[def ${N - 1}]`); + }); + + it('is bounded on a long unclosed backtick run (no inline-split ReDoS)', () => { + // F2(b): a huge unterminated backtick run must not cause quadratic + // backtracking in the inline-code split. Oversized lines skip the split + // entirely (left untouched), so this returns promptly. + const line = 'x' + '`'.repeat(200000); + const doc = `${line}\n\n[^1]: def`; + const t0 = Date.now(); + normalizeForeignMarkdown(doc); + expect(Date.now() - t0).toBeLessThan(2000); + }); }); describe('foreign markdown import acceptance (normalizer + canonical parser)', () => { diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.ts b/apps/server/src/integrations/import/utils/foreign-markdown.ts index 531171fe..f0079aa0 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.ts +++ b/apps/server/src/integrations/import/utils/foreign-markdown.ts @@ -74,10 +74,26 @@ function escapeFootnoteBody(body: string): string { * We split the line on inline-code spans (paired backtick runs) and rewrite only * the non-code segments. */ +// Above this length a single line is not split into inline-code spans (see +// below). A genuine markdown line carrying a footnote reference is never tens of +// KB; the cap only bypasses the inline-code protection for pathological lines. +const INLINE_SPLIT_MAX_LINE = 8192; + function rewriteRefsOutsideInlineCode( line: string, replace: (text: string) => string, ): string { + // The inline-code split alternation `(`+)(?:(?!\1)[\s\S])*\1` backtracks + // quadratically on a long UNCLOSED backtick run (its middle can consume the + // rest of the line, then fail to find a closing run and retry from each + // position). On an untrusted import this is a request-thread ReDoS. A real + // footnote line is short, so for an oversized line we skip the inline-code + // protection entirely and leave the line UNTOUCHED (rewriting it wholesale + // could corrupt a `[^id]` that legitimately lives inside inline code). This is + // a conservative bypass: an over-8KB line simply does not get its reference + // footnotes inlined — acceptable for a pathological input. + if (line.length > INLINE_SPLIT_MAX_LINE) return line; + // Alternation: an inline-code span (one or more backticks, then anything up to // the SAME run of backticks) OR a run of non-backtick text. Unterminated // backticks fall through as ordinary text (matched by the second branch on the @@ -161,6 +177,26 @@ function convertReferenceFootnotes(markdown: string): string { return markdown; } + // ONE precompiled alternation regex over ALL definition ids, built once per + // document (not once per definition per line). This makes pass 2 O(total text) + // instead of O(text × defs): a line with no reference pays a single failed + // scan, and the replacer looks the matched id up in `defs`. The previous + // per-def loop (`for (id) line.replace(new RegExp(...))`) was quadratic in the + // definition count — a modest upload with thousands of defs could freeze the + // request thread (and thus the whole instance, since import runs synchronously + // on it). The ids are escaped and joined; `defs` is the id→body lookup. + const refRe = new RegExp( + '\\[\\^(' + [...defs.keys()].map(escapeRegExp).join('|') + ')\\]', + 'g', + ); + const rewriteSegment = (segment: string): string => + segment.replace(refRe, (whole, id: string) => { + const body = defs.get(id); + // A ref whose id is not a real definition should not be reachable (the + // alternation only contains real ids), but stay defensive: leave it as-is. + return body === undefined ? whole : `^[${escapeFootnoteBody(body)}]`; + }); + // Pass 2: rewrite in-text references, skipping fenced code and dropped lines. const out: string[] = []; inFence = false; @@ -185,27 +221,33 @@ function convertReferenceFootnotes(markdown: string): string { continue; } - line = rewriteRefsOutsideInlineCode(line, (segment) => { - let s = segment; - for (const [id, body] of defs) { - const ref = new RegExp('\\[\\^' + escapeRegExp(id) + '\\]', 'g'); - s = s.replace(ref, `^[${escapeFootnoteBody(body)}]`); - } - return s; - }); + line = rewriteRefsOutsideInlineCode(line, rewriteSegment); out.push(line); } return out.join('\n'); } +/** + * Strip a single leading YAML front-matter block (`---\n…\n---`). Foreign files + * from Obsidian / Hugo / Jekyll / Notion — and Docmost's OWN git-sync page files + * — open with front-matter that the canonical parser does not consume, so + * without this it leaks into the body (and `title: Foo` above the closing `---` + * renders as a setext `

` that `extractTitleAndRemoveHeading` can hijack as + * the page title). This mirrors the strip the retired `markdownToHtml` layer did + * (editor-ext marked.utils.ts). It is a no-op for front-matter-free input. + */ +const YAML_FRONT_MATTER_RE = /^\s*---[\s\S]*?---\s*/; + /** * Normalize a foreign markdown string into Docmost's canonical markdown surface - * so the strict canonical parser accepts it losslessly. Currently this rewrites - * GFM reference footnotes into inline footnotes; add further fixture-driven - * foreign-surface cases here as they are found. + * so the strict canonical parser accepts it losslessly: strip a leading YAML + * front-matter block, then rewrite GFM reference footnotes into inline + * footnotes. Add further fixture-driven foreign-surface cases here as they are + * found. */ export function normalizeForeignMarkdown(markdown: string): string { if (!markdown) return markdown; - return convertReferenceFootnotes(markdown); + const withoutFrontMatter = markdown.replace(YAML_FRONT_MATTER_RE, '').trimStart(); + return convertReferenceFootnotes(withoutFrontMatter); } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9bedf2e5..f70155a5 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -750,6 +750,9 @@ importers: postmark: specifier: ^4.0.7 version: 4.0.7 + prom-client: + specifier: ^15.1.3 + version: 15.1.3 react: specifier: ^18.3.1 version: 18.3.1 @@ -5994,6 +5997,9 @@ packages: bind-event-listener@3.0.0: resolution: {integrity: sha512-PJvH288AWQhKs2v9zyfYdPzlPqf5bXbGMmhmUIY9x4dAUGIWgomO771oBQNwJnMQSnUIXhKu6sgzpBRXTlvb8Q==} + bintrees@1.0.2: + resolution: {integrity: sha512-VOMgTMwjAaUG580SXn3LacVgjurrbMme7ZZNYGSSV7mmtY6QQRh0Eg3pwIcntQ77DErK1L0NxkbetjcoXzVwKw==} + bl@4.1.0: resolution: {integrity: sha512-1W07cM9gS6DcLperZfFSj+bWLtaPGSOHWhPiGzXmvVJbRLdG82sH/Kn8EtW1VqWVA54AKf2h5k5BbnIbwF3h6w==} @@ -9324,6 +9330,10 @@ packages: process-warning@5.0.0: resolution: {integrity: sha512-a39t9ApHNx2L4+HBnQKqxxHNs1r7KF+Intd8Q/g1bUh6q0WIp9voPXJ/x0j+ZL45KF1pJd9+q2jLIRMfvEshkA==} + prom-client@15.1.3: + resolution: {integrity: sha512-6ZiOBfCywsD4k1BN9IX0uZhF+tJkV8q8llP64G5Hajs4JOeVLPCwpPVcpXy3BwYiUGgyJzsJJQeOIv7+hDSq8g==} + engines: {node: ^16 || ^18 || >=20} + prompts@2.4.2: resolution: {integrity: sha512-NxNv/kLguCA7p3jE8oL2aEBsrJWgAakBpgmgK6lpPWV+WuOmY6r2/zbAVnP+T8bQlA0nzHXSJSJW0Hq7ylaD2Q==} engines: {node: '>= 6'} @@ -10151,6 +10161,9 @@ packages: resolution: {integrity: sha512-4LeEWl96twnS2Q7Bz4MGqgazLqO+hJN63GZxXoIqh1T3VweYD997gbU1ItNsQafqqXTXd5WFyFdReLtwvRBNiw==} engines: {node: '>=18'} + tdigest@0.1.2: + resolution: {integrity: sha512-+G0LLgjjo9BZX2MfdvPfH+MKLCrxlXSYec5DaPYP1fe6Iyhf0/fSmJ0bFiZ1F8BT6cGXl2LpltQptzjXKWEkKA==} + terser-webpack-plugin@5.4.0: resolution: {integrity: sha512-Bn5vxm48flOIfkdl5CaD2+1CiUVbonWQ3KQPyP7/EuIl9Gbzq/gQFOzaMFUEgVjB1396tcK0SG8XcNJ/2kDH8g==} engines: {node: '>= 10.13.0'} @@ -16671,6 +16684,8 @@ snapshots: bind-event-listener@3.0.0: {} + bintrees@1.0.2: {} + bl@4.1.0: dependencies: buffer: 5.7.1 @@ -20482,6 +20497,11 @@ snapshots: process-warning@5.0.0: {} + prom-client@15.1.3: + dependencies: + '@opentelemetry/api': 1.9.0 + tdigest: 0.1.2 + prompts@2.4.2: dependencies: kleur: 3.0.3 @@ -21527,6 +21547,10 @@ snapshots: minizlib: 3.1.0 yallist: 5.0.0 + tdigest@0.1.2: + dependencies: + bintrees: 1.0.2 + terser-webpack-plugin@5.4.0(@swc/core@1.5.25(@swc/helpers@0.5.5))(webpack@5.106.0(@swc/core@1.5.25(@swc/helpers@0.5.5))): dependencies: '@jridgewell/trace-mapping': 0.3.31 -- 2.52.0 From 80fc30633b44acaa976801d5382dccb6fb5e6309 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 05:18:44 +0300 Subject: [PATCH 6/7] fix(#345): replace id-alternation regex with a fixed generic scanner + line-anchor frontmatter (review round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F7 [CRITICAL] The round-1 F2(a) fix built ONE alternation regex over all definition ids (`(id1|id2|...)`). On prefix-chain ids (a, aa, aaa, ...) V8's regex compiler blows its stack with a fatal, UNCATCHABLE 'RegExpCompiler Allocation failed' that kills the whole process — strictly worse than the original per-def thread-hang, and its match cost was still O(text x defs). Replaced with a single FIXED generic scanner `/\[\^([^\]]+)\]/g` plus a map lookup in the replacer: genuinely O(total text), no per-document regex compilation, cannot blow up. Output is identical (only real def ids are inlined). F8 [WARNING] The frontmatter strip regex was not line-anchored: it closed on the FIRST `---` anywhere, so a value containing a triple-dash (e.g. 'title: Q1 --- Q2') truncated the frontmatter and leaked the rest into the body. Replaced with the line-anchored shape the canonical parser already uses (page-file.ts): open on `---\n`, close on a `\n---` line. Adds tests: 4000 prefix-chain ids do not crash and stay fast; a frontmatter value containing '---' is stripped whole. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../import/utils/foreign-markdown.spec.ts | 29 ++++++++++++++ .../import/utils/foreign-markdown.ts | 40 +++++++++++-------- 2 files changed, 52 insertions(+), 17 deletions(-) diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts b/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts index 0e50dd2c..c0d8dac4 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts +++ b/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts @@ -126,6 +126,35 @@ describe('normalizeForeignMarkdown — GFM reference footnotes', () => { normalizeForeignMarkdown(doc); expect(Date.now() - t0).toBeLessThan(2000); }); + + it('does not crash or slow down on thousands of prefix-chain definition ids', () => { + // F7: the rewrite must use a FIXED generic scanner, not an alternation built + // from the ids. A `(a|aa|aaa|…)` alternation over prefix-chain ids blows the + // V8 regex compiler (FATAL RegExpCompiler Allocation failed — uncatchable, + // kills the process). A fixed scanner has no id-dependent compilation cost. + const N = 4000; + const ids = Array.from({ length: N }, (_, i) => 'a'.repeat(i + 1)); + const defs = ids.map((id) => `[^${id}]: body ${id.length}`).join('\n'); + const doc = `ref[^${ids[0]}] and[^${ids[N - 1]}] end\n\n${defs}`; + const t0 = Date.now(); + const out = normalizeForeignMarkdown(doc); + expect(Date.now() - t0).toBeLessThan(2000); + // Prefix disambiguation is correct: [^a] and [^aaaa...] inline their OWN body. + expect(out).toContain('^[body 1]'); + expect(out).toContain(`^[body ${N}]`); + }); + + it('strips front-matter whose value contains a triple-dash (line-anchored)', () => { + // F8: the block must close only on a `\n---` LINE, not the first inline + // `---`. A value like `title: Q1 --- Q2` must not truncate the front-matter + // and leak the rest (author/closing ---) into the body. + const out = normalizeForeignMarkdown( + '---\ntitle: Q1 --- Q2 results\nauthor: bob\n---\n\nReal body.', + ); + expect(out).toBe('Real body.'); + expect(out).not.toContain('author: bob'); + expect(out).not.toContain('Q2 results'); + }); }); describe('foreign markdown import acceptance (normalizer + canonical parser)', () => { diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.ts b/apps/server/src/integrations/import/utils/foreign-markdown.ts index f0079aa0..ef253a61 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.ts +++ b/apps/server/src/integrations/import/utils/foreign-markdown.ts @@ -177,23 +177,23 @@ function convertReferenceFootnotes(markdown: string): string { return markdown; } - // ONE precompiled alternation regex over ALL definition ids, built once per - // document (not once per definition per line). This makes pass 2 O(total text) - // instead of O(text × defs): a line with no reference pays a single failed - // scan, and the replacer looks the matched id up in `defs`. The previous - // per-def loop (`for (id) line.replace(new RegExp(...))`) was quadratic in the - // definition count — a modest upload with thousands of defs could freeze the - // request thread (and thus the whole instance, since import runs synchronously - // on it). The ids are escaped and joined; `defs` is the id→body lookup. - const refRe = new RegExp( - '\\[\\^(' + [...defs.keys()].map(escapeRegExp).join('|') + ')\\]', - 'g', - ); + // ONE fixed, generic scanner regex — NOT one built from the definition ids. + // It matches ANY `[^id]` shape, and the replacer decides per match via a map + // lookup whether that id is a real definition (replace) or not (leave as-is). + // This is genuinely O(total text) with no per-document regex compilation. + // + // Do NOT rebuild this as an alternation over `[...defs.keys()]`: a giant + // `(id1|id2|...)` alternation over thousands of ids can blow the V8 regex + // compiler's stack — a fatal, UNCATCHABLE "RegExpCompiler Allocation failed" + // on prefix-chain ids (`a`, `aa`, `aaa`, ...) that kills the whole process + // (worse than the earlier per-def thread-hang). A fixed scanner has no + // id-dependent compilation cost and cannot blow up. + const refRe = /\[\^([^\]]+)\]/g; const rewriteSegment = (segment: string): string => segment.replace(refRe, (whole, id: string) => { const body = defs.get(id); - // A ref whose id is not a real definition should not be reachable (the - // alternation only contains real ids), but stay defensive: leave it as-is. + // Only real definitions are inlined; an unknown id is left literal (same as + // the old per-def loop, which simply never matched it). return body === undefined ? whole : `^[${escapeFootnoteBody(body)}]`; }); @@ -234,10 +234,16 @@ function convertReferenceFootnotes(markdown: string): string { * — open with front-matter that the canonical parser does not consume, so * without this it leaks into the body (and `title: Foo` above the closing `---` * renders as a setext `

` that `extractTitleAndRemoveHeading` can hijack as - * the page title). This mirrors the strip the retired `markdownToHtml` layer did - * (editor-ext marked.utils.ts). It is a no-op for front-matter-free input. + * the page title). It is a no-op for front-matter-free input. + * + * LINE-ANCHORED (the same shape the canonical parser uses in + * prosemirror-markdown/page-file.ts): the block opens only on `---\n` at the + * very start and closes only on a `\n---` line. The retired `markdownToHtml` + * strip closed on the FIRST `---` ANYWHERE (an unanchored close), so a value + * containing a triple-dash (e.g. `title: Q1 --- Q2`) truncated the front-matter + * and leaked the rest into the body. An optional leading BOM is tolerated. */ -const YAML_FRONT_MATTER_RE = /^\s*---[\s\S]*?---\s*/; +const YAML_FRONT_MATTER_RE = /^\uFEFF?---\n[\s\S]*?\n---\n?/; /** * Normalize a foreign markdown string into Docmost's canonical markdown surface -- 2.52.0 From c5bff2d84ae76a3e58bbf1b55e7e5630c398f853 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 05:38:07 +0300 Subject: [PATCH 7/7] fix(#345): normalize CRLF before front-matter strip (review round 3) F9 [WARNING] The line-anchored front-matter regex from round 2 requires a bare LF after the opening `---`, so a Windows/CRLF foreign file (`---\r\n...`) slips past the strip and leaks its front-matter into the body (where `title: Foo` renders as a setext heading that title extraction hijacks). The canonical parser whose regex shape this copied (page-file.ts) normalizes CRLF -> LF BEFORE its FRONTMATTER_RE; the import path copied the regex but missed the normalization. normalizeForeignMarkdown now replaces CRLF with LF first (which also makes convertReferenceFootnotes' split('\n') consistent). Adds a CRLF fixture. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../import/utils/foreign-markdown.spec.ts | 12 ++++++++++++ .../import/utils/foreign-markdown.ts | 16 +++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts b/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts index c0d8dac4..8a43fa20 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts +++ b/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts @@ -144,6 +144,18 @@ describe('normalizeForeignMarkdown — GFM reference footnotes', () => { expect(out).toContain(`^[body ${N}]`); }); + it('strips a CRLF (Windows) front-matter block, not just LF', () => { + // F9: the line-anchored regex needs LF after the opening `---`, so a Windows + // file (`---\r\n…`) would slip past the strip and leak the front-matter into + // the body. normalizeForeignMarkdown normalizes CRLF -> LF first. + const out = normalizeForeignMarkdown( + '---\r\ntitle: Foo\r\ntags: [a]\r\n---\r\n\r\n# Heading\r\n\r\nBody.', + ); + expect(out).toBe('# Heading\n\nBody.'); + expect(out).not.toContain('title: Foo'); + expect(out).not.toContain('---'); + }); + it('strips front-matter whose value contains a triple-dash (line-anchored)', () => { // F8: the block must close only on a `\n---` LINE, not the first inline // `---`. A value like `title: Q1 --- Q2` must not truncate the front-matter diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.ts b/apps/server/src/integrations/import/utils/foreign-markdown.ts index ef253a61..2173bfdc 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.ts +++ b/apps/server/src/integrations/import/utils/foreign-markdown.ts @@ -247,13 +247,19 @@ const YAML_FRONT_MATTER_RE = /^\uFEFF?---\n[\s\S]*?\n---\n?/; /** * Normalize a foreign markdown string into Docmost's canonical markdown surface - * so the strict canonical parser accepts it losslessly: strip a leading YAML - * front-matter block, then rewrite GFM reference footnotes into inline - * footnotes. Add further fixture-driven foreign-surface cases here as they are - * found. + * so the strict canonical parser accepts it losslessly: normalize line endings, + * strip a leading YAML front-matter block, then rewrite GFM reference footnotes + * into inline footnotes. Add further fixture-driven foreign-surface cases here as + * they are found. */ export function normalizeForeignMarkdown(markdown: string): string { if (!markdown) return markdown; - const withoutFrontMatter = markdown.replace(YAML_FRONT_MATTER_RE, '').trimStart(); + // Normalize CRLF -> LF FIRST. The line-anchored front-matter regex requires a + // bare `\n` after the opening `---`, and convertReferenceFootnotes splits on + // `\n`; a Windows/CRLF foreign file (`---\r\n…`) would otherwise slip past the + // front-matter strip and leak into the body. The canonical parser + // (page-file.ts parsePageFile) normalizes the same way before its FRONTMATTER_RE. + const src = markdown.replace(/\r\n/g, '\n'); + const withoutFrontMatter = src.replace(YAML_FRONT_MATTER_RE, '').trimStart(); return convertReferenceFootnotes(withoutFrontMatter); } -- 2.52.0