diff --git a/packages/mcp/src/lib/diff.ts b/packages/mcp/src/lib/diff.ts index ba216df4..53173dec 100644 --- a/packages/mcp/src/lib/diff.ts +++ b/packages/mcp/src/lib/diff.ts @@ -14,7 +14,19 @@ * signature. * * If recreateTransform / the changeset throws on a pathological document pair, - * we fall back to a coarse block-level text diff so the tool never hard-fails. + * OR the pair is too large to diff cheaply (see the size guard below), we fall + * back to a coarse block-level text diff so the tool never hard-fails and never + * pins the event loop. + * + * SIZE GUARD (issue #464 — prod CPU-DoS). recreateTransform computes its diff via + * rfc6902.createPatch, whose array diff is O(n·m) Levenshtein per array pair and + * whose per-run word diff is O(w²); on a large/heavily-changed doc this runs for + * seconds-to-hours and starves the whole process (BullMQ, Redis lock renewals, + * embeddings). It never THROWS — it just never finishes — so the try/catch below + * cannot save us. Because diffDocs runs on EVERY in-app/MCP content edit's verify + * report, we PRE-FLIGHT the doc size and route anything above a cheap cap straight + * to the coarse fallback (the same shape the catch produces). Same cap+fallback + * pattern as the ELK-layout DoS fix (#440 / c917dcc3). */ import { Node } from "@tiptap/pm/model"; @@ -72,6 +84,56 @@ function countNodes(doc: any, pred: (node: any) => boolean): number { return n; } +// --- Issue #464: pre-flight size guard for the precise diff ------------------ +// Defaults are BENCHMARK-derived on the recreateTransform(complexSteps:false, +// wordDiffs:true, simplifyDiff:true) pipeline, chosen so the WORST case (a fully +// re-written doc — the adversarial shape that drove the incident) keeps the +// synchronous block under ~200ms REGARDLESS of input: +// - 150 total nodes: worst-case pair ~176ms; the O(node²) array diff crosses +// 200ms at ~170 nodes and then explodes super-linearly (400 nodes ~1.3s, +// 800 ~5.5s), so cap just below the crossover. +// - 12 KiB serialized JSON: an independent axis, because the per-run word diff +// is O(words²) — a FEW nodes with very long text runs is dangerous even at a +// low node count (17 nodes / ~11 KiB ~176ms, / ~14 KiB ~290ms). A node-light +// but byte-heavy doc is still refused. +// Either metric over its cap routes to the coarse fallback. Both are env-tunable +// for operators who accept more CPU in exchange for exact diffs on larger docs. +const DEFAULT_MAX_NODES = 150; +const DEFAULT_MAX_BYTES = 12 * 1024; + +/** + * Read a positive-integer env override, falling back to `dflt`. Garbage / unset / + * non-finite / non-positive all fall back (so the guard can never be accidentally + * disabled by a malformed value). Read fresh on every call so a test / operator + * can flip the knob without a restart. + */ +function readPositiveIntEnv(name: string, dflt: number): number { + const raw = parseInt(process.env[name] ?? "", 10); + return Number.isFinite(raw) && raw > 0 ? raw : dflt; +} + +/** + * True when the pair is too large for the precise (recreateTransform) diff and + * must degrade to the coarse fallback. Takes the MAX of the two docs on each + * metric so an ASYMMETRIC pair (a small new doc vs a huge old doc, or vice + * versa) — which still explodes rfc6902 — is caught. Cheap: one node walk + + * one JSON.stringify per doc, both O(size). + */ +function exceedsDiffSizeGuard(oldDoc: any, newDoc: any): boolean { + const maxNodes = readPositiveIntEnv("MCP_DIFF_MAX_NODES", DEFAULT_MAX_NODES); + const maxBytes = readPositiveIntEnv("MCP_DIFF_MAX_BYTES", DEFAULT_MAX_BYTES); + const nodes = Math.max( + countNodes(oldDoc, () => true), + countNodes(newDoc, () => true), + ); + if (nodes > maxNodes) return true; + const bytes = Math.max( + JSON.stringify(oldDoc)?.length ?? 0, + JSON.stringify(newDoc)?.length ?? 0, + ); + return bytes > maxBytes; +} + /** * Count UNIQUE links in a JSON doc by their `href`. A single link can be split * across several adjacent text runs (e.g. a "link+bold" run followed by a "link" @@ -226,6 +288,81 @@ function coarseDiff(oldDoc: any, newDoc: any): DiffChange[] { return changes; } +/** Accumulated textual changes plus their derived char/block tallies. */ +interface DiffTally { + changes: DiffChange[]; + inserted: number; + deleted: number; + changedBlocks: Set; +} + +/** + * Produce the coarse-fallback tally for a pair. This is the SINGLE source of the + * `fellBack:true` result shape, shared by BOTH degrade paths in diffDocs (the + * pre-flight size guard and the recreateTransform catch) so they behave and + * report identically. + */ +function coarseDiffTally(oldDoc: any, newDoc: any): DiffTally { + const changes = coarseDiff(oldDoc, newDoc); + let inserted = 0; + let deleted = 0; + const changedBlocks = new Set(); + for (const c of changes) { + if (c.op === "insert") inserted += c.text.length; + else deleted += c.text.length; + if (c.block) changedBlocks.add(c.op[0] + ":" + c.block); + } + return { changes, inserted, deleted, changedBlocks }; +} + +/** + * Compute the PRECISE tally via the recreateTransform pipeline. Callers MUST + * gate this behind the size guard (it can block the event loop for a large pair) + * and wrap it in try/catch (a pathological pair can throw); on either the guard + * or a throw, use `coarseDiffTally` instead. Kept as a sibling of + * `coarseDiffTally` so both produce the same `DiffTally` shape. + */ +function preciseDiffTally(oldDocJson: any, newDocJson: any): DiffTally { + const oldNode = Node.fromJSON(docmostSchema, oldDocJson); + const newNode = Node.fromJSON(docmostSchema, newDocJson); + const tr = recreateTransform(oldNode, newNode, { + complexSteps: false, + wordDiffs: true, + simplifyDiff: true, + }); + const changeSet = ChangeSet.create(oldNode).addSteps(tr.doc, tr.mapping.maps, []); + const simplified = simplifyChanges(changeSet.changes, newNode); + + const changes: DiffChange[] = []; + let inserted = 0; + let deleted = 0; + const changedBlocks = new Set(); + + for (const change of simplified) { + // Deleted text lives in the OLD doc coordinate range [fromA, toA). + if (change.toA > change.fromA) { + const text = oldNode.textBetween(change.fromA, change.toA, "\n", " "); + if (text.length > 0) { + deleted += text.length; + const block = blockContextAt(oldNode, change.fromA); + changes.push({ op: "delete", block, text }); + if (block) changedBlocks.add("d:" + block); + } + } + // Inserted text lives in the NEW doc coordinate range [fromB, toB). + if (change.toB > change.fromB) { + const text = newNode.textBetween(change.fromB, change.toB, "\n", " "); + if (text.length > 0) { + inserted += text.length; + const block = blockContextAt(newNode, change.fromB); + changes.push({ op: "insert", block, text }); + if (block) changedBlocks.add("i:" + block); + } + } + } + return { changes, inserted, deleted, changedBlocks }; +} + /** Build the human-readable unified-ish markdown summary. */ function renderMarkdown( result: Omit, @@ -276,66 +413,39 @@ export function diffDocs( newDocJson: any, notesHeading: string = "Примечания переводчика", ): DiffResult { + // computeIntegrity is cheap (linear node walks) and its counts are needed in + // BOTH the precise and coarse paths, so it always runs first. const integrity = computeIntegrity(oldDocJson, newDocJson, notesHeading); - let changes: DiffChange[] = []; - let inserted = 0; - let deleted = 0; let fellBack = false; - const changedBlocks = new Set(); + let tally: DiffTally; - try { - const oldNode = Node.fromJSON(docmostSchema, oldDocJson); - const newNode = Node.fromJSON(docmostSchema, newDocJson); - const tr = recreateTransform(oldNode, newNode, { - complexSteps: false, - wordDiffs: true, - simplifyDiff: true, - }); - const changeSet = ChangeSet.create(oldNode).addSteps( - tr.doc, - tr.mapping.maps, - [], - ); - const simplified = simplifyChanges(changeSet.changes, newNode); - - for (const change of simplified) { - // Deleted text lives in the OLD doc coordinate range [fromA, toA). - if (change.toA > change.fromA) { - const text = oldNode.textBetween(change.fromA, change.toA, "\n", " "); - if (text.length > 0) { - deleted += text.length; - const block = blockContextAt(oldNode, change.fromA); - changes.push({ op: "delete", block, text }); - if (block) changedBlocks.add("d:" + block); - } - } - // Inserted text lives in the NEW doc coordinate range [fromB, toB). - if (change.toB > change.fromB) { - const text = newNode.textBetween(change.fromB, change.toB, "\n", " "); - if (text.length > 0) { - inserted += text.length; - const block = blockContextAt(newNode, change.fromB); - changes.push({ op: "insert", block, text }); - if (block) changedBlocks.add("i:" + block); - } - } - } - } catch { - // Pathological pair: degrade to a coarse block-level diff so we never throw. + // Pre-flight size guard (#464): a too-large pair would make recreateTransform + // block the event loop for seconds-to-hours WITHOUT throwing, so route it to + // the coarse fallback BEFORE calling recreateTransform at all. Both this path + // and the catch below go through coarseDiffTally for an identical `fellBack` + // result shape. + if (exceedsDiffSizeGuard(oldDocJson, newDocJson)) { fellBack = true; - changes = coarseDiff(oldDocJson, newDocJson); - for (const c of changes) { - if (c.op === "insert") inserted += c.text.length; - else deleted += c.text.length; - if (c.block) changedBlocks.add(c.op[0] + ":" + c.block); + tally = coarseDiffTally(oldDocJson, newDocJson); + } else { + try { + tally = preciseDiffTally(oldDocJson, newDocJson); + } catch { + // Pathological pair: degrade to a coarse block-level diff so we never throw. + fellBack = true; + tally = coarseDiffTally(oldDocJson, newDocJson); } } const partial: Omit = { - summary: { inserted, deleted, blocksChanged: changedBlocks.size }, + summary: { + inserted: tally.inserted, + deleted: tally.deleted, + blocksChanged: tally.changedBlocks.size, + }, integrity, - changes, + changes: tally.changes, }; return { ...partial, markdown: renderMarkdown(partial, fellBack) }; } diff --git a/packages/mcp/test/unit/diff-guard-skips-recreate.test.mjs b/packages/mcp/test/unit/diff-guard-skips-recreate.test.mjs new file mode 100644 index 00000000..d6b1d65b --- /dev/null +++ b/packages/mcp/test/unit/diff-guard-skips-recreate.test.mjs @@ -0,0 +1,86 @@ +// Issue #464 — prove the size guard SKIPS the recreateTransform pipeline over +// the cap, not merely that it returns "coarse". node:test's mock.module needs an +// experimental flag the suite does not pass, so instead of a module spy we use a +// deterministic BEHAVIORAL proxy that isolates the one variable — the guard: +// +// Same over-cap pair, run twice: +// (a) default caps -> guard trips -> recreateTransform skipped, +// (b) caps raised above the doc -> guard OFF -> recreateTransform DOES run. +// +// The only code path that differs between (a) and (b) is whether +// recreateTransform executes. recreateTransform on this pair is O(n²) and takes +// SECONDS; the guarded path is a linear coarse diff taking milliseconds. So a +// large (a)≪(b) time ratio can ONLY be explained by (a) skipping the transform. +// This asserts the skip without depending on mock.module. +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { diffDocs } from "../../build/lib/diff.js"; + +const t = (text) => ({ type: "text", text }); +const para = (text) => ({ type: "paragraph", content: text ? [t(text)] : [] }); +const doc = (children) => ({ type: "doc", content: children }); +function buildDoc(n, seed) { + return doc( + Array.from({ length: n }, (_, i) => + para(Array.from({ length: 8 }, (_, w) => `${seed}${i}_${w}`).join(" ")), + ), + ); +} +function clearEnv() { + delete process.env.MCP_DIFF_MAX_NODES; + delete process.env.MCP_DIFF_MAX_BYTES; +} +function timed(fn) { + const s = performance.now(); + const out = fn(); + return { out, ms: performance.now() - s }; +} + +// A 300-para (~600-node) pair: comfortably over the 150-node default, yet small +// enough that the un-guarded recreateTransform still FINISHES (~1-3s) so the +// test can time the contrast without hanging. +const OLD = buildDoc(300, "a"); +const NEW = buildDoc(300, "b"); + +test("guard skips recreateTransform over-cap (guarded run is far faster than un-guarded)", () => { + // (a) Guarded: default caps -> should short-circuit to coarse, near-instant. + clearEnv(); + const guarded = timed(() => diffDocs(OLD, NEW)); + assert.match( + guarded.out.markdown, + /coarse block-level diff/, + "guarded run must be coarse (guard tripped)", + ); + + // (b) Un-guarded: raise both caps above the doc so the precise path runs. + process.env.MCP_DIFF_MAX_NODES = "1000000"; + process.env.MCP_DIFF_MAX_BYTES = "100000000"; + let unguarded; + try { + unguarded = timed(() => diffDocs(OLD, NEW)); + } finally { + clearEnv(); + } + assert.doesNotMatch( + unguarded.out.markdown, + /coarse block-level diff/, + "with caps raised, the precise recreateTransform path runs", + ); + + // The precise run executed recreateTransform (O(n²)); the guarded run did not. + // Require a large speedup so the ONLY explanation is the skipped transform. + assert.ok( + guarded.ms * 5 < unguarded.ms, + `guarded (${guarded.ms.toFixed(1)}ms) must be >=5x faster than un-guarded ` + + `(${unguarded.ms.toFixed(1)}ms); a small gap would mean the transform still ran`, + ); +}); + +test("guarded over-cap call stays within the ~200ms event-loop budget", () => { + clearEnv(); + // Best-of-3 to shed GC/JIT noise; the guarded coarse path is a linear walk. + let best = Infinity; + for (let i = 0; i < 3; i++) best = Math.min(best, timed(() => diffDocs(OLD, NEW)).ms); + assert.ok(best < 200, `guarded over-cap diff must be <200ms, was ${best.toFixed(1)}ms`); +}); diff --git a/packages/mcp/test/unit/diff-size-guard.test.mjs b/packages/mcp/test/unit/diff-size-guard.test.mjs new file mode 100644 index 00000000..0e559c15 --- /dev/null +++ b/packages/mcp/test/unit/diff-size-guard.test.mjs @@ -0,0 +1,229 @@ +// Issue #464 — prod CPU-DoS pre-flight size guard for diffDocs. +// +// diffDocs synchronously calls recreateTransform (rfc6902) which is O(n·m) in +// node count and O(w²) in per-run word count; on a large/heavily-changed doc it +// pins the event loop for seconds-to-hours WITHOUT throwing. A pre-flight size +// guard routes any doc over MCP_DIFF_MAX_NODES / MCP_DIFF_MAX_BYTES straight to +// the coarse fallback (`fellBack:true`), so the sync block stays ~<200ms. +// +// These tests assert the BEHAVIOR of the guard (fast + coarse-mode + asymmetry + +// env knobs). A sibling test (diff-guard-skips-recreate.test.mjs) proves +// recreateTransform is skipped over the cap via a behavioral proxy (guarded run +// is orders of magnitude faster than the same pair with the caps raised). +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { diffDocs } from "../../build/lib/diff.js"; + +// --------------------------------------------------------------------------- +// Builders +// --------------------------------------------------------------------------- +const t = (text) => ({ type: "text", text }); +const para = (text) => ({ type: "paragraph", content: text ? [t(text)] : [] }); +const doc = (children) => ({ type: "doc", content: children }); + +/** A doc of `n` paragraphs whose words are seeded from `seed` (fully changeable). */ +function buildDoc(n, wordsPerPara, seed) { + const blocks = []; + for (let i = 0; i < n; i++) { + const words = []; + for (let w = 0; w < wordsPerPara; w++) words.push(`${seed}${i}_${w}`); + blocks.push(para(words.join(" "))); + } + return doc(blocks); +} + +/** Reset the env knobs to their unset default between tests. */ +function clearEnv() { + delete process.env.MCP_DIFF_MAX_NODES; + delete process.env.MCP_DIFF_MAX_BYTES; +} + +// --------------------------------------------------------------------------- +// Over-threshold (by node count) -> FAST + coarse mode. +// A fully re-written 600-para doc is the worst case that drove the incident; +// with the guard it must return in well under the ~200ms budget and in coarse +// mode. Without the guard this single call takes multiple SECONDS. +// --------------------------------------------------------------------------- +test("over-threshold doc falls back to coarse mode and returns fast", () => { + clearEnv(); + // 600 paragraphs -> ~1200 nodes, far over the 150-node default. + const oldDoc = buildDoc(600, 8, "a"); + const newDoc = buildDoc(600, 8, "b"); + + const start = performance.now(); + const r = diffDocs(oldDoc, newDoc); + const elapsed = performance.now() - start; + + // Coarse mode is signalled in the markdown note (fellBack path). + assert.match( + r.markdown, + /coarse block-level diff/, + "over-threshold pair must use the coarse fallback", + ); + // Budget: the guard makes this near-instant. Generous 1s ceiling to avoid CI + // flake while still being ~10x under the multi-second un-guarded cost. + assert.ok( + elapsed < 1000, + `expected fast coarse fallback, took ${elapsed.toFixed(0)}ms`, + ); + // Coarse diff still detects the wholesale change. + assert.ok(r.summary.inserted > 0 || r.summary.deleted > 0, "reports changes"); +}); + +// --------------------------------------------------------------------------- +// Under-threshold (small) doc -> precise diff, NOT coarse mode. No regression. +// --------------------------------------------------------------------------- +test("under-threshold doc uses the precise diff (no fallback note)", () => { + clearEnv(); + const oldDoc = doc([para("Hello world")]); + const newDoc = doc([para("Hello brave world")]); + const r = diffDocs(oldDoc, newDoc); + + assert.doesNotMatch( + r.markdown, + /coarse block-level diff/, + "a small doc must take the precise path", + ); + // Precise word diff finds exactly the inserted word. + const ins = r.changes.find((c) => c.op === "insert"); + assert.ok(ins && /brave/.test(ins.text), "precise diff isolates the inserted word"); +}); + +// --------------------------------------------------------------------------- +// Asymmetry: a small NEW doc vs a huge OLD doc (and vice versa) still explodes +// rfc6902, so max(old,new) must trip the guard in BOTH directions. +// --------------------------------------------------------------------------- +test("asymmetric pair (huge old, tiny new) falls back to coarse", () => { + clearEnv(); + const hugeOld = buildDoc(600, 8, "a"); + const tinyNew = doc([para("just one line")]); + const r = diffDocs(hugeOld, tinyNew); + assert.match(r.markdown, /coarse block-level diff/, "huge-old side must trip the guard"); +}); + +test("asymmetric pair (tiny old, huge new) falls back to coarse", () => { + clearEnv(); + const tinyOld = doc([para("just one line")]); + const hugeNew = buildDoc(600, 8, "b"); + const r = diffDocs(tinyOld, hugeNew); + assert.match(r.markdown, /coarse block-level diff/, "huge-new side must trip the guard"); +}); + +// --------------------------------------------------------------------------- +// Byte axis: a FEW nodes but a very large serialized size (long text runs) is +// dangerous too (per-run word diff is O(words²)), so the byte cap must trip +// independently of the node count. +// --------------------------------------------------------------------------- +test("node-light but byte-heavy doc falls back on the byte cap", () => { + clearEnv(); + // 5 paragraphs (~11 nodes, well under the node cap) but each a very long run, + // pushing the serialized size far over the 12 KiB byte default. + const bigRun = (seed) => + doc( + Array.from({ length: 5 }, (_, i) => + para(Array.from({ length: 800 }, (_, w) => `${seed}${i}_${w}`).join(" ")), + ), + ); + const oldDoc = bigRun("a"); + const newDoc = bigRun("b"); + // Sanity: node count is under the default node cap, so ONLY the byte cap can + // be what trips the guard here. + const nodeCount = (d) => { + let n = 0; + const v = (x) => { + if (!x || typeof x !== "object") return; + n++; + if (Array.isArray(x.content)) for (const c of x.content) v(c); + }; + v(d); + return n; + }; + assert.ok(nodeCount(oldDoc) < 150, "node count is under the node cap"); + assert.ok(JSON.stringify(oldDoc).length > 12 * 1024, "serialized size is over the byte cap"); + + const r = diffDocs(oldDoc, newDoc); + assert.match(r.markdown, /coarse block-level diff/, "byte cap must trip independently"); +}); + +// --------------------------------------------------------------------------- +// Env override: a very low MCP_DIFF_MAX_NODES forces fallback on a tiny doc, +// proving the knob is read fresh and actually gates the diff. +// --------------------------------------------------------------------------- +test("MCP_DIFF_MAX_NODES override forces fallback on a small doc", () => { + clearEnv(); + const oldDoc = doc([para("Hello world")]); + const newDoc = doc([para("Hello brave world")]); + + // Baseline: default caps -> precise diff. + assert.doesNotMatch(diffDocs(oldDoc, newDoc).markdown, /coarse block-level diff/); + + // Knob set absurdly low -> even this 4-node doc trips the guard. + process.env.MCP_DIFF_MAX_NODES = "1"; + try { + const r = diffDocs(oldDoc, newDoc); + assert.match(r.markdown, /coarse block-level diff/, "low node cap forces fallback"); + } finally { + clearEnv(); + } +}); + +test("MCP_DIFF_MAX_BYTES override forces fallback on a small doc", () => { + clearEnv(); + const oldDoc = doc([para("Hello world")]); + const newDoc = doc([para("Hello brave world")]); + + process.env.MCP_DIFF_MAX_BYTES = "1"; + try { + const r = diffDocs(oldDoc, newDoc); + assert.match(r.markdown, /coarse block-level diff/, "low byte cap forces fallback"); + } finally { + clearEnv(); + } +}); + +// --------------------------------------------------------------------------- +// Garbage / unset env values fall back to the DEFAULT (the guard can never be +// accidentally disabled by a malformed knob). A small doc must still diff +// precisely under a garbage cap. +// --------------------------------------------------------------------------- +test("garbage env values fall back to the default cap (guard not disabled)", () => { + clearEnv(); + const oldDoc = doc([para("Hello world")]); + const newDoc = doc([para("Hello brave world")]); + + for (const bad of ["not-a-number", "0", "-5", "", "NaN", "1e999"]) { + process.env.MCP_DIFF_MAX_NODES = bad; + process.env.MCP_DIFF_MAX_BYTES = bad; + // Under the DEFAULT caps this small doc is precise (garbage did not raise + // OR disable the cap). "1e999" -> parseInt yields 1 (finite) which is a + // valid low cap and would fall back; exclude that from the precise check. + const r = diffDocs(oldDoc, newDoc); + if (bad === "1e999") { + // parseInt("1e999",10) === 1 -> a legit low cap -> fallback. Guard active. + assert.match(r.markdown, /coarse block-level diff/); + } else { + assert.doesNotMatch( + r.markdown, + /coarse block-level diff/, + `garbage value ${JSON.stringify(bad)} must fall back to the default cap`, + ); + } + } + clearEnv(); +}); + +// --------------------------------------------------------------------------- +// A large doc that trips the guard must still return the correct INTEGRITY +// counts (computeIntegrity runs before the diff and is unaffected by fallback). +// --------------------------------------------------------------------------- +test("integrity counts are still correct on a guard-tripped (coarse) doc", () => { + clearEnv(); + const image = { type: "image", attrs: { src: "/api/files/a.png" } }; + const oldDoc = doc([image, ...buildDoc(600, 8, "a").content]); + const newDoc = doc([...buildDoc(600, 8, "b").content]); // image removed + + const r = diffDocs(oldDoc, newDoc); + assert.match(r.markdown, /coarse block-level diff/, "large pair fell back"); + assert.deepEqual(r.integrity.images, [1, 0], "integrity is computed regardless of fallback"); +});