From e0a00e459f9d06f69e785e22d8e06af2a31b8c5b Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Thu, 12 Mar 2026 03:47:48 +0000 Subject: [PATCH] Matrix: harden media and credential handling --- extensions/matrix/src/matrix/client/config.ts | 1 + .../matrix/src/matrix/credentials.test.ts | 35 +++++++ extensions/matrix/src/matrix/credentials.ts | 8 +- .../matrix/src/matrix/monitor/media.test.ts | 26 ++++- extensions/matrix/src/matrix/monitor/media.ts | 24 ++--- extensions/matrix/src/matrix/sdk.ts | 13 ++- .../matrix/src/matrix/sdk/crypto-facade.ts | 17 +++- .../matrix/src/matrix/sdk/http-client.ts | 4 + .../matrix/sdk/read-response-with-limit.ts | 95 +++++++++++++++++++ .../matrix/src/matrix/sdk/transport.test.ts | 67 +++++++++++++ extensions/matrix/src/matrix/sdk/transport.ts | 23 ++++- 11 files changed, 287 insertions(+), 26 deletions(-) create mode 100644 extensions/matrix/src/matrix/sdk/read-response-with-limit.ts create mode 100644 extensions/matrix/src/matrix/sdk/transport.test.ts diff --git a/extensions/matrix/src/matrix/client/config.ts b/extensions/matrix/src/matrix/client/config.ts index cb2283375fa..147e1ab69f3 100644 --- a/extensions/matrix/src/matrix/client/config.ts +++ b/extensions/matrix/src/matrix/client/config.ts @@ -363,6 +363,7 @@ export async function resolveMatrixAuth(params?: { credentialsMatchConfig(cached, { homeserver: resolved.homeserver, userId: resolved.userId || "", + accessToken: resolved.accessToken, }) ? cached : null; diff --git a/extensions/matrix/src/matrix/credentials.test.ts b/extensions/matrix/src/matrix/credentials.test.ts index 637a15d7941..bb4da24748f 100644 --- a/extensions/matrix/src/matrix/credentials.test.ts +++ b/extensions/matrix/src/matrix/credentials.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { setMatrixRuntime } from "../runtime.js"; import { + credentialsMatchConfig, loadMatrixCredentials, clearMatrixCredentials, resolveMatrixCredentialsPath, @@ -116,4 +117,38 @@ describe("matrix credentials storage", () => { expect(fs.existsSync(currentPath)).toBe(false); expect(fs.existsSync(legacyPath)).toBe(false); }); + + it("requires a token match when userId is absent", () => { + expect( + credentialsMatchConfig( + { + homeserver: "https://matrix.example.org", + userId: "@old:example.org", + accessToken: "tok-old", + createdAt: "2026-01-01T00:00:00.000Z", + }, + { + homeserver: "https://matrix.example.org", + userId: "", + accessToken: "tok-new", + }, + ), + ).toBe(false); + + expect( + credentialsMatchConfig( + { + homeserver: "https://matrix.example.org", + userId: "@bot:example.org", + accessToken: "tok-123", + createdAt: "2026-01-01T00:00:00.000Z", + }, + { + homeserver: "https://matrix.example.org", + userId: "", + accessToken: "tok-123", + }, + ), + ).toBe(true); + }); }); diff --git a/extensions/matrix/src/matrix/credentials.ts b/extensions/matrix/src/matrix/credentials.ts index 95f29841953..70697d2db89 100644 --- a/extensions/matrix/src/matrix/credentials.ts +++ b/extensions/matrix/src/matrix/credentials.ts @@ -157,11 +157,13 @@ export function clearMatrixCredentials( export function credentialsMatchConfig( stored: MatrixStoredCredentials, - config: { homeserver: string; userId: string }, + config: { homeserver: string; userId: string; accessToken?: string }, ): boolean { - // If userId is empty (token-based auth), only match homeserver if (!config.userId) { - return stored.homeserver === config.homeserver; + if (!config.accessToken) { + return false; + } + return stored.homeserver === config.homeserver && stored.accessToken === config.accessToken; } return stored.homeserver === config.homeserver && stored.userId === config.userId; } diff --git a/extensions/matrix/src/matrix/monitor/media.test.ts b/extensions/matrix/src/matrix/monitor/media.test.ts index 7c690b3c4fa..19ee48cb57e 100644 --- a/extensions/matrix/src/matrix/monitor/media.test.ts +++ b/extensions/matrix/src/matrix/monitor/media.test.ts @@ -52,8 +52,10 @@ describe("downloadMatrixMedia", () => { file, }); - // decryptMedia should be called with just the file object (it handles download internally) - expect(decryptMedia).toHaveBeenCalledWith(file); + expect(decryptMedia).toHaveBeenCalledWith(file, { + maxBytes: 1024, + readIdleTimeoutMs: 30_000, + }); expect(saveMediaBuffer).toHaveBeenCalledWith( Buffer.from("decrypted"), "image/png", @@ -99,4 +101,24 @@ describe("downloadMatrixMedia", () => { expect(decryptMedia).not.toHaveBeenCalled(); expect(saveMediaBuffer).not.toHaveBeenCalled(); }); + + it("passes byte limits through plain media downloads", async () => { + const downloadContent = vi.fn().mockResolvedValue(Buffer.from("plain")); + + const client = { + downloadContent, + } as unknown as import("../sdk.js").MatrixClient; + + await downloadMatrixMedia({ + client, + mxcUrl: "mxc://example/file", + contentType: "image/png", + maxBytes: 4096, + }); + + expect(downloadContent).toHaveBeenCalledWith("mxc://example/file", { + maxBytes: 4096, + readIdleTimeoutMs: 30_000, + }); + }); }); diff --git a/extensions/matrix/src/matrix/monitor/media.ts b/extensions/matrix/src/matrix/monitor/media.ts index bfd31b51138..b099554ecee 100644 --- a/extensions/matrix/src/matrix/monitor/media.ts +++ b/extensions/matrix/src/matrix/monitor/media.ts @@ -16,25 +16,18 @@ type EncryptedFile = { v: string; }; +const MATRIX_MEDIA_DOWNLOAD_IDLE_TIMEOUT_MS = 30_000; + async function fetchMatrixMediaBuffer(params: { client: MatrixClient; mxcUrl: string; maxBytes: number; }): Promise<{ buffer: Buffer } | null> { - // The client wrapper exposes mxcToHttp for Matrix media URIs. - const url = params.client.mxcToHttp(params.mxcUrl); - if (!url) { - return null; - } - - // Use the client's download method which handles auth try { - const raw = await params.client.downloadContent(params.mxcUrl); - const buffer = Buffer.isBuffer(raw) ? raw : Buffer.from(raw); - - if (buffer.byteLength > params.maxBytes) { - throw new Error("Matrix media exceeds configured size limit"); - } + const buffer = await params.client.downloadContent(params.mxcUrl, { + maxBytes: params.maxBytes, + readIdleTimeoutMs: MATRIX_MEDIA_DOWNLOAD_IDLE_TIMEOUT_MS, + }); return { buffer }; } catch (err) { throw new Error(`Matrix media download failed: ${String(err)}`, { cause: err }); @@ -54,9 +47,12 @@ async function fetchEncryptedMediaBuffer(params: { throw new Error("Cannot decrypt media: crypto not enabled"); } - // decryptMedia handles downloading and decrypting the encrypted content internally const decrypted = await params.client.crypto.decryptMedia( params.file as Parameters[0], + { + maxBytes: params.maxBytes, + readIdleTimeoutMs: MATRIX_MEDIA_DOWNLOAD_IDLE_TIMEOUT_MS, + }, ); if (decrypted.byteLength > params.maxBytes) { diff --git a/extensions/matrix/src/matrix/sdk.ts b/extensions/matrix/src/matrix/sdk.ts index 173bcdcf4b6..5fcad2f70dd 100644 --- a/extensions/matrix/src/matrix/sdk.ts +++ b/extensions/matrix/src/matrix/sdk.ts @@ -570,7 +570,14 @@ export class MatrixClient { return this.client.mxcUrlToHttp(mxcUrl, undefined, undefined, undefined, true, false, true); } - async downloadContent(mxcUrl: string, allowRemote = true): Promise { + async downloadContent( + mxcUrl: string, + opts: { + allowRemote?: boolean; + maxBytes?: number; + readIdleTimeoutMs?: number; + } = {}, + ): Promise { const parsed = parseMxc(mxcUrl); if (!parsed) { throw new Error(`Invalid Matrix content URI: ${mxcUrl}`); @@ -579,8 +586,10 @@ export class MatrixClient { const response = await this.httpClient.requestRaw({ method: "GET", endpoint, - qs: { allow_remote: allowRemote }, + qs: { allow_remote: opts.allowRemote ?? true }, timeoutMs: this.localTimeoutMs, + maxBytes: opts.maxBytes, + readIdleTimeoutMs: opts.readIdleTimeoutMs, }); return response; } diff --git a/extensions/matrix/src/matrix/sdk/crypto-facade.ts b/extensions/matrix/src/matrix/sdk/crypto-facade.ts index e31131415cb..516ee2900ea 100644 --- a/extensions/matrix/src/matrix/sdk/crypto-facade.ts +++ b/extensions/matrix/src/matrix/sdk/crypto-facade.ts @@ -25,7 +25,10 @@ export type MatrixCryptoFacade = { isRoomEncrypted: (roomId: string) => Promise; requestOwnUserVerification: () => Promise; encryptMedia: (buffer: Buffer) => Promise<{ buffer: Buffer; file: Omit }>; - decryptMedia: (file: EncryptedFile) => Promise; + decryptMedia: ( + file: EncryptedFile, + opts?: { maxBytes?: number; readIdleTimeoutMs?: number }, + ) => Promise; getRecoveryKey: () => Promise<{ encodedPrivateKey?: string; keyId?: string | null; @@ -66,7 +69,10 @@ export function createMatrixCryptoFacade(deps: { eventType: string, stateKey?: string, ) => Promise>; - downloadContent: (mxcUrl: string) => Promise; + downloadContent: ( + mxcUrl: string, + opts?: { maxBytes?: number; readIdleTimeoutMs?: number }, + ) => Promise; }): MatrixCryptoFacade { return { prepare: async (_joinedRooms: string[]) => { @@ -116,8 +122,11 @@ export function createMatrixCryptoFacade(deps: { }, }; }, - decryptMedia: async (file: EncryptedFile): Promise => { - const encrypted = await deps.downloadContent(file.url); + decryptMedia: async ( + file: EncryptedFile, + opts?: { maxBytes?: number; readIdleTimeoutMs?: number }, + ): Promise => { + const encrypted = await deps.downloadContent(file.url, opts); const metadata: EncryptedFile = { url: file.url, key: file.key, diff --git a/extensions/matrix/src/matrix/sdk/http-client.ts b/extensions/matrix/src/matrix/sdk/http-client.ts index d047bcc9c77..638c845d48c 100644 --- a/extensions/matrix/src/matrix/sdk/http-client.ts +++ b/extensions/matrix/src/matrix/sdk/http-client.ts @@ -43,6 +43,8 @@ export class MatrixAuthedHttpClient { endpoint: string; qs?: QueryParams; timeoutMs: number; + maxBytes?: number; + readIdleTimeoutMs?: number; allowAbsoluteEndpoint?: boolean; }): Promise { const { response, buffer } = await performMatrixRequest({ @@ -53,6 +55,8 @@ export class MatrixAuthedHttpClient { qs: params.qs, timeoutMs: params.timeoutMs, raw: true, + maxBytes: params.maxBytes, + readIdleTimeoutMs: params.readIdleTimeoutMs, allowAbsoluteEndpoint: params.allowAbsoluteEndpoint, }); if (!response.ok) { diff --git a/extensions/matrix/src/matrix/sdk/read-response-with-limit.ts b/extensions/matrix/src/matrix/sdk/read-response-with-limit.ts new file mode 100644 index 00000000000..2077f56e5c3 --- /dev/null +++ b/extensions/matrix/src/matrix/sdk/read-response-with-limit.ts @@ -0,0 +1,95 @@ +async function readChunkWithIdleTimeout( + reader: ReadableStreamDefaultReader, + chunkTimeoutMs: number, +): Promise>> { + let timeoutId: ReturnType | undefined; + let timedOut = false; + + return await new Promise((resolve, reject) => { + const clear = () => { + if (timeoutId !== undefined) { + clearTimeout(timeoutId); + timeoutId = undefined; + } + }; + + timeoutId = setTimeout(() => { + timedOut = true; + clear(); + void reader.cancel().catch(() => undefined); + reject(new Error(`Matrix media download stalled: no data received for ${chunkTimeoutMs}ms`)); + }, chunkTimeoutMs); + + void reader.read().then( + (result) => { + clear(); + if (!timedOut) { + resolve(result); + } + }, + (err) => { + clear(); + if (!timedOut) { + reject(err); + } + }, + ); + }); +} + +export async function readResponseWithLimit( + res: Response, + maxBytes: number, + opts?: { + onOverflow?: (params: { size: number; maxBytes: number; res: Response }) => Error; + chunkTimeoutMs?: number; + }, +): Promise { + const onOverflow = + opts?.onOverflow ?? + ((params: { size: number; maxBytes: number }) => + new Error(`Content too large: ${params.size} bytes (limit: ${params.maxBytes} bytes)`)); + const chunkTimeoutMs = opts?.chunkTimeoutMs; + + const body = res.body; + if (!body || typeof body.getReader !== "function") { + const fallback = Buffer.from(await res.arrayBuffer()); + if (fallback.length > maxBytes) { + throw onOverflow({ size: fallback.length, maxBytes, res }); + } + return fallback; + } + + const reader = body.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + try { + while (true) { + const { done, value } = chunkTimeoutMs + ? await readChunkWithIdleTimeout(reader, chunkTimeoutMs) + : await reader.read(); + if (done) { + break; + } + if (value?.length) { + total += value.length; + if (total > maxBytes) { + try { + await reader.cancel(); + } catch {} + throw onOverflow({ size: total, maxBytes, res }); + } + chunks.push(value); + } + } + } finally { + try { + reader.releaseLock(); + } catch {} + } + + return Buffer.concat( + chunks.map((chunk) => Buffer.from(chunk)), + total, + ); +} diff --git a/extensions/matrix/src/matrix/sdk/transport.test.ts b/extensions/matrix/src/matrix/sdk/transport.test.ts new file mode 100644 index 00000000000..51f9104ef61 --- /dev/null +++ b/extensions/matrix/src/matrix/sdk/transport.test.ts @@ -0,0 +1,67 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { performMatrixRequest } from "./transport.js"; + +describe("performMatrixRequest", () => { + beforeEach(() => { + vi.unstubAllGlobals(); + }); + + it("rejects oversized raw responses before buffering the whole body", async () => { + vi.stubGlobal( + "fetch", + vi.fn( + async () => + new Response("too-big", { + status: 200, + headers: { + "content-length": "8192", + }, + }), + ), + ); + + await expect( + performMatrixRequest({ + homeserver: "https://matrix.example.org", + accessToken: "token", + method: "GET", + endpoint: "/_matrix/media/v3/download/example/id", + timeoutMs: 5000, + raw: true, + maxBytes: 1024, + }), + ).rejects.toThrow("Matrix media exceeds configured size limit"); + }); + + it("applies streaming byte limits when raw responses omit content-length", async () => { + const chunk = new Uint8Array(768); + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(chunk); + controller.enqueue(chunk); + controller.close(); + }, + }); + vi.stubGlobal( + "fetch", + vi.fn( + async () => + new Response(stream, { + status: 200, + }), + ), + ); + + await expect( + performMatrixRequest({ + homeserver: "https://matrix.example.org", + accessToken: "token", + method: "GET", + endpoint: "/_matrix/media/v3/download/example/id", + timeoutMs: 5000, + raw: true, + maxBytes: 1024, + }), + ).rejects.toThrow("Matrix media exceeds configured size limit"); + }); +}); diff --git a/extensions/matrix/src/matrix/sdk/transport.ts b/extensions/matrix/src/matrix/sdk/transport.ts index 8b2a7f8899d..fc5d89e1d28 100644 --- a/extensions/matrix/src/matrix/sdk/transport.ts +++ b/extensions/matrix/src/matrix/sdk/transport.ts @@ -1,3 +1,5 @@ +import { readResponseWithLimit } from "./read-response-with-limit.js"; + export type HttpMethod = "GET" | "POST" | "PUT" | "DELETE"; type QueryValue = @@ -107,6 +109,8 @@ export async function performMatrixRequest(params: { body?: unknown; timeoutMs: number; raw?: boolean; + maxBytes?: number; + readIdleTimeoutMs?: number; allowAbsoluteEndpoint?: boolean; }): Promise<{ response: Response; text: string; buffer: Buffer }> { const isAbsoluteEndpoint = @@ -152,7 +156,24 @@ export async function performMatrixRequest(params: { signal: controller.signal, }); if (params.raw) { - const bytes = Buffer.from(await response.arrayBuffer()); + const contentLength = response.headers.get("content-length"); + if (params.maxBytes && contentLength) { + const length = Number(contentLength); + if (Number.isFinite(length) && length > params.maxBytes) { + throw new Error( + `Matrix media exceeds configured size limit (${length} bytes > ${params.maxBytes} bytes)`, + ); + } + } + const bytes = params.maxBytes + ? await readResponseWithLimit(response, params.maxBytes, { + onOverflow: ({ maxBytes, size }) => + new Error( + `Matrix media exceeds configured size limit (${size} bytes > ${maxBytes} bytes)`, + ), + chunkTimeoutMs: params.readIdleTimeoutMs, + }) + : Buffer.from(await response.arrayBuffer()); return { response, text: bytes.toString("utf8"),