diff --git a/src/runtime/utils/search.ts b/src/runtime/utils/search.ts index 6d4325fb..5d6c903f 100644 --- a/src/runtime/utils/search.ts +++ b/src/runtime/utils/search.ts @@ -225,20 +225,26 @@ function truncateHTMLFromStart(html: string, maxLength: number, fieldTextLength: * parameter would let a caller pass any tag through to the `v-html` that renders * the result. * + * Splitting on the tag is what makes that hold. The previous version swapped the + * tags for `\0markO\0`/`\0markC\0`, escaped, then swapped back — and NUL is an + * ordinary character a snippet can carry, so what decided whether markup was + * emitted was a string the input could supply (#391). + * + * A whole sentinel in the input forged a tag outright. Worse, and easier: six of + * its seven bytes immediately before a *real* tag were enough, because the + * placeholder this function inserted for that tag completed the prefix. So + * `\0markO` came back as `markO\0` — the genuine highlight moved to + * the front of the text it was meant to mark. No crafted sequence, one stray + * fragment ahead of any highlight. + * * @param snippet Snippet from the search index, with `` marking the hits. * @returns HTML safe to render, with the highlight tags intact. */ export function sanitizeSnippet(snippet: string): string { - const tagOpen = '\0markO\0' - const tagClose = '\0markC\0' - - return escapeHTML( - snippet - .replaceAll('', tagOpen) - .replaceAll('', tagClose) - ) - .replaceAll(tagOpen, '') - .replaceAll(tagClose, '') + return snippet + .split(/(|<\/mark>)/) + .map(part => (part === '' || part === '') ? part : escapeHTML(part)) + .join('') } /** diff --git a/test/utils/search.spec.ts b/test/utils/search.spec.ts index 9e7c1738..9fdd146e 100644 --- a/test/utils/search.spec.ts +++ b/test/utils/search.spec.ts @@ -28,6 +28,65 @@ describe('sanitizeSnippet', () => { it('handles empty input', () => { expect(sanitizeSnippet('')).toBe('') }) + + it('does not let the snippet supply the placeholder', () => { + // The escape/restore round trip used to key on `\0markO\0`/`\0markC\0`. NUL + // is an ordinary character, so a snippet carrying those bytes came back out + // as markup — a `` in the output with none in the input (#391). + expect(sanitizeSnippet('before \0markO\0 after')).toBe('before \0markO\0 after') + expect(sanitizeSnippet('x \0markO\0INJECTED\0markC\0 y')).toBe('x \0markO\0INJECTED\0markC\0 y') + + // Carrying something that must be escaped as well, so this case cannot be + // satisfied by a function that returns its argument untouched. The two + // above can: their expected value is the input verbatim, which is exactly + // what a no-op produces. + expect(sanitizeSnippet('\0markO\0x')).toBe('\0markO\0<b>x</b>') + }) + + it('does not let a partial placeholder capture a real tag', () => { + // The lower bar, and the worse outcome. None of these carries a whole + // sentinel — only six of its seven bytes, sitting immediately before a real + // tag. That was enough, because the placeholder the function inserted *for + // that tag* completed the prefix, and the restore step then found a sentinel + // spanning the two. The genuine highlight moved: + // + // '\0markO' → 'markO\0' + // + // and with text on both sides, it moved onto text it was never meant to + // mark. One stray fragment ahead of any highlight, not a crafted sequence. + expect(sanitizeSnippet('\0markO')).toBe('\0markO') + expect(sanitizeSnippet('a\0markOhitb')).toBe('a\0markOhitb') + expect(sanitizeSnippet('\0markCtail')).toBe('\0markCtail') + }) + + it('does not let a forged tag unbalance the real ones', () => { + // Not only spurious emphasis: the closing tag was forgeable too, and could + // be placed ahead of its opener, so the markup reaching `v-html` came out + // unbalanced. + expect(sanitizeSnippet('\0markC\0\0markO\0')).toBe('\0markC\0\0markO\0') + expect(sanitizeSnippet('\0markO\0real')).toBe('\0markO\0real') + }) + + it('escapes an attribute a forged tag would have carried', () => { + // The bound on the old flaw, kept as a fixture rather than left implicit: + // escaping ran before the swap back, so even a forged tag could never take + // an attribute. A future rewrite must not quietly widen that. + expect(sanitizeSnippet('\0markO\0 onload=alert(1)')).toBe('\0markO\0 onload=alert(1)') + expect(sanitizeSnippet('x')).toBe('<mark onload=alert(1)>x') + + // The mirror. Neither case above holds a real *opening* tag, so both survive + // a break in that half of the condition — the malformed opener was never + // going to match as a delimiter either way. Here the opener is genuine and + // must be kept while the malformed closer is escaped. + expect(sanitizeSnippet('x')).toBe('x</mark onload=alert(1)>') + }) + + it('leaves a tag that only resembles the real one escaped', () => { + // Unchanged by the fix, but nothing asserted it: the split matches the exact + // tag, so case and stray whitespace stay escaped. + expect(sanitizeSnippet('x')).toBe('<MARK>x</MARK>') + expect(sanitizeSnippet('x')).toBe('<mark >x</mark >') + }) }) describe('highlight', () => {