From cf51b256cefd8c6ccde938fa9cec63cf9e69b977 Mon Sep 17 00:00:00 2001 From: Bojidar Marinov Date: Mon, 20 Oct 2025 09:10:13 +0300 Subject: [PATCH] Fix highlights in messages (or search results) breaking links (#30264) * Fix highlights in messages (or search results) breaking links Fixes #17011 and fixes #29807, by running the linkifier that turns text into links before the highlighter that adds highlights to text. * Fix jest test * Fix tests related to emojis and pills-inside-spoilers * Remove dead code * Address review comments around sanitizeParams * Address review comment about linkify-matrix * Fix code style * Refactor if statement per review --------- Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> --- package.json | 1 + src/HtmlUtils.tsx | 62 +++++++++++++------ src/Linkify.tsx | 12 +++- .../views/messages/EventContentBody.tsx | 15 +---- src/linkify-matrix.ts | 2 + src/renderer/index.ts | 8 +-- src/renderer/spoiler.tsx | 6 +- src/renderer/utils.tsx | 54 ++++++---------- test/unit-tests/HtmlUtils-test.tsx | 38 ++++++++++++ .../views/messages/TextualBody-test.tsx | 4 +- yarn.lock | 5 ++ 11 files changed, 128 insertions(+), 79 deletions(-) diff --git a/package.json b/package.json index 4b5456f4ec..a5be95ac59 100644 --- a/package.json +++ b/package.json @@ -122,6 +122,7 @@ "jsrsasign": "^11.0.0", "jszip": "^3.7.0", "katex": "^0.16.0", + "linkify-html": "4.3.2", "linkify-react": "4.3.2", "linkify-string": "4.3.2", "linkifyjs": "4.3.2", diff --git a/src/HtmlUtils.tsx b/src/HtmlUtils.tsx index 1d17710dab..027a51ea72 100644 --- a/src/HtmlUtils.tsx +++ b/src/HtmlUtils.tsx @@ -22,7 +22,7 @@ import { getEmojiFromUnicode } from "@matrix-org/emojibase-bindings"; import SettingsStore from "./settings/SettingsStore"; import { stripHTMLReply, stripPlainReply } from "./utils/Reply"; import { PERMITTED_URL_SCHEMES } from "./utils/UrlUtils"; -import { sanitizeHtmlParams, transformTags } from "./Linkify"; +import { linkifyHtml, sanitizeHtmlParams, transformTags } from "./Linkify"; import { graphemeSegmenter } from "./utils/strings"; export { Linkify, linkifyAndSanitizeHtml } from "./Linkify"; @@ -298,6 +298,7 @@ export interface EventRenderOpts { * Should inline media be rendered? */ mediaIsVisible?: boolean; + linkify?: boolean; } function analyseEvent(content: IContent, highlights: Optional, opts: EventRenderOpts = {}): EventAnalysis { @@ -320,6 +321,18 @@ function analyseEvent(content: IContent, highlights: Optional, opts: E }; } + if (opts.linkify) { + // Prevent mutating the source of sanitizeParams. + sanitizeParams = { ...sanitizeParams }; + sanitizeParams.allowedClasses ??= {}; + if (typeof sanitizeParams.allowedClasses.a === "boolean") { + // All classes are already allowed for "a" + } else { + sanitizeParams.allowedClasses.a ??= []; + sanitizeParams.allowedClasses.a.push("linkified"); + } + } + try { const isFormattedBody = content.format === "org.matrix.custom.html" && typeof content.formatted_body === "string"; @@ -346,19 +359,26 @@ function analyseEvent(content: IContent, highlights: Optional, opts: E ? new HtmlHighlighter("mx_EventTile_searchHighlight", opts.highlightLink) : null; + if (highlighter) { + // XXX: We sanitize the HTML whilst also highlighting its text nodes, to avoid accidentally trying + // to highlight HTML tags themselves. However, this does mean that we don't highlight textnodes which + // are interrupted by HTML tags (not that we did before) - e.g. foobar won't get highlighted + // by an attempt to search for 'foobar'. Then again, the search query probably wouldn't work either + // XXX: hacky bodge to temporarily apply a textFilter to the sanitizeParams structure. + sanitizeParams.textFilter = function (safeText) { + return highlighter.applyHighlights(safeText, safeHighlights!).join(""); + }; + } + if (isFormattedBody) { - if (highlighter) { - // XXX: We sanitize the HTML whilst also highlighting its text nodes, to avoid accidentally trying - // to highlight HTML tags themselves. However, this does mean that we don't highlight textnodes which - // are interrupted by HTML tags (not that we did before) - e.g. foobar won't get highlighted - // by an attempt to search for 'foobar'. Then again, the search query probably wouldn't work either - // XXX: hacky bodge to temporarily apply a textFilter to the sanitizeParams structure. - sanitizeParams.textFilter = function (safeText) { - return highlighter.applyHighlights(safeText, safeHighlights!).join(""); - }; + let unsafeBody = formattedBody!; + + if (opts.linkify) { + unsafeBody = linkifyHtml(unsafeBody); } - safeBody = sanitizeHtml(formattedBody!, sanitizeParams); + safeBody = sanitizeHtml(unsafeBody, sanitizeParams); + const phtml = new DOMParser().parseFromString(safeBody, "text/html"); const isPlainText = phtml.body.innerHTML === phtml.body.textContent; isHtmlMessage = !isPlainText; @@ -373,6 +393,9 @@ function analyseEvent(content: IContent, highlights: Optional, opts: E }); safeBody = phtml.body.innerHTML; } + } else if (opts.linkify) { + // If we are linkifying plain text, pass the result through sanitizeHtml so that the highlighter configured in sanitizeParams.textFilter gets applied. + safeBody = sanitizeHtml(linkifyHtml(escapeHtml(plainBody)), sanitizeParams); } else if (highlighter) { safeBody = highlighter.applyHighlights(escapeHtml(plainBody), safeHighlights!).join(""); } @@ -428,14 +451,15 @@ export function bodyToNode( }); let formattedBody = eventInfo.safeBody; - if (eventInfo.isFormattedBody && eventInfo.bodyHasEmoji && eventInfo.safeBody) { - // This has to be done after the emojiBody check as to not break big emoji on replies - formattedBody = formatEmojis(eventInfo.safeBody, true).join(""); - } - let emojiBodyElements: JSX.Element[] | undefined; - if (!eventInfo.safeBody && eventInfo.bodyHasEmoji) { - emojiBodyElements = formatEmojis(eventInfo.strippedBody, false) as JSX.Element[]; + + if (eventInfo.bodyHasEmoji) { + if (eventInfo.safeBody) { + // This has to be done after the emojiBody check as to not break big emoji on replies + formattedBody = formatEmojis(eventInfo.safeBody, true).join(""); + } else { + emojiBodyElements = formatEmojis(eventInfo.strippedBody, false) as JSX.Element[]; + } } return { strippedBody: eventInfo.strippedBody, formattedBody, emojiBodyElements, className }; @@ -458,7 +482,7 @@ export function bodyToHtml(content: IContent, highlights: Optional, op const eventInfo = analyseEvent(content, highlights, opts); let formattedBody = eventInfo.safeBody; - if (eventInfo.isFormattedBody && eventInfo.bodyHasEmoji && formattedBody) { + if (eventInfo.bodyHasEmoji && formattedBody) { // This has to be done after the emojiBody check above as to not break big emoji on replies formattedBody = formatEmojis(eventInfo.safeBody, true).join(""); } diff --git a/src/Linkify.tsx b/src/Linkify.tsx index 846bf8e82d..f324acd9b8 100644 --- a/src/Linkify.tsx +++ b/src/Linkify.tsx @@ -11,7 +11,7 @@ import sanitizeHtml, { type IOptions } from "sanitize-html"; import { merge } from "lodash"; import _Linkify from "linkify-react"; -import { _linkifyString, ELEMENT_URL_PATTERN, options as linkifyMatrixOptions } from "./linkify-matrix"; +import { _linkifyString, _linkifyHtml, ELEMENT_URL_PATTERN, options as linkifyMatrixOptions } from "./linkify-matrix"; import { tryTransformPermalinkToLocalHref } from "./utils/permalinks/Permalinks"; import { mediaFromMxc } from "./customisations/Media"; import { PERMITTED_URL_SCHEMES } from "./utils/UrlUtils"; @@ -213,6 +213,16 @@ export function linkifyString(str: string, options = linkifyMatrixOptions): stri return _linkifyString(str, options); } +/** + * Linkifies the given HTML-formatted string. This is a wrapper around 'linkifyjs/html'. + * + * @param {string} str HTML string to linkify + * @param {object} [options] Options for linkifyHtml. Default: linkifyMatrixOptions + * @returns {string} Linkified string + */ +export function linkifyHtml(str: string, options = linkifyMatrixOptions): string { + return _linkifyHtml(str, options); +} /** * Linkify the given string and sanitize the HTML afterwards. * diff --git a/src/components/views/messages/EventContentBody.tsx b/src/components/views/messages/EventContentBody.tsx index 04c37461ec..50d95642f8 100644 --- a/src/components/views/messages/EventContentBody.tsx +++ b/src/components/views/messages/EventContentBody.tsx @@ -11,7 +11,6 @@ import parse from "html-react-parser"; import { PushProcessor } from "matrix-js-sdk/src/pushprocessor"; import { bodyToNode } from "../../../HtmlUtils.tsx"; -import { Linkify } from "../../../Linkify.tsx"; import PlatformPeg from "../../../PlatformPeg.ts"; import { applyReplacerOnString, @@ -23,7 +22,6 @@ import { ambiguousLinkTooltipRenderer, codeBlockRenderer, spoilerRenderer, - replacerToRenderFunction, } from "../../../renderer"; import MatrixClientContext from "../../../contexts/MatrixClientContext.tsx"; import { useSettingValue } from "../../../hooks/useSettings.ts"; @@ -154,12 +152,6 @@ const EventContentBody = memo( const [mediaIsVisible] = useMediaVisible(mxEvent); const replacer = useReplacer(content, mxEvent, options); - const linkifyOptions = useMemo( - () => ({ - render: replacerToRenderFunction(replacer), - }), - [replacer], - ); const isEmote = content.msgtype === MsgType.Emote; @@ -170,8 +162,9 @@ const EventContentBody = memo( // Part of Replies fallback support stripReplyFallback: stripReply, mediaIsVisible, + linkify, }), - [content, mediaIsVisible, enableBigEmoji, highlights, isEmote, stripReply], + [content, mediaIsVisible, enableBigEmoji, highlights, isEmote, stripReply, linkify], ); if (as === "div") includeDir = true; // force dir="auto" on divs @@ -189,9 +182,7 @@ const EventContentBody = memo( ); - if (!linkify) return body; - - return {body}; + return body; }, ); diff --git a/src/linkify-matrix.ts b/src/linkify-matrix.ts index 94e796a178..b6ed8ee7fc 100644 --- a/src/linkify-matrix.ts +++ b/src/linkify-matrix.ts @@ -10,6 +10,7 @@ Please see LICENSE files in the repository root for full details. import * as linkifyjs from "linkifyjs"; import { type EventListeners, type Opts, registerCustomProtocol, registerPlugin } from "linkifyjs"; import linkifyString from "linkify-string"; +import linkifyHtml from "linkify-html"; import { getHttpUriForMxc, User } from "matrix-js-sdk/src/matrix"; import { @@ -293,3 +294,4 @@ registerCustomProtocol("mxc", false); export const linkify = linkifyjs; export const _linkifyString = linkifyString; +export const _linkifyHtml = linkifyHtml; diff --git a/src/renderer/index.ts b/src/renderer/index.ts index eaed7b71c3..3de73c8bbe 100644 --- a/src/renderer/index.ts +++ b/src/renderer/index.ts @@ -9,10 +9,4 @@ export { ambiguousLinkTooltipRenderer } from "./link-tooltip"; export { keywordPillRenderer, mentionPillRenderer } from "./pill"; export { spoilerRenderer } from "./spoiler"; export { codeBlockRenderer } from "./code-block"; -export { - applyReplacerOnString, - replacerToRenderFunction, - combineRenderers, - type RendererMap, - type Replacer, -} from "./utils"; +export { applyReplacerOnString, combineRenderers, type RendererMap, type Replacer } from "./utils"; diff --git a/src/renderer/spoiler.tsx b/src/renderer/spoiler.tsx index ee7f45f48e..7b71295c08 100644 --- a/src/renderer/spoiler.tsx +++ b/src/renderer/spoiler.tsx @@ -15,10 +15,12 @@ import Spoiler from "../components/views/elements/Spoiler.tsx"; * Replaces spans with `data-mx-spoiler` with a Spoiler component. */ export const spoilerRenderer: RendererMap = { - span: (span) => { + span: (span, params) => { const reason = span.attribs["data-mx-spoiler"]; if (typeof reason === "string") { - return {domToReact(span.children as DOMNode[])}; + return ( + {domToReact(span.children as DOMNode[], { replace: params.replace })} + ); } }, }; diff --git a/src/renderer/utils.tsx b/src/renderer/utils.tsx index 0cf37b9a8c..3109e9b447 100644 --- a/src/renderer/utils.tsx +++ b/src/renderer/utils.tsx @@ -8,7 +8,6 @@ Please see LICENSE files in the repository root for full details. import React, { type JSX } from "react"; import { type DOMNode, Element, type HTMLReactParserOptions, Text } from "html-react-parser"; import { type MatrixEvent, type Room } from "matrix-js-sdk/src/matrix"; -import { type Opts } from "linkifyjs"; /** * The type of a parent node of an element, normally exported by domhandler but that is not a direct dependency of ours @@ -65,29 +64,9 @@ export function applyReplacerOnString( }); } -/** - * Converts a Replacer function to a render function for linkify-react - * So that we can use the same replacer functions for both - * @param replacer The replacer function to convert - */ -export function replacerToRenderFunction(replacer: Replacer): Opts["render"] { - if (!replacer) return; - return ({ tagName, attributes, content }) => { - const domNode = new Element(tagName, attributes, [new Text(content)], "tag" as Element["type"]); - const result = replacer(domNode, 0); - if (result) return result; - - // This is cribbed from the default render function in linkify-react - if (attributes.class) { - attributes.className = attributes.class; - delete attributes.class; - } - return React.createElement(tagName, attributes, content); - }; -} - interface Parameters { isHtml: boolean; + replace: Replacer; // Required for keywordPillRenderer keywordRegexpPattern?: RegExp; // Required for mentionPillRenderer @@ -114,7 +93,7 @@ export type RendererMap = Partial< } >; -type PreparedRenderer = (parameters: Parameters) => Replacer; +type PreparedRenderer = (parameters: Omit) => Replacer; /** * Combines multiple renderers into a single Replacer function @@ -122,19 +101,22 @@ type PreparedRenderer = (parameters: Parameters) => Replacer; */ export const combineRenderers = (...renderers: RendererMap[]): PreparedRenderer => - (parameters) => - (node, index) => { - if (node.type === "text") { - for (const replacer of renderers) { - const result = replacer[Node.TEXT_NODE]?.(node, parameters, index); - if (result) return result; + (parameters) => { + const replace: Replacer = (node, index) => { + if (node.type === "text") { + for (const replacer of renderers) { + const result = replacer[Node.TEXT_NODE]?.(node, parametersWithReplace, index); + if (result) return result; + } } - } - if (node instanceof Element) { - const tagName = node.tagName.toLowerCase() as keyof HTMLElementTagNameMap; - for (const replacer of renderers) { - const result = replacer[tagName]?.(node, parameters, index); - if (result) return result; + if (node instanceof Element) { + const tagName = node.tagName.toLowerCase() as keyof HTMLElementTagNameMap; + for (const replacer of renderers) { + const result = replacer[tagName]?.(node, parametersWithReplace, index); + if (result) return result; + } } - } + }; + const parametersWithReplace: Parameters = { ...parameters, replace }; + return replace; }; diff --git a/test/unit-tests/HtmlUtils-test.tsx b/test/unit-tests/HtmlUtils-test.tsx index 97c8da6013..690f2a713a 100644 --- a/test/unit-tests/HtmlUtils-test.tsx +++ b/test/unit-tests/HtmlUtils-test.tsx @@ -86,6 +86,44 @@ describe("bodyToHtml", () => { expect(html).toMatchInlineSnapshot(`"test foo <b>bar"`); }); + it("should linkify and hightlight parts of links in plaintext message highlighting", () => { + getMockClientWithEventEmitter({}); + + const html = bodyToHtml( + { + body: "foo http://link.example/test/path bar", + msgtype: "m.text", + }, + ["test"], + { + linkify: true, + }, + ); + + expect(html).toMatchInlineSnapshot( + `"foo http://link.example/test/path bar"`, + ); + }); + + it("should hightlight parts of links in HTML message highlighting", () => { + const html = bodyToHtml( + { + body: "foo http://link.example/test/path bar", + msgtype: "m.text", + formatted_body: 'foo http://link.example/test/path bar', + format: "org.matrix.custom.html", + }, + ["test"], + { + linkify: true, + }, + ); + + expect(html).toMatchInlineSnapshot( + `"foo http://link.example/test/path bar"`, + ); + }); + it("does not mistake characters in text presentation mode for emoji", () => { const { asFragment } = render( diff --git a/test/unit-tests/components/views/messages/TextualBody-test.tsx b/test/unit-tests/components/views/messages/TextualBody-test.tsx index cb3d755f8b..78a1261116 100644 --- a/test/unit-tests/components/views/messages/TextualBody-test.tsx +++ b/test/unit-tests/components/views/messages/TextualBody-test.tsx @@ -188,7 +188,7 @@ describe("", () => { const { container } = getComponent({ mxEvent: ev }); const content = container.querySelector(".mx_EventTile_body"); expect(content.innerHTML).toMatchInlineSnapshot( - `"Chat with @user:example.com"`, + `"Chat with @user:example.com"`, ); }); @@ -206,7 +206,7 @@ describe("", () => { const { container } = getComponent({ mxEvent: ev }); const content = container.querySelector(".mx_EventTile_body"); expect(content.innerHTML).toMatchInlineSnapshot( - `"Visit #room:example.com"`, + `"Visit #room:example.com"`, ); }); diff --git a/yarn.lock b/yarn.lock index 11f7d27fdb..89c270291d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9296,6 +9296,11 @@ lines-and-columns@^1.1.6: resolved "https://registry.yarnpkg.com/lines-and-columns/-/lines-and-columns-1.2.4.tgz#eca284f75d2965079309dc0ad9255abb2ebc1632" integrity sha512-7ylylesZQ/PV29jhEDl3Ufjo6ZX7gCqJr5F7PKrqc93v7fzSymt1BpwEU8nAUXs8qzzvqhbjhK5QZg6Mt/HkBg== +linkify-html@4.3.2: + version "4.3.2" + resolved "https://registry.yarnpkg.com/linkify-html/-/linkify-html-4.3.2.tgz#ef84b39828c66170221af1a49a042c7993bd4543" + integrity sha512-RozNgrfSFrNQlprJSZIN7lF+ZVPj5Pz8POQcu1PYGAUhL9tKtvtWcOXOmlXjuGGEWHtC6gt6Q2U4+VUq9ELmng== + linkify-it@^4.0.1: version "4.0.1" resolved "https://registry.yarnpkg.com/linkify-it/-/linkify-it-4.0.1.tgz#01f1d5e508190d06669982ba31a7d9f56a5751ec"