Merge remote-tracking branch 'origin/develop' into develop

Author: t3chguy

CHANGELOG.md

@@ -1,3 +1,8 @@
+Changes in [34.11.0](https://github.com/matrix-org/matrix-js-sdk/releases/tag/v34.11.0) (2024-11-12)
+====================================================================================================
+# Security
+- Fixes for [CVE-2024-50336](https://nvd.nist.gov/vuln/detail/CVE-2024-50336) / [GHSA-xvg8-m4x3-w6xr](https://github.com/matrix-org/matrix-js-sdk/security/advisories/GHSA-xvg8-m4x3-w6xr).
+
 Changes in [34.10.0](https://github.com/matrix-org/matrix-js-sdk/releases/tag/v34.10.0) (2024-11-05)
 ====================================================================================================
 ## 🦖 Deprecations

spec/unit/content-repo.spec.ts

@@ -63,20 +63,6 @@ describe("ContentRepo", function () {
             );
         });
 
-        it("should put fragments from mxc:// URIs after any query parameters", function () {
-            const mxcUri = "mxc://server.name/resourceid#automade";
-            expect(getHttpUriForMxc(baseUrl, mxcUri, 32)).toEqual(
-                baseUrl + "/_matrix/media/v3/thumbnail/server.name/resourceid" + "?width=32#automade",
-            );
-        });
-
-        it("should put fragments from mxc:// URIs at the end of the HTTP URI", function () {
-            const mxcUri = "mxc://server.name/resourceid#automade";
-            expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual(
-                baseUrl + "/_matrix/media/v3/download/server.name/resourceid#automade",
-            );
-        });
-
         it("should return an authenticated URL when requested", function () {
             const mxcUri = "mxc://server.name/resourceid";
             expect(getHttpUriForMxc(baseUrl, mxcUri, undefined, undefined, undefined, undefined, true, true)).toEqual(
@@ -98,5 +84,30 @@ describe("ContentRepo", function () {
                     "/_matrix/client/v1/media/thumbnail/server.name/resourceid?width=64&height=64&method=scale&allow_redirect=true",
             );
         });
+
+        it("should drop mxc urls with invalid server_name", () => {
+            const mxcUri = "mxc://server.name:test/foobar";
+            expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
+        });
+
+        it("should drop mxc urls with invalid media_id", () => {
+            const mxcUri = "mxc://server.name/foobar:test";
+            expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
+        });
+
+        it("should drop mxc urls attempting a path traversal attack", () => {
+            const mxcUri = "mxc://../../../../foo";
+            expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
+        });
+
+        it("should drop mxc urls attempting to pass query parameters", () => {
+            const mxcUri = "mxc://server.name/foobar?bar=baz";
+            expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
+        });
+
+        it("should drop mxc urls with too many parts", () => {
+            const mxcUri = "mxc://server.name/foo//bar";
+            expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
+        });
     });
 });

spec/unit/matrixrtc/MatrixRTCSession.spec.ts

@@ -467,6 +467,36 @@ describe("MatrixRTCSession", () => {
             jest.useRealTimers();
         });
 
