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>

Author: bojidar-bg

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

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("");
     }

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.
  *

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;
     },
 );
 

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;

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

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>
+            );
         }
     },
 };

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,27 +93,30 @@ export type RendererMap = Partial<
     }
 >;
 
-type PreparedRenderer = (parameters: Parameters) => Replacer;
+type PreparedRenderer = (parameters: Omit<Parameters, "replace">) => Replacer;
 
 /**
  * Combines multiple renderers into a single Replacer function
  * @param renderers - the list of renderers to combine
  */
 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;
     };

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

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>"`,
             );
         });