mirror of
https://github.com/vxcontrol/pentagi.git
synced 2026-08-28 14:06:36 +00:00
fix(markdown-editor): close two table-pipe defects missed by prior reviews + hardening
A full-feature review surfaced two real defects both living in the file that fixes the table-pipe class — verified first-hand (repro + fix) through the real extension stack and live on docker: MEDIUM (data loss): escapeTablePipes escaped pipes only inside code spans and Go-template actions, NOT inside link/image destinations or bare autolink URLs. A cell like `[x](https://h/?a=1|2)` had its URL truncated at the pipe and the trailing cells silently dropped on the FIRST load — the exact H1 corruption class, uncovered for URLs. Fix: a URL_RUN pass escapes pipes inside any `scheme://` run (which never legitimately contains a space, so a `|` in it is always content, never a real separator). Live: the URL and trailing row now survive intact. MEDIUM (ReDoS): TABLE_DELIMITER_LINE `/…\|? *:?-+:? *(?:…)*\|? *$/` had two adjacent ` *` runs competing for the same characters, so a `|`-line followed by "dashes + a long space run + a non-matching tail" backtracked O(n²) (2.4s at 64k) on every parse — mount, external reset, paste. A crafted/AI-emitted doc froze the tab. Fix: a linear rewrite (trailing `(?: *\|)? *$`, per-cell spacing) — same matches, sub-millisecond on the pathological input. LOW: the imperative handle's insertAtCursor mutated a disabled editor (dispatch bypasses the editable gate) and dirtied the form — guard rich on `editor.isEditable`, raw on `disabled`. LOW (styleguide): `cs`→`computedStyle`, `looksLikeMarkdown`→`isMarkdownLike`, `endsTableBody`→`isTableBodyEnd`. Tests: URL-pipe round-trip (link/image/bare, converges) + a real-separator-safety case + a linear-regex ReDoS budget guard. 877 vitest green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
2e9eb96f75
commit
5eb4ea22ad
@@ -65,8 +65,10 @@ export function MarkdownEditorField({
|
||||
cycleToVariable: (variable) =>
|
||||
rawRef.current ? cycleTextareaToVariable(rawRef.current.textarea, variable) : false,
|
||||
focus: () => rawRef.current?.focus(),
|
||||
// A disabled field must not be mutated through the handle (a variable click mid-save
|
||||
// would splice the value and dirty the form); the rich branch guards on editor.isEditable.
|
||||
insertAtCursor: (text) => {
|
||||
if (rawRef.current) {
|
||||
if (!disabled && rawRef.current) {
|
||||
insertTextareaText(rawRef.current.textarea, text, onChange);
|
||||
}
|
||||
},
|
||||
@@ -76,7 +78,7 @@ export function MarkdownEditorField({
|
||||
focus: () => richRef.current?.focus(),
|
||||
insertAtCursor: (text) => richRef.current?.insertAtCursor(text),
|
||||
},
|
||||
[mode, onChange],
|
||||
[mode, onChange, disabled],
|
||||
);
|
||||
|
||||
if (mode === 'raw') {
|
||||
|
||||
@@ -20,7 +20,7 @@ const MARKDOWN_CUES = [
|
||||
/^>\s/m, // blockquote
|
||||
];
|
||||
|
||||
const looksLikeMarkdown = (text: string): boolean => MARKDOWN_CUES.some((cue) => cue.test(text));
|
||||
const isMarkdownLike = (text: string): boolean => MARKDOWN_CUES.some((cue) => cue.test(text));
|
||||
|
||||
export const shouldParseMarkdownOnPaste = (text: string, html: string, isCodeContext: boolean): boolean => {
|
||||
if (!text.trim() || isCodeContext) {
|
||||
@@ -36,7 +36,7 @@ export const shouldParseMarkdownOnPaste = (text: string, html: string, isCodeCon
|
||||
// through the schema, so bold/italic/links survive as marks. Markdown-looking text still wins the
|
||||
// markdown parse: a VS Code copy of markdown source arrives wrapped in syntax-color spans, and parsing
|
||||
// its text/plain (not the span noise) is the point of this plugin.
|
||||
if (html && !looksLikeMarkdown(text)) {
|
||||
if (html && !isMarkdownLike(text)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
|
||||
@@ -114,3 +114,44 @@ describe('pipe-less GFM tables (no outer pipe) — cells survive too', () => {
|
||||
expect(escapeTablePipes(src)).toBe(src);
|
||||
});
|
||||
});
|
||||
|
||||
describe('table cell with a pipe inside a URL — content survives load and converges', () => {
|
||||
it('escapes a pipe inside a link destination', () => {
|
||||
expect(escapeTablePipes('| A | B |\n| --- | --- |\n| [x](https://h/?a=1|2) | end |')).toBe(
|
||||
'| A | B |\n| --- | --- |\n| [x](https://h/?a=1\\|2) | end |',
|
||||
);
|
||||
});
|
||||
|
||||
it('escapes a pipe inside a bare autolink and an image src', () => {
|
||||
expect(escapeTablePipes('| A | B |\n| --- | --- |\n| https://h/?a=1|2 |  |')).toBe(
|
||||
'| A | B |\n| --- | --- |\n| https://h/?a=1\\|2 |  |',
|
||||
);
|
||||
});
|
||||
|
||||
it('leaves a real column delimiter (spaced pipe, no scheme run) untouched', () => {
|
||||
const table = '| A | B |\n| --- | --- |\n| http://h/x | plain |';
|
||||
|
||||
expect(escapeTablePipes(table)).toBe(table);
|
||||
});
|
||||
|
||||
it('keeps the URL and the trailing cell on round-trip, and converges', () => {
|
||||
const out = roundTrip('| A | B |\n| --- | --- |\n| [go](https://h/?x=1|2) | TRAILING |');
|
||||
|
||||
expect(out).toContain('TRAILING');
|
||||
expect(out).toContain('x=1\\|2');
|
||||
expect(roundTrip(out)).toBe(out);
|
||||
});
|
||||
});
|
||||
|
||||
describe('TABLE_DELIMITER_LINE is linear (ReDoS guard)', () => {
|
||||
it('scans a crafted delimiter-looking line with a long trailing space run in linear time', () => {
|
||||
// A `|`-line followed by "dashes + many spaces + non-matching tail" was O(n²) on the old regex
|
||||
// (~0.6s at 32k). The linear rewrite stays sub-millisecond; assert a generous budget.
|
||||
const evil = `x|y\n${'-'.repeat(50)}${' '.repeat(60000)}z\n`;
|
||||
const started = performance.now();
|
||||
|
||||
escapeTablePipes(evil);
|
||||
|
||||
expect(performance.now() - started).toBeLessThan(100);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
// marked's GFM table tokenizer splits every row on raw `|` BEFORE inline tokenization, so a pipe inside a
|
||||
// code span (`` `x | y` ``) or a Go-template action (`{{.X | upper}}`) in a cell creates a phantom
|
||||
// column and the trailing cells are silently DROPPED on load. The splitter does honor `\|` — and unescapes
|
||||
// it in the cell text — so pre-escaping those pipes before marked lexes protects the load side the same way
|
||||
// TunedTable's renderChildren escape protects the save side.
|
||||
// code span (`` `x | y` ``), a Go-template action (`{{.X | upper}}`), or a URL (`[x](http://a|b)`, a bare
|
||||
// `http://a|b`, an image src) in a cell creates a phantom column and the trailing cells are silently DROPPED
|
||||
// on load. The splitter does honor `\|` — and unescapes it in the cell text — so pre-escaping those pipes
|
||||
// before marked lexes protects the load side the same way TunedTable's renderChildren escape protects save.
|
||||
//
|
||||
// Scope must match EXACTLY the rows marked itself treats as the table — no more, no less:
|
||||
// • the header/delimiter pair must already parse as a table (matching cell counts, delimiter has |/:) —
|
||||
@@ -18,8 +18,14 @@
|
||||
// content, and our html tokenizers render such lines as literal text anyway).
|
||||
|
||||
const FENCE_LINE = /^ {0,3}(```|~~~)/;
|
||||
const TABLE_DELIMITER_LINE = /^ {0,3}\|? *:?-+:? *(?:\| *:?-+:? *)*\|? *$/;
|
||||
// Written to be linear: the trailing `(?: *\|)? *$` (not `\|? *$`) plus per-cell spacing keep any two space
|
||||
// runs from competing for the same characters, so a crafted delimiter-looking line can't force O(n²) backtracking.
|
||||
const TABLE_DELIMITER_LINE = /^ {0,3}\|? *:?-+:?(?: *\| *:?-+:?)*(?: *\|)? *$/;
|
||||
const TEMPLATE_ACTION = /\{\{[^{}]*\}\}/g;
|
||||
// A maximal non-space run containing a `scheme://` — a link/image destination or a bare autolink. It has no
|
||||
// spaces by construction, so any `|` in it is URL content (never a real cell separator, which needs spaces
|
||||
// around it or sits outside the run), and escaping it is always correct.
|
||||
const URL_RUN = /[^\s]*:\/\/[^\s]*/g;
|
||||
|
||||
// A table's body ends at a blank line or the first line that starts a different block — the same interrupts
|
||||
// marked's gfmTable body-row negative lookahead lists (heading, blockquote, fences, list, hr, indented code).
|
||||
@@ -33,7 +39,7 @@ const ENDS_TABLE_BODY = [
|
||||
/^(?: {4}| {0,3}\t)/,
|
||||
];
|
||||
|
||||
const endsTableBody = (line: string): boolean => ENDS_TABLE_BODY.some((rule) => rule.test(line));
|
||||
const isTableBodyEnd = (line: string): boolean => ENDS_TABLE_BODY.some((rule) => rule.test(line));
|
||||
|
||||
const isEscapedAt = (text: string, offset: number): boolean => {
|
||||
let isEscaped = false;
|
||||
@@ -121,7 +127,9 @@ const escapeRowPipes = (row: string): string => {
|
||||
index = closerStart + run.length;
|
||||
}
|
||||
|
||||
return result.replace(TEMPLATE_ACTION, (action) => escapeUnescapedPipes(action));
|
||||
return result
|
||||
.replace(TEMPLATE_ACTION, (action) => escapeUnescapedPipes(action))
|
||||
.replace(URL_RUN, (url) => escapeUnescapedPipes(url));
|
||||
};
|
||||
|
||||
export const escapeTablePipes = (markdown: string): string => {
|
||||
@@ -170,7 +178,7 @@ export const escapeTablePipes = (markdown: string): string => {
|
||||
// content. escapeRowPipes leaves structural pipes alone, so escaping the header can't skew its cell count.
|
||||
escapeRow(index);
|
||||
|
||||
for (let row = index + 2; row < lines.length && !endsTableBody(lines[row]!); row++) {
|
||||
for (let row = index + 2; row < lines.length && !isTableBodyEnd(lines[row]!); row++) {
|
||||
escapeRow(row);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -41,20 +41,20 @@ export function insertTextareaText(
|
||||
// Pixel offset of `position` from the textarea content top, measured via a hidden mirror so soft-wrapped
|
||||
// lines count — a logical-line count undershoots the scroll badly for wrapped templates.
|
||||
function caretOffsetTop(textarea: HTMLTextAreaElement, position: number): number {
|
||||
const cs = getComputedStyle(textarea);
|
||||
const computedStyle = getComputedStyle(textarea);
|
||||
const mirror = document.createElement('div');
|
||||
|
||||
mirror.style.fontFamily = cs.fontFamily;
|
||||
mirror.style.fontSize = cs.fontSize;
|
||||
mirror.style.fontWeight = cs.fontWeight;
|
||||
mirror.style.fontStyle = cs.fontStyle;
|
||||
mirror.style.lineHeight = cs.lineHeight;
|
||||
mirror.style.letterSpacing = cs.letterSpacing;
|
||||
mirror.style.wordSpacing = cs.wordSpacing;
|
||||
mirror.style.paddingTop = cs.paddingTop;
|
||||
mirror.style.paddingRight = cs.paddingRight;
|
||||
mirror.style.paddingBottom = cs.paddingBottom;
|
||||
mirror.style.paddingLeft = cs.paddingLeft;
|
||||
mirror.style.fontFamily = computedStyle.fontFamily;
|
||||
mirror.style.fontSize = computedStyle.fontSize;
|
||||
mirror.style.fontWeight = computedStyle.fontWeight;
|
||||
mirror.style.fontStyle = computedStyle.fontStyle;
|
||||
mirror.style.lineHeight = computedStyle.lineHeight;
|
||||
mirror.style.letterSpacing = computedStyle.letterSpacing;
|
||||
mirror.style.wordSpacing = computedStyle.wordSpacing;
|
||||
mirror.style.paddingTop = computedStyle.paddingTop;
|
||||
mirror.style.paddingRight = computedStyle.paddingRight;
|
||||
mirror.style.paddingBottom = computedStyle.paddingBottom;
|
||||
mirror.style.paddingLeft = computedStyle.paddingLeft;
|
||||
mirror.style.width = `${textarea.clientWidth}px`;
|
||||
mirror.style.boxSizing = 'border-box';
|
||||
mirror.style.whiteSpace = 'pre-wrap';
|
||||
|
||||
@@ -163,7 +163,10 @@ function MarkdownEditor({
|
||||
editor?.commands.focus();
|
||||
},
|
||||
insertAtCursor: (text: string) => {
|
||||
if (!editor) {
|
||||
// A disabled field must not be mutated through the handle — `dispatch` bypasses the editable
|
||||
// gate (which only blocks user input), so without this a variable click mid-save would edit
|
||||
// the doc and dirty the form.
|
||||
if (!editor || !editor.isEditable) {
|
||||
return;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user