From 1bf53e27017fc2e1e5040798407749dc00c2bc79 Mon Sep 17 00:00:00 2001 From: l1s5 Date: Sat, 31 Oct 2026 23:55:01 +0900 Subject: [PATCH 1/2] matrix: stricter mxc validation This commit refactors various instances of mxc matching so that there is a single well-tested regex that can be reused everywhere. The new regex is made to match the specification word for word, and should hopefully rule out most over- and undermatching. --- src/m2d/converters/event-to-message.js | 8 +- src/matrix/api.js | 12 +-- src/matrix/matrix-command-handler.js | 6 +- src/matrix/utils.js | 103 ++++++++++++++++++++++++- src/matrix/utils.test.js | 85 +++++++++++++++++++- src/web/routes/download-matrix.js | 10 ++- 6 files changed, 203 insertions(+), 21 deletions(-) diff --git a/src/m2d/converters/event-to-message.js b/src/m2d/converters/event-to-message.js index ced55a6..304ae22 100644 --- a/src/m2d/converters/event-to-message.js +++ b/src/m2d/converters/event-to-message.js @@ -244,7 +244,7 @@ function convertEmoji(mxcUrl, nameForGuess, allowSpriteSheetIndicator, allowLink if (!found) row = null } // Or, if we don't have an emoji right now, we search for the name instead. - const isLocalMxc = mxcUrl?.match(/^mxc:\/\/([^/]+)/)?.[1] === reg.ooye.server_name + const isLocalMxc = mxUtils.parseMxc(mxcUrl)?.serverName === reg.ooye.server_name if (!row && nameForGuess && isLocalMxc) { const nameForGuessLower = nameForGuess.toLowerCase() for (const guild of discord.guilds.values()) { @@ -542,10 +542,10 @@ function serialiseReplyContent(doc) { body.querySelectorAll("span[data-mx-spoiler]").forEach(e => e.textContent = "[spoiler]") // @ts-ignore - domino FilteredElementList does not implement NodeList for (const img of body.querySelectorAll("img").cache) { - const mxcUrlMatch = img.getAttribute("src")?.match(/mxc:\/\/[^"]+/) + const mxcUrl = img.getAttribute("src") const titleTextMatch = img.getAttribute("title")?.match(/:?([^:"]+)/) - if (mxcUrlMatch && titleTextMatch) { - img.replaceWith(convertEmoji(mxcUrlMatch[0], titleTextMatch[1], false, false)) + if (mxcUrl && mxUtils.parseMxc(mxcUrl) && titleTextMatch) { + img.replaceWith(convertEmoji(mxcUrl, titleTextMatch[1], false, false)) } } // convert
to \n since domino does not support innerText diff --git a/src/matrix/api.js b/src/matrix/api.js index 6edd1d2..a2a1ee8 100644 --- a/src/matrix/api.js +++ b/src/matrix/api.js @@ -10,6 +10,8 @@ const {sync, db, select} = passthrough const mreq = sync.require("./mreq") /** @type {import("./txnid")} */ const makeTxnId = sync.require("./txnid") +/** @type {import("./utils")} */ +const mxUtils = sync.require("./utils") const {reg} = require("./read-registration.js") const route = (strs, ...params) => strs.slice(1).reduce((a, c, i) => a + encodeURIComponent(params[i]) + c, strs[0]) @@ -476,18 +478,18 @@ async function ping() { async function getMedia(mxc, init = {}) { init = {...init} - const mediaParts = mxc?.match(/^mxc:\/\/([^/]+)\/([a-zA-Z0-9_-]+)$/) - assert(mediaParts) + const mediaParts = mxUtils.parseMxc(mxc) + assert(mediaParts, `not a valid mxc URL: ${JSON.stringify(mxc)}`) - let route = "download" + let endpoint = "download" let query = "" if (init.thumbnail) { - route = "thumbnail" + endpoint = "thumbnail" query = "?" + new URLSearchParams(Object.keys(init.thumbnail).map(k => [k, String(init.thumbnail?.[k])])) } - let url = `${mreq.baseUrl}/client/v1/media/${route}/${mediaParts[1]}/${mediaParts[2]}${query}` + const url = mreq.baseUrl + route`/client/v1/media/${endpoint}/${mediaParts.serverName}/${mediaParts.mediaId}` + query const res = await fetch(url, { headers: { Authorization: `Bearer ${reg.as_token}` diff --git a/src/matrix/matrix-command-handler.js b/src/matrix/matrix-command-handler.js index 4a27d9e..202c8fd 100644 --- a/src/matrix/matrix-command-handler.js +++ b/src/matrix/matrix-command-handler.js @@ -152,7 +152,7 @@ const commands = [{ /** @type {{url: string, name: string}[]} */ const toUpload = [] const nameMatch = realBody.match(/:([a-zA-Z0-9_]{2,}):/) - const mxcMatch = realBody.match(/(mxc:\/\/.*?)\b/) + const mxcUrl = mxUtils.findMxc(realBody)?.mxc if (event.content["m.relates_to"]?.["m.in_reply_to"]?.event_id) { const repliedToEventID = event.content["m.relates_to"]["m.in_reply_to"].event_id const repliedToEvent = await api.getEvent(event.room_id, repliedToEventID) @@ -171,8 +171,8 @@ const commands = [{ } } } - if (!toUpload.length && mxcMatch && nameMatch) { - toUpload.push({url: mxcMatch[1], name: nameMatch[1]}) + if (!toUpload.length && mxcUrl && nameMatch) { + toUpload.push({url: mxcUrl, name: nameMatch[1]}) } if (!toUpload.length) { return api.sendEvent(event.room_id, "m.room.message", { diff --git a/src/matrix/utils.js b/src/matrix/utils.js index 554baf3..3bf372b 100644 --- a/src/matrix/utils.js +++ b/src/matrix/utils.js @@ -188,8 +188,7 @@ async function getViaServers(roomID, api) { assert(match) return match[1] }) - .filter(server => !server.match(/^\[[a-f0-9:\]]+$/)) // No IPv6 servers - .filter(server => !server.match(/[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}/)) // No IPv4 servers + .filter(server => !server.match(_serverNameIpAddressRegex)) // No IPv4/IPv6 servers // I don't care enough to check ACLs .forEach(server => { const existing = servers.get(server) @@ -244,16 +243,108 @@ function getPublicUrlForMxc(mxc) { return `${reg.ooye.bridge_origin}/download/matrix/${serverAndMediaID}` } +const serverNameIpv4Address = String.raw`[0-9]{1,3}(?:\.[0-9]{1,3}){3}` +const serverNameIpv6Address = String.raw`[0-9A-Fa-f:.]{2,45}` +const serverNameDnsName = String.raw`[0-9A-Za-z.-]{1,255}` +const serverNameHostname = String.raw`(?:${serverNameIpv4Address}|\[${serverNameIpv6Address}\]|${serverNameDnsName})` +const serverNamePort = String.raw`[0-9]{1,5}` + +/** + * Pattern to match a `server-name`. + * @see https://spec.matrix.org/latest/appendices/#server-name specification + */ +const serverNamePattern = String.raw`${serverNameHostname}(?::${serverNamePort})?` +const serverNameRegex = new RegExp(String.raw`^${serverNamePattern}$`) +const serverNameIpAddressRegex = new RegExp(String.raw`^(?:${serverNameIpv4Address}|\[${serverNameIpv6Address}\])(?::${serverNamePort})?$`) + +/** + * Validate a server name according to the Matrix specification. + * @param {string} serverName + * @returns {boolean} + */ +function isServerName(serverName) { + if (serverName.startsWith(".") || serverName.endsWith(".")) return false + return serverNameRegex.test(serverName) +} + +/** + * Pattern to match a `media-id`, typically used in mxc:// URIs. + * @see https://spec.matrix.org/latest/client-server-api/#matrix-content-mxc-uris specification + * @see https://spec.matrix.org/latest/client-server-api/#content-repo-security-considerations mxc security considerations + */ +const mediaIdPattern = String.raw`[A-Za-z0-9_-]+` +const mediaIdRegex = new RegExp(String.raw`^${mediaIdPattern}$`) + +/** + * Validate a media ID according to the Matrix specification. + * @param {string} mediaId + * @returns {boolean} + */ +function isMediaId(mediaId) { + return mediaIdRegex.test(mediaId) +} + +/** + * Pattern to match an `mxc://` URI. + * Captures the components in named groups: `serverName` and `mediaId`. + * @see https://spec.matrix.org/latest/client-server-api/#matrix-content-mxc-uris specification + */ +const mxcUrlPattern = String.raw`mxc:\/\/(?${serverNamePattern})\/(?${mediaIdPattern})` +const anchoredMxcUriRegex = new RegExp(String.raw`^${mxcUrlPattern}$`) +const unanchoredMxcUriRegex = new RegExp(String.raw`\b${mxcUrlPattern}\b`, "g") + +/** + * @param {string} mxc + * @param {{[key: string]: string}} matchGroups + * @returns {{mxc: string, serverName: string, mediaId: string} | null} null if it isn't a valid mxc URI + */ +function _mxcFromMatchGroups(mxc, matchGroups) { + // A server name starting or ending with "." is *technically* valid, but not in practise. + // This also rules out "." and "..", which would become dot-segments (path traversal) in URL paths. + if (matchGroups.serverName.startsWith(".") || matchGroups.serverName.endsWith(".")) return null + return { + mxc: mxc, + serverName: matchGroups.serverName, + mediaId: matchGroups.mediaId, + } +} + +/** + * Parse an `mxc://` URI into its server name and media ID components. + * @param {string | null | undefined} mxc + * @returns {{mxc: string, serverName: string, mediaId: string} | null} null if it isn't a valid mxc URI + */ +function parseMxc(mxc) { + const matchGroups = mxc?.match(anchoredMxcUriRegex)?.groups + if (!mxc || !matchGroups) return null + return _mxcFromMatchGroups(mxc, matchGroups) +} + +/** + * Find and parse the first valid `mxc://` URI in a string, e.g. from a message body. + * @param {string | null | undefined} content + * @returns {{mxc: string, serverName: string, mediaId: string} | null} null if it doesn't contain a valid mxc URI + */ +function findMxc(content) { + if (!content) return null + for (const match of content.matchAll(unanchoredMxcUriRegex)) { + if (!match.groups) continue + const parsed = _mxcFromMatchGroups(match[0], match.groups) + if (parsed) return parsed + } + return null +} + /** * @param {string | null | undefined} mxc * @returns {string | undefined} mxc URL with protocol stripped, e.g. "cadence.moe/abcdef1234" */ function makeMxcPublic(mxc) { assert(hasher, "xxhash is not ready yet") - const mediaParts = mxc?.match(/^mxc:\/\/([^/]+)\/([a-zA-Z0-9_-]+)$/) + const mediaParts = parseMxc(mxc) if (!mediaParts) return undefined - const serverAndMediaID = `${mediaParts[1]}/${mediaParts[2]}` + const serverAndMediaID = `${mediaParts.serverName}/${mediaParts.mediaId}` const unsignedHash = hasher.h64(serverAndMediaID) const signedHash = unsignedHash - 0x8000000000000000n // shifting down to signed 64-bit range db.prepare("INSERT OR IGNORE INTO media_proxy (permitted_hash) VALUES (?)").run(signedHash) @@ -388,6 +479,10 @@ async function setUserPowerCascade(spaceID, mxid, power, api) { module.exports.bot = bot module.exports.BLOCK_ELEMENTS = BLOCK_ELEMENTS module.exports.eventSenderIsFromDiscord = eventSenderIsFromDiscord +module.exports.isServerName = isServerName +module.exports.isMediaId = isMediaId +module.exports.parseMxc = parseMxc +module.exports.findMxc = findMxc module.exports.makeMxcPublic = makeMxcPublic module.exports.getPublicUrlForMxc = getPublicUrlForMxc module.exports.getEventIDHash = getEventIDHash diff --git a/src/matrix/utils.test.js b/src/matrix/utils.test.js index 842c513..567a0a5 100644 --- a/src/matrix/utils.test.js +++ b/src/matrix/utils.test.js @@ -2,7 +2,7 @@ const {select} = require("../passthrough") const {test} = require("supertape") -const {eventSenderIsFromDiscord, getEventIDHash, MatrixStringBuilder, getViaServers, roomHasAtLeastVersion, removeCreatorsFromPowerLevels, setUserPower} = require("./utils") +const {eventSenderIsFromDiscord, getEventIDHash, MatrixStringBuilder, getViaServers, roomHasAtLeastVersion, removeCreatorsFromPowerLevels, setUserPower, parseMxc, findMxc} = require("./utils"); const util = require("util") /** @param {string[]} mxids */ @@ -417,4 +417,87 @@ test("set user power: privileged users must demote themselves", async t => { t.equal(called, 3) }) +const VALID_MXCS = [ + { serverName: "cadence.moe", mediaId: "KrwlqopRyMxnEBcWDgpJZPxh" }, + { serverName: "cadence.moe", mediaId: "a_b-c" }, + { serverName: "cadence.moe", mediaId: "-" }, + { serverName: "cadence.moe", mediaId: "_" }, + { serverName: "matrix.example.org:8448", mediaId: "abc" }, + { serverName: "xn--mnchen-3ya.example", mediaId: "abc" }, // münchen in punycode + { serverName: "Matrix.Example.org", mediaId: "abc" }, + { serverName: "123example.456example", mediaId: "abc" }, + { serverName: "localhost", mediaId: "abc" }, + { serverName: "localhost:8008", mediaId: "abc" }, + { serverName: "192.0.2.1", mediaId: "abc" }, + { serverName: "192.0.2.1:8448", mediaId: "abc" }, + { serverName: "[::1]", mediaId: "abc" }, + { serverName: "[2001:db8::]:8448", mediaId: "abc" }, + { serverName: "[2001:db8::1]:8448", mediaId: "abc" }, + { serverName: "[2001:db8:0:0:0:0:0:1]", mediaId: "abc" }, + { serverName: "[::ffff:192.0.2.1]", mediaId: "abc" }, +]; + +for (const expected of VALID_MXCS) { + const mxc = `mxc://${expected.serverName}/${expected.mediaId}` + expected.mxc = mxc + test(`parse mxc: ${mxc} is valid`, t => { + t.deepEqual(parseMxc(mxc), expected) + }) +} + +const INVALID_MXCS = [ + null, + undefined, + "", + "mxc://./abcd", + "mxc://../abcd", + "mxc://.example.org/abcd", + "mxc://example.org./abcd", + "mxc://example.org?x=1/abcd", + "mxc://example.org#x/abcd", + "mxc://example.org\\x/abcd", + "mxc://example.org\\abcd", + "mxc://%2e%2e/abcd", + "mxc://%2E%2E/abcd", + "mxc://example.org%2Fx/abcd", + "mxc://example.org%2F./abcd", + "mxc://example.org%2F../abcd", + "mxc://example.org/%2F..", + "mxc://exa mple.org/abcd", + "mxc://exa%20mple.org/abcd", + "mxc:///abcd", + "mxc://example.org/a/b", + "mxc://example.org/..", + "mxc://example.org/", + "https://example.org/abcd", + "mxc://..\\..\\..\\v3\\account\\whoami#/abcd", +]; + +for (const mxc of INVALID_MXCS) { + test(`parse mxc: ${mxc} is invalid`, t => { + t.equal(parseMxc(mxc), null) + }) +} + + +test("find mxc: finds mxc in surrounding text", t => { + t.deepEqual( + findMxc("look at mxc://cadence.moe/abc please"), + { mxc: "mxc://cadence.moe/abc", serverName: "cadence.moe", mediaId: "abc" }, + ) +}) + +test("find mxc: skips invalid matches, returns first valid one", t => { + t.deepEqual( + findMxc("mxc://../abc mxc://./abc mxc://cadence.moe/def mxc://cadende.moe/ghi"), + { mxc: "mxc://cadence.moe/def", serverName: "cadence.moe", mediaId: "def" }, + ) +}) + +test("find mxc: skips invalid matches, returns undefined", t => { + t.equal(findMxc("mxc://../abc mxc://./abc"), null) + t.equal(findMxc(""), null) + t.equal(findMxc(null), null) +}) + module.exports.mockGetEffectivePower = mockGetEffectivePower diff --git a/src/web/routes/download-matrix.js b/src/web/routes/download-matrix.js index f360a17..4587488 100644 --- a/src/web/routes/download-matrix.js +++ b/src/web/routes/download-matrix.js @@ -21,6 +21,8 @@ const emojiSheetConverter = sync.require("../../m2d/converters/emoji-sheet") /** @type {import("../../m2d/actions/sticker")} */ const sticker = sync.require("../../m2d/actions/sticker") +/** @type {import("../../matrix/utils")} */ +const mxUtils = sync.require("../../matrix/utils") const INLINE_CONTENT_TYPES = new Set([ "text/css", @@ -66,8 +68,8 @@ const MEDIA_THUMBNAIL_PRESETS = { const schema = { media: z.object({ - server_name: z.string(), - media_id: z.string() + server_name: z.stringFormat("server_name", mxUtils.isServerName), + media_id: z.stringFormat("media_id", mxUtils.isMediaId) }), mediaQuery: z.object({ preset: z.enum(Object.keys(MEDIA_THUMBNAIL_PRESETS)) // list of possible thumbnail presets @@ -76,8 +78,8 @@ const schema = { e: z.array(z.string()).or(z.string()) }), sticker: z.object({ - server_name: z.string().regex(/^[^/]+$/), - media_id: z.string().regex(/^[A-Za-z0-9_-]+$/) + server_name: z.stringFormat("server_name", mxUtils.isServerName), + media_id: z.stringFormat("media_id", mxUtils.isMediaId) }) } -- 2.34.1 From c70597a665a34891d35c2c4b305f44214cd808b2 Mon Sep 17 00:00:00 2001 From: l1s5 Date: Tue, 3 Nov 2026 01:37:16 +0900 Subject: [PATCH 2/2] matrix: stricter mxid validation This commit refactors various instances of mxid matching so that there is a single well-tested regex that can be reused everywhere. The new regex is made to match the specification word for word, and should hopefully rule out most over- and undermatching. By chance, this also fixes a bug in `src/discord/interactions/ping.js` where the autocomplete tried matching an mxid against a raw sql query rather than the provided input. --- src/d2m/converters/find-mentions.js | 17 ++--- src/d2m/converters/message-to-event.js | 4 +- src/discord/interactions/invite.js | 6 +- src/discord/interactions/permissions.js | 2 +- src/discord/interactions/ping.js | 4 +- src/m2d/converters/event-to-message.js | 8 +-- src/matrix/utils.js | 81 +++++++++++++++++---- src/matrix/utils.test.js | 86 ++++++++++++++++++++++- src/web/routes/guild.js | 2 +- src/web/routes/link.js | 2 +- src/web/routes/log-in-with-matrix.js | 4 +- src/web/routes/log-in-with-matrix.test.js | 4 +- 12 files changed, 181 insertions(+), 39 deletions(-) diff --git a/src/d2m/converters/find-mentions.js b/src/d2m/converters/find-mentions.js index e9c51c5..0107aa7 100644 --- a/src/d2m/converters/find-mentions.js +++ b/src/d2m/converters/find-mentions.js @@ -1,8 +1,9 @@ // @ts-check -const assert = require("assert").strict - const {reg} = require("../../matrix/read-registration") +const {sync} = require("../../passthrough") +/** @type {import("../../matrix/utils")} */ +const mxUtils = sync.require("../../matrix/utils") const userRegex = reg.namespaces.users.map(u => new RegExp(u.regex)) /** @@ -108,14 +109,14 @@ function tokenise(name) { */ function processJoined(joined) { return { - mxids: joined.map(j => { - const localpart = j.mxid.match(/@([^:]*)/) - assert(localpart) - return { - localpart: localpart[1].toLowerCase(), + mxids: joined.flatMap(j => { + const parsed = mxUtils.parseMxid(j.mxid) + if (!parsed) return [] + return [{ + localpart: parsed.localpart.toLowerCase(), mxid: j.mxid, displayname: j.displayname - } + }] }), names: joined.filter(j => j.displayname).map(j => { return { diff --git a/src/d2m/converters/message-to-event.js b/src/d2m/converters/message-to-event.js index 02ee613..9376154 100644 --- a/src/d2m/converters/message-to-event.js +++ b/src/d2m/converters/message-to-event.js @@ -735,9 +735,7 @@ async function messageToEvent(message, guild, options = {}, di) { let repliedToDisplayName let repliedToUserHtml if (repliedToEventRow?.source === 0 && repliedToEventSenderMxid) { - const match = repliedToEventSenderMxid.match(/^@([^:]*)/) - assert(match) - repliedToDisplayName = referenced.author.username || match[1] || "a Matrix user" // grab the localpart as the display name, whatever + repliedToDisplayName = referenced.author.username || mxUtils.parseMxid(repliedToEventSenderMxid)?.localpart || "a Matrix user" // grab the localpart as the display name, whatever repliedToUserHtml = tag`${repliedToDisplayName}` } else { repliedToDisplayName = referenced.author.global_name || referenced.author.username || "a Discord user" diff --git a/src/discord/interactions/invite.js b/src/discord/interactions/invite.js index 9c2c5ed..5b6e7c4 100644 --- a/src/discord/interactions/invite.js +++ b/src/discord/interactions/invite.js @@ -12,6 +12,8 @@ const createRoom = sync.require("../../d2m/actions/create-room") const createSpace = sync.require("../../d2m/actions/create-space") /** @type {import("../../matrix/api")} */ const api = sync.require("../../matrix/api") +/** @type {import("../../matrix/utils")} */ +const utils = sync.require("../../matrix/utils") /** @type {import("../../matrix/read-registration")} */ const {reg} = sync.require("../../matrix/read-registration") @@ -35,7 +37,7 @@ async function* _interact({data, channel, guild_id}, {api}) { /** @type {DiscordTypes.APIApplicationCommandInteractionDataStringOption[] | undefined} */ // @ts-ignore const options = data.options const input = options?.[0]?.value || "" - const mxid = input.match(/@[^@:]+:[a-z0-9:.-]+/)?.[0] + const mxid = utils.findMxid(input)?.mxid if (!mxid) return yield {createInteractionResponse: { type: DiscordTypes.InteractionResponseType.ChannelMessageWithSource, data: { @@ -117,7 +119,7 @@ async function* _interact({data, channel, guild_id}, {api}) { * @returns {Promise} */ async function _interactButton({channel, message}, {api}) { - const mxid = message.content.match(/`(@[^@:]+:[a-z0-9:-]+\.[a-z0-9.:-]+)`/)?.[1] + const mxid = utils.findMxid(message.content)?.mxid assert(mxid) const roomID = select("channel_room", "room_id", {channel_id: channel.id}).pluck().get() await api.inviteToRoom(roomID, mxid) diff --git a/src/discord/interactions/permissions.js b/src/discord/interactions/permissions.js index ce4af6c..34c89d3 100644 --- a/src/discord/interactions/permissions.js +++ b/src/discord/interactions/permissions.js @@ -104,7 +104,7 @@ async function* _interact({data, guild_id}, {api, utils}) { */ async function* _interactEdit({data, guild_id, message}, {api}) { // Get the person that will be inspected/edited - const mxid = message.content.match(/`(@[^@:]+:[a-z0-9:.-]+)`/)?.[1] + const mxid = utils.findMxid(message.content)?.mxid assert(mxid) const permission = data.values[0] diff --git a/src/discord/interactions/ping.js b/src/discord/interactions/ping.js index 45824be..1fd1224 100644 --- a/src/discord/interactions/ping.js +++ b/src/discord/interactions/ping.js @@ -68,7 +68,7 @@ async function* _interactAutocomplete({data, channel}, {api}) { let mxidMatches = select("member_cache", ["mxid", "displayname"], {room_id: roomID}, "AND displayname IS NOT NULL AND mxid LIKE ? ESCAPE '$' LIMIT 25").all(query) mxidMatches = mxidMatches.filter(match => { // don't include matches in domain part of mxid - if (!match.mxid.match(/^[^:]*/)?.includes(query)) return false + if (!utils.parseMxid(match.mxid)?.localpart.toLowerCase().includes(input.toLowerCase())) return false if (displaynameMatches.some(m => m.mxid === match.mxid)) return false return true }) @@ -118,7 +118,7 @@ async function* _interactCommand({data, channel, guild_id}, {api}) { assert(data.options?.[0]?.type === DiscordTypes.ApplicationCommandOptionType.String) const mxid = data.options[0].value - if (!mxid.match(/^@[^:]*:./)) { + if (!utils.parseMxid(mxid)) { return yield {createInteractionResponse: { type: DiscordTypes.InteractionResponseType.ChannelMessageWithSource, data: { diff --git a/src/m2d/converters/event-to-message.js b/src/m2d/converters/event-to-message.js index 304ae22..ddad94d 100644 --- a/src/m2d/converters/event-to-message.js +++ b/src/m2d/converters/event-to-message.js @@ -572,8 +572,8 @@ async function eventToMessage(event, guild, channel, di) { let replyLine = "" // Extract a basic display name from the sender - const match = event.sender.match(/^@(.*?):/) - if (match) displayName = match[1] + const localpart = mxUtils.parseMxid(event.sender)?.localpart + if (localpart) displayName = localpart // Try to extract an accurate display name and avatar URL from the member event const member = await getMemberFromCacheOrHomeserver(event.room_id, event.sender, di?.api) @@ -872,7 +872,7 @@ async function eventToMessage(event, guild, channel, di) { replyLine += authorMention } else { let senderName = select("member_cache", "displayname", {mxid: sender}).pluck().get() - if (!senderName) senderName = sender.match(/@([^:]*)/)?.[1] + if (!senderName) senderName = mxUtils.parseMxid(sender)?.localpart if (senderName) replyLine += `**Ⓜ${senderName}**` } replyLine = `-# > ${replyLine}${contentPreview}\n` @@ -896,7 +896,7 @@ async function eventToMessage(event, guild, channel, di) { } else { // Handle mention of a Matrix user by their mxid // Check if this Matrix user is actually the sim user from another old bridge in the room? - const match = mxid.match(/[^:]*discord[^:]*_([0-9]{6,}):/) // try to match @_discord_123456, @_discordpuppet_123456, etc. + const match = mxUtils.parseMxid(mxid)?.localpart.match(/discord.*_([0-9]{6,})$/) // try to match @_discord_123456, @_discordpuppet_123456, etc. if (match) return `${attributeValue} data-user-id="${match[1]}">` // Nope, just a real Matrix user. return whole diff --git a/src/matrix/utils.js b/src/matrix/utils.js index 3bf372b..61c188e 100644 --- a/src/matrix/utils.js +++ b/src/matrix/utils.js @@ -168,11 +168,11 @@ async function getViaServers(roomID, api) { for (const mxid of sorted) { if (!(mxid in joined)) continue // joined... if (userRegex.some(r => mxid.match(r))) continue // non-sim non-bot... - const match = mxid.match(/:(.*)/) - assert(match) + const serverName = parseMxid(mxid)?.serverName + if (!serverName) continue /* c8 ignore next - should be already covered by the userRegex test, but let's be explicit */ - if (candidates.includes(match[1])) continue // from a different server - candidates.push(match[1]) + if (candidates.includes(serverName)) continue // from a different server + candidates.push(serverName) break } // Candidates 2-3: Most popular servers in the room @@ -183,12 +183,8 @@ async function getViaServers(roomID, api) { .filter(mxid => !mxid.startsWith("@_")) // Quick check .filter(mxid => !userRegex.some(r => mxid.match(r))) // Full check .slice(0, 1000) // Just sample the first thousand real members - .map(mxid => { - const match = mxid.match(/:(.*)/) - assert(match) - return match[1] - }) - .filter(server => !server.match(_serverNameIpAddressRegex)) // No IPv4/IPv6 servers + .flatMap(mxid => parseMxid(mxid)?.serverName ?? []) + .filter(server => !server.match(serverNameIpAddressRegex)) // No IPv4/IPv6 servers // I don't care enough to check ACLs .forEach(server => { const existing = servers.get(server) @@ -335,6 +331,65 @@ function findMxc(content) { return null } +/** + * Pattern to match the localpart of a matrix user ID. + * This uses the historical grammar (any printable ASCII except ":"). + * This implementation also disallows "@" due to redos concerns. + * @see https://spec.matrix.org/latest/appendices/#user-identifiers specification + * @see https://spec.matrix.org/latest/appendices/#historical-user-ids historical user IDs + */ +const mxidLocalpartPattern = String.raw`[\x21-\x39\x3B-\x3F\x41-\x7E]*` + +/** + * Pattern to match a matrix user ID, e.g. "@cadence:cadence.moe". + * Captures the components in named groups: `localpart` and `serverName`. + * @see https://spec.matrix.org/latest/appendices/#user-identifiers specification + */ +const mxidPattern = String.raw`@(?${mxidLocalpartPattern}):(?${serverNamePattern})` +const unanchoredMxidRegex = new RegExp(mxidPattern, "g") +const anchoredMxidRegex = new RegExp(String.raw`^${mxidPattern}$`) + +/** + * @param {string} mxid + * @param {{[key: string]: string}} matchGroups + * @returns {{mxid: string, localpart: string, serverName: string} | null} null if it isn't a valid user ID + */ +function _mxidFromMatchGroups(mxid, matchGroups) { + if (mxid.length > 255) return null + if (matchGroups.serverName.startsWith(".") || matchGroups.serverName.endsWith(".")) return null + return { + mxid: mxid, + localpart: matchGroups.localpart, + serverName: matchGroups.serverName, + } +} + +/** + * Parse a matrix user ID into its localpart and server name components. + * @param {string | null | undefined} mxid + * @returns {{mxid: string, localpart: string, serverName: string} | null} null if it isn't a valid user ID + */ +function parseMxid(mxid) { + const matchGroups = mxid?.match(anchoredMxidRegex)?.groups + if (!mxid || !matchGroups) return null + return _mxidFromMatchGroups(mxid, matchGroups) +} + +/** + * Find and parse the first valid matrix user ID in a string, e.g. from a message body. + * @param {string | null | undefined} content + * @returns {{mxid: string, localpart: string, serverName: string} | null} null if it doesn't contain a valid user ID + */ +function findMxid(content) { + if (!content) return null + for (const match of content.matchAll(unanchoredMxidRegex)) { + if (!match.groups) continue + const parsed = _mxidFromMatchGroups(match[0], match.groups) + if (parsed) return parsed + } + return null +} + /** * @param {string | null | undefined} mxc * @returns {string | undefined} mxc URL with protocol stripped, e.g. "cadence.moe/abcdef1234" @@ -435,7 +490,7 @@ async function getEffectivePower(roomID, mxids, api) { */ async function setUserPower(roomID, mxid, newPower, api) { assert(roomID[0] === "!") - assert(mxid[0] === "@") + assert(parseMxid(mxid)) // Yes there's no shortcut https://github.com/matrix-org/matrix-appservice-bridge/blob/2334b0bae28a285a767fe7244dad59f5a5963037/src/components/intent.ts#L352 const {powerLevels, powers: {[mxid]: oldPowerLevel, [bot]: botPowerLevel}} = await getEffectivePower(roomID, [mxid, bot], api) @@ -465,7 +520,7 @@ async function setUserPower(roomID, mxid, newPower, api) { */ async function setUserPowerCascade(spaceID, mxid, power, api) { assert(spaceID[0] === "!") - assert(mxid[0] === "@") + assert(parseMxid(mxid)) let seenSpace = false for await (const room of api.generateFullHierarchy(spaceID)) { if (room.room_id === spaceID) seenSpace = true @@ -483,6 +538,8 @@ module.exports.isServerName = isServerName module.exports.isMediaId = isMediaId module.exports.parseMxc = parseMxc module.exports.findMxc = findMxc +module.exports.parseMxid = parseMxid +module.exports.findMxid = findMxid module.exports.makeMxcPublic = makeMxcPublic module.exports.getPublicUrlForMxc = getPublicUrlForMxc module.exports.getEventIDHash = getEventIDHash diff --git a/src/matrix/utils.test.js b/src/matrix/utils.test.js index 567a0a5..3bfffe2 100644 --- a/src/matrix/utils.test.js +++ b/src/matrix/utils.test.js @@ -2,7 +2,7 @@ const {select} = require("../passthrough") const {test} = require("supertape") -const {eventSenderIsFromDiscord, getEventIDHash, MatrixStringBuilder, getViaServers, roomHasAtLeastVersion, removeCreatorsFromPowerLevels, setUserPower, parseMxc, findMxc} = require("./utils"); +const {eventSenderIsFromDiscord, getEventIDHash, MatrixStringBuilder, getViaServers, roomHasAtLeastVersion, removeCreatorsFromPowerLevels, setUserPower, parseMxc, findMxc, parseMxid, findMxid} = require("./utils"); const util = require("util") /** @param {string[]} mxids */ @@ -479,7 +479,6 @@ for (const mxc of INVALID_MXCS) { }) } - test("find mxc: finds mxc in surrounding text", t => { t.deepEqual( findMxc("look at mxc://cadence.moe/abc please"), @@ -500,4 +499,87 @@ test("find mxc: skips invalid matches, returns undefined", t => { t.equal(findMxc(null), null) }) +const VALID_MXIDS = [ + { localpart: "cadence", serverName: "cadence.moe" }, + { localpart: "_ooye_bot", serverName: "cadence.moe" }, + { localpart: "Cadence", serverName: "cadence.moe" }, // historical + { localpart: "~!$%&'()*,;<>?[]^`{|}\"#\\", serverName: "cadence.moe" }, // historical + { localpart: "", serverName: "cadence.moe" }, + { localpart: "cadence", serverName: "matrix.example.org:8448" }, + { localpart: "cadence", serverName: "localhost" }, + { localpart: "cadence", serverName: "192.0.2.1:8448" }, + { localpart: "cadence", serverName: "[2001:db8::1]:8448" }, + { localpart: "a".repeat(255 - "@:cadence.moe".length), serverName: "cadence.moe" }, +]; + +for (const expected of VALID_MXIDS) { + const mxid = `@${expected.localpart}:${expected.serverName}` + expected.mxid = mxid + test(`parse mxid: ${mxid} is valid`, t => { + t.deepEqual(parseMxid(mxid), expected) + }) +} + +const INVALID_MXIDS = [ + null, + undefined, + "", + "@", + "@@:cadence.moe", + "@cadence", + "@cadence:", + "cadence:cadence.moe", + "!cadence:cadence.moe", + "#cadence:cadence.moe", + " @cadence:cadence.moe", + "@cadence:cadence.moe ", + "@cad ence:cadence.moe", + "@cadénce:cadence.moe", + "@cadence:cadence.moe:", + "@cadence:cadence.moe:port", + "@cadence:cadence.moe/path", + "@cadence:.cadence.moe", + "@cadence:cadence.moe.", + "@a:b:cadence.moe", + "@" + "a".repeat(256 - "@:cadence.moe".length) + ":cadence.moe", +]; + +for (const mxid of INVALID_MXIDS) { + test(`parse mxid: ${mxid} is invalid`, t => { + t.equal(parseMxid(mxid), null) + }) +} + +test("find mxid: finds mxid in surrounding text", t => { + t.deepEqual( + findMxid("invite `@cadence:cadence.moe` please"), + { mxid: "@cadence:cadence.moe", localpart: "cadence", serverName: "cadence.moe" }, + ) +}) + +test("find mxid: skips invalid matches, returns first valid one", t => { + const overlong = "@" + "a".repeat(256) + ":cadence.moe" + t.deepEqual( + findMxid(`${overlong} @::cadence.moe @cadence @cadence:cadence.moe @cadence2:cadence.moe`), + { mxid: "@cadence:cadence.moe", localpart: "cadence", serverName: "cadence.moe" }, + ) +}) + +test("find mxid: skips invalid matches, returns undefined", t => { + const overlong = "@" + "a".repeat(256) + ":cadence.moe" + t.equal(findMxid(`${overlong} @::cadence.moe @cadence`), null) +}) + +test("find mxid: skips multiple '@' occurences", t => { + t.deepEqual( + findMxid(" @@cadence:cadence.moe "), + { mxid: "@cadence:cadence.moe", localpart: "cadence", serverName: "cadence.moe" }, + ) + + t.deepEqual( + findMxid(" @@@cadence:cadence.moe "), + { mxid: "@cadence:cadence.moe", localpart: "cadence", serverName: "cadence.moe" }, + ) +}) + module.exports.mockGetEffectivePower = mockGetEffectivePower diff --git a/src/web/routes/guild.js b/src/web/routes/guild.js index b05fc13..557eeed 100644 --- a/src/web/routes/guild.js +++ b/src/web/routes/guild.js @@ -31,7 +31,7 @@ const schema = { guild_id: z.string().optional() }), invite: z.object({ - mxid: z.string().regex(/^@[^:]+:[a-z0-9:.-]+$/), + mxid: z.stringFormat("mxid", mxUtils.parseMxid), permissions: z.enum(["default", "moderator", "admin"]), guild_id: z.string().optional(), nonce: z.string().optional() diff --git a/src/web/routes/link.js b/src/web/routes/link.js index 8aba91f..a7abb20 100644 --- a/src/web/routes/link.js +++ b/src/web/routes/link.js @@ -119,7 +119,7 @@ as.router.post("/api/link-space", defineEventHandler(async event => { const inviteRow = guildRoute.getInviteTargetSpaces(session.data.mxid).find(s => s.room_id === spaceID) if (!inviteRow) throw createError({status: 403, message: "Forbidden", data: "You personally must invite OOYE to that space on Matrix"}) - const inviteServer = inviteRow.mxid.match(/:(.*)/)?.[1] + const inviteServer = utils.parseMxid(inviteRow.mxid)?.serverName assert(inviteServer) const via = [inviteServer] diff --git a/src/web/routes/log-in-with-matrix.js b/src/web/routes/log-in-with-matrix.js index 10cad45..425b678 100644 --- a/src/web/routes/log-in-with-matrix.js +++ b/src/web/routes/log-in-with-matrix.js @@ -14,10 +14,12 @@ const assert = require("assert").strict const pugSync = sync.require("../pug-sync") /** @type {import("../web-utils")} */ const webUtils = sync.require("../web-utils") +/** @type {import("../../matrix/utils")} */ +const mxUtils = sync.require("../../matrix/utils") const schema = { form: z.object({ - mxid: z.string().regex(/^@([^:]+):([a-z0-9:-]+\.[a-z0-9.:-]+)$/), + mxid: z.stringFormat("mxid", mxUtils.parseMxid), next: z.string().optional() }), token: z.object({ diff --git a/src/web/routes/log-in-with-matrix.test.js b/src/web/routes/log-in-with-matrix.test.js index d3746e4..cfcd203 100644 --- a/src/web/routes/log-in-with-matrix.test.js +++ b/src/web/routes/log-in-with-matrix.test.js @@ -22,7 +22,7 @@ test("log in with matrix: checks if mxid format looks valid", async t => { mxid: "x@cadence:cadence.moe" } })) - t.match(error.data.fieldErrors.mxid, /must match pattern/) + t.match(error.data.fieldErrors.mxid, /Invalid mxid/) }) test("log in with matrix: checks if mxid domain format looks valid", async t => { @@ -31,7 +31,7 @@ test("log in with matrix: checks if mxid domain format looks valid", async t => mxid: "@cadence:cadence." } })) - t.match(error.data.fieldErrors.mxid, /must match pattern/) + t.match(error.data.fieldErrors.mxid, /Invalid mxid/) }) test("log in with matrix: sends message to log in", async t => { -- 2.34.1