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>
2025-11-09 12:41:07 +01:00 · 2025-10-20 09:10:13 +03:00 · 2025-10-20 09:10:13 +03:00 · cf51b256ce
commit cf51b256ce
parent 9d973c88f9
11 changed files with 128 additions and 79 deletions
--- 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",
--- 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<string[]>, opts: EventRenderOpts = {}): EventAnalysis {
@ -320,6 +321,18 @@ function analyseEvent(content: IContent, highlights: Optional<string[]>, 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<string[]>, 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. foo<span/>bar 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. foo<span/>bar 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<string[]>, 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<string[]>, 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("");
    }
--- 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.
 *
--- 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(
            </As>
        );

-        if (!linkify) return body;
-
-        return <Linkify options={linkifyOptions}>{body}</Linkify>;
+        return body;
    },
 );

--- 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;
--- 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";
--- 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 <Spoiler reason={reason}>{domToReact(span.children as DOMNode[])}</Spoiler>;
+            return (
+                <Spoiler reason={reason}>{domToReact(span.children as DOMNode[], { replace: params.replace })}</Spoiler>
+            );
        }
    },
 };
--- 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<Parameters, "replace">) => 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;
    };
--- a/test/unit-tests/HtmlUtils-test.tsx
+++ b/test/unit-tests/HtmlUtils-test.tsx
@ -86,6 +86,44 @@ describe("bodyToHtml", () => {
        expect(html).toMatchInlineSnapshot(`"<span class="mx_EventTile_searchHighlight">test</span> foo &lt;b&gt;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 <a href="http://link.example/test/path" class="linkified" target="_blank" rel="noreferrer noopener">http://link.example/<span class="mx_EventTile_searchHighlight">test</span>/path</a> 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 <a href="http://link.example/test/path">http://link.example/test/path</a> bar',
+                format: "org.matrix.custom.html",
+            },
+            ["test"],
+            {
+                linkify: true,
+            },
+        );
+
+        expect(html).toMatchInlineSnapshot(
+            `"foo <a href="http://link.example/test/path" target="_blank" rel="noreferrer noopener">http://link.example/<span class="mx_EventTile_searchHighlight">test</span>/path</a> bar"`,
+        );
+    });
+
    it("does not mistake characters in text presentation mode for emoji", () => {
        const { asFragment } = render(
            <span className="mx_EventTile_body translate" dir="auto">
--- 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("<TextualBody />", () => {
            const { container } = getComponent({ mxEvent: ev });
            const content = container.querySelector(".mx_EventTile_body");
            expect(content.innerHTML).toMatchInlineSnapshot(
-                `"Chat with <a href="https://matrix.to/#/@user:example.com" rel="noreferrer noopener" class="linkified">@user:example.com</a>"`,
+                `"Chat with <a href="https://matrix.to/#/@user:example.com" class="linkified" rel="noreferrer noopener">@user:example.com</a>"`,
            );
        });

@ -206,7 +206,7 @@ describe("<TextualBody />", () => {
            const { container } = getComponent({ mxEvent: ev });
            const content = container.querySelector(".mx_EventTile_body");
            expect(content.innerHTML).toMatchInlineSnapshot(
-                `"Visit <a href="https://matrix.to/#/#room:example.com" rel="noreferrer noopener" class="linkified">#room:example.com</a>"`,
+                `"Visit <a href="https://matrix.to/#/#room:example.com" class="linkified" rel="noreferrer noopener">#room:example.com</a>"`,
            );
        });

--- 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"