From c4a851cb40fa8aef510aaa47da17857037819b55 Mon Sep 17 00:00:00 2001 From: Shevchik Igor Date: Sat, 15 Aug 2026 05:01:50 +0000 Subject: [PATCH] fix(ContentSearch): stop `sanitizeSnippet` rebuilding tags from its input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sanitizeSnippet` preserved real `` tags by swapping them for `\0markO\0`/`\0markC\0`, escaping everything, then swapping back. NUL is an ordinary character a snippet can carry, so what decided whether markup was emitted was a string the input could supply. A whole sentinel forged a tag outright: sanitizeSnippet('before \0markO\0 after') // → 'before after' — one out, none in sanitizeSnippet('\0markC\0\0markO\0') // → '' — a close ahead of its opener Worse, and easier: **six** of the seven bytes, immediately before a *real* tag, were enough. The placeholder this function inserted for that tag completed the prefix, and the restore step then found a sentinel spanning the two: sanitizeSnippet('a\0markOhitb') // → 'amarkO\0hitb' The genuine highlight moved, and with text on both sides it landed on text it was never meant to mark. No crafted sequence — one stray fragment ahead of any highlight. The first description of this defect, in #391 and in this branch's first revision, said the input had to carry the sentinel itself. That understated both the trigger and the consequence; an independent fuzzing pass found the prefix case. The function's own doc says the preserved tag is hardcoded *"on purpose: taking it as a parameter would let a caller pass any tag through to the `v-html` that renders the result."* The intent was right; the mechanism did not hold it. Not XSS, and the bound is worth stating because it is what keeps this a content bug: `escapeHTML` ran before the swap back, so content inside a forged region stayed escaped, and the emitted tag came from a fixed literal rather than from a capture group — no attribute could land inside it. Forgeable surface: the two strings, nothing else. The consequence is spoofed emphasis, relocated highlights and unbalanced markup reaching `v-html`, in snippets that come from whatever backend the host application passes to `` (`useContentSearch.ts:141,144`). Splitting on the real tags removes the guess, and with it the collision. Both implementations were run over ~2.1M generated inputs by an independent pass: every divergence traces to this family, and the new one never emitted a tag other than the two literals nor leaked an unescaped character. The regex has no quantifiers, and growth is linear to 61MB. Five tests added, each mutation-checked: dropping the regex capture, dropping the closing-tag half of the condition, and escaping the tags themselves are all caught; restoring the old implementation fails exactly the four forgery cases. One pins the *bound* rather than the bug — a forged tag could never carry an attribute — so a future rewrite cannot widen that silently. Pre-existing since `557a5178`, the original port, and untouched by #365, #371 and #388. The function came from upstream unchanged, so `nuxt/ui` very likely carries it too. Closes #391. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LWWrBHgfqGSbeU3V6UuMF8 --- src/runtime/utils/search.ts | 26 +++++++++------- test/utils/search.spec.ts | 59 +++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 10 deletions(-) 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', () => {