+        it("uses membershipExpiryTimeout from join config", async () => {
+            const realSetTimeout = setTimeout;
+            jest.useFakeTimers();
+            sess!.joinRoomSession([mockFocus], mockFocus, { membershipExpiryTimeout: 60000 });
+            await Promise.race([sentStateEvent, new Promise((resolve) => realSetTimeout(resolve, 500))]);
+            expect(client.sendStateEvent).toHaveBeenCalledWith(
+                mockRoom!.roomId,
+                EventType.GroupCallMemberPrefix,
+                {
+                    memberships: [
+                        {
+                            application: "m.call",
+                            scope: "m.room",
+                            call_id: "",
+                            device_id: "AAAAAAA",
+                            expires: 60000,
+                            expires_ts: Date.now() + 60000,
+                            foci_active: [mockFocus],
+
+                            membershipID: expect.stringMatching(".*"),
+                        },
+                    ],
+                },
+                "@alice:example.org",
+            );
+            await Promise.race([sentDelayedState, new Promise((resolve) => realSetTimeout(resolve, 500))]);
+            expect(client._unstable_sendDelayedStateEvent).toHaveBeenCalledTimes(0);
+            jest.useRealTimers();
+        });
+
         describe("non-legacy calls", () => {
             const activeFocusConfig = { type: "livekit", livekit_service_url: "https://active.url" };
             const activeFocus = { type: "livekit", focus_selection: "oldest_membership" };
@@ -478,6 +508,19 @@ describe("MatrixRTCSession", () => {
 
                 jest.useFakeTimers();
 
+                // preparing the delayed disconnect should handle the delay being too long
+                const sendDelayedStateExceedAttempt = new Promise<void>((resolve) => {
+                    const error = new MatrixError({
+                        "errcode": "M_UNKNOWN",
+                        "org.matrix.msc4140.errcode": "M_MAX_DELAY_EXCEEDED",
+                        "org.matrix.msc4140.max_delay": 7500,
+                    });
+                    sendDelayedStateMock.mockImplementationOnce(() => {
+                        resolve();
+                        return Promise.reject(error);
+                    });
+                });
+
                 // preparing the delayed disconnect should handle ratelimiting
                 const sendDelayedStateAttempt = new Promise<void>((resolve) => {
                     const error = new MatrixError({ errcode: "M_LIMIT_EXCEEDED" });
@@ -511,7 +554,14 @@ describe("MatrixRTCSession", () => {
                     });
                 });
 
-                sess!.joinRoomSession([activeFocusConfig], activeFocus, { useLegacyMemberEvents: false });
+                sess!.joinRoomSession([activeFocusConfig], activeFocus, {
+                    useLegacyMemberEvents: false,
+                    membershipServerSideExpiryTimeout: 9000,
+                });
+
+                expect(sess).toHaveProperty("membershipServerSideExpiryTimeout", 9000);
+                await sendDelayedStateExceedAttempt.then(); // needed to resolve after the send attempt catches
+                expect(sess).toHaveProperty("membershipServerSideExpiryTimeout", 7500);
 
                 await sendDelayedStateAttempt;
                 jest.advanceTimersByTime(5000);

src/content-repo.ts

@@ -1,5 +1,5 @@
 /*
-Copyright 2015 - 2021 The Matrix.org Foundation C.I.C.
+Copyright 2015 - 2024 The Matrix.org Foundation C.I.C.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -14,7 +14,22 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { encodeParams } from "./utils.ts";
+// Validation based on https://spec.matrix.org/v1.12/appendices/#server-name
+// We do not use the validation described in https://spec.matrix.org/v1.12/client-server-api/#security-considerations-5
+// as it'd wrongly make all MXCs invalid due to not allowing `[].:` in server names.
+const serverNameRegex =
+    /^(?:(?:\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})|(?:\[[\dA-Fa-f:.]{2,45}])|(?:[A-Za-z\d\-.]{1,255}))(?::\d{1,5})?$/;
+function validateServerName(serverName: string): boolean {
+    const matches = serverNameRegex.exec(serverName);
+    return matches?.[0] === serverName;
+}
+
+// Validation based on https://spec.matrix.org/v1.12/client-server-api/#security-considerations-5
+const mediaIdRegex = /^[\w-]+$/;
+function validateMediaId(mediaId: string): boolean {
+    const matches = mediaIdRegex.exec(mediaId);
+    return matches?.[0] === mediaId;
+}
 
 /**
  * Get the HTTP URL for an MXC URI.
@@ -36,7 +51,7 @@ import { encodeParams } from "./utils.ts";
  * for authenticated media will *not* be checked - it is the caller's responsibility
  * to do so before calling this function. Note also that `useAuthentication`
  * implies `allowRedirects`. Defaults to false (unauthenticated endpoints).
- * @returns The complete URL to the content.
+ * @returns The complete URL to the content, may be an empty string if the provided mxc is not valid.
  */
 export function getHttpUriForMxc(
     baseUrl: string,
@@ -51,14 +66,19 @@ export function getHttpUriForMxc(
     if (typeof mxc !== "string" || !mxc) {
         return "";
     }
-    if (mxc.indexOf("mxc://") !== 0) {
+    if (!mxc.startsWith("mxc://")) {
         if (allowDirectLinks) {
             return mxc;
         } else {
             return "";
         }
     }
 
+    const [serverName, mediaId, ...rest] = mxc.slice(6).split("/");
+    if (rest.length > 0 || !validateServerName(serverName) || !validateMediaId(mediaId)) {
+        return "";
+    }
+
     if (useAuthentication) {
         allowRedirects = true; // per docs (MSC3916 always expects redirects)
 
@@ -67,46 +87,31 @@ export function getHttpUriForMxc(
         // callers, hopefully.
     }
 
-    let serverAndMediaId = mxc.slice(6); // strips mxc://
     let prefix: string;
+    const isThumbnailRequest = !!width || !!height || !!resizeMethod;
+    const verb = isThumbnailRequest ? "thumbnail" : "download";
     if (useAuthentication) {
-        prefix = "/_matrix/client/v1/media/download/";
+        prefix = `/_matrix/client/v1/media/${verb}`;
     } else {
-        prefix = "/_matrix/media/v3/download/";
+        prefix = `/_matrix/media/v3/${verb}`;
     }
-    const params: Record<string, string> = {};
+
+    const url = new URL(`${prefix}/${serverName}/${mediaId}`, baseUrl);
 
     if (width) {
-        params["width"] = Math.round(width).toString();
+        url.searchParams.set("width", Math.round(width).toString());
     }
     if (height) {
-        params["height"] = Math.round(height).toString();
+        url.searchParams.set("height", Math.round(height).toString());
     }
     if (resizeMethod) {
-        params["method"] = resizeMethod;
-    }
-    if (Object.keys(params).length > 0) {
-        // these are thumbnailing params so they probably want the
-        // thumbnailing API...
-        if (useAuthentication) {
-            prefix = "/_matrix/client/v1/media/thumbnail/";
-        } else {
-            prefix = "/_matrix/media/v3/thumbnail/";
-        }
+        url.searchParams.set("method", resizeMethod);
     }
 
     if (typeof allowRedirects === "boolean") {
         // We add this after, so we don't convert everything to a thumbnail request.
-        params["allow_redirect"] = JSON.stringify(allowRedirects);
-    }
-
-    const fragmentOffset = serverAndMediaId.indexOf("#");
-    let fragment = "";
-    if (fragmentOffset >= 0) {
-        fragment = serverAndMediaId.slice(fragmentOffset);
-        serverAndMediaId = serverAndMediaId.slice(0, fragmentOffset);
+        url.searchParams.set("allow_redirect", JSON.stringify(allowRedirects));
     }
 
-    const urlParams = Object.keys(params).length === 0 ? "" : "?" + encodeParams(params);
-    return baseUrl + prefix + serverAndMediaId + urlParams + fragment;
+    return url.href;
 }

src/embedded.ts

@@ -162,7 +162,7 @@ export class RoomWidgetClient extends MatrixClient {
             data: T,
         ): Promise<R> => {
             try {
-                return await transportSend<T, R>(action, data);
+                return await transportSend(action, data);
             } catch (error) {
                 processAndThrow(error);
             }
@@ -174,7 +174,7 @@ export class RoomWidgetClient extends MatrixClient {
             data: T,
         ): Promise<R> => {
             try {
-                return await transportSendComplete<T, R>(action, data);
+                return await transportSendComplete(action, data);
             } catch (error) {
                 processAndThrow(error);
             }