Ignore permalink_prefix when serializing pills (#11726)

* Ignore permalink_prefix when serializing Markdown fixes element-hq/element-web/issues/26002 During serialization of messages, pills were wrongfully serialized to a URL starting with `permalink_prefix`. This is against the Spec (which mandates https://matrix.to/#/ links) and the resulting pills were not recognized as pills in other clients. Spec-Appendix concerning matrix.to links: https://spec.matrix.org/v1.8/appendices/#matrixto-navigation Signed-off-by: Lars Wickel <git@herkulessi.de> * Test for pill serialization with permalink_prefix set Signed-off-by: Lars Wickel <git@herkulessi.de> * Test that permalink_prefix is used for permalink creation Signed-off-by: Lars Wickel <git@herkulessi.de> * Fix lint issues introduced Signed-off-by: Lars Wickel <git@herkulessi.de> * Incorporate requested changes Signed-off-by: Lars Wickel <git@herkulessi.de> --------- Signed-off-by: Lars Wickel <git@herkulessi.de> Co-authored-by: herkulessi <git@herkulessi.de> Co-authored-by: David Baker <dbkr@users.noreply.github.com>
tchapgouv · Aug 1, 2024 · fa60edf · fa60edf
1 parent e6a3238
commit fa60edf
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 11 deletions.
diff --git a/src/editor/serialize.ts b/src/editor/serialize.ts
@@ -37,15 +37,15 @@ export function mdSerialize(model: EditorModel): string {
             case Type.AtRoomPill:
                 return html + part.text;
             case Type.RoomPill: {
-                const url = makeGenericPermalink(part.resourceId);
+                const url = makeGenericPermalink(part.resourceId, true);
                 // Escape square brackets and backslashes
                 // Here we use the resourceId for compatibility with non-rich text clients
                 // See https://github.com/vector-im/element-web/issues/16660
                 const title = part.resourceId.replace(/[[\\\]]/g, (c) => "\\" + c);
                 return html + `[${title}](${url})`;
             }
             case Type.UserPill: {
-                const url = makeGenericPermalink(part.resourceId);
+                const url = makeGenericPermalink(part.resourceId, true);
                 // Escape square brackets and backslashes; convert newlines to HTML
                 const title = part.text.replace(/[[\\\]]/g, (c) => "\\" + c).replace(/\n/g, "<br>");
                 return html + `[${title}](${url})`;

diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts
@@ -274,26 +274,48 @@ export class RoomPermalinkCreator {
     };
 }
 
-export function makeGenericPermalink(entityId: string): string {
-    return getPermalinkConstructor().forEntity(entityId);
+/**
+ * Creates a permalink for an Entity. If isPill is set it uses a spec-compliant
+ * prefix for the permalink, instead of permalink_prefix
+ * @param {string} entityId The entity to link to.
+ * @param {boolean} isPill Link should be pillifyable.
+ * @returns {string|null} The transformed permalink or null if unable.
+ */
+export function makeGenericPermalink(entityId: string, isPill = false): string {
+    return getPermalinkConstructor(isPill).forEntity(entityId);
 }
 
-export function makeUserPermalink(userId: string): string {
-    return getPermalinkConstructor().forUser(userId);
+/**
+ * Creates a permalink for a User. If isPill is set it uses a spec-compliant
+ * prefix for the permalink, instead of permalink_prefix
+ * @param {string} userId The user to link to.
+ * @param {boolean} isPill Link should be pillifyable.
+ * @returns {string|null} The transformed permalink or null if unable.
+ */
+export function makeUserPermalink(userId: string, isPill = false): string {
+    return getPermalinkConstructor(isPill).forUser(userId);
 }
 
-export function makeRoomPermalink(matrixClient: MatrixClient, roomId: string): string {
+/**
+ * Creates a permalink for a room. If isPill is set it uses a spec-compliant
+ * prefix for the permalink, instead of permalink_prefix
+ * @param {MatrixClient} matrixClient The MatrixClient to use
+ * @param {string} roomId The user to link to.
+ * @param {boolean} isPill Link should be pillifyable.
+ * @returns {string|null} The transformed permalink or null if unable.
+ */
+export function makeRoomPermalink(matrixClient: MatrixClient, roomId: string, isPill = false): string {
     if (!roomId) {
         throw new Error("can't permalink a falsy roomId");
     }
 
     // If the roomId isn't actually a room ID, don't try to list the servers.
     // Aliases are already routable, and don't need extra information.
-    if (roomId[0] !== "!") return getPermalinkConstructor().forRoom(roomId, []);
+    if (roomId[0] !== "!") return getPermalinkConstructor(isPill).forRoom(roomId, []);
 
     const room = matrixClient.getRoom(roomId);
     if (!room) {
-        return getPermalinkConstructor().forRoom(roomId, []);
+        return getPermalinkConstructor(isPill).forRoom(roomId, []);
     }
     const permalinkCreator = new RoomPermalinkCreator(room);
     permalinkCreator.load();
@@ -414,9 +436,15 @@ export function getPrimaryPermalinkEntity(permalink: string): string | null {
     return null;
 }
 
-function getPermalinkConstructor(): PermalinkConstructor {
+/**
+ * Returns the correct PermalinkConstructor based on permalink_prefix
+ * and isPill
+ * @param {boolean} isPill Should constructed links be pillifyable.
+ * @returns {string|null} The transformed permalink or null if unable.
+ */
+function getPermalinkConstructor(isPill = false): PermalinkConstructor {
     const elementPrefix = SdkConfig.get("permalink_prefix");
-    if (elementPrefix && elementPrefix !== matrixtoBaseUrl) {
+    if (elementPrefix && elementPrefix !== matrixtoBaseUrl && !isPill) {
         return new ElementPermalinkConstructor(elementPrefix);
     }
 

diff --git a/test/editor/serialize-test.ts b/test/editor/serialize-test.ts
@@ -14,10 +14,14 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+import { mocked } from "jest-mock";
+
 import EditorModel from "../../src/editor/model";
 import { htmlSerializeIfNeeded } from "../../src/editor/serialize";
 import { createPartCreator } from "./mock";
+import { IConfigOptions } from "../../src/IConfigOptions";
 import SettingsStore from "../../src/settings/SettingsStore";
+import SdkConfig from "../../src/SdkConfig";
 
 describe("editor/serialize", function () {
     describe("with markdown", function () {
@@ -104,6 +108,32 @@ describe("editor/serialize", function () {
             const html = htmlSerializeIfNeeded(model, {});
             expect(html).toBe('<ol start="2021">\n<li>foo</li>\n</ol>\n');
         });
+        describe("with permalink_prefix set", function () {
+            const sdkConfigGet = SdkConfig.get;
+            beforeEach(() => {
+                jest.spyOn(SdkConfig, "get").mockImplementation((key: keyof IConfigOptions, altCaseName?: string) => {
+                    if (key === "permalink_prefix") {
+                        return "https://element.fs.tld";
+                    } else return sdkConfigGet(key, altCaseName);
+                });
+            });
+
+            it("user pill uses matrix.to", function () {
+                const pc = createPartCreator();
+                const model = new EditorModel([pc.userPill("Alice", "@alice:hs.tld")], pc);
+                const html = htmlSerializeIfNeeded(model, {});
+                expect(html).toBe('<a href="https://matrix.to/#/@alice:hs.tld">Alice</a>');
+            });
+            it("room pill uses matrix.to", function () {
+                const pc = createPartCreator();
+                const model = new EditorModel([pc.roomPill("#room:hs.tld")], pc);
+                const html = htmlSerializeIfNeeded(model, {});
+                expect(html).toBe('<a href="https://matrix.to/#/#room:hs.tld">#room:hs.tld</a>');
+            });
+            afterEach(() => {
+                mocked(SdkConfig.get).mockRestore();
+            });
+        });
     });
 
     describe("with plaintext", function () {

diff --git a/test/utils/permalinks/Permalinks-test.ts b/test/utils/permalinks/Permalinks-test.ts
@@ -25,6 +25,8 @@ import {
     parsePermalink,
     RoomPermalinkCreator,
 } from "../../../src/utils/permalinks/Permalinks";
+import { IConfigOptions } from "../../../src/IConfigOptions";
+import SdkConfig from "../../../src/SdkConfig";
 import { getMockClientWithEventEmitter } from "../../test-utils";
 
 describe("Permalinks", function () {
@@ -391,6 +393,17 @@ describe("Permalinks", function () {
         expect(result).toBe("https://matrix.to/#/@someone:example.org");
     });
 
+    it("should use permalink_prefix for permalinks", function () {
+        const sdkConfigGet = SdkConfig.get;
+        jest.spyOn(SdkConfig, "get").mockImplementation((key: keyof IConfigOptions, altCaseName?: string) => {
+            if (key === "permalink_prefix") {
+                return "https://element.fs.tld";
+            } else return sdkConfigGet(key, altCaseName);
+        });
+        const result = makeUserPermalink("@someone:example.org");
+        expect(result).toBe("https://element.fs.tld/#/user/@someone:example.org");
+    });
+
     describe("parsePermalink", () => {
         it("should correctly parse room permalinks with a via argument", () => {
             const result = parsePermalink("https://matrix.to/#/!room_id:server?via=some.org");