From d7cef28a86c2f2da2b10491f52b477debcaf223d Mon Sep 17 00:00:00 2001 From: herkulessi Date: Tue, 3 Oct 2023 23:24:39 +0200 Subject: [PATCH 1/5] Ignore permalink_prefix when serializing Markdown fixes vector-im/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 --- src/editor/serialize.ts | 4 ++-- .../permalinks/ElementPermalinkConstructor.ts | 21 +++++++++++++------ src/utils/permalinks/PermalinkConstructor.ts | 8 +++---- src/utils/permalinks/Permalinks.ts | 12 +++++------ 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/editor/serialize.ts b/src/editor/serialize.ts index 8391183a654..14cb413a140 100644 --- a/src/editor/serialize.ts +++ b/src/editor/serialize.ts @@ -37,7 +37,7 @@ 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 @@ -45,7 +45,7 @@ export function mdSerialize(model: EditorModel): string { 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, "
"); return html + `[${title}](${url})`; diff --git a/src/utils/permalinks/ElementPermalinkConstructor.ts b/src/utils/permalinks/ElementPermalinkConstructor.ts index b75673dc1af..6956c8737c1 100644 --- a/src/utils/permalinks/ElementPermalinkConstructor.ts +++ b/src/utils/permalinks/ElementPermalinkConstructor.ts @@ -31,23 +31,32 @@ export default class ElementPermalinkConstructor extends PermalinkConstructor { } } - public forEvent(roomId: string, eventId: string, serverCandidates: string[]): string { + public forEvent(roomId: string, eventId: string, serverCandidates: string[], ispill = false): string { + if(ispill) { + return `https://matrix.to/#/${roomId}/${eventId}${this.encodeServerCandidates(serverCandidates)}`; + } return `${this.elementUrl}/#/room/${roomId}/${eventId}${this.encodeServerCandidates(serverCandidates)}`; } - public forRoom(roomIdOrAlias: string, serverCandidates?: string[]): string { + public forRoom(roomIdOrAlias: string, serverCandidates?: string[], ispill = false): string { + if(ispill) { + return `https://matrix.to/#/${roomIdOrAlias}${this.encodeServerCandidates(serverCandidates)}`; + } return `${this.elementUrl}/#/room/${roomIdOrAlias}${this.encodeServerCandidates(serverCandidates)}`; } - public forUser(userId: string): string { + public forUser(userId: string, ispill = false): string { + if(ispill) { + return `https://matrix.to/#/${userId}`; + } return `${this.elementUrl}/#/user/${userId}`; } - public forEntity(entityId: string): string { + public forEntity(entityId: string, ispill = false): string { if (entityId[0] === "!" || entityId[0] === "#") { - return this.forRoom(entityId); + return this.forRoom(entityId, [], ispill); } else if (entityId[0] === "@") { - return this.forUser(entityId); + return this.forUser(entityId, ispill); } else throw new Error("Unrecognized entity"); } diff --git a/src/utils/permalinks/PermalinkConstructor.ts b/src/utils/permalinks/PermalinkConstructor.ts index 4248b97885e..a241d38ec80 100644 --- a/src/utils/permalinks/PermalinkConstructor.ts +++ b/src/utils/permalinks/PermalinkConstructor.ts @@ -19,19 +19,19 @@ limitations under the License. * TODO: Convert this to a real TypeScript interface */ export default class PermalinkConstructor { - public forEvent(roomId: string, eventId: string, serverCandidates: string[] = []): string { + public forEvent(roomId: string, eventId: string, serverCandidates: string[] = [], ispill = false): string { throw new Error("Not implemented"); } - public forRoom(roomIdOrAlias: string, serverCandidates: string[] = []): string { + public forRoom(roomIdOrAlias: string, serverCandidates: string[] = [], ispill = false): string { throw new Error("Not implemented"); } - public forUser(userId: string): string { + public forUser(userId: string, ispill = false): string { throw new Error("Not implemented"); } - public forEntity(entityId: string): string { + public forEntity(entityId: string, ispill = false): string { throw new Error("Not implemented"); } diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index 537494b26b6..0bdbd49428f 100644 --- a/src/utils/permalinks/Permalinks.ts +++ b/src/utils/permalinks/Permalinks.ts @@ -274,26 +274,26 @@ export class RoomPermalinkCreator { }; } -export function makeGenericPermalink(entityId: string): string { - return getPermalinkConstructor().forEntity(entityId); +export function makeGenericPermalink(entityId: string, ispill = false): string { + return getPermalinkConstructor().forEntity(entityId, ispill); } -export function makeUserPermalink(userId: string): string { +export function makeUserPermalink(userId: string, ispill = false): string { return getPermalinkConstructor().forUser(userId); } -export function makeRoomPermalink(matrixClient: MatrixClient, roomId: string): string { +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().forRoom(roomId, [], ispill); const room = matrixClient.getRoom(roomId); if (!room) { - return getPermalinkConstructor().forRoom(roomId, []); + return getPermalinkConstructor().forRoom(roomId, [], ispill); } const permalinkCreator = new RoomPermalinkCreator(room); permalinkCreator.load(); From 2d12001613947f77897b3a19ec782ccd1af303f0 Mon Sep 17 00:00:00 2001 From: herkulessi Date: Mon, 9 Oct 2023 18:49:18 +0200 Subject: [PATCH 2/5] Test for pill serialization with permalink_prefix set Signed-off-by: Lars Wickel --- test/editor/serialize-test.ts | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/editor/serialize-test.ts b/test/editor/serialize-test.ts index 48644da1101..a8da931b5cb 100644 --- a/test/editor/serialize-test.ts +++ b/test/editor/serialize-test.ts @@ -14,10 +14,13 @@ 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 SettingsStore from "../../src/settings/SettingsStore"; +import SdkConfig from "../../src/SdkConfig"; describe("editor/serialize", function () { describe("with markdown", function () { @@ -104,6 +107,32 @@ describe("editor/serialize", function () { const html = htmlSerializeIfNeeded(model, {}); expect(html).toBe('
    \n
  1. foo
    2. \n
\n'); }); + describe("with permalink_prefix set", function () { + const sdkConfigGet = SdkConfig.get + beforeEach(() => { + jest.spyOn(SdkConfig, "get").mockImplementation((key: string, 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('Alice'); + }); + 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('#room:hs.tld'); + }); + afterEach(() => { + mocked(SdkConfig.get).mockRestore(); + }); + }); }); describe("with plaintext", function () { From bcf2771018fee053e202c03728c4bb3c7ac38d5f Mon Sep 17 00:00:00 2001 From: herkulessi Date: Mon, 9 Oct 2023 18:56:55 +0200 Subject: [PATCH 3/5] Test that permalink_prefix is used for permalink creation Signed-off-by: Lars Wickel --- test/utils/permalinks/Permalinks-test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/utils/permalinks/Permalinks-test.ts b/test/utils/permalinks/Permalinks-test.ts index 3c3bbbbec94..3adefc7db9a 100644 --- a/test/utils/permalinks/Permalinks-test.ts +++ b/test/utils/permalinks/Permalinks-test.ts @@ -25,6 +25,7 @@ import { parsePermalink, RoomPermalinkCreator, } from "../../../src/utils/permalinks/Permalinks"; +import SdkConfig from "../../../src/SdkConfig"; import { getMockClientWithEventEmitter } from "../../test-utils"; describe("Permalinks", function () { @@ -391,6 +392,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: string, 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"); From 22940ea1cd9e4fae946c8e2118b4931898052b55 Mon Sep 17 00:00:00 2001 From: Lars Wickel Date: Mon, 9 Oct 2023 19:52:47 +0200 Subject: [PATCH 4/5] Fix lint issues introduced Signed-off-by: Lars Wickel --- src/utils/permalinks/ElementPermalinkConstructor.ts | 6 +++--- test/editor/serialize-test.ts | 5 +++-- test/utils/permalinks/Permalinks-test.ts | 5 +++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/utils/permalinks/ElementPermalinkConstructor.ts b/src/utils/permalinks/ElementPermalinkConstructor.ts index 6956c8737c1..115619cfb40 100644 --- a/src/utils/permalinks/ElementPermalinkConstructor.ts +++ b/src/utils/permalinks/ElementPermalinkConstructor.ts @@ -32,21 +32,21 @@ export default class ElementPermalinkConstructor extends PermalinkConstructor { } public forEvent(roomId: string, eventId: string, serverCandidates: string[], ispill = false): string { - if(ispill) { + if (ispill) { return `https://matrix.to/#/${roomId}/${eventId}${this.encodeServerCandidates(serverCandidates)}`; } return `${this.elementUrl}/#/room/${roomId}/${eventId}${this.encodeServerCandidates(serverCandidates)}`; } public forRoom(roomIdOrAlias: string, serverCandidates?: string[], ispill = false): string { - if(ispill) { + if (ispill) { return `https://matrix.to/#/${roomIdOrAlias}${this.encodeServerCandidates(serverCandidates)}`; } return `${this.elementUrl}/#/room/${roomIdOrAlias}${this.encodeServerCandidates(serverCandidates)}`; } public forUser(userId: string, ispill = false): string { - if(ispill) { + if (ispill) { return `https://matrix.to/#/${userId}`; } return `${this.elementUrl}/#/user/${userId}`; diff --git a/test/editor/serialize-test.ts b/test/editor/serialize-test.ts index a8da931b5cb..552872eb360 100644 --- a/test/editor/serialize-test.ts +++ b/test/editor/serialize-test.ts @@ -19,6 +19,7 @@ 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"; @@ -108,9 +109,9 @@ describe("editor/serialize", function () { expect(html).toBe('
    \n
  1. foo
    2. \n
\n'); }); describe("with permalink_prefix set", function () { - const sdkConfigGet = SdkConfig.get + const sdkConfigGet = SdkConfig.get; beforeEach(() => { - jest.spyOn(SdkConfig, "get").mockImplementation((key: string, altCaseName?: string) => { + jest.spyOn(SdkConfig, "get").mockImplementation((key: keyof IConfigOptions, altCaseName?: string) => { if (key === "permalink_prefix") { return "https://element.fs.tld"; } else return sdkConfigGet(key, altCaseName); diff --git a/test/utils/permalinks/Permalinks-test.ts b/test/utils/permalinks/Permalinks-test.ts index 3adefc7db9a..e21634bf41b 100644 --- a/test/utils/permalinks/Permalinks-test.ts +++ b/test/utils/permalinks/Permalinks-test.ts @@ -25,6 +25,7 @@ import { parsePermalink, RoomPermalinkCreator, } from "../../../src/utils/permalinks/Permalinks"; +import { IConfigOptions } from "../../../src/IConfigOptions"; import SdkConfig from "../../../src/SdkConfig"; import { getMockClientWithEventEmitter } from "../../test-utils"; @@ -393,8 +394,8 @@ describe("Permalinks", function () { }); it("should use permalink_prefix for permalinks", function () { - const sdkConfigGet = SdkConfig.get - jest.spyOn(SdkConfig, "get").mockImplementation((key: string, altCaseName?: string) => { + 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); From ba116636d28bface41adfa83d3fe3944ea4589ae Mon Sep 17 00:00:00 2001 From: Lars Wickel Date: Sun, 15 Oct 2023 11:18:59 +0200 Subject: [PATCH 5/5] Incorporate requested changes Signed-off-by: Lars Wickel --- .../permalinks/ElementPermalinkConstructor.ts | 21 +++------ src/utils/permalinks/PermalinkConstructor.ts | 8 ++-- src/utils/permalinks/Permalinks.ts | 46 +++++++++++++++---- 3 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/utils/permalinks/ElementPermalinkConstructor.ts b/src/utils/permalinks/ElementPermalinkConstructor.ts index 115619cfb40..b75673dc1af 100644 --- a/src/utils/permalinks/ElementPermalinkConstructor.ts +++ b/src/utils/permalinks/ElementPermalinkConstructor.ts @@ -31,32 +31,23 @@ export default class ElementPermalinkConstructor extends PermalinkConstructor { } } - public forEvent(roomId: string, eventId: string, serverCandidates: string[], ispill = false): string { - if (ispill) { - return `https://matrix.to/#/${roomId}/${eventId}${this.encodeServerCandidates(serverCandidates)}`; - } + public forEvent(roomId: string, eventId: string, serverCandidates: string[]): string { return `${this.elementUrl}/#/room/${roomId}/${eventId}${this.encodeServerCandidates(serverCandidates)}`; } - public forRoom(roomIdOrAlias: string, serverCandidates?: string[], ispill = false): string { - if (ispill) { - return `https://matrix.to/#/${roomIdOrAlias}${this.encodeServerCandidates(serverCandidates)}`; - } + public forRoom(roomIdOrAlias: string, serverCandidates?: string[]): string { return `${this.elementUrl}/#/room/${roomIdOrAlias}${this.encodeServerCandidates(serverCandidates)}`; } - public forUser(userId: string, ispill = false): string { - if (ispill) { - return `https://matrix.to/#/${userId}`; - } + public forUser(userId: string): string { return `${this.elementUrl}/#/user/${userId}`; } - public forEntity(entityId: string, ispill = false): string { + public forEntity(entityId: string): string { if (entityId[0] === "!" || entityId[0] === "#") { - return this.forRoom(entityId, [], ispill); + return this.forRoom(entityId); } else if (entityId[0] === "@") { - return this.forUser(entityId, ispill); + return this.forUser(entityId); } else throw new Error("Unrecognized entity"); } diff --git a/src/utils/permalinks/PermalinkConstructor.ts b/src/utils/permalinks/PermalinkConstructor.ts index a241d38ec80..4248b97885e 100644 --- a/src/utils/permalinks/PermalinkConstructor.ts +++ b/src/utils/permalinks/PermalinkConstructor.ts @@ -19,19 +19,19 @@ limitations under the License. * TODO: Convert this to a real TypeScript interface */ export default class PermalinkConstructor { - public forEvent(roomId: string, eventId: string, serverCandidates: string[] = [], ispill = false): string { + public forEvent(roomId: string, eventId: string, serverCandidates: string[] = []): string { throw new Error("Not implemented"); } - public forRoom(roomIdOrAlias: string, serverCandidates: string[] = [], ispill = false): string { + public forRoom(roomIdOrAlias: string, serverCandidates: string[] = []): string { throw new Error("Not implemented"); } - public forUser(userId: string, ispill = false): string { + public forUser(userId: string): string { throw new Error("Not implemented"); } - public forEntity(entityId: string, ispill = false): string { + public forEntity(entityId: string): string { throw new Error("Not implemented"); } diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index 0bdbd49428f..1c2526b49e9 100644 --- a/src/utils/permalinks/Permalinks.ts +++ b/src/utils/permalinks/Permalinks.ts @@ -274,26 +274,48 @@ export class RoomPermalinkCreator { }; } -export function makeGenericPermalink(entityId: string, ispill = false): string { - return getPermalinkConstructor().forEntity(entityId, ispill); +/** + * 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, ispill = false): 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, ispill = false): 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, [], ispill); + if (roomId[0] !== "!") return getPermalinkConstructor(isPill).forRoom(roomId, []); const room = matrixClient.getRoom(roomId); if (!room) { - return getPermalinkConstructor().forRoom(roomId, [], ispill); + 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); }