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

@@ -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 => {
+		mxids: joined.flatMap(j => {
-			const localpart = j.mxid.match(/@([^:]*)/)
+			const parsed = mxUtils.parseMxid(j.mxid)
-			assert(localpart)
+			if (!parsed) return []
-			return {
+			return [{
-				localpart: localpart[1].toLowerCase(),
+				localpart: parsed.localpart.toLowerCase(),
 				mxid: j.mxid,
 				displayname: j.displayname
-			}
+			}]
 		}),
 		names: joined.filter(j => j.displayname).map(j => {
 			return {

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(/^@([^:]*)/)
+					repliedToDisplayName = referenced.author.username || mxUtils.parseMxid(repliedToEventSenderMxid)?.localpart || "a Matrix user" // grab the localpart as the display name, whatever
-					assert(match)
-					repliedToDisplayName = referenced.author.username || match[1] || "a Matrix user" // grab the localpart as the display name, whatever
 					repliedToUserHtml = tag`<a href="https://matrix.to/#/${repliedToEventSenderMxid}">${repliedToDisplayName}</a>`
 				} else {
 					repliedToDisplayName = referenced.author.global_name || referenced.author.username || "a Discord user"

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<DiscordTypes.APIInteractionResponse>}
  */
 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)

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]

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: {

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(/^@(.*?):/)
+	const localpart = mxUtils.parseMxid(event.sender)?.localpart
-	if (match) displayName = match[1]
+	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

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(/:(.*)/)
+		const serverName = parseMxid(mxid)?.serverName
-		assert(match)
+		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
+		if (candidates.includes(serverName)) continue // from a different server
-		candidates.push(match[1])
+		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 => {
+		.flatMap(mxid => parseMxid(mxid)?.serverName ?? [])
-			const match = mxid.match(/:(.*)/)
+		.filter(server => !server.match(serverNameIpAddressRegex)) // No IPv4/IPv6 servers
-			assert(match)
-			return match[1]
-		})
-		.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`@(?<localpart>${mxidLocalpartPattern}):(?<serverName>${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

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

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()

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]
 

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({

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 => {