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

@@ -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) {
+		if (mxcUrl && mxUtils.parseMxc(mxcUrl) && titleTextMatch) {
-			img.replaceWith(convertEmoji(mxcUrlMatch[0], titleTextMatch[1], false, false))
+			img.replaceWith(convertEmoji(mxcUrl, titleTextMatch[1], false, false))
 		}
 	}
 	// convert <br> to \n since domino does not support innerText

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_-]+)$/)
+	const mediaParts = mxUtils.parseMxc(mxc)
-	assert(mediaParts)
+	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}`

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) {
+			if (!toUpload.length && mxcUrl && nameMatch) {
-				toUpload.push({url: mxcMatch[1], name: nameMatch[1]})
+				toUpload.push({url: mxcUrl, name: nameMatch[1]})
 			}
 			if (!toUpload.length) {
 				return api.sendEvent(event.room_id, "m.room.message", {

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(_serverNameIpAddressRegex)) // No IPv4/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
 		// 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:\/\/(?<serverName>${serverNamePattern})\/(?<mediaId>${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

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

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(),
+		server_name: z.stringFormat("server_name", mxUtils.isServerName),
-		media_id: z.string()
+		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(/^[^/]+$/),
+		server_name: z.stringFormat("server_name", mxUtils.isServerName),
-		media_id: z.string().regex(/^[A-Za-z0-9_-]+$/)
+		media_id: z.stringFormat("media_id", mxUtils.isMediaId)
 	})
 }